The new overlay wallpaper looks great!

Is there a reason it starts below the panel? This makes panel look drained of color. Would it work if it were to start behind the panel the same way as we have the regular wallpaper start behind the panel?

The panel looked bad indeed, because the semitransparent white panel background got mixed with the semitransparent black wallpaper overlay. I've put the wallpaper overlay below the panel now, so that the panel looks cleaner.

I just noticed that I wasn't really paying attention when making some actor backgrounds transparent instead of black (like the workspace previews). I now re-introduced some issues that were fixed in these commits:
95c87f87fa2bdf340713b222305207202aea29c4
d07433d445411cd124e571575ae7b592b1b363d1

If I'm not mistaken, the issues are that through the workspace preview spacing, you see the sidebar when leaving the overlay, and parts of the Applications/Documents modes when switching between them and the main overlay mode.

An other solution is needed when the new overlay wallpaper is used. I don't think it could be solved by adding a 'mask layer' (as possible in Photoshop - not sure whether Clutter supports something like it), or by placing another wallpaper clone above parts of the Applications/Documents sections when needed. Those methods would be hard to implement when the workspaces are scaled and slided.

Maybe, in addition to the sliding/zooming animations, fading parts in and out can make the transitions look better?

Created attachment 132852 [details] [review]
A patch that clips the sideshow when we enter and leave the expanded display view, so that portions of it don't show up behind the wokrspaces.

Other suggestions that are not directly related to this patch: 

1) Make the overlay wallpaper a bit lighter, perhaps by setting 
ROOT_OVERLAY_COLOR to 0x000000bb instead of 0x000000cc

2) Use the center of the wallpaper or one of the "third points" as suggested by Owen as the center of the zoomed in overlay wallpaper. If you use the "third point", I would suggest the lower right one, as a quick survey of GNOME wallpapers makes me think that something interesting is displayed on the lower right in more cases.
http://en.wikipedia.org/wiki/Rule_of_thirds

To use the center of the wallpaper just set:
    background.x = -global.screen_width / 2;
    background.y = -global.screen_height / 2;

3) backOver variable should have height global.screen_height - Panel.PANEL_HEIGHT

4) This patch still doesn't address seeing the sideshow between the workspaces as you zoom in and out of the overlay when you have multiple workspaces and the active one is a workspace other than the top left one. Similar clipping technique can be applied in that situation. In my opinion, it is not as big of a deal as the situation that is being fixed by this patch. We'd still need to do something about this._backdrop in the workspaces.js, as having it have the transparent color definitely defeats the point (i.e. we'd likely need to delete the code related to it as long as it doesn't have any other functions).

Thank you for the clipping patch, it fixes the largest problem well. I agree
that the other transition problem is less important. I've tried fixing it
with right-side clipping too, but it looked awkward when I let the clipping
follow the left side of the workspace previews. (Especially the search box
disappearing looked bad.)

Have you considered scaling and positioning the sideshow the same way as the
workspace previews during transitions, instead of putting it on a fixed
position? It would add to the zooming-out effect, as if the menu has always
been at the left side of the workspaces. This would fix the second transition
problem, and maybe look more logical.

I made the overlay wallpaper lighter. It doesn't seem to badly decrease
readability, even on a white wallpaper.

Used the lower right rule of thirds intersection point and added a comment
about it. In a later stage (this can be after applying this patch), it would
be good to discuss this choice with the Art team indeed, as Owen suggested.

Fixed the backOver height and deleted the this._backdrop code; after some
testing I don't see any other functions for it.

Review of C code; in general it looks pretty good:

+PKG_CHECK_MODULES(X11, gdk-x11-2.0 clutter-x11-0.9 clutter-glx-0.9)

The split here isn't by type of dependency, it's by where they are used. Making this separate from PKG_CHECK_MODULES(MUTTER_PLUGIN,...) will just prevent pkg-config from properly removing duplicate flags.

  Atom native_atom = gdk_x11_get_xatom_by_name ("_XROOTPMAP_ID");
  GdkAtom atom = gdk_x11_xatom_to_atom (native_atom);

This is just the same as

  GdkAtom atom = gdk_atom_intern("_XROOTPMAP_ID")

GdkAtom's are display independent. But see below.

