GNOME Bugzilla – Bug 698656
GtkInfoBar has lots of empty space above and below
Last modified: 2013-05-25 18:46:36 UTC
Created attachment 242221 [details] screenshot of Trash folder with bloated GtkInfoBar above With GTK built from git master, GtkInfoBars in applications such as Nautilus and Epiphany expand to take up lots of vertical space - they have lots of empty space above and below. To see this, simply visit the Trash folder in Nautilus. I'll attach a screenshot (I'm using Ambiance, but the bug occurs with Adwaita too).
*** Bug 698637 has been marked as a duplicate of this bug. ***
odd, not happening with my testcases here.
bisection points at 56167944e57f73bf1beba0b03be0843e39bceee7
<tristan> mclasen, not sure but... what I can say is... that implementation should not say that "the box" expands... just because a child property wants it to expand, but only should report itself as expanding if one of the child widget's hexpand/vexpand properties say to expand <mclasen> but again, whether the box expands or not should't really be at play here ? <tristan> mclasen, well... a quick glance at the code shows that calling gtk_box_pack_start() does not incur gtk_widget_queue_compute_expand(), but calling gtk_container_set_child_property (box, child, "expand", TRUE); .. does <tristan> that was my first indication <tristan> it might be off, but that's the difference I can see <tristan> and yes, whether the box expands should definitely be at play here <tristan> if the GtkInfoBar (Which is a box) expands vertically, you get the effect displayed in the screenshot on that bug <mclasen> oh, _that_ box <tristan> heh <mclasen> I thought we were talking about the box that contains the infobar <tristan> no I've been looking at 56167944e57f73bf1beba0b03be0843e39bceee7 <tristan> which is a pretty small patch <mclasen> ok, so your analysis is that this was a latent bug in gtkbox.c <tristan> I think a careful inspection of gtk_box_compute_expand(), to ensure it doesnt report itself as expanding based on child properties, would fix this all around <mclasen> it behaves differently depending how you add the children, even if all the child properties and properties end up the same <tristan> yes that's my quick analysis <mclasen> could you put that in the bug, so we don't forget ? <tristan> it behaves differently with gtk_box_pack_start() and gtk_container_child_set_property() yes... it queues an expand computation on itself in the latter case <tristan> ok... I have to run though... <tristan> I wont forget
Created attachment 242390 [details] [review] Proposed patch to fix expand behavior of GtkBox This patch fixes GtkBox to not interfere with the recursive hexpand/vexpand calculations by checking it's own packing properties. In simpler terms: gtk_box_pack_start (box, child, TRUE, TRUE, 0); Should in no way cause 'box' to 'hexpand' or 'vexpand'. only 'child' should expand inside 'box'. Can someone please try this patch with epiphany/nautilus ?
Thanks, Tristan! I tried the patch. The info bar looks fine in Epiphany and Nautilus with this change.
While this may fix it, I don't think the analysis: > Should in no way cause 'box' to 'hexpand' or 'vexpand'. only 'child' > should expand inside 'box'. it true. https://bugzilla.gnome.org/show_bug.cgi?id=628902#c10 is quite clear in that the original intent of the code was to do just that.
Looking at the nautilus code, I really think it is wrong and needs to be fixed. It is packing both the info bar and the file list in a box with expand=TRUE. It can only ever have worked by accident. nautilus_window_slot_add_extra_location_widget needs to be fixed to pack the extra widget with expand=FALSE
Created attachment 242650 [details] [review] Cause gtk_box_pack_start() to initially compute expand when a child is added (In reply to comment #7) > While this may fix it, I don't think the analysis: > > > Should in no way cause 'box' to 'hexpand' or 'vexpand'. only 'child' > > should expand inside 'box'. > > it true. > > https://bugzilla.gnome.org/show_bug.cgi?id=628902#c10 > > is quite clear in that the original intent of the code was to do just that. It is quite clear that the original intent of gtk_box_compute_expand() was to do just that, but that is not a statement towards it's correctness. Let's say I pack a vbox into an hbox, and the vbox is packed with the packing property expand=True. This is an indication that the vbox should expand inside the hbox, but not an indication that the hbox itself should expand in it's parent. The fact that this bug never showed up before seems to indicate that the need-expand flag was never set to TRUE by gtk_box_pack_start(), since gtk_widget_queue_compute_expand() was never called on the widget, so this behaviour lay dormant in much code. If you were to add the call to gtk_widget_queue_compute_expand() in gtk_box_pack_start() then you will encounter this behaviour of abnormally growing widgets (I presume it will happen in many cases, as gtk_box_pack_start() is a very widely used function). The documentation here also points out that it means: https://developer.gnome.org/gtk3/unstable/GtkBox.html#GtkBox--c-expand "Whether the child should receive extra space when the parent grows" Which is clearly different from: "Whether the child should request extra space and cause it's parent to grow" This is however a delicate situation, since the code has been dormant for a long time, calling gtk_widget_queue_compute_expand() from gtk_box_pack_start() and ensuring the "intended" behaviour of gtk_box_compute_expand() takes effect will certainly have disastrous effects, since most existing code that calls gtk_box_pack_start() does not expect this side effect. Here is an experiment worth trying, revert the GtkInfoBar conversion to use composite templates and apply the attached patch: git revert 56167944e57f73bf1beba0b03be0843e39bceee7 patch -p1 < enable-compute-expand.patch (the attached patch) My bet is that nautilus and epiphany will still demonstrate the same unexpected behaviour. If it does, this is a good testament that fixing GtkBox to do what was intended by gtk_box_compute_expand() is indeed incorrect.
I tried the experiment Tristan proposed above. I had to resolve some merge conflicts when reverting the GtkInfoBar conversion, but it wasn't a huge deal. With that revert plus Tristan's patch, Epiphany does indeed demonstrate the unexpected behavior, i.e. has a huge tall infobar. With Nautilus the infobar is also huge, and I also see an empty area of corresponding size even in views where the info bar is absent, such as my home directory (I'll attach a screenshot). By the way, I haven't thought about these layout issues and have no opinion about how to resolve this - that's up to you guys. :)
Created attachment 242684 [details] screenshot of Nautilus with the info bar change reverted plus Tristan's patch
(In reply to comment #9) [...] > Let's say I pack a vbox into an hbox, and the vbox is packed with the packing > property expand=True. > > This is an indication that the vbox should expand inside the hbox, but not an > indication that the hbox itself should expand in it's parent. The vbox can't expand unless the hbox gets more space than it asked for - ie setting expand on the vbox without setting it on the hbox rarely makes sense. The tediousness of this manual propagation is exactly what GtkWidget::h/vexpand are supposed to solve. > The fact that this bug never showed up before seems to indicate that the > need-expand flag was never set to TRUE by gtk_box_pack_start(), since > gtk_widget_queue_compute_expand() was never called on the widget, so this > behaviour lay dormant in much code. If you were to add the call to > gtk_widget_queue_compute_expand() in gtk_box_pack_start() then you will > encounter this behaviour of abnormally growing widgets (I presume it will > happen in many cases, as gtk_box_pack_start() is a very widely used > function). > > The documentation here also points out that it means: > https://developer.gnome.org/gtk3/unstable/GtkBox.html#GtkBox--c-expand > > "Whether the child should receive extra space when the parent grows" > > Which is clearly different from: > > "Whether the child should request extra space and cause it's parent to grow" Nice try, but that is not what h/vexpand does, nor how it is documented. [...] > If it does, this is a good testament that fixing GtkBox to do what > was intended by gtk_box_compute_expand() is indeed incorrect. I don't think you can talk about correctness at all unless you've given a precise definition of the intended behavior. What we should talk about instead is compatibility. Clearly we are in a bad situation because there is code in gtkbox.c now which tries to propagate expand child properties, but sometimes does not succeed in doing so. There's two possible changes here: 1) the patch you attached, which makes GtkBox never propagate the expand child property 2) alternatively, fix GtkBox to always propagate expand child properties, regardless how the child was added It would be good to do some research into which of these options breaks more existing users. That's more productive than trying to argue about 'correctness'.
We've discussed this a bit more in bug 628902, and perhaps we should indeed open a separate bug for this issue, (as it's not exactly nautilus's problem, nor is directly related to the topic of bug 628902).
Created attachment 242954 [details] [review] Don't expand extra widgets
Review of attachment 242954 [details] [review]: This looks good to me regardless of the more general problem.
Comment on attachment 242954 [details] [review] Don't expand extra widgets Attachment 242954 [details] pushed as c8a8750 - Don't expand extra widgets
Great - can we close this now?
OK - since I filed it I guess I'll close it. :)
Actually I think I was overeager in doing that - I know there's an underlying GTK issue here which may not be resolved. Reopening; up to you guys to decide when this is fixed.