GNOME Bugzilla – Bug 769162
GtkTable: compute_expand buggy
Last modified: 2016-08-01 13:28:46 UTC
Created attachment 332118 [details] [review] gtktable: queue compute_expand when attaching a new widget and ignore hidden children It took into account the child properties of hidden children which according to gtk_widget_compute_expand () should be ignored. When adding a new child it didn't queue a expand recompute which is needed because the expand logic depends on the GtkAttachOptions set with each child.
Review of attachment 332118 [details] [review]: ::: gtk/deprecated/gtktable.c @@ +792,3 @@ priv->children = g_list_prepend (priv->children, table_child); + gtk_widget_queue_compute_expand (GTK_WIDGET (table)); This should not be needed, because gtk_widget_set_parent() will check if the child needs to expand, and will queue a compute expand on the parent.
I'd split this patch into two separate commits. The first chunk is okay, but the second one warrants more investigation.
(In reply to Emmanuele Bassi (:ebassi) from comment #1) > Review of attachment 332118 [details] [review] [review]: > > ::: gtk/deprecated/gtktable.c > @@ +792,3 @@ > priv->children = g_list_prepend (priv->children, table_child); > > + gtk_widget_queue_compute_expand (GTK_WIDGET (table)); > > This should not be needed, because gtk_widget_set_parent() will check if the > child needs to expand, and will queue a compute expand on the parent. See the comment here: https://git.gnome.org/browse/gtk+/tree/gtk/gtkwidget.c#n9589 In this case the child has no expand set and nothing gets queued, expand only comes into play through the new parent child prop. (In reply to Emmanuele Bassi (:ebassi) from comment #2) > I'd split this patch into two separate commits. OK
OK, ignore the above. As discussed in https://bugzilla.gnome.org/show_bug.cgi?id=628902#c50 which included the patch adding expand flags support to GtkTable: https://github.com/GNOME/gtk/commit/48de50df890b23aa92e6a158866721cf5eb7a48d it was decided the child prop expand flags shouldn't be propagated up. This is currently implemented that way in GtkBox for example. In GtkTable child props do propagate up, but due to the missing queue mentioned above it only happens in case a child queues a compute_expand after being added. And sometime between 3.18 and 3.20 GtkCombobox decided to do that queue which is why my layouts broke with 3.20. So I propose to no longer include GtkTable child props in compute_expand as originally planned and as was practically the case until 3.18.
Created attachment 332138 [details] [review] gtktable: don't try to propagate expand related child props in compute_expand() It tried to set the expand state if either xexpand/yexpand where true. Due to a missing queue_compute_expand when adding a child it actually only computed the expand state in case a child queued after being added or in case a child had the expand property set (see optimization in gtk_widget_set_parent) In my case this broke layouts as a child of GtkCombBox started setting an exand flag with 3.20 which queued a compute_expand, which in turn propagated an expand child props set for a cell in the same table up and overrode the expand child prop of a parent GtkBox. This removes the custom compute_expand implementation to match the behaviour of GtkBox (don't propagate child prop expand flags but let child expand flags override the child props) and not get random expand behaviour depending on whether and when child widgets set their expand state.
Created attachment 332139 [details] [review] gtkbox: remove unnecessary queue_compute_expand() The expand child property does not have any effect on the expand state of the GtkBox, so queuing a compute_expand when changing it is not needed.
Attachment 332138 [details] pushed as a72f1c7 - gtktable: don't try to propagate expand related child props in compute_expand() Attachment 332139 [details] pushed as 4556d0f - gtkbox: remove unnecessary queue_compute_expand()
Thanks!