Created attachment 213859 [details] [review]
actor: Hide children before ourselves

Fixes errors like:
Gtk-WARNING **: GtkClutterEmbed 0x11a3300 is mapped but visible child GtkClutterOffscreen 0x15d0490 is not mapped
when destroying a GtkClutterActor.

GTK+'s invariants checks expect children to not be visible anymore
if they're not mapped. This can happen if we hide the container before
telling the children that they're hidden.

Review of attachment 213859 [details] [review]:

looks good to me

Created attachment 213861 [details] [review]
actor: Hide children before ourselves

Fixes errors like:
Gtk-WARNING **: GtkClutterEmbed 0x11a3300 is mapped but visible child GtkClutterOffscreen 0x15d0490 is not mapped
when destroying a GtkClutterActor.

GTK+'s invariants checks expect children to not be visible anymore
if they're not mapped. This can happen if we don't forcefully hide the
GtkClutterActor's widget before unrealizing it.

Comment on attachment 213859 [details] [review]
actor: Hide children before ourselves

Right path, but incorrect fix. Not sure how I managed to make this pass my tests...

*** Bug 656577 has been marked as a duplicate of this bug. ***

Review of attachment 213861 [details] [review]:

looks good

Attachment 213861 [details] pushed as e70da5b - actor: Hide children before ourselves

This breaks gnome-boxes (which is getting these warnings), with this patch applied its top toolbar becomes invisible during VM creation.

(In reply to comment #8)
> This breaks gnome-boxes (which is getting these warnings), with this patch
> applied its top toolbar becomes invisible during VM creation.

I don't see how this could not be a problem in your app, the widget is _unrealized_ the line after.

I didn't get to the bottom of this yet, but here is what I've found so far. Boxes uses a ClutterBox containing notebooks and various other UI elements. In particular it has a 'topbar' actor. When moving through the various UI states, this actor gets removed from the box and then readded. When it's removed, gtk_clutter_actor_unrealize gets called, and the widget gets hidden with gtk_widget_hide. When it's readded, it's not unhidden.

If I get a backtrace of the unrealize call, I get
 
Breakpoint 10, gtk_clutter_actor_unrealize (actor=0x160c740) at ./gtk-clutter-actor.c:160
160       GtkClutterActor *clutter = GTK_CLUTTER_ACTOR (actor);
(gdb) bt

+ Trace 230226

In particular, there's a clutter_actor_unrealize_not_hiding call in the backtrace whose doc says: "
 * This function is separate from clutter_actor_unrealize() because it
 * does not automatically hide the actor.
 * Actors need not be hidden to be unrealized, they just need to
 * be unmapped. In fact we don't want to mess up the application's
 * setting of the "visible" flag, so hiding is very undesirable.
 "

Well, I need to look more into this after getting some sleep ;)

The GTK+ semantics are different though. You can't show an unrealized widget. Unrealizing it means it's not connected to the display anymore, so it cannot show.

the semantics of Clutter are the same: a mapped widget (i.e.: visible and associated with a mapped parent) has to be realized as well (even though, in Clutter terms, there's nothing going on inside the ::realize virtual).

if you're removing a child from an actor and then add it back, it should be shown - unless you called hide() while the former child did not have a parent; adding a child to an actor should cause the show() method to be called, which will ensure that the state is correct - realize -> map -> show. since the widget is visible when it's removed, show() will short-circuit out, and will most likely not cause one of the virtual functions in GtkClutterActor to be called - which would explicitly show the GtkWidget.

calling gtk_widget_hide() inside GtkClutterActor::unrealize is, thus, not entirely correct, but it works because the invariant is not that the unrealized widget has to be !visibile: the invariant is that the widget being unrealized is !mapped.

what happens when you replace gtk_widget_hide() with gtk_widget_unmap()?

the other thing to understand is why removing the actor from the ClutterBox (which, by the way, is deprecated and you should not be using it) does not cause unmap() on itself to be called - thus leading to a gtk_widget_unmap() on the widget.

Reverted and added a testcase.

commit 9df4f174c42176ea879d3216181a606362041811
Author: Bastien Nocera <hadess@hadess.net>
Date:   Wed May 16 13:10:26 2012 +0100

    examples: Add/remove actors at run-time
    
    When animating, every 3 seconds, remove an actor from the
    animation or create a new one so we always have 3 or 4 actors
    spinning.

commit ab0625eefc39906ebf1e8d94799cd4636de50d6a
Author: Bastien Nocera <hadess@hadess.net>
Date:   Wed May 16 12:46:12 2012 +0100

    Revert "actor: Hide children before ourselves"
    
    This reverts commit e70da5bea9a3aa91b5950a3ad9d3f58b69ad12fc.

(In reply to comment #12)
> what happens when you replace gtk_widget_hide() with gtk_widget_unmap()?

If I change this in gtk_clutter_actor_unrealize, the toolbar does not get hidden, but I still get the warnings in the console.

I added map/unmap implementations to gtk-clutter-actor.c, but it doesn't seem to really make a difference (although we should still probably do this).

Here is what is happening:

We're removing the GtkClutterActor actor from its parent. This will cause us to unmap, unrealize and finally remove the widget from the GtkClutterEmbed container. At the end of that we'll have a mapped previous parent (GtkClutterEmbed) and an unmapped unrealized GtkClutterActor that has no parent. This is a valid state.

However, during the intermediate state where we just unmapped or unrealized the GtkClutterActor we'll have a stage with a mapped parent (GtkClutterEmbed) with a non-mapped yet still showed child, which breaks the Gtk+ invariants.

Gtk itself handles this by postponing invariant checks when inside gtk_widget_unparent (), but this is not public API so we can't do that ourselves.
However, I think the solution is to drop the explicit widget_unmap and widget_unrealize calls from GtkClutterActor and just rely on the implicit unmap and unrealize that happens in gtk_widget_unparent.

Created attachment 214643 [details] [review]
Don't explicitly unrealize widget in clutter_actor_unrealize

We rely on the implicit unrealize in the gtk_container_remove call as
otherwise we'll temporarily be in a state where we can breaking the
gtk+ invariant that the child (GtkClutterActor) is visible but
non-mapped while the parent (GtkClutterEmbed) is mapped.

Review of attachment 214643 [details] [review]:

looks good to me; other clutter-gtk users may want to chime in and test the patch.

gnome-control-center also has a working solution. If it works in the testcase, it's fine by me. I would add a reference to the explanation, or the explanation itself in the function though.

Attachment 214643 [details] pushed as 154cdc9 - Don't explicitly unrealize widget in clutter_actor_unrealize