Created attachment 169585 [details] [review]
Add horizontal and vertical expand flags, compute_expand() to GtkWidget

GtkWidget now has flags for horizontal and vertical expand, and
a compute_expand() method. compute_expand() is used by containers
to set a default expand flag. (If a widget has expand set explicitly,
it always overrides the results of compute_expand.)

GtkContainer has a default implementation of compute_expand which
simply walks over all child widgets and sets expand=TRUE
if any child is expanding.

The overall effect is that you only have to set expand on
leaf nodes in the widget tree, while previously you had to
set expand all the way up the tree as you packed every
container. Also, now containers need not have their own child
properties for expand.

For old containers which do have "expand" child properties,
they should override compute_expand and include the child
properties in whether the container is expand=TRUE.
Also, the old container should use
"child_prop_expand || gtk_widget_get_expand()" everywhere
it previously used simply "child_prop_expand"

Created attachment 169586 [details] [review]
Support GtkWidget expand properties in GtkBox

This consists of:
* expand a child if either child->expand || gtk_widget_get_expand(child)
* override compute_expand so that child->expand will cause us to
  return TRUE for gtk_widget_get_expand()

Created attachment 169587 [details] [review]
add tests/testexpand.c used to test the expand props on GtkWidget

There are two colored boxes with toggle buttons nested
inside several GtkBox. Toggling these to expand mode
should automatically propagate expansion up through
the several GtkBox such that resizing the window
results in resizing the colored boxes.

Review of attachment 169585 [details] [review]:

::: gtk/gtkwidget.c
@@ +3113,3 @@
   gtk_widget_set_parent_window (widget, NULL);
+
+  gtk_widget_queue_compute_expand (old_parent); /* parent may no longer expand */

I guess this could be conditional on the child being expand and visible ?

@@ +3304,3 @@
 
+      /* a parent may now be expand=FALSE since we're hidden. */
+      gtk_widget_queue_compute_expand (widget);

Could be conditional on the child being expand, I guess ?

@@ +10430,3 @@
+  g_return_val_if_fail (GTK_IS_WIDGET (widget), FALSE);
+
+  /* We never make a window expand if not even showing. */

s/window/widget/ in the comment ?

