GNOME Bugzilla – Bug 626018
Port desktop background stuff to GSettings
Last modified: 2010-11-09 17:54:01 UTC
Created attachment 167109 [details] [review] Port desktop background handling to gsettings Continuation of removing last traces of GConf, see bug 626017 for my previous cleanup patches. Using desktop background schema from gsettings-desktop-schemas and defines. Since Nautilus uses gnome-bg from gnome-desktop package, we're still tied with their API. As a result, there's still usage of gconf for saving data. Better to port this in gnome-desktop directly (involving API break) rather than making our own workarounds. Gnome-bg should be also ported to use gdesktop-enums.h and support opacity. I've also removed the desktop_gconf_notification_timeout thing, which I suppose was used as a workaround for delaying gconf keys change notification, incoming separately for each monitored key. Now that since we have GSettings/dconf, I expect multiple keys being changed at once, atomically. That's also why I listen to 'change-event' and not 'changed' signal. Setting as 'needs-work' since we want to make desktop background fully functional and not having options on two places.
Filed bug 626021 for porting gnome-bg to GSettings.
Review of attachment 167109 [details] [review]: Looks mostly good, and removed code is good too :) I inlined a comment below. ::: libnautilus-private/nautilus-directory-background.c @@ +195,3 @@ { + /* Set the keys to default values. There's no way to get + * schema default values in GSettings though. You should use g_settings_reset() for this.
Created attachment 168417 [details] [review] complete port Attaching new patch, which is a complete port to new gnome-bg API - see bug 626021. Obsolete code removed, we now rely on gnome-bg code for loading and saving configuration. Monitoring code is still there. Needs bug 627369 to be fixed in order to receive GSettings keys change notifications.
Review of attachment 168417 [details] [review]: Looks increasingly good; how hard would it be to drop EelBackground entirely and only use GnomeBG in Nautilus? What do we need EelBackground for that GnomeBG isn't capable to do? ::: eel/eel-background.c @@ +1092,3 @@ + + filename = gnome_bg_get_filename (background->details->bg); + if (gnome_bg_get_draw_background (background->details->bg) && filename != NULL) Please add braces around if statements like this. @@ +1104,3 @@ + if (shading == G_DESKTOP_BACKGROUND_SHADING_VERTICAL || + shading == G_DESKTOP_BACKGROUND_SHADING_HORIZONTAL) { + *color = eel_gradient_new (start_color, end_color, shading == G_DESKTOP_BACKGROUND_SHADING_HORIZONTAL); Wrong indentation.
(In reply to comment #4) > Review of attachment 168417 [details] [review]: > > Looks increasingly good; how hard would it be to drop EelBackground entirely > and only use GnomeBG in Nautilus? What do we need EelBackground for that > GnomeBG isn't capable to do? Thanks for the review. I'm doing small changes to the gnome-desktop part, will commit this later with the whitespace fixes. From what I understood the EelBackground is heavily used for setting per-folder background and also acts as a universal drop target when dragging an image file. While it is possible to do some refactoring and split desktop vs. folder background handling, I don't think it's worth it at the moment.
Review of attachment 168417 [details] [review]: Setting as 'needs-work' as the nautilus code has been refactored and simplified in the meantime.
Created attachment 172627 [details] [review] patch new-generation A new patch, against master, much much more simplified (thx Cosimo for ripping out the old code). Still the g_idle roundtrip is there to avoid recursion from events (save_to_bg -> emits changed event -> load_from_bg --> deadlock). Dropping image file properly sets new background. TODO: when 632225 is in, bind to global GSettings object instead of using private one.
Created attachment 172726 [details] [review] patch new-generation (In reply to comment #7) > TODO: when 632225 is in, bind to global GSettings object instead of using > private one. Attaching updated patch to reflect these recent changes.
Review of attachment 172726 [details] [review]: Looks good to me to commit after the gnome-bg API changes are in.
Created attachment 173824 [details] [review] nautilus patch Little update for latest gnome-bg changes, only passing our GSettings object instance to gnome_bg_{load_from,save_to}_preferences().
commit b118093fbc2b5a236a4502ac6e053ab9e64a9db1 Author: Tomas Bzatek <tbzatek@redhat.com> Date: Tue Nov 9 18:51:50 2010 +0100 Port desktop background handling to GSettings See bug 626018 for details.