GNOME Bugzilla – Bug 741590
Allow setting the lock screen background
Last modified: 2015-01-21 10:41:14 UTC
We can set the desktop background from gnome-photos, but not the lockscreen background. We should be able to do that too.
Created attachment 293639 [details] [review] Use macros instead of hard-coded strings Use macros for GSetting's keys
Created attachment 293640 [details] [review] Change names to distinguish between background and desktop background This would make it easy to distinguish between lockscreen background and desktop background. And let's use the generic term 'background' to refer to both of these background types.
Created attachment 293641 [details] [review] Allow setting the lock screen background from the preview
Review of attachment 293639 [details] [review]: ::: src/photos-application.c @@ +60,3 @@ +#define WP_SHADING_KEY "color-shading-type" +#define WP_PCOLOR_KEY "primary-color" +#define WP_SCOLOR_KEY "secondary-color" I see that you got these from gnome-control-center/panels/background/cc-background-panel.c :) Since these are local to this file (or translation unit), it would be better to use static const gchar *. eg., static const gchar *DESKTOP_BACKGROUND_SCHEMA = "org.gnome.desktop.background"; static const gchar *DESKTOP_KEY_PICTURE_URI = "picture-uri"; ... This would be more consistent with how things are elsewhere in the code base (see src/photos-query-builder.c) and avoid the problems of using preprocessor macros (eg., not having the symbols in the debugger).
Review of attachment 293640 [details] [review]: Or, we can call one background and the other screensaver to match the names of the schemas, which are org.gnome.desktop.background and org.gnome.desktop.screensaver. The user visible string for the screensaver can be "Set as Lock Screen" to match the Settings -> Background UI.
Created attachment 293944 [details] [review] Use variable names instead of hard coded strings
Created attachment 293945 [details] [review] change variable names to point to their meaning clearly
Created attachment 293946 [details] [review] allow setting lockscreen background
Review of attachment 293944 [details] [review]: A couple of small things. The subject (or the first line) of the commit message should not have a trailing period. See the third point of https://wiki.gnome.org/Git/CommitMessages ::: src/photos-application.c @@ +93,3 @@ +static const gchar *DESKTOP_KEY_COLOR_SHADING_TYPE = "color-shading-type"; +static const gchar *DESKTOP_KEY_PRIMARY_COLOR = "primary-color"; +static const gchar *DESKTOP_KEY_SECONDARY_COLOR = "secondary-color"; Move these below - between the enum and the typedef. See src/photos-indexing-notification.c The reason is that we club everything that has got to do with defining the type (the private struct, signals, properties, G_DEFINE_TYPE*, etc.) together at the top, and then put the other stuff (like constants, strings, etc.) below that.
Created attachment 294042 [details] [review] Part 1: Use variable names instead of hard coded strings
Created attachment 294043 [details] [review] Part 3: allow setting lockscreen background
Created attachment 294044 [details] [review] Part 2: change variable names to point to their meaning clearly fixed the commit message, previous commit message exceeding 72 character limit
Review of attachment 294042 [details] [review]: Perfect.
Review of attachment 294044 [details] [review]: ::: src/photos-application.c @@ +496,2 @@ static void +photos_application_set_desktop_bg_download (GObject *source_object, GAsyncResult *res, gpointer user_data) The nomenclature is actually getting a bit confusing now. I am saying this because the name of the GSettings pointer is bg_settings, while some of the functions are now desktop_bg or desktop_background. I think we should stick to 'bg' or 'background' here. ::: src/photos-preview-menu.ui @@ +17,3 @@ <item> + <attribute name="action">app.set-desktop-background</attribute> + <attribute name="label" translatable="yes">Set as Desktop Background</attribute> Is this change really needed? I think "Set as Background" is fine. It is consistent with Settings -> Background, and I don't want to needlessly change user visible strings because they affect translations.
Review of attachment 294043 [details] [review]: ::: src/photos-application.c @@ +62,3 @@ GResource *resource; GSettings *bg_settings; + GSettings *screensaver_settings; If you want to use 'bg_settings' as the name of the other pointer, then 'ss_settings' is a more fitting name. Or rename the other one to 'background_settings' in the previous patch. @@ +63,3 @@ GSettings *bg_settings; + GSettings *screensaver_settings; + GSettings *current_settings; We should not use the current_settings pointer as a private variable because it does not represent a state of the object. Instead it is used to carry local state from one async function to another. More about it later. @@ +73,3 @@ GSimpleAction *sel_none_action; GSimpleAction *set_bg_action; + GSimpleAction *set_screensaver_action; Ditto. @@ +533,3 @@ +/* + Common function to set both desktop and lockscreen background. +*/ Is the comment really needed? It should be obvious from reading the code. :) @@ +548,3 @@ + priv->current_settings = priv->bg_settings; + else if (g_strcmp0 (name, "set-lock-background") == 0) + priv->current_settings = priv->screensaver_settings; The problem with doing it this way is that if the user sets the background again while the previous download is still going on, then the value of priv->current_settings will change while the previous operation is still going on. This can happen if someone sets the background and lockscreen background in quick succession and the pictures are not local. Instead, you can retrieve the settings pointer using g_object_get_data (see below), and pass that as user_data in photos_base_item_download_async. @@ +948,3 @@ g_action_map_add_action (G_ACTION_MAP (self), G_ACTION (priv->sel_none_action)); priv->set_bg_action = g_simple_action_new ("set-desktop-background", NULL); What you can do is use g_object_set_data_full on the action to set the settings pointer on it. ::: src/photos-preview-menu.ui @@ +20,3 @@ </item> + <item> + <attribute name="action">app.set-lock-background</attribute> Let's stick to 'screensaver' for all user invisible / internal names. So 'set-screensaver'. @@ +21,3 @@ + <item> + <attribute name="action">app.set-lock-background</attribute> + <attribute name="label" translatable="yes">Set as Lockscreen Background</attribute> It is spelt Lock Screen, and should be 'Set as Lock Screen' for consistency with Settings -> Background.
Created attachment 294548 [details] [review] Part 2 :Change variable names to point to their meaning clearly This would help avoid the confusion with new settings pertaining to setting of lockscreen background from the application
Created attachment 294549 [details] [review] Part 3: Allow setting the lock screen background from the preview
Review of attachment 294549 [details] [review]: Much better. ::: src/photos-application.c @@ +939,2 @@ g_action_map_add_action (G_ACTION_MAP (self), G_ACTION (priv->set_bg_action)); + g_object_set_data_full (G_OBJECT (priv->set_bg_action), "settings", priv->bg_settings, g_object_unref); There should be a g_object_ref to match the g_object_unref. @@ +943,3 @@ + g_signal_connect_swapped (priv->set_ss_action, "activate", G_CALLBACK (photos_application_set_bg_common), self); + g_action_map_add_action (G_ACTION_MAP (self), G_ACTION (priv->set_ss_action)); + g_object_set_data_full (G_OBJECT (priv->set_ss_action), "settings", priv->ss_settings, g_object_unref); Ditto.
Review of attachment 294548 [details] [review]: Perfect. Thanks, Pranav.
Created attachment 295081 [details] [review] Allow setting the lock screen background from the preview Fixed the previous patch and committed.
Thank you for working on this.