Created attachment 175603 [details] [review]
background: draw the background on startup if show-desktop-icons unset

Ray - can you review the attached - it results in no background when you log in if you have nautilus off. (I'm guessing it's new since the gsettings conversion because gconf probably had some excess notifications.)

Review of attachment 175603 [details] [review]:

::: plugins/background/gsd-background-manager.c
@@ +228,3 @@
+                } else {
+                        gnome_bg_load_from_preferences (manager->priv->bg,
+                                                        manager->priv->settings);

So this first hunk makes a ton of sense and could be committed right away. It should probably be a separate commit than the second hunk, though, since it's an indepedendent change.

@@ +405,3 @@
         if (!show_desktop_icons) {
                 setup_bg (manager);
+                draw_background (manager, FALSE);

On the surface this change seems quite straight-forward and apparent, but there's a little subtlety here, I think. I mean it's more than just a forgotten line in a previously seldom used code path.

- First, setup_bg calls gnome_bg_load_from_preferences.  load_from_preferences will cause a "changed" signal to get emitted the first time it's called (implicitly from gnome_bg_set_filename and friends).  Looking through history, the code used to connect to that "changed" signal and react to it by calling draw_background.  Reading through the code, it no longer listens for the "changed" signal but instead tracks a "change-event" signal on the gsetting object.  So the bug apparently has arisen because the "changed" signal has the semantics of always firing once on startup and the "change-event" signal has the semantics of only firing for changes after startup.  Whether the right fix is to make it use the "changed" signal again or to do draw_background at startup, I'm not sure.  Tomas would be a good person to talk to, I think. Certainly your approach has the appeal of being a lot less invasive.  I would say the commit message should give some history about why the call is needed now when it wasn't before.
- Second, the code as designed does a cross fade for new wallpapers and no cross fade from new frames within animated wallpapers.  Your change disables the cross fade for the initial draw.  In a no-nautilus-uncomposited world that means we won't get a cross fade from login screen to session.  Clearly, that case isn't one to spend too much time worrying about.  In the case we do care about (with shell doing compositing etc) we really don't want a g-s-d managed crossfade, I think, since it makes sense for the shell to manage effects and the initial transition from login screen to session is an effect. (and the root window isn't redirected anyhow so g-s-d couldn't visibly do the crossfade even if it wanted to).  I guess my point is, changing draw_background cross_fade=TRUE to cross_fade=FALSE makes sense by default, but it should happen across the board, not just at startup (or maybe only if gdk_screen_is_composited is TRUE?).  If we do make that change, though, I think it should be very explicit with a commit of its own (so either do a commit first that changes all draw_background (manager, TRUE) calls and then add your missing draw_background call, or do add your missing draw_background call with a value of TRUE, and then toggle it to FALSE with all other draw_background (...TRUE) calls in a subsequent commit.

Created attachment 175715 [details] [review]
background: handle Nautilus running sporadically

I agree the current situation is stupid and yes, the "changed" signal connection should not have been removed, my fault. This will solve the no-background on startup case.

The logic is broken but given the special purpose of GnomeBg, it works currently. GnomeBg is not watching GSettings for changes, thus we have the "change-event" signal here, in which handler we need to call gnome_bg_load_from_preferences(). This is completely different what "changed" signal of GnomeBg is meant to do at the moment - it's spawned when some of its properties change. E.g. gnome_bg_load_from_preferences() spawns "changed" when some settings differ.

With nautilus not being the app that runs all the time (kicked out of session/startup, show-desktop-icons = TRUE and only spawned when folder window is required -- though this is stupid combination) I guess we can't count with the behaviour we have. The attached patch could fix corner cases and ensure we always watch for GSettings changes. Note that this patch is quite separate from what we discuss here and listening to GnomeBg's changed signal is still required. I couldn't test the patch due to recent requirement bumps in g-s-d. Basically it would be better to check every time if nautilus is running and do nothing if it also handles the desktop. Please discuss.


(In reply to comment #3)
> ::: plugins/background/gsd-background-manager.c
> @@ +228,3 @@
> +                } else {
> +                        gnome_bg_load_from_preferences (manager->priv->bg,
> +                                                       
> manager->priv->settings);
> 
> So this first hunk makes a ton of sense and could be committed right away. It
> should probably be a separate commit than the second hunk, though, since it's
> an indepedendent change.

Good catch.

> In the case we do care about (with shell doing compositing etc) we really
> don't want a g-s-d managed crossfade, I think, since it makes sense for the 
> shell to manage effects and the initial transition from login screen to
> session is an effect. (and the root window isn't redirected anyhow so g-s-d 
> couldn't visibly do the crossfade even if it wanted to).

Does gnome-shell manage background in some way? Can we still draw to xroot window while composited?

Review of attachment 175715 [details] [review]:

::: plugins/background/gsd-background-manager.c
@@ +171,3 @@
+                                                     "show-desktop-icons");
+
+        if (nautilus_is_running () && show_desktop_icons) {

So this change makes a lot of sense.  We don't care about whether nautilus is running, just about whether its managing the desktop.  It might make sense to rename nautilus_is_running to nautilus_is_drawing_background or something and envelop the show-desktop-icons check directly in the function.

@@ +224,3 @@
+        gnome_bg_load_from_preferences (manager->priv->bg,
+                                        manager->priv->settings);
+        draw_background (manager, use_crossfade);

Seems fine.

@@ +274,3 @@
 {
         manager->priv->timeout_id = 0;
+

Seems fine.  I guess we're doing a little more work in the nautilus-is-managing-the-desktop case, but that's no big deal.

(In reply to comment #4)
> The logic is broken but given the special purpose of GnomeBg, it works
> currently. GnomeBg is not watching GSettings for changes, thus we have the
> "change-event" signal here, in which handler we need to call
> gnome_bg_load_from_preferences(). This is completely different what "changed"
> signal of GnomeBg is meant to do at the moment - it's spawned when some of its
> properties change. E.g. gnome_bg_load_from_preferences() spawns "changed" when
> some settings differ.
Okay, I don't understand why gnomebg doesn't listen for changes itself, but i'm sure there's a good reason and won't question it :-)

> With nautilus not being the app that runs all the time (kicked out of
> session/startup, show-desktop-icons = TRUE and only spawned when folder window
> is required -- though this is stupid combination) I guess we can't count with
> the behaviour we have. The attached patch could fix corner cases and ensure we
> always watch for GSettings changes. Note that this patch is quite separate from
> what we discuss here and listening to GnomeBg's changed signal is still
> required. I couldn't test the patch due to recent requirement bumps in g-s-d.
> Basically it would be better to check every time if nautilus is running and do
> nothing if it also handles the desktop. Please discuss.
Makes sense.

> Does gnome-shell manage background in some way? Can we still draw to xroot
> window while composited?
Well, you can draw to the root window but it's being completely covered up by a full screen window (called the cow).  The cow is the "movie player" that all your redirected windows are drawn on (with added drop shadows and other effects).  Almost all visible windows are redirected to this window.  One exception is the root window.  That means when you draw to the root window, it won't show up on screen since it's covered up by the cow. This isn't normally a problem because we store what pixmap the root window is drawn with in a property on the root window called _XROOTPMAP_ID.  mutter looks at the property and handles drawing the background of the cow with the same pixmap.  pixmaps don't have events though, so you won't get updates as the same pixmap changes (you'll only get updates when new pixmaps are in place).  That means I don't think we'll get intermediate frames from a cross fade.

(In reply to comment #6)
> pixmaps
> don't have events though, so you won't get updates as the same pixmap changes
> (you'll only get updates when new pixmaps are in place).  That means I don't
> think we'll get intermediate frames from a cross fade.
One idea is we could ChangeProperty the _XROOTPMAP_ID to the same value it was before during animation so the compositor would know to update the cow.

I think a better approach though is to disable crossfades from settings daemon if we're compositing and have any effects happen from the shell/mutter side.

(In reply to comment #7)
> (In reply to comment #6)
> > pixmaps
> > don't have events though, so you won't get updates as the same pixmap changes
> > (you'll only get updates when new pixmaps are in place).  That means I don't
> > think we'll get intermediate frames from a cross fade.
> One idea is we could ChangeProperty the _XROOTPMAP_ID to the same value it was
> before during animation so the compositor would know to update the cow.
> 
> I think a better approach though is to disable crossfades from settings daemon
> if we're compositing and have any effects happen from the shell/mutter side.

Not sure it works for us to draw a pixmap after gnome-control-center has called XDestroyPixmap() on it even if we have previously bound it to a texture. Would be nice if it worked but the specs don't guarantee it, and there have been problems with recent versions of Mesa. That means we can't do a cross-fade between two successive XROOTPMAP_ID images.

Probably better long-term for us to link to GnomeBg and handle everything ourselves - that would avoid race conditions on startup, etc. In that world, Nautilus would probably create it's desktop window as ARGB with a transparent background.

For the short term, I'd say just do whatever you were doing in GNOME 2.32 for cross fades.

(In reply to comment #8)
> Not sure it works for us to draw a pixmap after gnome-control-center has called
> XDestroyPixmap() on it even if we have previously bound it to a texture. Would
> be nice if it worked but the specs don't guarantee it, and there have been
> problems with recent versions of Mesa. That means we can't do a cross-fade
> between two successive XROOTPMAP_ID images.
Oh, an implication of this is that mutter binds straight to the root window pixmap with texture-from-pixmap or some such, not just sourcing from it (like I originally thought). In that case, the texture should get updated automatically under the covers when the pixmap changes, I guess, and cross fades should work okay without doing anything special.

> Probably better long-term for us to link to GnomeBg and handle everything
> ourselves - that would avoid race conditions on startup, etc. In that world,
> Nautilus would probably create it's desktop window as ARGB with a transparent
> background.

right, although i don't think there's going to be a nautilus desktop window long term?

> For the short term, I'd say just do whatever you were doing in GNOME 2.32 for
> cross fades.
I was making an assumption it wouldn't work, but it sounds like it might continue to work okay.

Comment on attachment 175603 [details] [review]
background: draw the background on startup if show-desktop-icons unset

It sounds like Tomas thinks we should add support for the "changed" signal back instead of calling draw_background from manager_start so I'm marking this rejected.  I'll push the top hunk.

Created attachment 175783 [details] [review]
background: always check if Nautilus is running

(In reply to comment #5)
> ::: plugins/background/gsd-background-manager.c
> @@ +171,3 @@
> +                                                     "show-desktop-icons");
> +
> +        if (nautilus_is_running () && show_desktop_icons) {
> 
> So this change makes a lot of sense.  We don't care about whether nautilus is
> running, just about whether its managing the desktop.  It might make sense to
> rename nautilus_is_running to nautilus_is_drawing_background or something and
> envelop the show-desktop-icons check directly in the function.
Right, only called once, makes sense.

> @@ +274,3 @@
>  {
>          manager->priv->timeout_id = 0;
> +
> 
> Seems fine.  I guess we're doing a little more work in the
> nautilus-is-managing-the-desktop case, but that's no big deal.
Perhaps. In the old code we bailed out on the beginning but if something happened (like Nautilus quit) we would end with no event listening. Is that what you meant here?

(In reply to comment #6)
> Okay, I don't understand why gnomebg doesn't listen for changes itself, but i'm
> sure there's a good reason and won't question it :-)
For special cases like gnome-screensaver we need to load settings either from a different place or use system defaults. See bug 632566 for details. Once we have a better way for accessing defaults in GSettings, we could make gnome-bg cleaner.

(In reply to comment #10)
> (From update of attachment 175603 [details] [review])
> It sounds like Tomas thinks we should add support for the "changed" signal back
> instead of calling draw_background from manager_start so I'm marking this
> rejected.  I'll push the top hunk.
Attached new patch.

Created attachment 175784 [details] [review]
background: reintroduce GnomeBg "changed" signal

Second patch adding the "changed" signal listener back. Slightly modified the event path due to this change.

Review of attachment 175784 [details] [review]:

Looks great! So in response to configuration changes we call load_from_preferences again which may emit a changed signal. We only ever draw the background in response to the changed signal getting emitted (or screen size changes).

I think this is good to go (although I'd flesh out the commit message a little more)

Committed to master, please check if it resolves your issues.

commit d7a6f707c9e6f924fea7565967d9c5c1ad8b1533
Author: Tomas Bzatek <tbzatek@redhat.com>
Date:   Tue Dec 7 16:40:09 2010 +0100

    background: reintroduce GnomeBg "changed" signal
    
    GnomeBG sends "changed" signal when any property changes, also when
    settings is loaded from GSettings. Listening to it would assure
    that we set background on plugin startup.
    
    See bug 636191 for details.

commit d22b75db508a792e3786a2011b707e930a0f0355
Author: Tomas Bzatek <tbzatek@redhat.com>
Date:   Tue Dec 7 16:24:19 2010 +0100

    background: always check if Nautilus is running
    
    This changes the way we detect that Nautilus is managing desktop.
    We can no longer rely on having Nautilus a key component in the session.
    
    See bug 636191 for details.