Created attachment 153722 [details] [review]
[St] Make allocation handling more consistent

In StBin, StBoxLayout, and StTable, if a child has a potential
allocation that is larger than its preferred size, we give it its
preferred size instead. However, the corresponding
get_preferred_height/width methods were not making the same
assumption, which meant that if we had more width than the widget
wanted, we would allocate it its preferred width, but with the height
that corresponded to the larger width.

Fix this by defining new helpers _st_actor_get_preferred_width() and
_st_actor_get_preferred_height() and using them everywhere. Also, make
StBin and StTable use _st_allocate_fill() rather than having
nearly-identical duplicate copies of the code.

Review of attachment 153722 [details] [review]:

Generally looks like a very good fix that reduces the amount of duplicated code.

I think the way you handle things in _st_actor_get_preferred_height/width is going to cause significant inefficiency. If we mix calls to get_preferred_height() with different values of for_width, then we lose caching and suddenly things are going to slow way down. I think the basic assumption here that we are making is that we either:

 - Call get_preferred_width() with a for_height of -1 and then call get_preferred_height() with the width we will allocate at

Or:

 - Call get_preferred_height() with a for_width of -1 and then call get_preferred_width() with the width we will allocate at

So, in both functions, you should actually use separate code paths when the for_XXX is -1. 

 - if for_XXX is -1, just call the underlying function directly
 - if for_XXX has a different value, then it will be free (because of caching to) to call get_preferred_XXX() passing in a for_YYY of -1, and we should use that to constrain for_XXX based on natural_XXX.

Don't _st_actor_get_preferred_XXX() need x_fill/y_fill arguments to match _st_allocate_fill()? If we are filling, then the constraint to the natural size shouldn't happen.

