Created attachment 207537 [details] [review]
After some prodding from drago01, I'm attaching the patches on the bug. Let's

hope I don't kill the Bugzilla server in the process... welp.

--

st: Fix formatting

Created attachment 207538 [details] [review]
st: Remove st-overflow-box

It's unused, and has been for some time now.

Created attachment 207539 [details] [review]
test-theme: Don't use deprecated API

Created attachment 207540 [details] [review]
test-recorder: Don't use deprecated API

Created attachment 207541 [details] [review]
st-texture-cache: Use ClutterActor, not ClutterGroup

ClutterGroup is deprecated, and since ClutterActor is concrete, we
can use that now instead.

Created attachment 207542 [details] [review]
st-scroll-bar: Use clutter_actor_destroy in dispose

Created attachment 207543 [details] [review]
st-scroll-bar: Set the handle as a child of the bar, not the trough

The handle was a child of the trough, but it was allocated and painted
like it was a child of the bar. This will wreak havoc when we port over
to the new Clutter API, so let's just make it a child of the bar.

Created attachment 207544 [details] [review]
st-scroll-bar: Remove stepper buttons

This was a feature that was never used by the Shell.

Created attachment 207545 [details] [review]
st-scroll-bar: Clean up get_preferred_width/height

With the steppers gone, we can remove this macro madness

Created attachment 207546 [details] [review]
st: Don't use deprecated API

clutter_actor_set_parent and clutter_actor_unparent are both
deprecated, and come from a time before a well-thought API
was introduced.

Created attachment 207547 [details] [review]
st: Simplify paint_volume handling

Use a new convenience method, clutter_paint_volume_union_box, to grab our
paint box. Additionally, because the paint box is relative to the actor's
modelview (its allocation in most cases), make st_theme_node_get_paint_box
return something relative to the actor box too.

Created attachment 207548 [details] [review]
st-widget: Make into a concrete class

ClutterActor is concrete, so StWidget should be too.

Created attachment 207549 [details] [review]
st-widget: Don't use deprecated APIs

clutter_container_foreach is deprecated

Created attachment 207550 [details] [review]
st-widget: Copy get_focus_chain from StContainer

This is a prerequisite to getting rid of StContainer

Created attachment 207551 [details] [review]
st-widget: Keep track of first/last children by property notifiers

Created attachment 207552 [details] [review]
st-widget: Add st_widget_paint_background

Some actors do dirty things like chaining up to their parent's
parent in order to paint the CSS elements but skip painting
the children. Let's add a new public st_widget_paint_background
method intended to help these vfunc overrides.

Created attachment 207553 [details] [review]
st-widget: Implement a proper get_preferred_width/height

Now that StWidget is concrete and instantiable, we need to do something
other than return an adjusted 0 for width and height. Just chain up
to ClutterActor's default implementation, which uses the layout manager.

Created attachment 207554 [details] [review]
st-widget: Implement a proper allocate

If we want to replace ClutterGroup/StGroup, we need to allocate properly.
We can't rely on the default implementation here because we need to adjust
for margin/padding.

Created attachment 207555 [details] [review]
st-container: Remove custom ClutterContainer implementation

This is a quick prerequisite to getting rid of StContainer for real.
We simply port StContainer to be a quick wrapper on top of the concrete
ClutterContainer implementation in ClutterActor to make the transition
hopefully seamless and easier to understand.

Created attachment 207556 [details] [review]
st-box-layout: Remove insert_actor/insert_before

Now that 'insert_child_at_index' and 'insert_child_below' exist
on ClutterActor, these aren't necessary.

Created attachment 207557 [details] [review]
shell, st: Use clutter_actor_set_allocation rather than chaining up

Since we now have a fixed layout manager by default in StWidget which
always runs, any subclasses of StWidget which want custom allocation
semantics should use clutter_actor_set_allocation rather than chaining
up.

Review of attachment 207537 [details] [review]:

LGTM.

Created attachment 207558 [details] [review]
st: Remove custom paint/pick implementations

ClutterActor paints all children by default.

Created attachment 207559 [details] [review]
st: Remove st-group

Now that ClutterActor/StWidget is concrete, we don't need it.

