iirc without the meta_window_calc_showing() prior to the workspace switch effect, the workspace in the effect would be empty to start with, with the actors becoming visible randomly as the recalculation in the idle happens.

I was aware of this, but I think the triggering of the map effect only happens if the window was not mapped previously, for example, if you start Mutter on a running desktop machine, with workspaces already populated. For windows that get created after Mutter was started, this should generally not arise. It's still a bug that needs fixing; opened to suggestions.

The recent changes to priorities in Clutter (and the corresponding outstanding Mutter change in bug 568874) should make sure the any calc-showing idles that are queued up run before the first frame is drawn.

In general, I don't have a good understanding of the interaction of live-hidden-windows with Mutter effects.

If we have live-hidden-windows:

 * Windows that are minimized or on other desktops are *mapped*
 * The app will still repaint the windows and we'll still get damage events
 * Memory will be consumed to store the window image
 * We can use ClutterClone to display thumbnails/previews/etc of such windows

If we don't have live-hidden-windows:

 * Windows that are minimized or on other desktops are *unmapped*
 * If we've previously called NameWindowPixmap, we can use that pixmap until we destroy it
 * We need to destroy any pixmap we have around to get memory savings
 * Once we've destroyed the pixmap, we can't paint the window in any way

The only difference I really see between effects for the two cases at a theoretical level is that in the no-live-hide-windows case, if a window is unmapped during an effect, we should wait to free the current pixmap until the effect is complete.

(The need for the "hidden window group" in the live-hidden-windows case is unclear to me. The map state of the clutter actor should have absolutely nothing to do with the map state of the X window.)

If I was designing the compositor interface ,what I'd expect would be that there would be operations that reflected the different ways that the current set of visible windows could change:

 add_window (window unwithdrawn)
 remove_window (window withdrawn)
 minimize_window
 unminimize_window
 switch_workspace
 window_workspace_changed
 
[The map effect in this bug should only run on add_window, and window_workspace_changed]

Those are the only ways that the current set of visible windows can change. And we might as well give them to the compositor raw and immediately. Trying to wait until idle doesn't work because then you have "hybrids" that can no longer be described in these terms. The compositor already has to be able to handle these things happening while effects are going on, so waiting until idle doesn't help with that either.

Then separately, we want notification when the map state of the window changes:

 map_window
 unmap_window

The main thing we do there is that when we get an unmap_window, we either immediately free the Pixmap/texture or set a flag to do it when the effect ends.

The set of operations isn't all that different from what we have currently, but it's not all that the same either. At the end of all that analysis, I'm not a whole lot closer to understanding the right quick fix. If we get the change in bug 568874 in, then we could try removing the live-hidden-windows special casing from meta_workspace_queue_calc_showing() and see if things still work.

And yes, the problem occurs for all workspace switches, not just for starting Mutter.

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

Removing the live-hidden-windows special casing doesn't work without more code changes. 

I'd say the basic problem is that the current workspace switching works out by miracle. The plugins do some of it, the core code does other parts. The core code has checks like:

 clutter_actor_get_parent (CLUTTER_ACTOR (cw)) != info->window_group

to see if the window is hidden which interacts in complex ways with the way that workspace switching is implemented in plugins.

(The default plugin and the gnome-shell plugin both implement workspace switching by reparenting incoming and out-going windows to groups, then reparenting them back at the end.)

