Created attachment 86547 [details] [review]
register with dbus earlier

The above patch registers with gnome-settings-daemon with the session bus earlier, and defers loading of the various subsystems to idle handlers so that bus requests can be processed in between the subsystem calls.

What if "awake" returns and the caller assumes g-s-d is fully prepared - while it is actually not? Aren't you introducing some kind of race condition?

So to be clear, you're saying,

gnome-session calls Awake and blocks. When Awake returns gnome-session continues assuming the settings daemon is fully loaded.  With the proposed change, Awake will return before the settings daemon is fully loaded and so gnome-session will continue to load the session while settings daemon is loading in the background.

I don't think this could cause real problems in practice.  It might mean for instance that font display properties, theme, and screen resolution might change while things are loading.

If it does turn out to be a problem, then a new signal could be added to the dbus interface "Loaded" or something that would get emitted when the settings daemon is fully loaded (or subsystem specific signals could get emitted when specific subsystems are loaded).  gnome-session could then block for the signal.

Probably not worth adding the new signal unless it fixes a problem someone is actually running into though.

Consider another scenario: some capplet "awakes" g-s-d while it is not loaded (for example, crashed at startup (or the user is in another desktop;). I know for fact that all capplets do it (there is activate_settings_daemon in capplets/common).

I am just not sure whether some capplet relies on the g-s-d being fully executed - don't you know, by any chance? I think it should be checked before this patch is applied, shouldn't it?

Well, I believe that most of the capplets set gconf keys in response to user input and expect the settings daemon to act on those changes.

This means that if the settings daemon is still loading there will be a delay between the user choosing an option and it actually taking effect.

Again, though, I don't think that's a real problem in practice.  

1) For most users the settings daemon will already be running.  One of the cases you mentioned, the crash scenario isn't really likely.  If settings daemon crashes, gnome-session restarts it.  If the settings daemon crashes in a loop until gnome-session gives up then the odds of it crashing when the capplet tries to start it are pretty high, too, so we lose anyway.

2) For the other desktop case, users may see a small delay when, e.g., changing themes, but changing themes is already a fairly slow operation.  And it's not like the slowdown wasn't present before, it's just moved from when the dialog first comes up to while (and potentially after) the dialog comes up.

If it does end up being a problem, we can always do what I mentioned in comment 3.

OK, I am happy with your explanations. If there are no more objections - I can commit your patch, if you like.

Sounds fine with me, unless Sebastien has some comments first.

I think rodrigo is working on redesigning g-s-d (http://mail.gnome.org/archives/gnomecc-list/2007-March/msg00103.html), maybe ask him if the change doesn't conflict with his work before commiting

Hi Rodrigo, can you comment on how this patch interacts with your changes?

Sebastien, I considered that patch as kind of intermediate step - but you're right, we have to ask Rodrigo.

It conflicts with the changes I am planning, that's why I haven't answered to the bug. I was waiting to make my changes to include some of the changes (which might make sense) on SVN trunk.

So, not sure if we want it for 2.18, if so, please commit there, and leave the bug open and I'll take care of it for 2.19

Rodrigo, doesn't it look like a too big change for 2.18? I would not commit it into 2.18 unless there are very strong reasons to do so.

I have incorporated some of the changes to SVN trunk, please Matthias have a look if it is enough. Reopen if there's something more missing.

Matthias isn't actually on the CC list of this bug, but I'll have a look :-)  Thanks for looking into it.