Created attachment 157052 [details] [review]
Add StGroup container

Currently if we want a stylable fixed-layout container, we use either
a Clutter.Group inside an St.Bin, or we abuse St.BoxLayout.

Comment on attachment 157052 [details] [review]
Add StGroup container

I think we do want this. However, St already has copies of most of clutter-group's methods, in st-private (eg, _st_container_add_actor), which are used by all the other container types and should be shared by StGroup too. It might make more sense at this point to create an StContainerBase class containing those methods, and make all the St container types be subclasses of it. (And maybe a protected st_container_get_children_list method that returns priv->children without copying it, for use by subclass methods like allocate, etc. Or else move priv->children to non-priv.)

I also don't think we need to worry about ClutterLayoutManagers in StGroup. Whether or not they're a good idea, they're contrary to everything else in St. (This might mean that it's easier to import the clutter-1.0 clutter-group than the clutter-1.2 one.)

It might also be useful to experiment with having StGroup flip its child allocations around for ST_TEXT_DIRECTION_RTL. Maybe it could have a property saying whether or not to do that, if in some cases it helped and in others it messed things up.

Created attachment 157366 [details] [review]
Move shared container methods from st-private to a common base class

Add StContainer, which implements the ClutterContainer interface based
on the container methods in st-private.

Created attachment 157367 [details] [review]
Port containers to StContainer

Make the existing containers subclass StContainer instead of using
the methods from st-private.

Created attachment 157368 [details] [review]
Remove container methods from StPrivate

All consumers now subclass StContainerBase, so the methods can be
removed.

Created attachment 157369 [details] [review]
Add StGroup container

Currently if we want a stylable fixed-layout container, we use either
a Clutter.Group inside an St.Bin, or we abuse St.BoxLayout.