(In reply to comment #2)
> (The need for the "hidden window group" in the live-hidden-windows case is
> unclear to me. The map state of the clutter actor should have absolutely
> nothing to do with the map state of the X window.)

You have to avoid the case where the user can interact with the actor for a window that is unmapped, so the live hidden windows have to be stashed away somehow. Having a group for them means you can maintain correct x,y position for them, and do not have to mess with their individual z-order.

> If I was designing the compositor interface ,what I'd expect would be that
> there would be operations that reflected the different ways that the current
> set of visible windows could change:
> 
>  add_window (window unwithdrawn)
>  remove_window (window withdrawn)
>  minimize_window
>  unminimize_window
>  switch_workspace
>  window_workspace_changed

Agreed; iirc, the way things are at present is because originally the compositor was simply driven by the X events from an xevent hook.

That also reminds me, Unminimize as an event is rather problematic with the live-hidden-windows. The WM spec is flawed in this regard, I described the problem at http://bugzilla.openedhand.com/show_bug.cgi?id=1443.

> [The map effect in this bug should only run on add_window, and
> window_workspace_changed]

The question is whether you need a map effect like that at all. add_window and window_workspace_changed seem conceptually very different to me; the current map effect was really meant to be the add_window() effect above.

(In reply to comment #4)
> (The default plugin and the gnome-shell plugin both implement workspace
> switching by reparenting incoming and out-going windows to groups, then
> reparenting them back at the end.)

We should prohibit this approach, it's asking for trouble. A cleaner way of doing this is for the effect to use clones; I rewrote the Moblin effect along these lines a while back, and should do the same for the default plugin.

(In reply to comment #5)
> (In reply to comment #2)
> > (The need for the "hidden window group" in the live-hidden-windows case is
> > unclear to me. The map state of the clutter actor should have absolutely
> > nothing to do with the map state of the X window.)
> 
> You have to avoid the case where the user can interact with the actor for a
> window that is unmapped, so the live hidden windows have to be stashed away
> somehow. Having a group for them means you can maintain correct x,y position
> for them, and do not have to mess with their individual z-order.

Why doesn't hiding and showing the actor work as well? It seems to have all the same properties.

> > If I was designing the compositor interface ,what I'd expect would be that
> > there would be operations that reflected the different ways that the current
> > set of visible windows could change:
> > 
> >  add_window (window unwithdrawn)
> >  remove_window (window withdrawn)
> >  minimize_window
> >  unminimize_window
> >  switch_workspace
> >  window_workspace_changed
> 
> Agreed; iirc, the way things are at present is because originally the
> compositor was simply driven by the X events from an xevent hook.

I'm looking more into this at the moment, and the biggest problem in implementing the above is dealing with the work (like placing) that is delayed until the window is first mapped.

At the point that the compositor is triggered for the animation, you want all the properties of the window to be "just there", but it seems inadvisable to remove the delaying of work for newly added windows to idle - compressing events is going to chill out a lot of possible weird conditions.

Maybe the right thing to do is to force calc_showing immediately if switch_workspace is triggered.

[...]

> > [The map effect in this bug should only run on add_window, and
> > window_workspace_changed]
> 
> The question is whether you need a map effect like that at all. add_window and
> window_workspace_changed seem conceptually very different to me; the current
> map effect was really meant to be the add_window() effect above.

I'm not sure when a window would move *onto* the current workspace. It isn't very common. But say, if double-clicking on the icon for an open Nautilus window moved it to the current desktop, then you'd want to treat it pretty much exactly like a new window. (Currently it doesn't move - it just "demands-attention")

> (In reply to comment #4)
> > (The default plugin and the gnome-shell plugin both implement workspace
> > switching by reparenting incoming and out-going windows to groups, then
> > reparenting them back at the end.)
> 
> We should prohibit this approach, it's asking for trouble. A cleaner way of
> doing this is for the effect to use clones; I rewrote the Moblin effect along
> these lines a while back, and should do the same for the default plugin.

I don't think there's anything horrible with the reparenting approach as long 
as it's documented what the effects can and cannot do. The clone approach should work fine too - (as long as the window group is hidden - drawing occluded windows is a big performance since) - makes dealing with other simultaneous effects a bit easier since you are just defining everything as frozen during the animation.

- Owen

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #2)
> > > (The need for the "hidden window group" in the live-hidden-windows case is
> > > unclear to me. The map state of the clutter actor should have absolutely
> > > nothing to do with the map state of the X window.)
> > 
> > You have to avoid the case where the user can interact with the actor for a
> > window that is unmapped, so the live hidden windows have to be stashed away
> > somehow. Having a group for them means you can maintain correct x,y position
> > for them, and do not have to mess with their individual z-order.
> 
> Why doesn't hiding and showing the actor work as well? It seems to have all the
> same properties.

My recollection is that we used to just hide actors to start with but had issues with maintaining the z order of the visible windows, and punting the hidden windows elsewhere made things simpler. Lot of things have changed both in Clutter and Mutter since, so this might no longer be an issue.

> I don't think there's anything horrible with the reparenting approach as long 
> as it's documented what the effects can and cannot do. The clone approach
> should work fine too - (as long as the window group is hidden - drawing
> occluded windows is a big performance since) - makes dealing with other
> simultaneous effects a bit easier since you are just defining everything as
> frozen during the animation.

The problem with reparenting is that since it's just a glorified remove/add operation, interferes with the stacking order. Also, I feel generally uneasy about any plugin being able to just 'steal' my 'windows'.

After digging into this, I decided that my thesis:

 Compositor operations should indicate state changes like minimization

Was misguided - the reason that that doesn't work out is that when you start an animation, you want everything (in particular placement) to already have been sorted out by Metacity. And by the time Metacity has sorted everything out (in it's idle functions), you don't necessarily have a clear simple state change.

So I took a different approach and made the compositor operation be a simple:

 show the window
 hide the window

(Since in the end, that's what you are doing), with a "hint" of what effect should be shown passed along by the Metacity core.

I've put a patch set implementing that in bug 587251. (Not here to avoid completely confusing what this bug is about, though the fix for this bug is part of that patch set.)

I went with the show/hide approach rather than the reparenting, because it seems less likely to cause problems with stacking order and overall simple.

Leaving the question of how to implement switch workspace effects to the side; I think my patches should work with any of the current options.

Since bug 587251 has been closed, I assume we can close this one as well.