Created attachment 137520 [details] [review]
Split shadow code into a separate file

Separate code related to creating the gaussian-blurred shadow texture
into a separate file.

Move the definition of MetaCompositor into a compositor-private.h
so that the shadow code can cache the source in the compositor
structure.

Created attachment 137521 [details] [review]
Separate source and header files for MutterWindow

compositor.c: Move MutterWindow code to mutter-window.c;
 rename map_win() to mutter_window_map(), etc.

mutter-window-private.h: New private header file for
 MutterWindow functions used internally to the compositor.

compositor-mutter.h: Move MutterWindow declarations to
 mutter-window.h; move a couple of private functions to
 compositor-private.h

compositor-private.h: Move MetaCompScreen declaration to here:
 Conceptually it's private to compositor.c, but MutterWindow
 manipulates some of the lists directly for now.

mutter-plugin.c compositor.c: Don't call mutter_window_effect_completed()
 for MUTTER_PLUGIN_SWITCH_WORKSPACE, but use a new
 mutter_switch_workspace_completed(), since the window is
 just used to identify a screen.

Created attachment 137522 [details] [review]
Reindent and reorganize compositor.h

Reindent compositor.h in a more consistent fashion, and group
logically related functions together.

Created attachment 137523 [details] [review]
Fix function names in debug tracing statements

Refer to meta_compositor_* not the old clutter_cmp_* names.

Created attachment 137524 [details] [review]
Remove unused dock_windows list from MetaCompScreen

MetaCompScreen.doc_windows was kept updated, but never used.

Created attachment 137525 [details] [review]
Don't move hidden windows to the desktop layer

Putting hidden windows in the desktop layer is pointless - in
the desktop layer isn't necessary below all visible windows,
and we are hiding the windows by other means. And the movement
isn't reliable because nothing sets stack->needs_relayer, so
windows can get stuck in the desktop layer after being
rehidden.

Created attachment 137526 [details] [review]
Move window repair and reshape to a paint function

Add a paint function that checks all windows for repair and
shape updates; this:

 - simplifies the logic for when a window needs to be repaired
 - avoids duplicate work when we get multiple damage effects
 - avoids the need to look ahead in the event queue

Instead of relying on repair to implicitly resize the
MutterWindow actor, set the size explicitly when the core
code updates the geometry. (This is needed because we haven't
repaired yet when we start an animation, and the animation
may depend on the size to, e.g., rescale from the center.)

Because the core geometry update happens before we start
maximize/unmaximize effects we need to work around this by
passing both the old and new geometry to the compositor.

Created attachment 137527 [details] [review]
Remove include_destroy parameter to mutter_window_effect_in_progress()

Clean up mutter_window_effect_in_progress() by removing the
include_destroy parameter which was used only in one place that
could be easily done otherwise.

Created attachment 137528 [details] [review]
Remove unused focus_window member of MetaCompScreen

Remove some old code; the compositor no longer tracks
the focus window.

Created attachment 137529 [details] [review]
Simplify relationship between mapping and visibility

Previously, changes to the visibility of a window could be indicated
by meta_compositor_map_window(), meta_compositor_unminimize_window(),
meta_compositor_set_window_hidden(), etc, with the exact behavior
depending on the 'live_hidden_windows' preference.

Simplify this so that visibility is controlled by:

 meta_compositor_show_window()
 meta_compositor_hide_window()

With an 'effect' parameter provided to indicate the appropriate
effect (CREATE/UNMINIMIZE/MINIMIZE/DESTROY/NONE.)

The map state of the window is signalled separately by:

 meta_compositor_map_window()
 meta_compositor_unmap_window()

And is used only to control resource handling.

Other changes:

 * The desired effect on show/hide is explicitly stored in
   MetaWindow, avoiding the need for the was_minimized flag.
   At idle, once we calculate the window state, we pass the
   effect to the compositor if it matches the new window
   state, and then clear the effect to start over for future
   map state changes.

 * meta_compositor_switch_workspace() is called before any windows
   are hidden or shown, allowing the compositor to avoid hiding
   or showing an effect for windows involved in the switch.
   http://bugzilla.gnome.org/show_bug.cgi?id=582341

 * Handling of post-effect cleanups for MutterWindow are
   simplified - instead of trying to do different things based
   on the individual needs of different effects, we just wait until
   all effects complete and sync the window state to what it
   should be.

 * On unmap, once we destroy the pixmap, we tell ClutterX11Pixmap
   that we've done so, so it can clean up and unbind. (The
   unbinding doesn't seem to be working properly because of
   ClutterGLXPixmap or video driver issues.)

