GNOME Bugzilla – Bug 679168
st-box-layout: Always pass on for_width/height
Last modified: 2012-12-03 17:45:29 UTC
This is precluding, at least, having IconGrids with > 1 row inside vertical BoxLayouts. I'm not 100% sure it's the correct fix though. Jasper pointed me to https://github.com/clutter-project/mx/commit/df8cbce32cca3da3cfac4b62a980c7548418808f but says that this was probably before "Clutter had a request mode machinery". I don't see any ill effects running with it though.
Created attachment 217679 [details] [review] st-box-layout: Always pass on for_width/height We shouldn't be lying to our children.
Also see the remaining patch in bug 672543.
(In reply to comment #2) > Also see the remaining patch in bug 672543. Note that the patch there only has one of the two changes, because I *did* find regressions for the other one (I don't remember the exact details, but it was in some of the popup menus)
I also want to get Emmanuele's opinion: ClutterBoxLayout has the same issue: http://git.gnome.org/browse/clutter/tree/clutter/clutter-box-layout.c#n493 as both were stolen from Mx, so we should probably fix up both while we're at it.
ClutterBoxLayout was taken from GtkBox - the line in comment 4 is the Clutter-equivalent of the GTK+ idiomatic gtk_widget_get_preferred_width(), which lets the widget compute the size of the child without an height, e.g.: http://git.gnome.org/browse/gtk+/tree/gtk/gtkbox.c#n1041 the layout policy for GtkBox (and ClutterBoxLayout) is to request the sum of all widths and the maximum height of all the children if orientation==horizontal, and the sum of all heights and the maximum width of all the children if orientation==vertical. if anything ought to be changed is to remove the for_height/for_width and always pass -1, to make the code exactly like GtkBox.
(In reply to comment #5) > ClutterBoxLayout was taken from GtkBox - the line in comment 4 is the > Clutter-equivalent of the GTK+ idiomatic gtk_widget_get_preferred_width(), > which lets the widget compute the size of the child without an height, e.g.: > > http://git.gnome.org/browse/gtk+/tree/gtk/gtkbox.c#n1041 > > the layout policy for GtkBox (and ClutterBoxLayout) is to request the sum of > all widths and the maximum height of all the children if > orientation==horizontal, and the sum of all heights and the maximum width of > all the children if orientation==vertical. if anything ought to be changed is > to remove the for_height/for_width and always pass -1, to make the code exactly > like GtkBox. The issue, I believe, happens when you have nested StBoxLayouts (or ClutterBoxLayouts) of the same orientation -- if one is constrained, the other won't notice, and you'll get odd clipping when using a multiline layout or something.
Created attachment 217928 [details] [review] iconGrid: Handle preferred height requests for infinite widths Request enough height to fit all children in a single line instead of requesting 0. -- Ok, so it seems that the real bug is in IconGrid. This patch fixes at least an obvious wrong case but there are probably others.
(In reply to comment #7) > Ok, so it seems that the real bug is in IconGrid. This patch fixes at > least an obvious wrong case but there are probably others. That's not true. Florian found a case where this happened in a modal dialog, and other people have had the same happen in a BoxLayout.
er, other people have had the same happen in a popup menu.
I also want to point out that ClutterBoxLayout changed recently. I don't know if inheriting it into StBoxLayout will "fix" this issue or not.
So, Florian, if you can find a standalone test with either St.BoxLayout or Clutter.BoxLayout, I'd be really appreciated to investigation. I tried for a few hours a while ago, and couldn't make one up.
Review of attachment 217928 [details] [review]: Yes.
This patch has landed, right?
(In reply to comment #13) > This patch has landed, right? I don't think so. I think it isn't needed with the current code. It's only with the search results re-design that the issue becomes visible.
I still have this patch in my search re-layout branch; can we please get this in?
The iconGrid patch? Sure.
Okay, I pushed it to master now. Attachment 217928 [details] pushed as 29cb10f - iconGrid: Handle preferred height requests for infinite widths