Created attachment 187407 [details] [review]
St: add StLayoutContainer

This bolts the Clutter LayoutManager machinery onto the back of the rest of the
StContainer stuff, allowing us to separate layout and containers, as well as
re-use common layout implementations.

Hmm...what's the high level motivation here?  You just think the code is cleaner?

The motivation was to use the existing layout managers in Clutter: http://docs.clutter-project.org/docs/clutter/unstable/ch04.html

I was particularly excited about BinLayout and FlowLayout.

It's hard to say whether or not this would be useful without seeing some code that uses it

(In reply to comment #4)
> It's hard to say whether or not this would be useful without seeing some code
> that uses it

I remember seeing a lot of uses of Shell.GenericContainer doing something that could be done a lot easier with one of the builtin layout managers. For instance, I believe iconGrid's BaseIcon is basically a simple BoxLayout.

I'll hack up something that replaces some of these usages with the builtin Clutter managers in a day or two.

(In reply to comment #5)
> I remember seeing a lot of uses of Shell.GenericContainer doing something that
> could be done a lot easier with one of the builtin layout managers. For
> instance, I believe iconGrid's BaseIcon is basically a simple BoxLayout.

No, it would use St.BoxLayout then - the generic container is used to make BaseIcon square, e.g. add left/right padding if the label is shown. Given that most build-in layout managers have a rough equivalent in St (Fixed->Group, Bin->Bin, Box->BoxLayout, Table->Table), I don't think there are lots of generic containers which can easily be replaced.
(It might be worth considering the use of the clutter layout managers in the container implementations though)

I finally remembered my original use while working on the Starburst: I wanted to overlay an enumerable count in the bottom-right. ClutterBinLayout has a screenshot that looks precisely what I wanted to do: http://docs.clutter-project.org/docs/clutter/unstable/ClutterBinLayout.html

Unfortunately, St.Bin only supports one child.

Ah - you are aware of Shell.Stack?

Another benefit: we don't have to play feature catch-up to Clutter.

Stare at bug #614356 for a little while.
Then go to http://docs.clutter-project.org/docs/clutter/unstable/ClutterBoxLayout.html#ClutterBoxLayout--homogeneous

Comment on attachment 187407 [details] [review]
St: add StLayoutContainer

Sharing layout implementations with upstream/mx (and getting new features for free that way) is a good argument. But...

>+  st_theme_node_get_content_box (theme_node, box, &content_box);
>+
>+  clutter_layout_manager_allocate (priv->manager,
>+                                   CLUTTER_CONTAINER (actor),
>+                                   &content_box, flags);

This won't work right; ClutterLayoutManager assumes that you are passing @actor's own allocation in to it, and so therefore it completely ignores the top/left coordinates (since they are assumed to be the position of @actor within its own parent), and so it will lay out the children inside the rectangle (0, 0, content_box.x2-content_box.x1, content_box.y2-content_box.y1), meaning it will overlap the border/padding on the top and left, and leave a corresponding gap on the bottom/right.

In bug 613907, while trying to figure out how to make StGroup work, we considered translating the coordinate system (so that 0,0 referred to the top left of the content box) before allocating the children, and then translating it back afterward. But we realized that would cause problems if the children looked at their coordinates later on (eg, when tweening).

However, maybe it could work if the coordinate system was *always* transformed that way... though if we were going to do that we might want to / have to do it at the StWidget level, and it would likely have further side effects I'm not thinking of...

(In reply to comment #10)
> (From update of attachment 187407 [details] [review])
> Sharing layout implementations with upstream/mx (and getting new features for
> free that way) is a good argument. But...
> 
> >+  st_theme_node_get_content_box (theme_node, box, &content_box);
> >+
> >+  clutter_layout_manager_allocate (priv->manager,
> >+                                   CLUTTER_CONTAINER (actor),
> >+                                   &content_box, flags);
> 
> This won't work right; ClutterLayoutManager assumes that you are passing
> @actor's own allocation in to it, and so therefore it completely ignores the
> top/left coordinates (since they are assumed to be the position of @actor
> within its own parent), and so it will lay out the children inside the
> rectangle (0, 0, content_box.x2-content_box.x1, content_box.y2-content_box.y1),
> meaning it will overlap the border/padding on the top and left, and leave a
> corresponding gap on the bottom/right.

Could that assumption be considered a bug in Clutter?

> In bug 613907, while trying to figure out how to make StGroup work, we
> considered translating the coordinate system (so that 0,0 referred to the top
> left of the content box) before allocating the children, and then translating
> it back afterward. But we realized that would cause problems if the children
> looked at their coordinates later on (eg, when tweening).

Realistically, would tweening inside of a container managed by a layout manager work anyway? If not, I could just offset the content_box by the padding values.

> However, maybe it could work if the coordinate system was *always* transformed
> that way... though if we were going to do that we might want to / have to do it
> at the StWidget level, and it would likely have further side effects I'm not
> thinking of...

(In reply to comment #11)
> Could that assumption be considered a bug in Clutter?

No, it's a feature. ClutterActor has no concept of borders/padding, so no built-in container would ever want the semantics that you want here, and if clutter_layout_manager_allocate() did work that way, then they'd have to construct a new ClutterActorBox to pass to it rather than just being able to reuse their own allocation_box (which may have arbitrary x1 and y1 coordinates).

It could perhaps be considered poor API; you could argue that clutter_layout_manager_allocate() should take a width and a height rather than an allocation_box, since that's all it actually uses it for.

> Realistically, would tweening inside of a container managed by a layout manager
> work anyway?

Yes, if the layout manager was ClutterFixedLayout. (And if we had StLayoutContainer, then StGroup could just become an StLayoutContainer that used ClutterFixedLayout, the same way ClutterGroup does.)

(In reply to comment #10)
> (From update of attachment 187407 [details] [review])
> Sharing layout implementations with upstream/mx (and getting new features for
> free that way) is a good argument. But...
> 
> >+  st_theme_node_get_content_box (theme_node, box, &content_box);
> >+
> >+  clutter_layout_manager_allocate (priv->manager,
> >+                                   CLUTTER_CONTAINER (actor),
> >+                                   &content_box, flags);
> 
> This won't work right; ClutterLayoutManager assumes that you are passing
> @actor's own allocation in to it,

no, this is incorrect. the box passed is generally the same passed to the allocate method in Clutter because we don't have padding or margin inside ClutterBox, but the Actor implementation using a LayoutManager can decide to pass whatever box it wishes to the layout manager.

the important bit is that the Actor's implementation uses the box passed to the allocate() method when chaining up, so that the code in ClutterActor can store the correct geometry.

(In reply to comment #13)
> > This won't work right; ClutterLayoutManager assumes that you are passing
> > @actor's own allocation in to it,
> 
> no, this is incorrect. the box passed is generally the same passed to the
> allocate method in Clutter because we don't have padding or margin inside
> ClutterBox, but the Actor implementation using a LayoutManager can decide to
> pass whatever box it wishes to the layout manager.

Maybe that was how you meant for it to have been implemented, but that's not what actually got written. ClutterBinLayout, ClutterBoxLayout, ClutterFixedLayout, ClutterFlowLayout, and ClutterTableLayout all ignore the x1 and y1 of the passed-in ClutterActorBox, and allocate within the range 0,0 to x2-x1,y2-y1.

And also, if the layout manager is supposed to obey the passed-in x1,y1, then you *can't* pass in your own allocation (since it may have an arbitrary x1,y1) and so ClutterBox and ClutterGroup (and any out-of-tree layout manager users that copied them) would be wrong.

(In reply to comment #10)
> However, maybe it could work if the coordinate system was *always* transformed
> that way... though if we were going to do that we might want to / have to do it
> at the StWidget level, and it would likely have further side effects I'm not
> thinking of...

So our potential hope here is to do something like the CSS box model, where 0, 0 indicates the top/left of the content box, and we'll translate back to Clutter coordinates when we paint our children?

... or should we just "fix" ClutterLayoutManager instead?

Created attachment 194645 [details] [review]
box: Munge the allocation passed to the layout manager

The actor is in charge of providing to the LayoutManager the available
allocation. ClutterBox should not just pass the box it got from its
parent: it should, instead, provide a normalized box, with an origin in
(0, 0) and the available size.

Created attachment 194646 [details] [review]
layout-managers: Take into account the allocations's origin

If an actor using a LayoutManager has attributes like margin or padding
then it'll have to shave them from the available allocation before
passing it to the LayoutManager::allocate() implementation. Layout
managers should, thus, not assume that the origin of the allocation is
in (0, 0), but that into account that the passed ActorBox might have a
different origin.

these two patches do not introduce regressions in Clutter, and codify what was the intended model for ClutterLayoutManager.

ClutterGroup/ClutterFixedLayout is defined to always be as big as its children, so it's not necessary to modify it.

Created attachment 194657 [details] [review]
St: add StLayoutContainer

This bolts the Clutter LayoutManager machinery onto the back of the rest of the
StContainer stuff, allowing us to separate layout and containers, as well as
re-use common layout implementations.



Thanks, ebassi! The patches do work, and they don't break the Shell too badly.

Now, I have to hack a bit in here: to get the nice Container.add(), I had to
make get_child_meta work, so that's done. Unfortunately, ClutterContainer
requries that I set a child_meta_type, but I can only do that after I have a
LayoutManager set. As an additional hiccup, the method to get the
child_meta_type on a LayoutManager is private.

ClutterBox has the same problem, by the way, so you should look at fixing that
and I can do the same here.

I don't know how badly the other three iface methods are for proper working.
The included test does work, though!

Comment on attachment 194657 [details] [review]
St: add StLayoutContainer

This is incorrect.

Created attachment 194661 [details] [review]
St: add StLayoutContainer

This bolts the Clutter LayoutManager machinery onto the back of the rest of the
StContainer stuff, allowing us to separate layout and containers, as well as
re-use common layout implementations.



OK, this is strange. Can anyone else verify that with ebassi's patches and this
patch, that running the test gives you a strange allocation, where x1 >= x2?

Additionally, the child layout seems to be independent of the border-width, and
I'm not sure how to make FlowLayout to flow onto other lines.

Consider this a WIP for now.

Review of attachment 194661 [details] [review]:

StLayoutContainer looks okay to me.

::: js/ui/environment.js
@@ +61,3 @@
+    // because of how layout managers work.
+    St.LayoutContainer.prototype.get_child_meta = function(actor) {
+        return this.layout.get_child_meta(this, actor);

this is not correct.

get_child_meta() on a Container is not the equivalent of get_child_meta() on a LayoutManager.

::: src/st/st-layout-container.c
@@ +17,3 @@
+
+/* Code largely based on clutter-box.c
+ * Written by Emanuelle Bassi, originally licensed under LGPL 2.1.

my name is "Emmanuele", not "Emanuelle". ;-)

@@ +189,3 @@
+
+  theme_node = st_widget_get_theme_node (ST_WIDGET (actor));
+  st_theme_node_get_content_box (theme_node, box, &content_box);

you should probably make sure that the size of the content box is not 0, and skip the LayoutManager->allocate() pass if it is.

> OK, this is strange. Can anyone else verify that with ebassi's patches and this
> patch, that running the test gives you a strange allocation, where x1 >= x2?

you're twiddling the content area, so you'll have to make sure that the area is valid (e.g. is has a positive size) after changing it.

> Additionally, the child layout seems to be independent of the border-width, and
> I'm not sure how to make FlowLayout to flow onto other lines.

you need to set an explicit size to get it to reflow in the corresponding direction.

Comment on attachment 194645 [details] [review]
box: Munge the allocation passed to the layout manager

Emmanuele committed these to clutter master

Created attachment 195504 [details] [review]
clutter-flow-layout: fix non-0,0 offset allocation

(this is what caused the warnings: it was using the offset to compute
the x1 of the first child, but not the x2)

Created attachment 195505 [details] [review]
run-js-test: fix

Needs to explicitly initialize the ShellGlobal now

Comment on attachment 194661 [details] [review]
St: add StLayoutContainer

>+    // StLayoutContainer needs another monkey patch
>+    // because of how layout managers work.
>+    St.LayoutContainer.prototype.get_child_meta = function(actor) {
>+        return this.layout.get_child_meta(this, actor);
>+    };

As Emmanuele noted, this is wrong, it just happens to make our add() method do the right thing.

I'd say _patchContainerClass() should check if the class has a "layout-manager" property, and if so, it should patch it with alternate add() method that uses the layout child meta rather than the container child meta.

>+st_layout_container_paint (ClutterActor  *actor)
>+st_layout_container_pick (ClutterActor        *actor,
>+st_layout_container_get_paint_volume (ClutterActor *actor,

I don't know why we never implemented these at the StContainer level. StBoxLayout needs its own implementations because of scrolling, but everything else does it the same way. So we should fix that. (As a separate patch, before this one.)

>+  pspec = g_param_spec_object ("layout",

that should be "layout-manager" (which must mean that you're using the wrong name in the JS tests right now)

Review of attachment 195505 [details] [review]:

Makes sense.

(In reply to comment #25)
> Created an attachment (id=195504) [details] [review]
> clutter-flow-layout: fix non-0,0 offset allocation
> 
> (this is what caused the warnings: it was using the offset to compute
> the x1 of the first child, but not the x2)

Fixes the problem for me.

Created attachment 195534 [details] [review]
tests: Allow other arguments to be passed to the test runner

Created attachment 195535 [details] [review]
StContainer: Implement standard pick and paint methods

Instead of each subclass have slightly different implementations of the same
fundamental pick/paint methods, implement one in StContainer and junk the rest.

Created attachment 195536 [details] [review]
St: add StLayoutContainer

This bolts the Clutter LayoutManager machinery onto the back of the rest of the
StContainer stuff, allowing us to separate layout and containers, as well as
re-use common layout implementations.



OK, this should fix most of the review comments

Review of attachment 195504 [details] [review]:

looks good

Created attachment 195748 [details] [review]
StContainer: Implement standard pick and paint methods

Instead of each subclass have slightly different implementations of the same
fundamental pick/paint methods, implement standard vfuncs for subclasses to
use. We can't just set the vfuncs on StContainer directly: if subclasses
like BoxLayout want to override these methods and chain up, it will result
in double painting.



Just add a better commit message.

Created attachment 195749 [details] [review]
St: add StLayoutContainer

This bolts the Clutter LayoutManager machinery onto the back of the rest of the
StContainer stuff, allowing us to separate layout and containers, as well as
re-use common layout implementations.



Sorry, Emmanuele.

Created attachment 195810 [details] [review]
St: add StLayoutContainer

This bolts the Clutter LayoutManager machinery onto the back of the rest of the
StContainer stuff, allowing us to separate layout and containers, as well as
re-use common layout implementations.



Add quick check to test if we need to allocate or not.

Created attachment 198367 [details] [review]
St: add StLayoutContainer

This bolts the Clutter LayoutManager machinery onto the back of the rest of the
StContainer stuff, allowing us to separate layout and containers, as well as
re-use common layout implementations.



Rebase, fix some annotations and whitespace issues.

Comment on attachment 195748 [details] [review]
StContainer: Implement standard pick and paint methods

looks good, but you should do get_paint_volume as well

Comment on attachment 198367 [details] [review]
St: add StLayoutContainer

> 	st/st-im-text.h				\
>+	st/st-layout-container.h	\
> 	st/st-label.h				\

> 	st/st-label.c				\
>+	st/st-layout-container.c	\
> 	st/st-overflow-box.c			\

align the "\"s

>+ * Written by Emanuelle Bassi, originally licensed under LGPL 2.1.

Emmanuele



what happened to the JS bits?

Created attachment 198880 [details] [review]
st-container: Implement standard pick and paint methods

Instead of each subclass have slightly different implementations of the same
fundamental pick/paint methods, implement standard vfuncs for subclasses to
use. We can't just set the vfuncs on StContainer directly: if subclasses
like BoxLayout want to override these methods and chain up, it will result
in double painting.



This better?

Created attachment 198881 [details] [review]
St: add StLayoutContainer

This bolts the Clutter LayoutManager machinery onto the back of the rest of the
StContainer stuff, allowing us to separate layout and containers, as well as
re-use common layout implementations.



--

Add back the JS bits, fix typo...

As mentioned on IRC, this has a bit of a problem with defining properties like
"col-spacing" and such on layout managers through CSS. We have a few options:

1. Hardcode properties we want to filter for each layout-manager type.

2. Iterate through the propspecs and look for an appropriate CSS property.
   Prepend "-layout" so it becomes "-layout-col-spacing" to prevent likely
   name collisions.

Any opinions?

Comment on attachment 195534 [details] [review]
tests: Allow other arguments to be passed to the test runner

does what it says, but maybe it would be better to use the "--" convention instead? (what if you want to pass -v or -g to run-js-test?)

Comment on attachment 198880 [details] [review]
st-container: Implement standard pick and paint methods

hm... I didn't realize when I commented on the last patch that StContainer was already implementing get_paint_volume. Anyway, I meant that it should be done in the same way that paint() and pick() are, with each subclass having to opt in specifically.

In particular... StBoxLayout clips its children to its allocation when it is scrollable, so it doesn't to call StContainer's get_paint_box() implementation in that case. (Which it does currently, which means that it will sometimes be repainted when it doesn't need to be.)

Comment on attachment 198881 [details] [review]
St: add StLayoutContainer

this is still missing the environment.js changes

Created attachment 199967 [details] [review]
tests: Allow other arguments to be passed to the test runner

Somebody with better bash-fu than I could probably find a different route.

Created attachment 199968 [details] [review]
st-container: Implement standard pick and paint methods

Instead of each subclass have slightly different implementations of the same
fundamental pick/paint methods, implement standard vfuncs for subclasses to
use. We can't just set the vfuncs on StContainer directly: if subclasses
like BoxLayout want to override these methods and chain up, it will result
in double painting.



Gotcha.

Created attachment 199969 [details] [review]
St: add StLayoutContainer

This bolts the Clutter LayoutManager machinery onto the back of the rest of the
StContainer stuff, allowing us to separate layout and containers, as well as
re-use common layout implementations.



I have no idea how those disappeared.

Created attachment 200028 [details] [review]
st-container: Implement standard pick and paint methods

Instead of each subclass have slightly different implementations of the same
fundamental pick/paint methods, implement standard vfuncs for subclasses to
use. We can't just set the vfuncs on StContainer directly: if subclasses
like BoxLayout want to override these methods and chain up, it will result
in double painting.



--

Add chains for ShellStack and ShellGenericContainer, as they're St containers too.

Make sure to psuedo-chain up in StBoxLayout in the non-scrollable case too.

Since the first clutter apocalypse, this isn't useful anymore.