(In reply to comment #1)
> Created an attachment (id=137520) [edit]
> Split shadow code into a separate file

This is pretty clearly just moving code between files, and assuming it still compiles, it's obviously correct. (Although having looked at the code now, we really ought to be computing gaussian_map at compile time and storing it as const data.)


(In reply to comment #2)
> Created an attachment (id=137521) [edit]
> Separate source and header files for MutterWindow

Likewise, mostly just obviously-correct moving code around, except that you changed meta_xattrs_copy() to use g_malloc() but left meta_xattrs_free() using XFree().

Also, in mutter-window.c:

+      /* Need to reset the map_state for map_win() to work */

s/map_win/mutter_window_map/



(In reply to comment #3)
> Created an attachment (id=137522) [edit]
> Reindent and reorganize compositor.h

Likewise "if it still compiles it must be right"


(In reply to comment #4)
> Created an attachment (id=137523) [edit]
> Fix function names in debug tracing statements

Good.


(In reply to comment #5)
> Created an attachment (id=137524) [edit]
> Remove unused dock_windows list from MetaCompScreen
> 
> MetaCompScreen.doc_windows was kept updated, but never used.

Good except for ^^^^^ that typo in the commit message


(In reply to comment #6)
> Created an attachment (id=137525) [edit]
> Don't move hidden windows to the desktop layer

That seems correct.


(In reply to comment #7)
> Created an attachment (id=137526) [edit]
> Move window repair and reshape to a paint function

I don't really understand the parts whose behavior this changes, so I can't really comment on it. It seems like mutter_window_detach() may be a bad name for whatever it's doing.


(In reply to comment #8)
> Created an attachment (id=137527) [edit]
> Remove include_destroy parameter to mutter_window_effect_in_progress()
> 
> Clean up mutter_window_effect_in_progress() by removing the
> include_destroy parameter which was used only in one place that
> could be easily done otherwise.

We used to pass FALSE in mutter_window_sync_actor_position() as well, which you need to either work around, or else justify in the commit message.


(In reply to comment #9)
> Created an attachment (id=137528) [edit]
> Remove unused focus_window member of MetaCompScreen

Good


(In reply to comment #10)
> Created an attachment (id=137529) [edit]
> Simplify relationship between mapping and visibility

This seems basically right, although it's bigger and trickier than the others so it's harder to be sure...

+#include "boxes.h"

not clear why that got added to mutter-window-private.h

   /* Desktop switching flags */
-  guint		    needs_map              : 1;
-  guint		    needs_unmap            : 1;
   guint		    needs_repair           : 1;

I believe the comment is now bogus (or may have been even before?)

+                                            gboolean     managing_screen,

I see that this means "being called from meta_screen_manage_all_windows()", but that's not really obvious without some looking around. I'd either make it "gboolean no_effect" or "MetaCompEffect effect".

+  /* Focusing a window will cause it to immediately be shown, so we need
+   * to tell the compositor that we are switching workspaces first
+   */

The comment doesn't really make sense if you aren't reading the diff. (That is, it only makes sense if you know that the code which follows the comment used to occur earlier in the function and had to be moved.)

+ *   workspace and is chnaged to become visible on the active

s/chnaged/changed/

+ * @META_COMP_EFFECT_UNMINIMIZE: The window should be shown
+ *   as minimizing from its icon geometry.

s/minimizing/unminimizing/

+ * meta_compositor_show_window() and meta_compositor_hint_window()

s/hint/hide/

(In reply to comment #11)
> (In reply to comment #2)
> > Created an attachment (id=137521) [edit]
> > Separate source and header files for MutterWindow
> 
> Likewise, mostly just obviously-correct moving code around, except that you
> changed meta_xattrs_copy() to use g_malloc() but left meta_xattrs_free() using
> XFree().

Good catch, fixed.

> Also, in mutter-window.c:
> 
> +      /* Need to reset the map_state for map_win() to work */
> 
> s/map_win/mutter_window_map/

Yeah, but not going to fix, because the comment and what it refers to goes away later in the patch set.

> (In reply to comment #5)
> > Created an attachment (id=137524) [edit]
> > Remove unused dock_windows list from MetaCompScreen
> > 
> > MetaCompScreen.doc_windows was kept updated, but never used.
> 
> Good except for ^^^^^ that typo in the commit message

Fixed.

> (In reply to comment #7)
> > Created an attachment (id=137526) [edit]
> > Move window repair and reshape to a paint function
> 
> I don't really understand the parts whose behavior this changes, so I can't
> really comment on it. It seems like mutter_window_detach() may be a bad name
> for whatever it's doing.

Quite likely (what it does is cut the relationship between the pixmap that backs the window and the actor showing the window; e.g. we do this
when the pixmap is no longer reflecting the window's contents because the
window has been unmapped and no longer has a backing pixmap.)

I'll add a comment. Don't have a much better name (maybe unset_pixmap())

> (In reply to comment #8)
> > Created an attachment (id=137527) [edit]
> > Remove include_destroy parameter to mutter_window_effect_in_progress()
> > 
> > Clean up mutter_window_effect_in_progress() by removing the
> > include_destroy parameter which was used only in one place that
> > could be easily done otherwise.
> 
> We used to pass FALSE in mutter_window_sync_actor_position() as well, which you
> need to either work around, or else justify in the commit message.

I guess I'll just mention it in the commit, there's no justification since there was no reason for passing FALSE in sync_actor_position() that I could
figure out.

> (In reply to comment #10)
> > Created an attachment (id=137529) [edit]
> > Simplify relationship between mapping and visibility
> 
> This seems basically right, although it's bigger and trickier than the others
> so it's harder to be sure...

Yeah, it's definitely what was left when I split out the easy stuff :-)
 
> +#include "boxes.h"
> 
> not clear why that got added to mutter-window-private.h

I added it at some point because the build was breaking without it, but I tried removing it again, and seems to compile fine.

>    /* Desktop switching flags */
> -  guint                    needs_map              : 1;
> -  guint                    needs_unmap            : 1;
>    guint                    needs_repair           : 1;
> 
> I believe the comment is now bogus (or may have been even before?)

I couldn't out what the comment was about before; so left it, but I guess
you could take it as referring to the needs_map/unmap flags which were used when switching workspaces. Removed.

> +                                            gboolean     managing_screen,
> 
> I see that this means "being called from meta_screen_manage_all_windows()", but
> that's not really obvious without some looking around. I'd either make it
> "gboolean no_effect" or "MetaCompEffect effect".

I like the MetaCompEffect suggestion.

> +  /* Focusing a window will cause it to immediately be shown, so we need
> +   * to tell the compositor that we are switching workspaces first
> +   */
> 
> The comment doesn't really make sense if you aren't reading the diff. (That is,
> it only makes sense if you know that the code which follows the comment used to
> occur earlier in the function and had to be moved.)

Yeah, improved to:

+  /* This needs to be done after telling the compositor we are switching
+   * workspaces since focusing a window will cause it to be immediately
+   * shown and that would confuse the compositor if it didn't know we
+   * were in a workspace switch.
    */

> + *   workspace and is chnaged to become visible on the active
> 
> s/chnaged/changed/

fixed.

> + * @META_COMP_EFFECT_UNMINIMIZE: The window should be shown
> + *   as minimizing from its icon geometry.
> 
> s/minimizing/unminimizing/

fixed.
 
> + * meta_compositor_show_window() and meta_compositor_hint_window()
> 
> s/hint/hide/

fixed.

Pushed with those changes (and a couple of small bug fixes discovered over the last week.)

Thanks for the reviews!

Unfortunately commit 6726fcd25dbd335ec64a2415bb6aacf79903c6ae broken things for us with the Moblin plugin; our desktop window no longer paints, and I am unable to switch away from workspace 1 using the gnome workspace switcher applet (if I click a workspace, the switch happens, but as soon as the pointer leaves the applet window, a switch back to workspace 1 is initiated somehow). I am investigating this further.

I have identified the issues in the moblin plugin, they are due to assumptions we were making that no longer hold, but in no way what could be describe as a bug in the new mutter code, so I am closing this bug again.