Created attachment 207560 [details] [review]
st: Remove st-container

Now that ClutterActor is a container, this isn't necessary.

Created attachment 207561 [details] [review]
st: Remove st-tooltip

StTooltip has been plagued by lots of issues, and we recently ditched
it in the dash. Remove it for good.

Created attachment 207562 [details] [review]
st: Remove custom text direction stuff

Clutter has its own built-in system for managing text directions, like GTK+.
Convert over to use this.

Created attachment 207563 [details] [review]
st-private: Remove _st_allocate_fill

The very similar clutter_actor_allocate_align_fill is close enough
that this is just needless duplication. Additionally, allocate_fill
already inverts the align if the text direction is RTL, so we don't
need to do that here.

Review of attachment 207538 [details] [review]:

You need to kill the docs as well (remove from st-docs.sgml.in)

Review of attachment 207539 [details] [review]:

LGTM.

Review of attachment 207547 [details] [review]:

This one seems to break st-scroll-view-fade

Review of attachment 207540 [details] [review]:

Looks good.

Attachment 207539 [details] pushed as ebe72e1 - test-theme: Don't use deprecated API
Attachment 207540 [details] pushed as 570a029 - test-recorder: Don't use deprecated API

Created attachment 207578 [details] [review]
st: Remove st-overflow-box

It's unused, and has been for some time now.

Created attachment 207579 [details] [review]
st-scroll-bar: Remove stepper buttons

This was a feature that was never used by the Shell.



I forgot to remove a few references to the fw/bw buttons in the custom paint handler (rebase issue, as usual)

Created attachment 207580 [details] [review]
st: Don't use deprecated API

clutter_actor_set_parent and clutter_actor_unparent are both
deprecated, and come from a time before a well-thought API
was introduced.


Remove a pair of unnecessary refs/unrefs.

Review of attachment 207578 [details] [review]:

Fine to kill.

Review of attachment 207541 [details] [review]:

Looks good.

Created attachment 207582 [details] [review]
st-scroll-view: Remove unnecessary VISIBLE checks

clutter_actor_paint already checks for VISIBLE before painting



Hm. For some reason I lost this in my rebases. This is supposed to go before the "st: Don't use deprecated API" patch in the stack.

Review of attachment 207542 [details] [review]:

Looks good.

Review of attachment 207543 [details] [review]:

OK.

Created attachment 207587 [details] [review]
st-texture-cache: Use ClutterActor, not ClutterGroup

ClutterGroup is deprecated, and since ClutterActor is concrete, we
can use that now instead.



Whoops, I had the panel.js change in the wrong commit.

Review of attachment 207545 [details] [review]:

OK.

Review of attachment 207562 [details] [review]:

Actually I hate the "Clutter.get_default_text_direction() == Clutter.TextDirection.RTL" stuff all over the place ..
Can't we have some "is_rtl()" method?

Review of attachment 207579 [details] [review]:

Looks good.

Attachment 207542 [details] pushed as 44686ba - st-scroll-bar: Use clutter_actor_destroy in dispose
Attachment 207543 [details] pushed as bed5068 - st-scroll-bar: Set the handle as a child of the bar, not the trough
Attachment 207545 [details] pushed as 6528f83 - st-scroll-bar: Clean up get_preferred_width/height
Attachment 207578 [details] pushed as ca575ef - st: Remove st-overflow-box
Attachment 207579 [details] pushed as 88eb246 - st-scroll-bar: Remove stepper buttons

Review of attachment 207561 [details] [review]:

Looks good, some unrelated changes got in though. Not sure whether it is worth to split them out.

::: src/st/st-widget.c
@@ +1815,3 @@
     g_string_append_printf (desc, " \"%s\"", name);
 
+  if (!append_actor_text (desc, actor))

Unrelated to the st-tooltip removal?

@@ +1821,2 @@
       /* Do a limited search of @actor's children looking for a label */
+      children = clutter_actor_get_children (actor);

This too.

Review of attachment 207587 [details] [review]:

The commit message makes no sense. ClutterActor has always been concrete.

