GNOME Bugzilla – Bug 698433
Geometry management broken in GtkBin widgets
Last modified: 2018-04-15 00:02:49 UTC
At some point GtkBin height-for-width / width-for-height stopped working. Try ./tests/testheightforwidth and select the "Horizontal Box" test. It's immediately visible that the button in the top left is not requesting enough height-for-width anymore.
Created attachment 241975 [details] [review] Fix height-for-width requests in GtkBin Unfortunately, this was not the silliest code on earth. Various GtkBin derived widget request various sizes for various reasons, an example of this is the space to draw a check mark in a check button, or the space required to draw the border relief of a GtkButton. These elements are inside the widget's allocation, the widget actually requires this space to draw on, so it cannot either be handled by a call to GtkWidgetClass.adjust_size_request() (in case you were thinking in might be possible to handle this in another way). The code in GtkBin is a one-size-fits-all implementation for GtkBin widgets, and it's important that the class methods are invoked *directly* instead of calling gtk_widget_get_preferred_height_for_width() and incorrectly caching those values or adding offsets which would render the code in GtkBin incorrect. If the code in GtkBin, which is a little scary to look at, is undesirable, then the alternative is to implement height-for-width/width-for-height requests manually in every single GtkBin subclass. Personally I think that the GtkBin solution is better than implementing it separately in every subclass.
That's a bug in GtkButton. GtkButton should implement get_preferred_width_for_height and get_preferred_height_for_width vfuncs that correctly respect all the weird button properties.
You're stepping around the issue here. GtkBin does this automatically for all subclasses. If you want to remove the code I put in GtkBin, I suggest you do so without breaking GtkButton, GtkCheckButton and GtkRadioButton, and any other GtkBin derived class *before* removing the code in place which works. Right now I have broken Check buttons which contain wrapping labels, because you decided to remove code which works, while neglecting to fix the issue. Note, before you can go ahead and remove my GtkBin code, you will need to write *separate* implementations of get_preferred_height_for_width()/ get_preferred_width_for_height for GtkButton _and_ GtkCheckButton (possibly GtkRadioButton needs a separate implementation too). I really don't mind that you prefer a different solution than GtkBin's automated implementation (which *works*), but I really don't appreciate that you guys neglectfully removed code that works, on the grounds that it's "The silliest code on earth", without either investigating the reason for which the code was there in the first place, or implementing a functional alternative. Please, please guys... don't go breaking things that work just because you don't like them.
Fixed in master and gtk-3-8 branches.
You committed the wrong fix. You meant to commit the one that implements hfw for buttons.
No I did not. GtkBin provides a less-than-perfect implementation that works for any given subclass of GtkBin with some exceptions. You removed that code, breaking height for width in classes such as GtkAlignment, GtkButton, GtkCheckButton and any other GtkBin derived widget. My code works for all of these implementations, and if you want to remove it, for whatever reason, then you should implement height-for-width implementations for every single GtkBin subclass that may require height-for-width implementations. Which means at least GtkAlignment, GtkButton and GtkCheckButton need separate custom implementations, which I expect you to write *before* removing the all-purpose implementation in GtkBin which works. This would still be a matter of taste though, fwiw the GtkBin implementation works fine for any GtkBin subclass that does not have dynamic content besides it's child widget (I did create custom implementations for GtkFrame and GtkExpander, since those bin widgets have additional dynamic content which needs to be calculated separately from the bin child). The GtkBin general purpose code was there intentionally so that a majority of GtkBin subclasses would only have to implement get_preferred_width() and get_preferred_height()... it does "more with less" and my preference would be to just keep it. Anyway, for now it's fixed and works properly again, if you remove it (again) then please do so without introducing regressions (and you can only do that for stock GTK+ widgets, removing the GtkBin code also potentially breaks any GtkBin subclasses in third party code, which is another good argument for keeping the trustworthy code in GtkBin).
*** Bug 698953 has been marked as a duplicate of this bug. ***
Reopening. Benjamin has broken it again. I reiterate, if you're going to throw away code on the grounds of your personal taste, do so without causing regressions. Suggesting that I should run around fixing code that you broke, for a matter of your own personal taste, is rude, insolent, and holding me hostage to your regression. I expect that you will take responsibility for your regressions.
Removing the GtkBin logic is pretty clearly an ABI break, guys.
(In reply to comment #9) > Removing the GtkBin logic is pretty clearly an ABI break, guys. That depends on your definition of ABI. And nowhere is it guaranteed what GTK will do if you write any GtkWidget subclass that is not CONSTANT_SIZE but doesn't implement get_height_for_width or get_width_for_height vfuncs. But as an exercise, you are free to try to come up with a description that describes how GtkBin did behave.
(In reply to comment #10) > (In reply to comment #9) > > Removing the GtkBin logic is pretty clearly an ABI break, guys. > > That depends on your definition of ABI. And nowhere is it guaranteed what GTK > will do if you write any GtkWidget subclass that is not CONSTANT_SIZE but > doesn't implement get_height_for_width or get_width_for_height vfuncs. You overlook the point, again. By regressing GtkBin, you've broken GtkAlignment, GtkButton, GtkCheckButton and possibly other widgets. I do not ask that you live forever with the GtkBin implementation if you find it so displeasing (while I think the GtkBin implementation is perfectly fine, and strictly speaking it would be an ABI break to remove it). But I do insist that if you wish to remove the GtkBin logic, you take responsibility for the regressions that it causes *first*, i.e. you cannot remove that code without first adding individual height-for-width implementations at least to all of GTK+'s GtkBin widgets, all of which currently rely on the code you wish to remove. > > But as an exercise, you are free to try to come up with a description that > describes how GtkBin did behave. That's pretty simple: GtkBin provides an automated height-for-width/width-for-height implementation for any subclass that does not contain any dynamic content besides it's child. Dynamic content here, refers to content who's size can be allocation contextual (so the implementation cannot be trusted for say, a GtkFrame or a GtkExpander, because those GtkBin widgets contain additional dynamic content besides the bin child).
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