Created attachment 82607 [details] [review]
maemo patch implementing GtkDialog::content-area-spacing

Hmm, I am not 100% convinced that it makes the same amount of sense as the other spacing style properties, but I won't object. I do object to the "Since: memo 1.0"
in the docstring, though...

(In reply to comment #2)
> Hmm, I am not 100% convinced that it makes the same amount of sense as the
> other spacing style properties, but I won't object. I do object to the "Since:
> memo 1.0"
> in the docstring, though...

oops, that was not to stay in there of course ;)
as for the property itself, maemo themes make active use of it, the question is whether we want to support that for upstream themes as well. i tend to say yes, similar to the other spacing properties (but wasn't entirely sure which is why it got filed as a bug).

Created attachment 91290 [details] [review]
Updated Patch (includes the requested fixes)

Looks fine, except that the Since should probably say Since: 2.14, since we are already on the last stretch towards 2.12

Why doesn't this make the style property default to 2 instead of 0? That would make it HIG compliant by default.

(In reply to comment #6)
> Why doesn't this make the style property default to 2 instead of 0? That would
> make it HIG compliant by default.

because that'd be a possibly incompatible change and a different change/bug to discuss actually.

But this seems to me to already be a hugely incompatible change. There are tons of dialogues in GNOME that do "gtk_box_set_spacing (GTK_DIALOG (dialog)->vbox, SOMETHING)" either in their _init function (for derived classes) or after instantiating the dialogue; with this patch gtkdialog's style-set handler will re-set them all to 0, breaking their HIGginess.

(Actually always using "2" wouldn't cut it either, since it needs to be 14 for "alert" style dialogues.)

(In reply to comment #8)
> But this seems to me to already be a hugely incompatible change. There are tons
> of dialogues in GNOME that do "gtk_box_set_spacing (GTK_DIALOG (dialog)->vbox,
> SOMETHING)" either in their _init function (for derived classes) or after
> instantiating the dialogue; with this patch gtkdialog's style-set handler will
> re-set them all to 0, breaking their HIGginess.

Maybe we could not set any value if the property hasn't been modified? Like setting it to -1 by default and ignoring that (that may be stupid, but something with the same effect). The only important thing here is being able to set this from the rc files, really.

> 
> (Actually always using "2" wouldn't cut it either, since it needs to be 14 for
> "alert" style dialogues.)
> 

What about adding a property GtkBox::spacing-set ? struct _GtkBox can still be expanded by a 1 bit guint for that property.

Created attachment 112228 [details] [review]
Added spacing_set

This is an enhanced version of the previous patch, adding a GtkBox::spacing-set property. Also Since was bumped up one version. Now the new style property is only respected if the spacing was not set manually before.

(In reply to comment #11)
> Created an attachment (id=112228) [edit]
> Added spacing_set

 update_spacings (GtkDialog *dialog)
+  g_object_get (G_OBJECT (dialog),
+                "spacing-set", &spacing_set,
+                NULL);

I assume that should be dialog->vbox.

+  if (!spacing_set)
+    gtk_box_set_spacing (GTK_BOX (dialog->vbox), content_area_spacing);

Now spacing_set==TRUE and the spacing will not be updated on theme change.

(In reply to comment #11)
> Created an attachment (id=112228) [edit]
> Added spacing_set
> 
> This is an enhanced version of the previous patch, adding a GtkBox::spacing-set
> property. Also Since was bumped up one version. Now the new style property is
> only respected if the spacing was not set manually before.

I think it's better to maka GtkBox::spacing-set an internal flag. I.e. don't export a property here, but instead just add:
  void     _gtk_box_set_spacing_set (GtkBox*,gboolean);
  gboolean _gtk_box_get_spacing_set (GtkBox*);
Rationale is that the only reason we introduce this flag is for an internal workaround, so we don't need to add and maintain public API for this.

Created attachment 112411 [details] [review]
Made spacing_set private

I changed GtkBox:spacing-set into a private variable with _gtk_box_{get,set}_spacing_set according to Tim's suggestion. This also solves the problem that spacing-set would be set by internal use.

(In reply to comment #14)
> Created an attachment (id=112411) [edit]
> Made spacing_set private

The patch looks mostly good now, except:

+gboolean _gtk_box_get_spacing_set (GtkBox *box)
+{

return values in function definitions need to go on their own line.

@@ -59,6 +59,9 @@ struct _GtkBox
   GList *children;
   gint16 spacing;
   guint homogeneous : 1;
+
+  /*< private >*/
+  guint spacing_set : 1;

You might just wait a week here, then scratch "/*< private >*/" and just use GSEAL (spacing_set) ;)

Created attachment 113253 [details] [review]
content-area-spacing, update to after GSEAL

Updated the patch according to the comments after GSEAL was merged.

Ping :)

Any chance we still can have this in 2.14?

The last patch looks good and can go in. The property docs could be a bit more elaborate though, e.g. add something along:
  ...this is the default spacing used for dialog->box and will be used
  unless gtk_box_set_spacing() is called on dialog->box directly...

Created attachment 116125 [details] [review]
content-area-spacing, updated, better docs

The same patch, with improved doc comment

Lets hold nonessential api additions until 2.15 at this point 

Please update the since tags to 2.16.