@@ +10436,3 @@
+  if (orientation == GTK_ORIENTATION_HORIZONTAL)
+    {
+      gtk_widget_compute_expand (widget);

Could pull the compute_expand before the if here.

::: gtk/gtkwidget.h
@@ +795,3 @@
+void     gtk_widget_set_expand_set       (GtkWidget      *widget,
+                                          GtkOrientation  orientation,
+                                          gboolean        set);

This is a little out-of-style for gtk. I'd expect separate
h-expand / v-expand properties with separate setters and getters, instead passing the orientation in like this.

I was going back and forth on the orientation arg. 

For get_expand, it's generally going to be used like in the gtkbox.c patch, you can kind of see the mess you'd get, rather than:

 child->expand || gtk_widget_get_expand(child->widget, box->orientation)

you probably have to make a temporary variable so you can write something clunky like
 expand = orientation == GTK_ORIENTATION_HORIZONTAL ? get_h_expand() : get_v_expand() 

probably in gtkbox.c, you'd end up just making a little wrapper that took the orientation arg.

On the other hand, for the setter, it'd usually be used by an app, and there I agree you want two setters. Also, the object props (not added yet) have to be separate h-expand and v-expand. so two setters would be consistent.

split up will also be consistent with the padding patch so that's probably good.


I guess I agree it needs to be split up, but it's tempting to have the get_expand() with orientation also, just for containers to use ...

Thats true, hmm. We could have both separate getters to go with the separate properties, and the orientation-taking one for the benefit of flippable containers.

Patches depend on bug 628884, it seems.

Review of attachment 169585 [details] [review]:

::: gtk/gtkwidget.h
@@ +284,3 @@
+  guint GSEAL (h_expand_set) : 1;        /* whether to use application-forced  */
+  guint GSEAL (v_expand_set) : 1;        /* instead of computing from children */
+

Quite a few bits used for expand. I guess you could try to get away without separate bits for h_expand and computed_h_expand.

Hmm. doesn't look like we can lose the separate h_expand and computed_h_expand because if you toggled off h_expand_set then the h_expand flag would mysteriously change.

What we could do potentially is move h_expand, v_expand, h_expand_set, v_expand_set to AuxInfo; or only h_expand and v_expand to AuxInfo if we want to look up the AuxInfo less. Since only a few widgets in a window would typically have expand set on them, in theory.

Seems a bit clunky to do until we actually run out of bits, though.
We don't have to prematurely optimize if GtkWidget struct is private, in theory.

Created attachment 169640 [details] [review]
Add horizontal and vertical expand flags, compute_expand() to GtkWidget

GtkWidget now has flags for horizontal and vertical expand, and
a compute_expand() method. compute_expand() is used by containers
to set a default expand flag. (If a widget has expand set explicitly,
it always overrides the results of compute_expand.)

GtkContainer has a default implementation of compute_expand which
simply walks over all child widgets and sets expand=TRUE
if any child is expanding.

The overall effect is that you only have to set expand on
leaf nodes in the widget tree, while previously you had to
set expand all the way up the tree as you packed every
container. Also, now containers need not have their own child
properties for expand.

For old containers which do have "expand" child properties,
they should override compute_expand and include the child
properties in whether the container is expand=TRUE.
Also, the old container should use
"child_prop_expand || gtk_widget_get_expand()" everywhere
it previously used simply "child_prop_expand"

Created attachment 169641 [details] [review]
Support GtkWidget expand properties in GtkBox

This consists of:
* expand a child if either child->expand || gtk_widget_get_expand(child)
* override compute_expand so that child->expand will cause us to
  return TRUE for gtk_widget_get_expand()

Created attachment 169642 [details] [review]
add tests/testexpand.c used to test the expand props on GtkWidget

There are two colored boxes with toggle buttons nested
inside several GtkBox. Toggling these to expand mode
should automatically propagate expansion up through
the several GtkBox such that resizing the window
results in resizing the colored boxes.

I decided it might make sense if 
gtk_widget_get_h_expand() returned simply widget->h_expand to match the 
property exactly
and then
gtk_widget_compute_expand(widget, orientation)
returns the computed value to be used by layout containers.

I also added the object properties and some docs.

The object props include an "expand" prop which sets h and v at the same time.

Review of attachment 169640 [details] [review]:

::: gtk/gtkwidget.c
@@ +10569,3 @@
+ * The computed expand value uses either the expand setting explicitly
+ * set on the widget itself, or, if none has been explicitly set,
+ * the widget will expand if any of its children wants to expand.

This should probably be a little more vague, like:

"...or, if none has been explicitly set, the widget may expand if some of its children do."

@@ +10689,3 @@
+ * this function, to see whether a widget, or any of its children,
+ * has the expand flag set. If any child of a widget needs to
+ * expand, the parent needs to expand also.

Again, could be a little less assertive here.

@@ +10722,3 @@
+ * By default, widgets automatically expand if any of their children
+ * want to expand. (To see if a widget will automatically expand
+ * given its current children and state, call gtk_widget_compute_expand()).

Could add something like: "A container implementation can decide how the expandability of the children affects the expandability of the container, by overriding the ... method.

Review of attachment 169641 [details] [review]:

::: gtk/gtkbox.c
@@ +682,3 @@
+
+      if (our_expand && opposite_expand)
+        break;

As I said on the list, shouldn't this require that _all_ children are expandable before making the container expandable in the opposite direction ?

Eg an hbox should only be vertically expandable, if every child is...

Looking at http://git.gnome.org/browse/gtk+/commit/?h=widget-expand&id=6f3c6caadaf9b8c015572c69857b1e241cdc5d26 - in hide/show, it should probably call queue_compute_expand() on the parent.

Created attachment 170108 [details] [review]
Add horizontal and vertical expand flags, compute_expand() to GtkWidget

GtkWidget now has flags for horizontal and vertical expand, and
a compute_expand() method. compute_expand() is used by containers
to set a default expand flag. (If a widget has expand set explicitly,
it always overrides the results of compute_expand.)

GtkContainer has a default implementation of compute_expand which
simply walks over all child widgets and sets expand=TRUE
if any child is expanding.

The overall effect is that you only have to set expand on
leaf nodes in the widget tree, while previously you had to
set expand all the way up the tree as you packed every
container. Also, now containers need not have their own child
properties for expand.

For old containers which do have "expand" child properties,
they should override compute_expand and include the child
properties in whether the container is expand=TRUE.
Also, the old container should use
"child_prop_expand || gtk_widget_compute_expand()" everywhere
it previously used simply "child_prop_expand"

Created attachment 170109 [details] [review]
Support GtkWidget expand properties in GtkBox

This consists of:
* expand a child if either child->expand || gtk_widget_get_expand(child)
* override compute_expand so that child->expand will cause us to
  return TRUE for gtk_widget_get_expand()

Created attachment 170110 [details] [review]
add tests/testexpand.c used to test the expand props on GtkWidget

There are two colored boxes with toggle buttons nested
inside several GtkBox. Toggling these to expand mode
should automatically propagate expansion up through
the several GtkBox such that resizing the window
results in resizing the colored boxes.

I'm still interested in this. I think we should get it landed...

I pushed widget-expand-3 with a rebase, rename h_expand to hexpand to match the margins patch, and docs tweaks as suggested. I deleted widget-expand and widget-expand-2 old branches.

I don't think box should require all children to be expand in the opposing orientation before expanding.

If you had a scrolled window that could usefully grow and drop scrollbars in both directions, inside a box, that behavior should not vanish just because you pack something next to it. The thing next to it can just get extra space and align or whatever. Boxes already force all children to expand in the opposing direction up to the size of the largest child in that direction, before these patches. i.e. in the opposing direction everything is basically packed fill=true expand=true already.

The remaining work items (which may or may not block landing what we have):

* magic resizability behavior on GtkWindow
* do the equivalent of the GtkBox patch to GtkTable (I think that's the only other container?)

I don't think I can do these this weekend but I can hopefully do them soon. GtkTable is trivial and mechanical, GtkWindow may be a little tricky.

Expander, Toolbar, Notebook and Paned are other widgets where I'm not sure how child expanding flags should be honored. GtkBin is pretty obvious after I guess.

Containers where we currently have some expand-like child properties include:

GtkBox (done)
GtkTable (easy)
GtkWrapBox (propagate)
GtkToolbar (probably no propagation)
GtkNotebook (propagate for content, but not for tabs)
GtkPaned (propagate ?)
GtkToolItemGroup (don't propagate ?)
GtkToolPalette (expand here is really 'expanded', should probably be renamed)

there are some containers that don't have expand-like child properties, but should
probably still pass expand on:

GtkExpander (only propagate if expanded ?)
GtkFrame (propagate)
GtkScrolledWindow (propagate)

The ones that propagate always should have no patch, since gtkcontainer already does that by default.

I wonder if it's worth an override in toolbar, notebook tabs, and tool item group. While yes it doesn't make sense to set expand on these things, at the same time, it's not like expand is set by default. So putting in the override would just be "if you set expand like a dork, we ignore you" ; maybe it's better to avoid the extra code and say "don't do that then" - and if someone comes up with some reason to expand, fine? Though if someone does want to expand a toolbar, even if we override to ignore it by default, they could do it by setting expand on both contents and toolbar itself.

Anyway it seems like avoiding special cases could make the whole expand thing easier to understand; if I'm messing around in a UI builder maybe toolbar should just work like anything else to illustrate the expand concept, instead of me having to know that some containers special-case ignore me if I ask to do something silly.

GtkExpander only propagate if expanded sounds right (since the contents are effectively hidden if not expanded). There is a possible weird side effect though, which is forcing the toplevel to resize when you collapse the expander. Say your scrolled window in an expander is the only expanding thing in the window. When you open expander, you can resize the window up. But when you close the expander, the window will become not resizable and snap to the min size. I don't know, arguably that's the correct behavior anyway, and it can be fixed by manually forcing resizability on the toplevel.

An alternate behavior I haven't seen on other platforms would be for GtkExpander to always propagate expand and if you resize and the expander gets extra space of at least child_min_size, it auto-opens. Between 0 and child_min_size of allocation maybe the arrow starts to rotate as a function of the allocation. Too fancy?

Rather than patching GtkExpander specifically, maybe having gtk_widget_set_child_visible() do the right thing would be appropriate. However, this would make GtkNotebook do the wrong thing I think, since notebook size request and expansion seems to always be a function of all pages even hidden ones.

The auto-open-expander-on-extra-space idea is not good among other reasons because generally expanders are to hide "advanced" or clutter type of stuff, and so people probably don't want them open just because there's space or someone happened to maximize the window.

(In reply to comment #23)
> GtkToolbar (probably no propagation)
> GtkNotebook (propagate for content, but not for tabs)
I think I agree with Havoc here that we should propagate.

> GtkPaned (propagate ?)
At least. We might also want to expand into the orientation's direction by default. It's weird if you can resize both widgets inside the paned but not the paned itself.
Also, the default position of the paned could maybe be influenced by the expand flags of its children?

> GtkExpander (only propagate if expanded ?)
I agree with Havoc that it should always propagate. It's weird if the resizability of windows depends on the expand state of an expander. (Fwiw, the "expander" and the "expand" flags have nothing to do with each other, can we find a better naming for the flags?)

> GtkScrolledWindow (propagate)
Hrm, don't you always want a scrolled window to expand if it has a scrollbar in that direction? I can't imagine a case where you'd say "keep my scrolled window small so the user has to scroll".
You could argue you'd have the contents of the scrolled window on expand, but I can at least find examples for where that's not the case: property editors in the GTK tests for exmaple, like when clicking columns in tests/testtreeview.

How exactly do:
gtk_widget_set_h_expand_set(widget, TRUE);
and
gtk_widget_set_halign(widget, GTK_ALIGN_FILL)

differ?

I get how the expand propagation is nice, but it seems to me that ALIGN_FILL already covers the leaf widget "should expand" case.

Interesting question, short answer I believe is that expand=true|false is whether the widget wants (requests?) extra space while alignment is what widget does if it gets extra space.

In most cases, widgets don't get extra space if they don't want it, however, the cases where they do get unrequested extra space are the cases where align=FILL differs in intent from expand=true.

Say you have some buttons in a vbox. They would all be the same width, and may or may not have expand=true. However, all but one of them will have extra horizontal size. You could legitimately want any of the alignments within the extra horizontal space - make all buttons match (FILL) or left-align them or whatever. I don't think you necessarily want the whole vbox to be hexpand=true, though; i.e. you probably want to be able to make the max width of the button box just match the largest button.

Same basic thing arises with a SizeGroup or table column or whatever.


If we collapsed expand and fill I guess it might look like:
 PackMode = { EXPAND, CENTER, START, END }

Issues might include

1. right now the align default is FILL for (in my opinion) back compat reasons; I don't much like the default but changing it would make all existing expand=TRUE layouts break. (expand=TRUE here means expand flag on Box, Table, etc.)  Assuming propagation, defaulting to PackMode.EXPAND would be bad... most windows only have a couple things that should expand, so this default would be backward big time. But defaulting to say CENTER is huge, epic, very visible API breakage. (Because it breaks the existing expand/fill child properties by vetoing them on the child widget level.)

2. if a widget doesn't expand, it can't get extra allocation to align within, in general. (the exception is if the widget's size is tied to that of another widget which does expand.) So for the most part, CENTER, START, END would never do anything. You would have to use a GtkAlignment again I think...

I think maybe what it ends up looking like is to drop alignment, only have expand=true|false, and go back to using a GtkAlignment when alignment is desired.

Hmm. To avoid that, I guess the PackMode could have yet another setting:

 PackMode = { FILL, CENTER, START, END, NO_EXPAND } 

Where FILL, CENTER, START, END all imply expand=true and NO_EXPAND means don't give us extra space - so it doesn't matter how we position the widget's allocation inside extra space. However, this breaks because of "unrequested" extra space... to handle "unrequested" space you need to add FILL_NO_EXPAND, CENTER_NO_EXPAND, START_NO_EXPAND, END_NO_EXPAND and of course that's just a separate expand flag again.

I think having expand separate is much clearer, tbh.

Created attachment 172151 [details] [review]
gtkwindow wip patch

Here is a patch that almost kinda works. I can grow a window until the scrollbar of the scrolled content disappears, and then the expandability of the scrolledwindow is flipped, and the window picks it up. Unfortunately, it then resets the window max size to the requisition, which causes the window to jump back in size,  flicker wildly, or start wandering, depending on which side you drag. I haven't found a way to make the max size the current size without causing infinite loops.

I think the problem may be that scrolled window expand should depend on scrollbar *policy* not scrollbar *visibility*

expand flag should not be changing during a user resize operation

(In reply to comment #31)
> I think the problem may be that scrolled window expand should depend on
> scrollbar *policy* not scrollbar *visibility*
> 
> expand flag should not be changing during a user resize operation

Hmm, maybe. But then we won't get the magic 'window stops resizing if fully expanded'. Maybe that is not so important.

But I do think it is a problem that the window jumps to a smaller size if it is not longer resizable. The same thing happens when you set resizable to FALSE.  The window gets shrunken to its requisition.

I think it's right that a nonresizable window gets shrunk to a fixed size. If you have a nonresizable window then basically it always resizes to just match its contents. It shouldn't be a problem in real life since you wouldn't become nonresizable during a user resize operation or anything.

However this definitely interacts with that long thread on gtk-devel-list right now... the min size may not be an especially 'good' size. Maybe nonresizable windows should snap to something closer to the default size instead of just the min size request.

To get "magic stop resizing when fully expanded" I think we'd have to add max size to min and natural, in the get_preferred_{width,height} calls. "max size" means "ignore expand flag above this size"

> I think the problem may be that scrolled window expand should depend on
> scrollbar *policy* not scrollbar *visibility*

Hmm, but which way will it go ?

expand = policy != never || child_exand

?

This seems slightly counter-intuitive to me, since allowing a scrollbar is in a
way saying 'its ok if this gets a smaller area, the user can scroll'

hmm, just noticed the way I described toplevel resizing in the original bug description is a bit more elaborate. I proposed that for a window with no expand flags, min size = min size, max size = natural size

In that scenario, for a scrolled window, it shouldn't be expand=true at all in general. The min size would show almost nothing, the natural size would drop scrollbars and show everything. 

So it would be 
scrolled_window_expand = (policy == never && child_expand)

i.e. propagate if no scrollbar, otherwise sw doesn't need to expand beyond natural size?

> So it would be 
> scrolled_window_expand = (policy == never && child_expand)
> 
> i.e. propagate if no scrollbar, otherwise sw doesn't need to expand beyond
> natural size?


Yes, that makes sense from a can afford to be small' perspective. But it is very natural to have the content area (eg in gedit) in a scrolledwindow with policy == automatic), and this would defeat the 'make content area expand, and it will get all extra space' idea.

maybe ScrolledWindow just isn't a special case - it expands if the child does? But to just allow resizing between "tiny" and "natural size of child" you don't need to set expand=true on anything. expand is only needed if you want the child to grow above its natural size / don't want to set max size on the window.

Of course, there is this issue that setting max sizes on windows produces sort of bizarre UI behavior, at least for window types that are normally maximizable / fullscreenable.

This also does not mean that simething like a IRC user list or a file chooser resizes the window all the time as users/files are added and removed, right?

Just pointing that out so that tree views with changing number of contents are on everybody's radar.

(In reply to comment #38)
> This also does not mean that simething like a IRC user list or a file chooser
> resizes the window all the time as users/files are added and removed, right?
> 
> Just pointing that out so that tree views with changing number of contents are
> on everybody's radar.

No, expand is all about accepting extra space, not about forcing things to be larger.

This isn't really different from the current set_resizeable behavior; if you are resizeable=false then the window just resizes itself to match the content, if you are resizeable=true then the user's resize "sticks" unless the min size goes up and then the window self-resizes up to the min size.

The idea "set max size to natural size if nothing is expand=true" does add a new twist, which is an auto-computed max size that may not equal min size. So the window is resizeable but with a max, while current resizeable=true windows have only a min, not a max; and current resizeable=false windows have a max but it equals min. So the max size=natural, means the window is somewhat resizeable, but if the max goes down, the window would autoshrink, just as currently when the min goes up, the window autogrows.

This is probably all logical. yeah it can misbehave in certain cases, but if you have a list of files or users, you should set expand=true which would remove the max size, I guess.

One thing is that if doing this, now we have that issue Owen raised for min size, only on the max size as well... i.e. what's the for-width to compute the max height? The max height becomes something really short I guess?

So maybe it's simpler to dump the "max size of natural size" idea.

The one case that really misbehaves with max sze == natural size is that the scrolled window currently doesn't set a natural size that lets you get rid of the scrollbars. If we fix that, things will behave relatively nicely, I think.

Created attachment 172275 [details] [review]
another iteration of the window patch

I have now merged the branch. I've not done anything special for GtkScrolledWindow, so it just propagates its childs expand. And I've left out the window resizability.

The patch above almost seems to work fine, but there are some cases where it gets confused, if you set and then unset expand or expand-set flags on the window or its children. Either the expand propagation is screwy, or I haven't quite mastered the resizability updates in gtkwindow right. 

Something to look at later...

retitling for whats left.

(In reply to comment #10)
> Created an attachment (id=169640) [details] [review]
> Add horizontal and vertical expand flags, compute_expand() to GtkWidget

Havoc, I think part of this reasoning is wrong.

Some description of the fallout is up in bug 698656.

It seems now that gtk_container_child_set_property() is used to
set the child "expand" property, it's causing gtk_box_compute_expand()
to actually be called hit.

> 
> GtkWidget now has flags for horizontal and vertical expand, and
> a compute_expand() method. compute_expand() is used by containers
> to set a default expand flag. (If a widget has expand set explicitly,
> it always overrides the results of compute_expand.)
> 
> GtkContainer has a default implementation of compute_expand which
> simply walks over all child widgets and sets expand=TRUE
> if any child is expanding.
> 
> The overall effect is that you only have to set expand on
> leaf nodes in the widget tree, while previously you had to
> set expand all the way up the tree as you packed every
> container. Also, now containers need not have their own child
> properties for expand.
> 
> For old containers which do have "expand" child properties,
> they should override compute_expand and include the child
> properties in whether the container is expand=TRUE.

This part I think is wrong.

Application code which calls:
   gtk_box_pack_start (box, child, TRUE, TRUE, 0);

Does not expect that packing a child into the box, will cause the box
itself to expand (nor is the 'expand' child property of GtkBox documented
to have this effect).

> Also, the old container should use
> "child_prop_expand || gtk_widget_get_expand()" everywhere
> it previously used simply "child_prop_expand"

This part makes perfect sense, GtkBox should let children
expand based on their hexpand/vexpand flags at allocation time.

The end result should be that:

  o Adding a widget with hexpand/vexpand TRUE to a GtkBox will
    cause that child to expand inside it's parent _and_ cause
    the GtkBox to expand in _its_ parent.

  o Adding a widget to a GtkBox with the packing 'expand'
    property set to TRUE, should cause the child to expand
    in the box, but certainly not cause the box itself to
    expand in it's parent.

Currently, we have the above desired behaviour because gtk_box_pack_start()
does not call gtk_widget_queue_compute_expand(), so all the code which
expects the traditional behaviour of GtkBox is not broken because the
->compute_expand() of GtkBox is not hit (thankfully, because this
avoids breakage in much existing code, which would otherwise need to
be fixed by explicitly setting h/vexpand = FALSE on any parenting
GtkBox).

The gtk_box_compute_expand() implementation is called as a result of:

  o gtk_container_child_set_property() is called on the box, and then
    only if the child property has changed from it's default

  o gtk_widget_set_[hv]expand() is called on a child _after_ adding
    it to the GtkBox (rather unlikely).

  o gtk_widget_set_parent() is called for a child with an explicit
    hexpand/vexpand property set, this is also unlikely for any
    code calling gtk_box_pack_start().

However it is not called as a result of gtk_box_pack_start() with an
expand property set to TRUE (which is the majority use case of
gtk_box_pack_start()).

It's been a few years here, but I believe we encountered something exactly like  bug 698656 when initially landing these changes and may have discussed the issue on the mailing list. I don't see the mail immediately though. But maybe GtkToolbar had this problem at some point? (that it expanded because its children did?) I think the expand stuff may have broken toolbars. Just a faint memory. Maybe the discussion was in bugzilla or IRC, who knows.

If you don't want an expanding child to "propagate" up to make its parents expand, you are supposed to explicitly set expand=FALSE on the parent, right? I think that was the intent.

It is certainly a bug that setting the "expand" child property calls queue_compute_expand but that pack_start does not. So yeah that probably was hiding other issues including potentially that this patch wasn't compatible enough. Those two should be made equivalent one way or another. (Probably pack_start should queue_compute_expand also, but then count_expand_children should either use child->expand or not).

Either way you fix this will probably break *something* at this point...

Sorry if anything above doesn't make sense, it has been a while.

Oh, comment 22 through comment 24 talk about toolbars even.

FWIW *ignoring compatibility issues* I think it's correct and was the intent that packing a child with expand=TRUE would propagate up (at least to the next ancestor which explicitly set expand=FALSE). Or if the box itself has expand=FALSE then packing an expandable child in the box should not cause the box to expand.

It looks pretty clear to me that the pack_start not propagating expand up the tree was an unintentional mistake. However, I don't know the extent of the back compat issues that were papered over by this mistake.

(In reply to comment #46)
> It's been a few years here, but I believe we encountered something exactly like
>  bug 698656 when initially landing these changes and may have discussed the
> issue on the mailing list. I don't see the mail immediately though. But maybe
> GtkToolbar had this problem at some point? (that it expanded because its
> children did?) I think the expand stuff may have broken toolbars. Just a faint
> memory. Maybe the discussion was in bugzilla or IRC, who knows.
> 
> If you don't want an expanding child to "propagate" up to make its parents
> expand, you are supposed to explicitly set expand=FALSE on the parent, right? I
> think that was the intent.
> 
> It is certainly a bug that setting the "expand" child property calls
> queue_compute_expand but that pack_start does not. So yeah that probably was
> hiding other issues including potentially that this patch wasn't compatible
> enough. Those two should be made equivalent one way or another. (Probably
> pack_start should queue_compute_expand also, but then count_expand_children
> should either use child->expand or not).
> 
> Either way you fix this will probably break *something* at this point...
> 
> Sorry if anything above doesn't make sense, it has been a while.

Thanks for your quick reply Havoc.

It makes sense to me, yes, and I agree whichever way you fix it, it will
probably break *something*, I'm pretty convinced that it will break less
things to just remove the ->compute_expand() vfunc from GtkBox, and I also
think that would have been more logical to begin with.

Rationale:

  o The "expand" child property is essentially deprecated in favour of
    "hexpand"/"vexpand" properties on GtkWidget (if not deprecated,
    at least the latter is 'preferred').

    However using the "hexpand"/"vexpand" properties does produce
    the advertised results (both that it effects the parent, and
    the GtkBox takes the [hv]expand attributes into account when
    allocating, very nice).

  o GtkBox "expand" child property is not documented to effect the
    box's expand request, neither is it mentioned anywhere I can see
    in the porting guide[0] (I may have missed it's mention, though).

As it looks to me that we advertised that gtk_box_pack_start() should
be usable in the same way as in GTK+2, I would expect any ported
application code to have the same expectation.

I'm quite sure that more code will suffer by enforcing this original
intent than by removing the intent all together, otherwise we will
find ourselves adding many calls to gtk_widget_set_[hv]expand(box, FALSE)
to many ported applications, but if others strongly believe that we
should "stick to our guns" on this instead, I won't argue the case further.

[0]:https://developer.gnome.org/gtk3/stable/migrating.html

I think the section on expand in the porting guide is intended to cover this: https://developer.gnome.org/gtk3/stable/ch25s02.html#idm46912315965200

'If you experience sizing problems with widgets in ported code, carefully check the "expand" and "fill" flags of your boxes.'

But yeah the bottom line is it depends on the back compat. Maybe the most compatible thing is to just leave it all as-is :-/

I don't know how much will really break; you would sort of have to try a fix in either direction and see what breaks in each case.

Maybe this should be a new/separate bug btw ("pack_start and expand child property have inconsistent behavior").

I agree that compatibility will have to be deciding factor - which change breaks less is the one we should take.

Either way, we should expand the documentation around expand flags - the documentation of GtkBox::expand should spell out that it does not propagate up, and point to GtkWidget::h/vexpand as an alternative that does.

I agree that compatibility will have to be deciding factor - which change breaks less is the one we should take.

Either way, we should expand the documentation around expand flags - the documentation of GtkBox::expand should spell out that it does not propagate up, and point to GtkWidget::h/vexpand as an alternative that does.

We're moving to gitlab! As part of this move, we are moving bugs to NEEDINFO if they haven't seen activity in more than a year. If this issue is still important to you and still relevant with GTK+ 3.22 or master, please reopen it and we will migrate it to gitlab.

As announced a while ago, we are migrating to gitlab, and bugs that haven't seen activity in the last year or so will be not be migrated, but closed out in bugzilla.

If this bug is still relevant to you, you can open a new issue describing the symptoms and how to reproduce it with gtk 3.22.x or master in gitlab:

https://gitlab.gnome.org/GNOME/gtk/issues/new