Created attachment 248642 [details] [review]
st: Make BoxLayout use a Clutter layout manager internally

With the BoxLayout containers in St and Mx and the ClutterBoxLayout
manager, we now have three more or less diverged implementations of
the same layout policy.

While removing StBoxLayout entirely in favor of ClutterLayoutManager
would be the fashionable thing to do, there are obvious drawbacks:

 - it is the only actor we have that implements the scrollable interface
 - it conveniently exposes its spacing property in CSS
 - last but not least, it is used all over the place

So do the next best thing and make our implementation use the
Clutter layout manager internally - that way, the change is
transparent to users, while we get to refer most of the tricky
bits to Clutter. win-win!

Created attachment 248643 [details] [review]
st: Remove StBoxLayoutChild property getter

Nothing reads those properties now, so just kill the getter.

Created attachment 248644 [details] [review]
st: Remove empty dispose/finalize methods from StBoxLayoutChild

If the only thing we do is chain up to the parent, there's no
point in not using the parent's implementation directly.

Created attachment 248645 [details] [review]
st: Translate StBoxLayoutChild properties to ClutterBoxChild

With StBoxLayout using the Clutter layout manager, it will now respect
actors' expand/align properties. However for the change to be transparent,
we need to support the existing child meta properties as well. Do
this by simply translating them to the corresponding ClutterBoxChild
properties.

Created attachment 248646 [details] [review]
st: Notify on BoxLayout property changes

We didn't do this before, but it might come in handy eventually,
so add change notifications.

Review of attachment 248642 [details] [review]:

We should potentially look at making StScrollView use a ClutterScrollActor in the future. Making that transparent as well would be cool.

This looks good to me.

Review of attachment 248644 [details] [review]:

I have never understood this either.

Review of attachment 248645 [details] [review]:

Hm, not sure if I like this. Fiddling with the actor is dirty, but I feel it's less dirty than using something which ClutterBoxLayout will flat out ignore in some circumstances.

Marking reviewed as a "eh". Could be swayed either way.

Review of attachment 248643 [details] [review]:

Uh, this doesn't seem very GObject-friendly to me. I use these properties all the time in looking-glass debugging.

Review of attachment 248646 [details] [review]:

::: src/st/st-box-layout.c
@@ +553,3 @@
+  const char *prop_name = g_param_spec_get_name (pspec);
+
+  if (g_object_class_find_property (G_OBJECT_GET_CLASS (self), prop_name))

Sort of nice that all the properties map out like that, though I suppose when things have been copy/pasted from Mx in two different ways, it's expected to happen.

Created attachment 248654 [details] [review]
st: Translate StBoxLayoutChild properties to ClutterBoxChild

Translate properties in property_get() as well.


(In reply to comment #8)
> Hm, not sure if I like this. Fiddling with the actor is dirty, but I feel it's
> less dirty than using something which ClutterBoxLayout will flat out ignore in
> some circumstances.

One alternative would be to leave the properties as-is and then call a sync_properties() function from set_property() and constructed() to sync the corresponding ClutterLayoutMeta properties, but I'm not sure it's worth the extra code (not to mention that it would miss Clutter->St translation, in case someone starts to mess with the ClutterLayoutMeta props directly).

Another approach would be to remove StBoxLayoutChild altogether and adjust the monkey patching in environment.js instead (basically: change "let meta = this.get_child_meta(actor);" to "let meta = this.get_child_meta(actor) || this.layout_manager.get_child_meta(this, actor);". However I don't see how we can deal with differing default values and different alignment types in a non-ugly way in that case.

Long-term we should encourage a transition to the standard x-expand, x-align etc. properties, but I don't see a good reason for enforcing it now by breaking code all over the place (I have a similar set for StTable which is a lot less used, so I'm more open to requiring people to adjust code there (up to removing the widget altogether and port to Clutter.TableLayout directly))

Review of attachment 248654 [details] [review]:

Assuming you've tested and it works fine, this is good.

This doesn't seem to apply to master.

Attachment 248642 [details] pushed as aa39475 - st: Make BoxLayout use a Clutter layout manager internally
Attachment 248644 [details] pushed as 741b204 - st: Remove empty dispose/finalize methods from StBoxLayoutChild
Attachment 248646 [details] pushed as 6a19b7c - st: Notify on BoxLayout property changes
Attachment 248654 [details] pushed as 144f7d6 - st: Translate StBoxLayoutChild properties to ClutterBoxChild