::: js/ui/panel.js
@@ +122,3 @@
+    },
+
+    _onVisibleNotify: function() {

Err ... all this have nothing to do with the cache?

Review of attachment 207587 [details] [review]:

By "concrete" I mean "non-abstract". I'm quite sure that's a recent change in ClutterActor.

The cleanup in panel.js is because ClutterActor no longer has get_nth_child, but I didn't like the code in there anyway and decided to rewrite it.

Review of attachment 207561 [details] [review]:

I originally had this near the start of the patch stack, so the random unrelated changes are probably from random rebases picked up along the way. I forget why I moved it to the end... I'll probably move it back near the start again, just so we can land this giant cleanup earlier.

Created attachment 207623 [details] [review]
st-texture-cache: Use ClutterActor, not ClutterGroup

ClutterGroup is deprecated, and since ClutterActor is concrete, we
can use that now instead.



OK, here's a new patch stack, rebased.

This patch is the same, reattached for completeness.

Created attachment 207624 [details] [review]
st-scroll-view: Remove unnecessary VISIBLE checks

clutter_actor_paint already checks for VISIBLE before painting



This patch is the same as well.

Created attachment 207625 [details] [review]
st: Don't use deprecated API

clutter_actor_set_parent and clutter_actor_unparent are both
deprecated, and come from a time before a well-thought API
was introduced.

Created attachment 207626 [details] [review]
st: Remove st-tooltip

StTooltip has been plagued by lots of issues, and we recently ditched
it in the dash. Remove it for good.



This patch is now much closer to the start of the stack.

Created attachment 207627 [details] [review]
st-widget: Make into a concrete class

ClutterActor is concrete, so StWidget should be too.



The same one line patch, again.

Created attachment 207628 [details] [review]
st-widget: Don't use deprecated APIs

clutter_container_foreach is deprecated

Created attachment 207629 [details] [review]
st-widget: Don't explicitly check for ClutterContainer inheritance

Since all ClutterActors implement the ClutterContainer interface, there
isn't a case where this check could fail.

Created attachment 207630 [details] [review]
st-widget: Copy get_focus_chain and navigate_focus from StContainer

This is a prerequisite to getting rid of StContainer

Created attachment 207631 [details] [review]
st-widget: Keep track of first/last children by property notifiers

Created attachment 207632 [details] [review]
st-widget: Add a proper paint, add st_widget_paint_background

Since we want to paint children by default in StWidget, we need to
provide a way for custom subclasses to paint their CSS backgrounds
without painting children... introducing st_widget_paint_background.

This also removes the hacky things that some subclasses of StBin did
to prevent their one child to be painted by StBin.

Created attachment 207633 [details] [review]
st-widget: Implement a proper get_preferred_width/height

Now that StWidget is concrete and instantiable, we need to do something
other than return an adjusted 0 for width and height. Just chain up
to ClutterActor's default implementation, which uses the layout manager.

Created attachment 207634 [details] [review]
st-widget: Implement a proper allocate

Created attachment 207635 [details] [review]
shell, st: Use clutter_actor_set_allocation rather than chaining up

Since we now have a fixed layout manager by default in StWidget which
always runs, any subclasses of StWidget which want custom allocation
semantics should use clutter_actor_set_allocation rather than chaining
up.

Created attachment 207636 [details] [review]
shell, st: Remove custom paint/pick implementations

ClutterActor paints all children by default.



This is the new milestone patch that I want to hit for today.
Nothing should break with any of these previous patches, it's
the ones after this that I'm currently looking at for breaks.

Created attachment 207637 [details] [review]
st-container: Remove custom ClutterContainer implementation

This is a quick prerequisite to getting rid of StContainer for real.
We simply port StContainer to be a quick wrapper on top of the concrete
ClutterContainer implementation in ClutterActor to make the transition
hopefully seamless and easier to understand.

Created attachment 207638 [details] [review]
st-box-layout: Remove insert_actor/insert_before

Now that 'insert_child_at_index' and 'insert_child_below' exist
on ClutterActor, these aren't necessary.

Created attachment 207639 [details] [review]
st: Simplify paint_volume handling

Use a new convenience method, clutter_paint_volume_union_box, to grab our
paint box. Additionally, because the paint box is relative to the actor's
modelview (its allocation in most cases), make st_theme_node_get_paint_box
return something relative to the actor box too.



This patch is still broken, and I need to investigate.

Created attachment 207640 [details] [review]
st: Remove st-group

Now that ClutterActor/StWidget is concrete, we don't need it.

Created attachment 207641 [details] [review]
st: Remove st-container

Now that ClutterActor is a container, this isn't necessary.

Created attachment 207642 [details] [review]
st: Remove custom text direction stuff

Clutter has its own built-in system for managing text directions, like GTK+.
Convert over to use this.

Created attachment 207643 [details] [review]
st: Remove _st_allocate_fill

The very similar clutter_actor_allocate_align_fill is close enough
that this is just needless duplication. Additionally, allocate_fill
already inverts the align if the text direction is RTL, so we don't
need to do that here.

Created attachment 207644 [details] [review]
tests: Enable the vertical fade in the scrolling tests

Review of attachment 207629 [details] [review]:

Looks good.

Review of attachment 207639 [details] [review]:

Well you said its broken so marking as needs-work to get it off the list.

Review of attachment 207623 [details] [review]:

Yeah you are right ClutterActor has been abstract before.

Review of attachment 207624 [details] [review]:

OK.

Review of attachment 207625 [details] [review]:

Looks good.

Review of attachment 207626 [details] [review]:

Looks good.

Review of attachment 207627 [details] [review]:

Looks good.

Review of attachment 207628 [details] [review]:

Code looks good, commit message could be improved a bit though.

Review of attachment 207644 [details] [review]:

Why? 

We have st-scroll-view-sizing for that.

Review of attachment 207638 [details] [review]:

Looks good but the st-container changes don't really match the commit message.

Review of attachment 207638 [details] [review]:

insert_actor/insert_before were the only users of move_child/move_before, so they're safe to remove now.

Review of attachment 207638 [details] [review]:

> insert_actor/insert_before were the only users of move_child/move_before, so
they're safe to remove now.

Indeed, didn't notice that.

Attachment 207623 [details] pushed as 740388c - st-texture-cache: Use ClutterActor, not ClutterGroup
Attachment 207624 [details] pushed as f9e456b - st-scroll-view: Remove unnecessary VISIBLE checks
Attachment 207625 [details] pushed as 92ee174 - st: Don't use deprecated API
Attachment 207626 [details] pushed as d81958a - st: Remove st-tooltip


Pushed the next ACN'd four patches. Most of the st-widget patches should be pushed as a group, so I err'd on the side of caution there.

Review of attachment 207630 [details] [review]:

::: src/st/st-widget.c
@@ +1601,3 @@
+  ClutterActor *child;
+
+  for (l = children, ret = NULL; l; l = l->next)

Just initialize ret before the loop.

@@ +1703,3 @@
+    }
+
+  if (cmp)