Do we have sufficient tests in test/interactive so that we are testing the different code paths? (I'm not really sure how to test width for height - do we have any actors that exhibit interesting width for height behaviors?)

Created attachment 153844 [details] [review]
[StTable] fix x-align/y-align properties to be StAlign, not double

This puts it in sync with StBin and StBoxLayout

(and fixes a bug in the previous patch, which was mistakenly passing
double-valued align values from StTable to _st_allocate_fill)

Created attachment 153845 [details] [review]
[St] Make allocation handling more consistent

fixes _st_actor_get_preferred_width/height as recommended above.

i considered making both methods take both x_fill and y_fill args, to make
it less confusing that you have to pass x_fill to get_height and y_fill to
get_width. But the methods already have a mix of vertical and horizontal
args, so I eventually decided it wasn't that confusing.

(In reply to comment #2)
> Do we have sufficient tests in test/interactive so that we are testing the
> different code paths?

Clearly not or we would have run into this problem before. :)

And to that extent, this patch at least doesn't make anything worse; all the parts of it that we're actually currently using still work, and the parts of it that we aren't using might not work, but then again, they might not have worked before either...

Review of attachment 153845 [details] [review]:

I think the handling of the mixed height-for-width and width-for-height case is suspect, and a few style comments, otherwise looks good to commit.

::: src/shell-slicer.c
@@ +101,1 @@
   double x_align, y_align;

Trivial: Might read better if it was double x_align_factor, y_align_factor rather than the awkward _enum suffix.

::: src/st/st-box-layout.c
@@ +545,3 @@
+        {
+          clutter_container_child_get ((ClutterContainer*) self, child,
+                                       "y-fill", &child_fill,

I think this would be clearer with the _st_actor_get_preferred_width() calls duplicated into a is_vertical and !is_vertical case, that could then merge with the next if () block too.

::: src/st/st-private.c
@@ +65,3 @@
+
+      g_object_get (G_OBJECT (actor), "request-mode", &mode, NULL);
+      if (mode == CLUTTER_REQUEST_HEIGHT_FOR_WIDTH)

Hmmm, some question in my mind whether it makes sense to check the mode - we know at this point that we are doing height-for-width allocation of the child. So, the preference of the child doesn't really make any difference - it isn't going to affect the order we get-the-width, get-the-height then allocate. And in fact, whether we clamp to the natural width or not, we'll still mismatch what happens in _st_allocate_fill.

I think the most-correct thing in the mismatch case - when we have, say, a StLabel in a horizontal box layout is:

 _st_actor_get_preferred_height(-1)
   min_width, natural_width = child.actor_get_preferred_width(-1)
   return child.get_preferred_height(natural_width);

 _st_actor_get_preferred_width(for_height)
   child.actor_get_preferred_width(-1)

Actually, I don't think StBoxLayout does width-for-height negotiation, eve n when horizontally - and AFAIK, we have no HEIGHT_FOR_WIDTH actors, so this is somewhat academic. But it may be simpler to just code the above than to write down a big fixme.

Review of attachment 153844 [details] [review]:

Looks good

(In reply to comment #6)
> Hmmm, some question in my mind whether it makes sense to check the mode - we
> know at this point that we are doing height-for-width allocation of the child.
> So, the preference of the child doesn't really make any difference.

We need to care about the child's preference because _st_allocate_fill() does. If the child is HEIGHT_FOR_WIDTH, then _st_allocate_fill() is never going to give it more than its natural width, so _st_actor_get_preferred_height() has to behave as though it's never called with a for_width greater than the natural width (but _st_actor_get_preferred_height() has to just fall through to clutter, and vice-versa for WIDTH_FOR_HEIGHT), whether the parent is doing height-for-width or width-for-height.

Created attachment 153876 [details] [review]
[St] Make allocation handling more consistent

updated shell_slicer_paint_child() and st-box-layout.c:get_content_preferred_width per suggestions

(In reply to comment #8)
> (In reply to comment #6)
> > Hmmm, some question in my mind whether it makes sense to check the mode - we
> > know at this point that we are doing height-for-width allocation of the child.
> > So, the preference of the child doesn't really make any difference.
> 
> We need to care about the child's preference because _st_allocate_fill() does.

The first part of my comment was a little stale. I did figure out why that part - that _st_allocate_fill() did different things, but it still wasn't make sense to me you were actually doing.

Say we have a Chinese vertical-text wrapping label in a vertical box.

 - We ask it "how wide do you want to be" - it probably gives back a single column of width for both minimum and preferred width
 
 - We compute a width for the vbox based on the label and the natural widths of all the other containers in the box.

 - We say "OK, if you are this wide, how tall do you want to be". It ignores the passed in width, and gives the height of the text laid out in a single column.

 - In st_allocate_fill, we ask it "how tall do you want to be", it gives the single column height.

   Then we ask "if you are this tall, how wide do you want to be" - it says a single column wide.

   We center the single column in the box.

This has two problems: first the layout is not very good. There's nothing you can really do about the bad layout without implementing "reverse engineered wrapping" in the label - wrapping toward a target height. Which is hard. (And may not look good. A very narrow wrap height might be quite hard to read.)

But the second problem - what I consider to be a pretty serious problem - is that we've broken requisition catching since in the above sequence we've called both get_preferred_height() and get_preferred_width() with different values of for_X. And text layout is expensive, so breaking requisition caching is a bad thing.

To me, _st_allocate_fill() needs to exactly identical to what happened above. Which means that it does actually need to do different things depending on whether the container is width for height or height for width - there's not one-size-fits-all code.

But that being said, I think what you are doing in get_preferred_X() is probably right anyways - not because it corresponds to what _st_allocate_fill() is doing - because that isn't right anyways - but because we really shouldn't trust get_preferred_width(-1) on a width-for-height child. (It doesn't make any difference for the simple Chinese label, but if we trusted the single line natural width we'd break a hypothetical label that could wrap towards a target width.) The natural width of a width-for-height container isn't the *maximum* useful width, it's rather the width at the maximum useful height.

owen clarified on irc that his objection is now "_st_allocate_fill is already
broken", and so the patch here is ok

Attachment 153844 [details] pushed as f52744c - [StTable] fix x-align/y-align properties to be StAlign, not double
Attachment 153876 [details] pushed as 5331d3e - [St] Make allocation handling more consistent