Created attachment 141256 [details] [review]
Show the background when there is no desktop window

This patch fixes the issue for me, while keeping the old appearance intact when Nautilus is running. Thanks to Owen for figuring out and explaining how the background is drawn.

Comment on attachment 141256 [details] [review]
Show the background when there is no desktop window

I think we need to go through the effort of only adding the extra actor when there is no desktop window ... there will be a significant performance penalty from drawing an extra copy of the background pixmap.

I don't have an exact step-by-step for tracking changes to whether there is a desktop window. Something along the lines of:

 - Track the current workspace changing using the 'workspace-switched' signal on the screen
 - Track the set of windows on the active workspace  with 'window-added', 'window-removed'
 - Find the desktop window the same way as Workspace._init does - looking for a window of type Meta.WindowType.DESKTOP.

For the overview, it's fine to just do a static computation and not deal with nautilus exiting while the overview is up.

Created attachment 146168 [details] [review]
Patch for this bug

Review of attachment 146168 [details] [review]:

Thanks for the patch! Generally, looks good. Needs a bit of reorganization to fit into our style.

For the commit message, can you:

 - Append the bug URL to the bug rather than prepending 'Bug 591912 - ' to the subject
 - Include a body in the commit message (separated by a blank line) that describes what the change does

::: js/ui/main.js
@@ +116,3 @@
 
+    let background = global.create_root_pixmap_actor();
+    background.set_name ("BACKGROUND_ACTOR");

Instead of doing this, you should just store the background actor in a global variable in this file; I think it's find to always add it and just hide/show it rather than adding and removing it. It's drawing which is expensive not having the actor there.