+  if (gdk_property_get(window, atom, GDK_TARGET_PIXMAP,
+                       0, INT_MAX - 3,
+                       FALSE,
+                       &prop_type, NULL, &prop_size,
+                       (guchar **)&pixmaps) &&

I really don't like using gdk_property_get(), *ever*, because it doesn't have well defined semantics. Using XGetWindowProperty looks something liek:

===
 guchar *data;
 if (XGetWindowProperty (xdisplay, xwindow, xatom, AnyPropertyType,
                         0, LONG_MAX,
                         False,
                         &type, &format, &nitems, &bytes_after, &data) && 
    type != None) 
  {
     /* Got a property */
     if (type == XA_PIXMAP && format == 32 && nitems == 1) {
         /* Was what we expected */
         pixmap = *(Pixmap *)data;
     }
     XFree(data);
 }
===

+    g_warning ("Could not get the root window pixmap");

A bad property might be worth a warning, but not having a root window pixmap at all is perfectly legitimate and should not be warned about.

+/*
+ * Called when the root window pixmap is destroyed.
+ */
+static void

Probably better to say "root window pixmap actor" is destroyed, the above is a little confusing.

+      gdk_window_add_filter (window, root_window_filter, global);

I'd like to see a comment here about how this relates to Metacity event handling. I just went through and checked that it was OK, but it wasn't obvious to me. Suggestion:

 /* Metacity handles some root window property updates in its global
  * event filter, though not this one. For all root window property
  * updates, the global filter returns GDK_FILTER_CONTINUE, so our
  * window specific filter will be called.
  */

+      /* The pixmap actor is only referenced by its clones. */
+      g_object_ref_sink (global->root_pixmap);

Your refcounting here is still isn't right.

 - When you create the root pixmap it will have one floating reference
   counting
 - g_object_ref_sink() transforms it into one non-floating reference
   count.
 - creating the clutter clone will change the reference count to 2
 - When the clone is freed, the reference count will go back to 1

I think you want a:

 if (created_new_pixmap)
   g_object_unref(global->root_pixmap)

at the end before you return the clone.

In the wallpaper branch, the issues mentioned by Owen are now fixed and Marina's overlay enter/leave animation fixes are applied. They look better and I don't see any strangely (dis)appearing search boxes more. I also added some comments about them.

On IRC, Marina suggested adding an item to the Todo wiki page for creating a better solution for the described animation problem. This would be done by adding another root window pixmap actor stacked between the workspaces and the sideshow, which would be clipped so that only the part behind the workspaces is shown. This is a similar appoach to the original one (putting another black box behind the workspaces), but should be implemented using clipping instead of positioning in order to keep the wallpaper textures aligned.

Created attachment 133071 [details] [review]
Comments, small code structure improvements for the sideshow clipping.

Here is a patch with some comment changes and code structure improvements for the sideshow clipping. In addition to changing the comments, I did the following:

- removed dummy tweens, since they now duplicated the actual animation tweens
- reordered tween parameters to have the same order as in other tweens
- renamed _animationDone() to _showDone() since it is now only called once from show()
- added this._sideshow.actor.remove_clip() call to _hideDone()
- removed TRANSPARENT_COLOR which is no longer used
- returned result from getWidthToTopActiveWorkspace() immediately instead of keeping an extra result variable

Feel free to review/edit the comments. The full wallpaper patch is good to commit to the common repository, as far as I can tell.

Created attachment 133082 [details] [review]
Candidate commit for the master branch

Thanks! I only removed some trailing and double spaces and fixed a typo.

If the commit message in the attached patch looks good too, I'll try to commit it to the official master branch.

Wow! The result is quite astonishing!

One question though: IMHO the overlay is not transparent enough. I did check several times that I had actually pulled your commit, before noticing that my wallpaper was indeed shown, but very dark. I'd suggest you change
> ROOT_OVERLAY_COLOR.from_pixel(0x000000aa);
to something like
> ROOT_OVERLAY_COLOR.from_pixel(0x00000080);
or a little higher value if you prefer.


And what about the top panel? Do you think it would be nice to use the wallpaper as its background? That could be more logical, and avoid the strange difference we have between backgrounds...

I think being subtle is good ... though whether the current is too subtle probably depends on your background - if it's dark and subtle to begin with, it will end up almost as a constant black.

The panel does look a bit weird to me - would it be better to extend the black "root overlay" underneath the panel as well?

This bug probably should be closed in any case, and enhancements handled separately.

A lighter ROOT_OVERLAY_COLOR will make the text less readable. Compare:

http://picasaweb.google.com/sander.dijkhuis/GNOMEShell#5327947705896991970
http://picasaweb.google.com/sander.dijkhuis/GNOMEShell#5327947705963611202

Both have a plain white wallpaper, but the first has #000000bb as ROOT_OVERLAY_COLOR and the second has #00000080. A new overlay design with more solid rectangles behind the text could solve this (see Brian's mockups), but that would be something for a new bug indeed.

The top panel has the same background as the rest of the overlay, but without the extra 'root overlay'.

Putting the 'root overlay' underneath the panel made it look too grey - see comments #2 and #3 or the original screenshot. Another way to make the panel look nicer in overlay mode should also be suggested in a new bug. (I personally like the current look though, not changing too much when entering overlay mode.)

Agreed, maybe my wallpaper was too dark. I'm trying others to see the result. Though, note that people using plain white wallpapers are strange guys to me... :-p

Playing with wallpapers, I noticed that they are not shown in the overlay with the same size as Nautilus's background: they are always zoomed, which means you only see a rough third of them. Is that due to the fact that you don't resize them as Nautilus does (Mozaic, Zoom, Centered...)? A strange fact is that the picture looks pixellized, just as if it was zoomed too much (see attached screenshot, with one of GNOME's default backgrounds).

Created attachment 133204 [details]
Pixellized background