Created attachment 172924 [details] [review]
[gsm] Add autostart condition through GSettings

A couple of questions though:
- Do we still need to support those GNOME autostart conditions through GConf, or would just GSettings do?
- Looking through my /usr/share/applications, the only app that used "AutostartCondition=GNOME" was nautilus, and none of the screen readers, or others used it.
- What happens when we enable the magnifier (by ticking the magnifier enabled gsettings key) and gnome-shell enables its builtin magnifier, and gnome-session starts a separate magnifier?

The patch looks good at first sight.

It looks like the patch doesn't support one weird thing we had, iirc: indirection. Where a gconf key was actually just containing the path to another key which was the boolean one. Not sure we still need that, though. Probably not.

Btw, I like the fact that you check if the schema exists. Nice way to avoid crashes if the schema is missing ;-)

(In reply to comment #2)
> A couple of questions though:
> - Do we still need to support those GNOME autostart conditions through GConf,
> or would just GSettings do?

We probably still want gconf, in some way. Maybe we can have a degraded support for that, where we use a small helper that would look at the gconf value (but we wouldn't monitor the key anymore). This way we don't link to gconf, which is always nice.

Another thing we might want to do is to use GSettings instead of GNOME in the value of AutostartCondition: this would also work in other desktops that use GSettings. And this makes it easier to avoid the gconf vs gsettings thing.

> - Looking through my /usr/share/applications, the only app that used
> "AutostartCondition=GNOME" was nautilus, and none of the screen readers, or
> others used it.

Look at /etc/xdg/autostart and /usr/share/gnome/autostart :-)

> - What happens when we enable the magnifier (by ticking the magnifier enabled
> gsettings key) and gnome-shell enables its builtin magnifier, and gnome-session
> starts a separate magnifier?

We need a way to mark autostart apps for Classic GNOME vs GNOME Shell, so we would use this. We'll probably bring back "profiles" in gnome-session with this, but well... So just ignore this for now, I'd say.

(In reply to comment #3)
> The patch looks good at first sight.
> 
> It looks like the patch doesn't support one weird thing we had, iirc:
> indirection. Where a gconf key was actually just containing the path to another
> key which was the boolean one. Not sure we still need that, though. Probably
> not.

I couldn't find any applications that did that, and the code doesn't seem to implement it. Maybe you're confusing with the gtk modules loading (which does do that...).

> Btw, I like the fact that you check if the schema exists. Nice way to avoid
> crashes if the schema is missing ;-)

It still will crash if the key doesn't exist, which could be a problem with mismatching versions. Not that big a deal I guess.

> (In reply to comment #2)
> > A couple of questions though:
> > - Do we still need to support those GNOME autostart conditions through GConf,
> > or would just GSettings do?
> 
> We probably still want gconf, in some way. Maybe we can have a degraded support
> for that, where we use a small helper that would look at the gconf value (but
> we wouldn't monitor the key anymore). This way we don't link to gconf, which is
> always nice.
> 
> Another thing we might want to do is to use GSettings instead of GNOME in the
> value of AutostartCondition: this would also work in other desktops that use
> GSettings. And this makes it easier to avoid the gconf vs gsettings thing.

Right. I'll do those changes.

> > - Looking through my /usr/share/applications, the only app that used
> > "AutostartCondition=GNOME" was nautilus, and none of the screen readers, or
> > others used it.
> 
> Look at /etc/xdg/autostart and /usr/share/gnome/autostart :-)

Better ;)

> > - What happens when we enable the magnifier (by ticking the magnifier enabled
> > gsettings key) and gnome-shell enables its builtin magnifier, and gnome-session
> > starts a separate magnifier?
> 
> We need a way to mark autostart apps for Classic GNOME vs GNOME Shell, so we
> would use this. We'll probably bring back "profiles" in gnome-session with
> this, but well... So just ignore this for now, I'd say.

OK, will file a separate bug for that.

Created attachment 172943 [details] [review]
[gsm] Add autostart condition through GSettings

This should fix those comments you had.

Created attachment 172985 [details] [review]
[gsm] Add autostart condition through GSettings

Comment on attachment 172943 [details] [review]
[gsm] Add autostart condition through GSettings

Only connect to the "changed" signal for the key we're interested in, and indentation fixes.

Review of attachment 172985 [details] [review]:

::: gnome-session/gsm-autostart-app.c
@@ +317,3 @@
+        if (strchr (key, ' ') == NULL)
+                return FALSE;
+

I guess you can remove this part now?

@@ +741,3 @@
                 g_object_unref (client);
+        } else if (kind == GSM_CONDITION_GSETTINGS) {
+                disabled = !setup_gsettings_condition_monitor(GSM_AUTOSTART_APP (app), key, FALSE);

I'd think that we don't need the same function here: when this gets called, we should already have app->priv->condition_settings that is set to something, so we could just do g_settings_get_boolean(), couldn't we? (If condition_settings is NULL, it just means the schema doesn't exist)

This means we can get rid of the last parameter to setup_gsettings_condition_monitor().

Created attachment 173177 [details] [review]
[gsm] Add autostart condition through GSettings

And updated for the latest review comments.

Comment on attachment 173177 [details] [review]
[gsm] Add autostart condition through GSettings

Thanks, please commit!

Committed, thanks!


commit 2f56ee1acfdbe0158408317b29a2fb81dc5c960b
Author: Bastien Nocera <hadess@hadess.net>
Date:   Thu Oct 21 12:16:30 2010 +0100

    [gsm] Add autostart condition through GSettings
    
    https://bugzilla.gnome.org/show_bug.cgi?id=632724