@@ +117,3 @@
+    let background = global.create_root_pixmap_actor();
+    background.set_name ("BACKGROUND_ACTOR");
+    function _showBackground(screen) {

Put this at the toplevel as _onWorkspaceSwitched; calling it showBackground is confusing when it could show or hide, or do nothing and just set up signal connections. Since the function doesn't use the 'screen' parameter, you can omit it - it's always fine to call a JS function with more arguments that it has. Then you won't have to pass in the screen when calling the function initially.

@@ +120,3 @@
+        let has_desktop = false;
+        let workspace = screen.get_active_workspace ();
+        function _show() {

This shouldn't be doubly-nested. Just have a showBackground()/hideBackground() method at the toplevel.

@@ +132,3 @@
+            }
+        }
+        workspace.connect('window-added', function(win) {

As the workspace is switched, you are going to build up more and more connections, you need to save the returned connection ID and disconnect when switching the workspace again.

@@ +134,3 @@
+        workspace.connect('window-added', function(win) {
+            if (win.window_type == Meta.WindowType.DESKTOP) {
+                _hide();

Generally should avoid {} for single-line if/else clauses. (Though two blocks of an if/else should be consistent so if either has {} then both get them.)

@@ +145,3 @@
+        for (let i = 0; i < windows.length; i++) {
+            if (windows[i] === undefined)
+                continue;

I don't see how this could happen, and there are lots of other places we iterate over windows without checking for this.

::: src/shell-global.c
@@ +856,3 @@
+                    NULL);
+      clutter_actor_set_size (CLUTTER_ACTOR (global->root_pixmap),
+                              width, height);

Can you add a comment here: when we handle screen size changes elsewhere, it needs to be handled here as well?

Created attachment 146919 [details] [review]
Updated patch

Review of attachment 146919 [details] [review]:

Looking pretty good. Some small details still, and I don't think resizing the actor from ::paint is a good idea.

::: js/ui/main.js
@@ +122,3 @@
+    _onWorkspaceSwitched(global.screen, -1);
+    background.connect('paint', function() {
+        background.set_size(global.screen_width, global.screen_height);

I don't like setting the size in paint; it basically breaks the ordering of resize/paint that clutter expects. I'd just set the size here on creation and assume we'll fix this up once we start reacting to screen size changes.

@@ +141,3 @@
+    background.show();
+}
+function hideBackground() {

Blank line above

@@ +151,3 @@
+        let old_workspace = screen.get_workspace_by_index(from);
+
+        if (background.windowAddedSignalId !== undefined)

I think it's cleaner to use separate globals rather than tacking properties onto the window object like this.

@@ +169,3 @@
+        return win.window_type == Meta.WindowType.DESKTOP &&
+            (win.get_workspace() == workspaceNum ||
+             win.get_meta_window().is_on_all_workspaces());

It looks to me that workspace.list_windows() will return the windows on the workspace, including sticky windows, so it doesn't seem necessary to do the filtering of global.get_windows() manually.

@@ +173,3 @@
+
+    let windows = global.get_windows().filter(_isDesktop);
+    if (windows.length == 0)

You can use 

 if (global.get_windows().some(_isDesktop))
   hideBackground();
 ...

::: src/shell-global.c
@@ +791,3 @@
+                    NULL);
+      clutter_actor_set_size (CLUTTER_ACTOR (global->root_pixmap),
+                              width, height);

Same comment as in the JS

Created attachment 147590 [details] [review]
Corrected patch

Now do resize background's actor in notify::screen-width.
For use of workspace.list_windows need to correct it annotation.

Created attachment 147591 [details] [review]
Correct meta_workspace_list_windows annotation.

Review of attachment 147591 [details] [review]:

Looks good, thanks!  I'll take care of pushing.

Comment on attachment 147591 [details] [review]
Correct meta_workspace_list_windows annotation.

Attachment 147591 [details] pushed as d59a9c2 - Correct meta_workspace_list_windows annotation.

Review of attachment 147590 [details] [review]:

Looking good, the handling of size changes looks like a good way of doing it. A few details.

::: js/ui/main.js
@@ +127,3 @@
+    background.lower_bottom();
+    _onWorkspaceSwitched(global.screen, -1);
+    global.connect("notify::screen-width", function() {

You should connect to both here - depending on screen-width being notified when screen height changes breaks encapsulation, even if it does work with the current code.

::: src/shell-global.c
@@ +817,3 @@
+
+  g_signal_handlers_disconnect_by_func (stage,
+                                        global_stage_notify_width,

This needs a (gpointer)global_stage_notify_width cast - there's no automatic cast of function pointers to gpointer in C. But actually, you should just connect to the stage at initialization and leave it connected and guard the clutter_actor_set_size() with if (global->root_pixmap). Since it doesn't make sense to have screen-width/screen-height notified properly only when a root pixmap actor is created. That could cause a lot of confusion later :-)

@@ +904,3 @@
 
+      g_signal_connect (stage, "notify::width",
+                        G_CALLBACK (global_stage_notify_width), global);

You need to connect to both width and height here; clutter actor actually short-circuits on no-op changes; and while it's *unlikely* that screen geometry would change in a way that only the height got changed, I think it's easier to be correct than to document an approximation.

Created attachment 147621 [details] [review]
Corrected patch

Doesn't look like this patch detects when the pixmap changes.  That is necessary for supporting manually changing the wallpaper and for wallpaper slideshows.

Unless there is a bug somewhere else...

(In reply to comment #13)
> Doesn't look like this patch detects when the pixmap changes.  That is
> necessary for supporting manually changing the wallpaper and for wallpaper
> slideshows.
background is ClutterClone.
all works fine for me(include wallpaper slideshow).

Ok, then I'm seeing a bug in gnome-settings-daemon where it isn't assuming responsibility for changing the backgrounds after nautilus gives up that role.  Nevermind.  Seems to work well for me then.  :)

I rebased the patch on master, and it seemed to work just fine, until I used the zoom-window-on-scroll functionality in the overview. Then gnome-shell bails out with:

ERROR:gi/object.c:685:object_instance_constructor: assertion failed: (unthreadsafe_template_for_constructor.info == NULL || strcmp(g_base_info_get_name( (GIBaseInfo*) priv->info), g_base_info_get_name( (GIBaseInfo*) unthreadsafe_template_for_constructor.info)) == 0)
Shell killed with signal 6

This happens reliably when the patch is applied, independent of the nautilus show_desktop setting - can someone confirm that this is not an unlucky combination of patches?

Created attachment 148407 [details] [review]
Patch for gjs

> ERROR:gi/object.c:685:object_instance_constructor: assertion failed:
> (unthreadsafe_template_for_constructor.info == NULL ||
> strcmp(g_base_info_get_name( (GIBaseInfo*) priv->info), g_base_info_get_name(
> (GIBaseInfo*) unthreadsafe_template_for_constructor.info)) == 0)
> Shell killed with signal 6

Gnome-shell crashed on following line:
lightbox.js:39
        this._children = container.get_children();

GJS can't create JSObject from ClutterGLXTexturePixmap.
If I wrap ClutterGLXTexturePixmap with ClutterGroup, problem disappear.
object_instance_constructor(gjs/gi/object.c) have a strange bug. Some times, It return proto with different type(like in this case).

Review of attachment 148407 [details] [review]:

Hey, it doesn't really work to have gjs patches attached to gnome-shell bugs. Can you file a new bug against gjs with this? (You can mark a dependency of this bug on that bug.)

Created attachment 151879 [details] [review]
Updated patch

Looks nice.  Others can review the patch.

Created attachment 152356 [details] [review]
show background when nautilus isn't drawing the desktop.

Merge with current HEAD

Review of attachment 152356 [details] [review]:

::: src/shell-global.c
@@ +470,3 @@
     {
+      /* without this, problem appear with creating new Clones from root_pixmap */
+      update_root_window_pixmap (global);

The patch looks great, except for this. The comment needs to explain what problems occur and why. It isn't at all clear to me from this comment.

My expectation is that when the screen size changes, the old root window pixmap will work until replaced (repeating or truncated), and then we'll be notified by a property notify.

If there's some sort of race condition, then that race condition can occur whether or not the screen resolution is changing.

Created attachment 152362 [details] [review]
show background when nautilus isn't drawing the desktop.

> My expectation is that when the screen size changes, the old root window pixmap
> will work until replaced (repeating or truncated), and then we'll be notified
> by a property notify

X server does not emits a root window change event on screen resize.

(In reply to comment #24)
> Created an attachment (id=152362) [details] [review]
> show background when nautilus isn't drawing the desktop.
> 
> > My expectation is that when the screen size changes, the old root window pixmap
> > will work until replaced (repeating or truncated), and then we'll be notified
> > by a property notify
> 
> X server does not emits a root window change event on screen resize.

Based on discussion with Maxim, reading code, and some experimentation, here's my current guess about what is going on:

 - When the screen size changes, gnome-settings-daemon resets the root window pixmap several times. This might be happening because gsd_background_manager.c connects to both GdkScreen::monitors-changed and GdkScreen::size-changed. (I don't see how the screen size could change without changing the monitors.)

 - Each change creates a new persistant X connection. X connection numbers get reused, so it's pretty likely you'll go from:

_XROOTPMAP_ID(PIXMAP): pixmap id # 0x1200001
_XROOTPMAP_ID(PIXMAP): pixmap id # 0x4400001
_XROOTPMAP_ID(PIXMAP): pixmap id # 0x1200001

 - If this happens, update_root_window_pixmap() will still be called and 
   clutter_x11_texture_pixmap_set_pixmap() will still be called

 - clutter_x11_texture_pixmap still does most of its work, but there is a check:

  if (priv->pixmap != pixmap)
    {
[...]
      new_pixmap = TRUE;

      /* The damage object is created on the window if there is any
       * but if there is no window, then we create it directly on
       * the pixmap, so it needs to be recreated with a change in
       * pixmap.
       */
      if (priv->automatic_updates && new_pixmap && !priv->window)
        {
          free_damage_resources (texture);
          create_damage_resources (texture);
        }
    }

   So, when the pixmap gets set back to the same numeric value (though to a different pixmap), Clutter says "hey, no change", and doesn't recreate the damage object. The damage object still points to the old pixmap, so we don't notice changes to the new pixmap.

The added call to update_root_window_pixmap() doesn't make this problem impossible, but it makes it more likely we'll catch the intermediate state and see a change in numeric value.

Can you check if changing update_root_window_pixmap() to do set the Pixmap to None and then to the real value:

+  clutter_x11_texture_pixmap_set_pixmap (CLUTTER_X11_TEXTURE_PIXMAP (global->root_pixmap),
                                         None);
  clutter_x11_texture_pixmap_set_pixmap (CLUTTER_X11_TEXTURE_PIXMAP (global->root_pixmap),
                                         root_pixmap_id);

works around the problem without the need for the call to update_root_window-pixmap() in emit_screen_size_changed_cb()?

(In reply to comment #25)
> Can you check if changing update_root_window_pixmap() to do set the Pixmap to
> None and then to the real value

Same effect (update_root_window_pixmap never called).

(In reply to comment #26)
> (In reply to comment #25)
> > Can you check if changing update_root_window_pixmap() to do set the Pixmap to
> > None and then to the real value
> 
> Same effect (update_root_window_pixmap never called).

What version of:

 gnome-settings-daemon
 gome-desktop

do you have?

(In reply to comment #27)

> What version of:
> 
>  gnome-settings-daemon
>  gome-desktop
> 
> do you have?

2.28.1 (from ubuntu 9.10)

Hey folks.  I haven't followed the review that closely but did want to point out that 2.28 had a few issues related to drawing the backgrounds with gsd.  FYI. One issue off the top of my head that we fixed (iirc in F12 and 2.29) was:
https://bugzilla.gnome.org/show_bug.cgi?id=601203

(In reply to comment #29)
> Hey folks.  I haven't followed the review that closely but did want to point
> out that 2.28 had a few issues related to drawing the backgrounds with gsd. 
> FYI. One issue off the top of my head that we fixed (iirc in F12 and 2.29) was:
> https://bugzilla.gnome.org/show_bug.cgi?id=601203

with 2.29, all works fine.

(In reply to comment #29)
> Hey folks.  I haven't followed the review that closely but did want to point
> out that 2.28 had a few issues related to drawing the backgrounds with gsd. 
> FYI. One issue off the top of my head that we fixed (iirc in F12 and 2.29) was:
> https://bugzilla.gnome.org/show_bug.cgi?id=601203

Ah, that would explain what Maxim is seeing. The patch there was not backported to the 2.28 branch and is not in 2.28.1. If this is the case, you should observe that the background pixmap doesn't update properly when you are not in gnome-shell.

But I'm still confused about one thing - I don't see how calling update_root_window_pixmap() works around the gnome-settings-daemon problem. Whether or not we update the root window pixmap, then we still have a situation where:

 - Pixmap size == 1024x768
 - ClutterTexture size = 1400x1050
 - Clone size = 1400x1050

And since your patch sets the repeat on the ClutterTexture on, I would expect to see a partially tiled copy of the old window pixmap.

(In reply to comment #30)
> (In reply to comment #29)
> > Hey folks.  I haven't followed the review that closely but did want to point
> > out that 2.28 had a few issues related to drawing the backgrounds with gsd. 
> > FYI. One issue off the top of my head that we fixed (iirc in F12 and 2.29) was:
> > https://bugzilla.gnome.org/show_bug.cgi?id=601203
> 
> with 2.29, all works fine.

OK, whether or not I fully understand the problem, it sounds like it was a workaround for a bug in g-s-d. Since it's only affecting:

 - No nautilus running
 - When the window is resized
 - For people with a buggy version of gnome-settings-daemon

I don't think a workaround is necessary. If you are happy with your patch with the call to update_root_window_pixmap() in emit_screen_size_changed_cb removed, please go ahead and commit it.

I think the problem that I described in comment 25 can happen, but it doesn't seem to be happening frequently, and I don't have a good idea of how to fix it, so let's just ignore it :-)

(In reply to comment #32)

> I think the problem that I described in comment 25 can happen, but it doesn't
> seem to be happening frequently, and I don't have a good idea of how to fix it,
> so let's just ignore it :-)

I did file http://bugzilla.openedhand.com/show_bug.cgi?id=1963 to record the possibility of a problem.

Patch was comitted a while ago.