(In reply to comment #2)
> It might also be useful to experiment with having StGroup flip its child
> allocations around for ST_TEXT_DIRECTION_RTL. Maybe it could have a property
> saying whether or not to do that, if in some cases it helped and in others it
> messed things up.

I did experiment with it, maybe I ported the wrong portion of the code to St.Group, but the result ranged from somewhat broken to completely unusable ... the problem is that being a fixed container, StGroup pretty much delegates the positioning to the caller, which is bound to create conflicts.

We should definitively keep RTL in mind though when refactoring the workspaces mess.

Comment on attachment 157366 [details] [review]
Move shared container methods from st-private to a common base class

First off, for committing I think it makes sense to squash the first three patches together.

>@@ -0,0 +1,244 @@
>+#include <st/st-container.h>

Copy the "file header" from st-widget.c: emacs modeline, copyright/license, and config.h include. (All of the "real" code in this file is just copied from clutter-group.c, so keeping the OpenedHand/Intel copyright is correct.)

>+G_DEFINE_TYPE_WITH_CODE (StContainer, st_container, ST_TYPE_WIDGET,
>+                         G_IMPLEMENT_INTERFACE (CLUTTER_TYPE_CONTAINER,
>+                                                clutter_container_iface_init));

Use G_DEFINE_ABSTRACT_TYPE_WITH_CODE() (which takes the same arguments), since StContainer doesn't have code for allocating and drawing its children, and so is only useful when subclassed.

>+GList **
>+st_container_get_children_list (StContainer *container)

I don't like having this return a GList**... (more on this in the review of the next patch)

>+  return &ST_CONTAINER (container)->priv->children;

"container" is already an StContainer. You don't need to cast it.

>+  /* Using g_list_foreach instead of iterating the list manually
>+     because it has better protection against the current node being
>+     removed. This will happen for example if someone calls
>+     clutter_container_foreach(container, clutter_actor_destroy) */

can you fix this comment to use the same style as the rest of the file?

    /* blah blah
     * blah
     */

>+  /* See comment in _st_container_raise() for this */

fix function name (no leading "_")

>+GList **st_container_get_children_list    (StContainer *container);

put a comment before this:

    /* Only to be used by subclasses of StContainer */

Comment on attachment 157367 [details] [review]
Port containers to StContainer

>+  children = st_container_get_children_list (ST_CONTAINER (actor));
>+  for (iter = *children; iter; iter = iter->next)

(So this is an example of a change that would be [marginally] simpler if st_container_get_children_list() returned a GList* instead of a GList**. I'm not going to comment on the remaining trivial differences though.)

>@@ -206,13 +207,13 @@ shell_generic_container_remove_all (ShellGenericContainer *self)
> {
>   /* copied from clutter_group_remove_all() */

given that StGroup then will also have this, and StBoxLayout/StOverflowBox also have it, we should just make st_container_remove_all() (and I guess st_container_destroy_all()), which would then simplify this and make _get_children_list() not need to return a **.

>@@ -1325,8 +1264,8 @@ st_box_layout_move_child (StBoxLayout  *self,
>-  priv->children = g_list_delete_link (priv->children, item);
>-  priv->children = g_list_insert (priv->children, actor, pos);
>+  *children = g_list_delete_link (*children, item);
>+  *children = g_list_insert (*children, actor, pos);

ok, this is one spot where I'm not sure what you'd do if _get_children_list() didn't return a **.

However, looking at this code also makes me realize that StBoxLayout can't just use st_container_sort_z_order(), because that would mess up the left-to-right order of the children. You should just override it with a method that does nothing for now.

Comment on attachment 157368 [details] [review]
Remove container methods from StPrivate

good, but as mentioned before, should be squashed with the previous two

Comment on attachment 157369 [details] [review]
Add StGroup container

>+st_group_real_paint (ClutterActor *actor)

Remove "real" from all of the method implementation names.

>+  children = st_container_get_children_list (ST_CONTAINER (actor));
>+  g_list_foreach (*children, (GFunc) clutter_actor_paint, NULL);

you could just use clutter_container_foreach here.

>+  /* Chain up so we get a bounding box pained (if we are reactive) */

typo ("painted"), and the clutter_container_foreach comment applies again.

>+  /* The size is defined as the distance from the origin to the right-hand
>+     edge of the rightmost actor */
>+  if (min_width)
>+    *min_width = min_right;
>+
>+  if (natural_width)
>+    *natural_width = natural_right;
>+}

You need to use st_theme_node_adjust_preferred_width() to add space for borders and padding. Likewise in st_group_get_preferred_height().

>+      clutter_actor_allocate_preferred_size (child, flags);

And this is also wrong, because you need to allocate relative to content_box, not box.

I was going to say "you can just copy what StBoxLayout does for fixed-allocation children", except apparently StBoxLayout uses clutter_actor_allocate_preferred_size, so it's wrong too. Of course, that means that existing code using Clutter.Group-inside-St.Bin will have coordinates in terms of content_box, and existing code using St.BoxLayout-with-fixed-positioning will have coordinates in terms of the full allocation, so regardless of which way St.Group works, half of the code will need to be adjusted...

(After some discussion on IRC...) OK, Owen and Colin agree that the allocation should be relative to get_content_box, so do that. Fixing StBoxLayout can happen later, since that will probably require adjusting the code that currently uses it.

>+st_group_remove_all (StGroup *group)

as discussed earlier, this should go into StContainer

>+st_group_get_n_children (StGroup *group)
>+st_group_get_nth_child (StGroup *group,

these seem like unnecessary optimizations that we don't need to copy from ClutterGroup. (If we really did need them, we'd probably want them at the StContainer [or even ClutterContainer] level.)

Created attachment 157514 [details] [review]
Move shared container methods from st-private to a common base class

(In reply to comment #9)
> ok, this is one spot where I'm not sure what you'd do if _get_children_list()
> didn't return a **.

It also happens to be the one spot which made me change the original function from GList* to GList** ... but seriously, I have no idea how to implement this inside StBoxLayout without "full" access to the list, so I moved the function into StContainer (which is a little odd, as it's only used for StBoxLayout ...)

I thought about waiting with squashing the patches, but as there is already a pretty clear separation by file it shouldn't make it harder to review.

Created attachment 157515 [details] [review]
Add StGroup container

(In reply to comment #11)
> And this is also wrong, because you need to allocate relative to content_box,
> not box.
> 
> I was going to say "you can just copy what StBoxLayout does for
> fixed-allocation children", except apparently StBoxLayout uses
> clutter_actor_allocate_preferred_size, so it's wrong too. Of course, that means
> that existing code using Clutter.Group-inside-St.Bin will have coordinates in
> terms of content_box, and existing code using
> St.BoxLayout-with-fixed-positioning will have coordinates in terms of the full
> allocation, so regardless of which way St.Group works, half of the code will
> need to be adjusted...
> 
> (After some discussion on IRC...) OK, Owen and Colin agree that the allocation
> should be relative to get_content_box, so do that. Fixing StBoxLayout can
> happen later, since that will probably require adjusting the code that
> currently uses it.

I'm not too sure about that - the Clutter.Group-inside-St.Bin case is not guaranteed to work either without code modifications; while Clutter.Group (does not handle the positions of children at all) and the existing St containers (do handle the positions of children entirely) are pretty clear, introducing a container which relies on outside code to position its children to then slightly modify their positions sounds a little odd ...

(In reply to comment #13)
> introducing a
> container which relies on outside code to position its children to then
> slightly modify their positions sounds a little odd ...

Hm... yeah, actually, because querying child.x after it's been layed out will return the *allocated* value (including offset for border and padding size). So this will cause a border+padding-sized jump at the start of tweener animations when tweener does (basically) "child.x = child.x + delta" and StGroup adds border+padding to that value again.

The problem with saying that StBoxLayout's way is right is that people have to take the borders/padding into account manually, and will generally forget to do so if the default theme doesn't have borders/padding there, and then if a user/theme modifies it, the code will end up positioning things wrong.

I think we could fix this by using clutter_actor_allocate_preferred_size() like before, but then playing with the actor's transformation matrix the same way StBoxLayout does to implement scrolling. (Essentially, when painting the children, we'd be "scrolled" so that 0,0 was inside the borders+padding rather than outside.) So you'd have to override apply_transform(), paint(), and pick() more or less like StBoxLayout does.

Although that would also be tricky, because code that tried to position something relative to "group.width" or "group.height" would work when there was no padding but not if there was...

Maybe St.Bin+Clutter.Group wasn't that bad an idea :-}

I think the point about x/y vs. width/height is important - if you have a Group container with a size that is determined by geometry negotiation the position of the children almost inevitably depends on the allocated size of the container - if you aren't taking the size of the container into account then the children might end up outside the edges of the container.

So, if you need to call st_theme_node_get_content_box() to get the size that you are going to fit the children into, using the returned X/Y as well as the returned width/height makes sense and probably is more expected than the reverse.

Now, this does cast some doubt on the exercise, since if you nest a Clutter.Group inside a St.Widget you don't have to worry about either issue.

There are other issues with mixing groups and geometry allocation - by the above hypothesis that the positions of the children depends on the size we should be positioning the children when the allocation changes, but we can't since that will trigger the clutter warning about queueing a resize in allocate. 

And I think the fundamental idea of having a single position for a child may be broken in the case where we want to mix computed position and animation - we should have the "positioned" location of the child and then also the "current" location of the child.

So, maybe we need to look a bit more carefully and characterize all the places where we are using Clutter.Group and try to characterize what we are doing there.

Places where we currently used both fixed positioning and CSS:

  - WorkspacesView.GenericWorkspacesView: (Clutter.Group inside St.Bin).
    Fixed positioning is needed to animate workspaces in/out of the view.
    CSS is used to set the inter-workspace spacing (which
    GenericWorkspacesView needs to read out of the theme node manually).

  - MessageTray.MessageTray: (St.BoxLayout with only fixed-position
    children). Fixed positioning is needed to animate the different tray
    pieces on/off the screen. CSS is used to set the tray height and draw a
    gradient background.

  - Overview.Overview: (St.BoxLayout with only fixed-position children).
    Fixed positioning is needed to animate the overview in/out. CSS is used
    to set background color.

(I may have missed others, but I don't think so.)

(There are also lots of places where we use Clutter.Group with no associated CSS, just to make it easier to treat a group of actors as a group. Eg, to move all the windows together when switching workspaces, or to show and hide a group of actors all at once. These wouldn't benefit from St.Group, regardless of how it worked.)

In all of these cases, it's fixed-positioning all the way up.

A simple solution that would make all of these cases happy would be to say that StGroup ignores borders and padding, but obeys other CSS...

Created attachment 160412 [details] [review]
Add StGroup container

(In reply to comment #16)
> A simple solution that would make all of these cases happy would be to say that
> StGroup ignores borders and padding, but obeys other CSS...

Mmmmh, okay...

Comment on attachment 160412 [details] [review]
Add StGroup container

(In reply to comment #16)
> A simple solution that would make all of these cases happy would be to say that
> StGroup ignores borders and padding, but obeys other CSS...

Mmmmh, to obey CSS rules like background-color, we need to paint the group itself, so st_group_paint() should chain up to the parent, right? Of course, this will draw the borders as well - so can we say that StGroup ignores padding and doesn't take borders into account when positioning children (with a big fat warning to not use borders with StGroup)?

Created attachment 160726 [details] [review]
Add StGroup container

Updated patch which
1) chains up to st_widget_paint
2) resets "border" and "padding" properties

Comment on attachment 160726 [details] [review]
Add StGroup container

hm... i thought i replied to this bug on friday, but i must have started writing it and then it got lost in a browser crash (flash == death on F13 x86_64 last week).

sorry. anyway, owen and i talked a bit, and decided that having it work like StBoxLayout does now (and like you did before) would be ok. So attachment 157369 [details] [review] plus the non-positioning-related fixes from comment 11.

We could perhaps add an "st_group_get_content_box()" method for use by people who did explicitly want to animate within the bounds of the borders and padding.

Comment on attachment 157514 [details] [review]
Move shared container methods from st-private to a common base class

>-  st_box_layout_move_child(self, actor, pos);
>+  st_container_move_child(ST_CONTAINER (self), actor, pos);

fix the spacing there (space before paren)

>+void    st_container_move_child           (StContainer  *container,
>+                                           ClutterActor *actor,
>+                                           int           pos);
>+
>+/* Only to be used by subclasses of StContainer */
>+GList *st_container_get_children_list     (StContainer *container);

move_child should be after that comment as well.

(In reply to comment #20)
> sorry. anyway, owen and i talked a bit, and decided that having it work like
> StBoxLayout does now (and like you did before) would be ok. So attachment
> 157369 [details] plus the non-positioning-related fixes from comment 11.


Mmmmh, if there's no chaining up in st_group_paint(), we won't be able to change those:

(In reply to comment #16)
>   - MessageTray.MessageTray: [..] CSS is used to [...] draw a
>     gradient background.
> 
>   - Overview.Overview: [...] CSS is used to set background color.

Comment on attachment 157514 [details] [review]
Move shared container methods from st-private to a common base class

Attachment 157514 [details] pushed as 54d11b6 - Move shared container methods from st-private to a common base class

(In reply to comment #22)
> Mmmmh, if there's no chaining up in st_group_paint(), we won't be able to
> change those:

oh, sorry, i thought that was connected to the "ignore borders" part somehow. yes, you're right, we need to chain up correctly there

Created attachment 160747 [details] [review]
Add StGroup container

Updated patch from attachment 157369 [details] [review].

Created attachment 160750 [details] [review]
[WIP] Use St.Group where appropriate

Of the use cases mentioned by Dan above, St.BoxLayout in MessageTray.MessageTray is still missing.

Comment on attachment 160747 [details] [review]
Add StGroup container

Attachment 160747 [details] pushed as 1e3bf0e - Add StGroup container

Created attachment 161067 [details] [review]
[StGroup] Respect CSS sizing in size requests

Currently the size of an StGroup depends exclusively on the group's
children - it should be possible to override this behaviour with
fixed values in the CSS.

We need this if we want to replace the StBoxLayout in MessageTray.MessageTray with an StGroup.

Created attachment 161068 [details] [review]
Use St.Group where appropriate

There are some places in the code where we use both fixed positioning
and CSS. Currently we use either a combination of ClutterGroup and StBin,
or we uses StBoxLayout with fixed positioning. Replace those with the new
StGroup container.

Replace all places mentioned by Dan in comment 16 with StGroup.

Comment on attachment 161067 [details] [review]
[StGroup] Respect CSS sizing in size requests

>+  node_width = st_theme_node_get_width (node);

maybe "css_width" (etc) instead of "node_width"?

>+  /* The width is usually determined by the children, unless
>+     an explicit size is specified */

The existing comments were "weird" because the file was mostly copied from clutter, but please fix the new/moved comments to use the style we use elsewhere ("*" at the start of each line, "*/" on a line by itself).

>+  if (node_width != -1)
>     {
>+      min_right = natural_right = node_width;

it's weird to start with a "width", then assign it to "right", then assign it back to "width" at the end. I think you can just rename min_right and natural_right to min_width and natural_width; it's clear that the point of the code is that the width is determined by the rightmost point.

basically the same comments apply to the height code.

I'm assuming that treating "width" as "min_width" if there's no min-width specified matches what's done elsewhere?

Created attachment 161081 [details] [review]
[StGroup] Respect CSS sizing in size requests

(In reply to comment #30)
> I'm assuming that treating "width" as "min_width" if there's no min-width
> specified matches what's done elsewhere?

Not really - in general, width is assumed to refer to the natural width; it seemed weird to me that the minimum width can be actually larger than the natural width that way ... changed the code to do basically what st_theme_node_adjust_width does (except for the border/padding adjustments).

I adressed the other comments as well, but I'm not sure if that still make sense - it now obscures the fact that only the setting of the out variables really changes.

(In reply to comment #31)
> I adressed the other comments as well, but I'm not sure if that still make
> sense - it now obscures the fact that only the setting of the out variables
> really changes.

you could split the renames into a separate commit if you wanted it to be clearer

Attachment 161068 [details] pushed as 03a0809 - Use St.Group where appropriate
Attachment 161081 [details] pushed as f0645d4 - [StGroup] Respect CSS sizing in size requests

(In reply to comment #32)
> you could split the renames into a separate commit if you wanted it to be
> clearer

Yeah - decided not to anyway ...