Created attachment 248637 [details] [review]
actor: Minor cleanup

Just a drive-by cleanup, but didn't bother filing a separate bug ...

Created attachment 248638 [details] [review]
box-layout: Fix child offsets

Created attachment 248639 [details] [review]
bin-layout: Fix offsets

Created attachment 248641 [details] [review]
box-layout: Fix (legacy) expand/fill properties

Review of attachment 248637 [details] [review]:

looks good.

Review of attachment 248641 [details] [review]:

this is obviously correct.

Created attachment 248717 [details] [review]
table-layout: Fix default values for expand/fill child properties

According to their param spec, those properties should default to
FALSE, yet they are initialized to TRUE. Make the initialization
match the documented behavior, otherwise constructs like

  expand = clutter_actor_needs_expand () || meta->x_expand;

cannot be used with the recommended ClutterActor properties if
the row/column should not expand.

Review of attachment 248717 [details] [review]:

mmh, this seems to be breaking the interactive table layout test.

usually, when GParamSpec and initialization code do not match we fix the GParamSpec instead, as changing the initialization will break existing code that does not initialize those values. in this case it's a bit of a mess, with the old API and the new one.

since we have ClutterGridLayout I wonder if we shouldn't just deprecate ClutterTableLayout altogether...

the allocation passed to the ClutterLayoutManager::allocate() has been adjusted (when clutter_actor_allocate() is called on the actor) to take into account the margin set on the actor. we normalize that allocation to be pure [ width, height] because the actor is going to paint its children with allocation.x and allocation.y already placed into the modelview matrix initial transform, so adding the allocation's origin to the allocation of the children would, effectively, cause the children to have their origin shifted.

just so that I understand attachment 248638 [details] [review] and attachment 248639 [details] [review]: when you say "callers" you're implying that you're effectively overriding ClutterActor::allocate() on the actor and then calling clutter_layout_manager_allocate(), am I correct? so you're taking the allocation passed to the actor, fudging around with it, and then passing it to the layout manager's allocate() implementation.

I think I'll need some examples for this.

(In reply to comment #8)
> usually, when GParamSpec and initialization code do not match we fix the
> GParamSpec instead, as changing the initialization will break existing code
> that does not initialize those values.

Yeah, feel free to reject the patch - it's just ugly that it will require using the old properties over the new ones, but not a big deal.

(In reply to comment #9)
> just so that I understand attachment 248638 [details] [review] and attachment 248639 [details] [review]: when you say
> "callers" you're implying that you're effectively overriding
> ClutterActor::allocate() on the actor and then calling
> clutter_layout_manager_allocate(), am I correct?

Exactly, see https://git.gnome.org/browse/gnome-shell/tree/src/st/st-widget.c#n425. We adjust the allocation to take the container's CSS into account. I'm sure we can come up with a different approach to make LayoutManagers work for us (the most elegant probably to translate borders/padding into margins), but at least the documentation for clutter_actor_allocate_align_fill (which may still be used by layout managers provided by clutter) suggests that non-0 offsets are supported - in which case they really should be interpreted consistently.

Created attachment 249052 [details] [review]
table-layout: Fix default values for expand/fill child properties

Currently the default values according to their param spec don't
match the actually used defaults, so update the former to reflect
the actual behavior.