I'd prefer an explicit comparison.

@@ +1816,3 @@
+      else
+        {
+          clutter_actor_get_allocation_box (CLUTTER_ACTOR (widget), &sort_data.box);

Just use widget_actor instead of casting again.

@@ +2359,3 @@
+ *
+ * Gets a list of the focusable children of @widget, in "Tab"
+ * order. By default, this returns all visible

By default? There is no way for the caller to change that.

Review of attachment 207631 [details] [review]:

"by property notifiers" -> "by using property notifiers" ...
Probably better to split this into header and add body (the commit message).

Code looks good.

Review of attachment 207632 [details] [review]:

Looks good.

Review of attachment 207644 [details] [review]:

We have a separate test for that so no need to add it here.

Review of attachment 207633 [details] [review]:

Looks good.

Review of attachment 207634 [details] [review]:

Looks good.

Review of attachment 207635 [details] [review]:

Looks good.

Review of attachment 207636 [details] [review]:

Looks good.

Review of attachment 207637 [details] [review]:

Looks good.

Review of attachment 207640 [details] [review]:

Looks good.

Review of attachment 207641 [details] [review]:

Looks good.

When applying all unpushed patches from here the panel is initially non reactive until I toggle the overview using the super key.

So something is broken here ... I am not sure which one of them (st widget patches) is broken though.

Review of attachment 207627 [details] [review]:

Marking this needs work as per previous comment (does not mean that *this* patch is broken but the st-widget set).

> Going to the overview and back causes "Clutter-CRITICAL **:
> clutter_actor_iter_next: assertion `ri->age == ri->root->priv->age' failed"
> spew. Currently unknown what actor is having trouble here.

This was found to be the MetaBackgroundActor. This also fixes a potentially large memory leak:

    http://git.gnome.org/browse/mutter/commit/?id=c844bab2325d0316f3561fd92169a6cb6c67da8e

Created attachment 207881 [details] [review]
st-widget: Make into a concrete class

ClutterActor is concrete, so StWidget should be too.



Here comes a new patch stack. Big thanks to drago01 for helping me figure out
the paint volume messyness.

The same one-line patch, again.

Created attachment 207882 [details] [review]
st-widget: Don't use deprecated API

clutter_container_foreach is deprecated, so let's replace that
with some ClutterActorIter usage. Additionally, remove the checks
for ClutterContainer, as all ClutterActors are now ClutterContainers.

Created attachment 207883 [details] [review]
st-widget: Don't explicitly check for ClutterContainer inheritance

Since all ClutterActors implement the ClutterContainer interface, there
isn't a case where this check could fail.

Created attachment 207884 [details] [review]
st-widget: Copy get_focus_chain and navigate_focus from StContainer

We can't get rid of the implementations in StContainer just yet,
as StContainer still keeps its own child list. But this should
lower the amount of code that has to be moved around when we
remove StContainer.

Created attachment 207885 [details] [review]
st-widget: Keep track of first/last children

Clutter now provides two new properties on ClutterActor - first-child
and last-child, so we have notifiers on when they change. Unfortunately,
it still doesn't help us too much - we need to keep track of the previous
values of the properties so we can remove their pseudoclasses.

Created attachment 207886 [details] [review]
st-widget: Add a proper paint, add st_widget_paint_background

Since we want to paint children by default in StWidget, we need to
provide a way for custom subclasses to paint their CSS backgrounds
without painting children... introducing st_widget_paint_background.

Additionally, remove any custom paint/pick handlers added by subclasses
of StWidget that just painted their children. This will cause double
painting if left alone.

This also removes the hacky things that some subclasses of StBin did
to prevent their one child to be painted by StBin.

Created attachment 207887 [details] [review]
st-widget: Implement a proper get_preferred_width/height

Now that StWidget is concrete and instantiable, we need to do something
other than return an adjusted 0 for width and height. Just chain up
to ClutterActor's default implementation, which uses the layout manager.

Created attachment 207888 [details] [review]
st-widget: Implement a proper allocate

Since an StWidget now has children, it needs to allocate those children
properly. Defer to the currently installed layout manager, like Clutter
does.

Now that we have something that allocates children in St, to prevent
double allocations, we use clutter_actor_set_allocation rather than
chaining up to StWidget::allocate.



To help facilitate rebases, I'm squashing the two paint patches and the two allocation patches.

Created attachment 207889 [details] [review]
st-container: Remove ClutterContainer implementation

Now that ClutterActor has a ClutterContainer implementation, we
can start removing StContainer. To help make this a bit more
understandable, instead of converting everything at once, make
StContainer a compatible API wrapper around the ClutterActor
implementation, and then we'll remove those wrappers in later
commits.



I'm making this a lot more light-weight. Before I removed all the destroy_children
uses in the JS code (which had some unnoticed bugs). Now I'm splitting them into
a couple patches -- right now we just have some basic wrappers which we'll remove
later.

Created attachment 207890 [details] [review]
st-container: Remove st_container_destroy_children

It was a simple wrapper around clutter_actor_destroy_all_children.

Created attachment 207891 [details] [review]
st-box-layout: Remove insert_actor/insert_before

Now that 'insert_child_at_index' and 'insert_child_below' exist
on ClutterActor, these aren't necessary.

Created attachment 207892 [details] [review]
st: Simplify paint_volume handling

Use a new convenience method, clutter_paint_volume_union_box, to grab our
paint box. Additionally, because the paint box is relative to the actor's
modelview (its allocation in most cases), make st_theme_node_get_paint_box
return something relative to the actor box too.

StBoxLayout needs to be corrected for the fade effect to work - it never
paints outside its allocation. It is unknown why it worked when chaining
up to the near-identical StContainer implementation.

Created attachment 207893 [details] [review]
st: Remove st-group

Now that ClutterActor/StWidget is concrete, we don't need it.

Created attachment 207894 [details] [review]
st-container: Remove st_container_get_children_list

Replace it with the new actor iteration APIs.



This is removed after st-group so we don't have to port that over.

Created attachment 207895 [details] [review]
st: Remove st-container

At this point, StContainer is a dummy class that does nothing, so it's
safe to remove.

Created attachment 207896 [details] [review]
st: Remove custom text direction stuff

Clutter has its own built-in system for managing text directions, like GTK+.
Convert over to use this.

Created attachment 207897 [details] [review]
st: Remove _st_allocate_fill

The very similar clutter_actor_allocate_align_fill is close enough
that this is just needless duplication. Additionally, allocate_fill
already inverts the align if the text direction is RTL, so we don't
need to do that here.

Review of attachment 207892 [details] [review]:

This is better but unfortunately still broken.
Theme node transitions look weird (ex. dash).

Created attachment 208162 [details] [review]
st: Simplify paint_volume handling

Use a new convenience method, clutter_paint_volume_union_box, to grab our
paint box. Additionally, because the paint box is relative to the actor's
modelview (its allocation in most cases), make st_theme_node_get_paint_box
return something relative to the actor box too.

StBoxLayout needs to be corrected for the fade effect to work - it never
paints outside its allocation. It is unknown why it worked when chaining
up to the near-identical StContainer implementation.



Transitions are fixed now.

Review of attachment 207881 [details] [review]:

Looks good.

Review of attachment 207882 [details] [review]:

Looks good.

Review of attachment 207883 [details] [review]:

Looks good.

Review of attachment 207884 [details] [review]:

Looks good.

Review of attachment 207885 [details] [review]:

Looks good.

Review of attachment 207885 [details] [review]:

Looks good.

Review of attachment 207885 [details] [review]:

Looks good.

Review of attachment 207886 [details] [review]:

Looks good.

Review of attachment 207889 [details] [review]:

Looks good.

Review of attachment 207890 [details] [review]:

Looks good.

Review of attachment 207891 [details] [review]:

Looks good.

Review of attachment 207893 [details] [review]:

Looks good.

Review of attachment 207894 [details] [review]:

Fine to commit with the casts fixed.

::: src/shell-generic-container.c
@@ +130,3 @@
   st_widget_paint_background (ST_WIDGET (actor));
 
+  for (child = clutter_actor_get_first_child (CLUTTER_ACTOR (self));

Useless cast just use actor.

@@ +150,3 @@
   CLUTTER_ACTOR_CLASS (shell_generic_container_parent_class)->pick (actor, color);
 
+  for (child = clutter_actor_get_first_child (CLUTTER_ACTOR (self));

Useless cast just use actor.

Review of attachment 207895 [details] [review]:

Looks good.

Review of attachment 207896 [details] [review]:

I still hate the "Clutter.get_default_text_direction() == Clutter.TextDirection.RTL" stuff but ... well.

Review of attachment 207897 [details] [review]:

Looks good.

Review of attachment 208162 [details] [review]:

Looks good and works now.
"It never paints outside its allocation. " the point was that it never does that scrolling is enabled. (The scrolling causes a clip to the content box).

::: src/st/st-widget.c
@@ +709,3 @@
     st_theme_node_get_paint_box (theme_node, &alloc_box, &paint_box);
 
+  clutter_paint_volume_union_box (volume, &paint_box);

You'd have to wait for the clutter patch to get in for this.

Attachment 207881 [details] pushed as b47fd6d - st-widget: Make into a concrete class
Attachment 207882 [details] pushed as f19ee78 - st-widget: Don't use deprecated API
Attachment 207883 [details] pushed as fbcea03 - st-widget: Don't explicitly check for ClutterContainer inheritance
Attachment 207884 [details] pushed as 3736d81 - st-widget: Copy get_focus_chain and navigate_focus from StContainer
Attachment 207885 [details] pushed as a9f728d - st-widget: Keep track of first/last children
Attachment 207886 [details] pushed as 64b2c5d - st-widget: Add a proper paint, add st_widget_paint_background
Attachment 207887 [details] pushed as cc2f5d1 - st-widget: Implement a proper get_preferred_width/height
Attachment 207888 [details] pushed as ea19790 - st-widget: Implement a proper allocate
Attachment 207889 [details] pushed as 72dad59 - st-container: Remove ClutterContainer implementation
Attachment 207890 [details] pushed as c892610 - st-container: Remove st_container_destroy_children
Attachment 207891 [details] pushed as a8b0816 - st-box-layout: Remove insert_actor/insert_before


It should be safe to push all patches up to here.

Review of attachment 208162 [details] [review]:

<magcius> drago01, the pv commit has another regression
<magcius> drago01, click on the workspace thumbnail in the overview -- the windows won't animate back, you'll just see the root background and then your windows suddenty appear

Adding CLUTTER_PAINT=disable-culling seems to fix it... I still have no idea why this is happening.

Comment on attachment 207894 [details] [review]
st-container: Remove st_container_get_children_list

Attachment 207894 [details] pushed as d2aab9d - st-container: Remove st_container_get_children_list

Created attachment 208522 [details] [review]
st: Account for children in StWidget's get_paint_volume

Now that StWidget is a group of sorts, it needs to account for its children
in its paint volume. Unfortunately, this causes havoc for StBoxLayout, so it
needs fixing - it's unknown why it worked when chaining up to near-identical
code in StContainer.

Created attachment 208523 [details] [review]
st: Remove st-group

Now that ClutterActor/StWidget is concrete, we don't need it.

Created attachment 208524 [details] [review]
st: Remove st-container

At this point, StContainer is a dummy class that does nothing, so it's
safe to remove.

Created attachment 208527 [details] [review]
st: Account for children in StWidget's get_paint_volume

Now that StWidget is a group of sorts, it needs to account for its children
in its paint volume. Unfortunately, this causes havoc for StBoxLayout, so it
needs fixing - it's unknown why it worked when chaining up to near-identical
code in StContainer.



OK, so the problem was that we weren't bailing out if a child had a bad
paint volume, as we were ignoring the return value.

Now that we're back to unioning the children *after* we integrate the theme
node paint box, we don't need clutter_paint_volume_union_paint_box, and the
"big cleanup" doesn't really help.

Created attachment 208528 [details] [review]
st: Remove st-group

Now that ClutterActor/StWidget is concrete, we don't need it.



This is slightly different, as I pushed the get_children_list patch to help
with memory cleanups.

Created attachment 208529 [details] [review]
st: Remove st-container

At this point, StContainer is a dummy class that does nothing, so it's
safe to remove.

Created attachment 208530 [details] [review]
st: Remove custom text direction stuff

Clutter has its own built-in system for managing text directions, like GTK+.
Convert over to use this.



I know you're not ecstatic about this, but ebassi wasn't happy about a
clutter_actor_is_rtl, and I don't believe that adding st_widget_is_rtl
is a good thing to do.

Created attachment 208531 [details] [review]
st: Remove _st_allocate_fill

The very similar clutter_actor_allocate_align_fill is close enough
that this is just needless duplication. Additionally, allocate_fill
already inverts the align if the text direction is RTL, so we don't
need to do that here.

Review of attachment 208527 [details] [review]:

Looks good, it seems to even fix bug 670636 for me.

Review of attachment 208528 [details] [review]:

Looks good.

Review of attachment 208529 [details] [review]:

Looks good.

Review of attachment 208530 [details] [review]:

As I said before looks fine but I sill hate the code duplication all over the place, but that is not something this patch introduces.

Review of attachment 208531 [details] [review]:

Looks good.

Attachment 208527 [details] pushed as be3eb30 - st: Account for children in StWidget's get_paint_volume
Attachment 208528 [details] pushed as bb862e2 - st: Remove st-group
Attachment 208529 [details] pushed as d528567 - st: Remove st-container
Attachment 208530 [details] pushed as 15f881f - st: Remove custom text direction stuff
Attachment 208531 [details] pushed as 24ad59e - st: Remove _st_allocate_fill


Yay!