GNOME Bugzilla – Bug 302347
patch to add gnome-screensaver support
Last modified: 2005-07-25 11:58:44 UTC
Here is a patch to add gnome-screensaver support. Should anyone ever need such a thing...
Created attachment 45803 [details] [review] patch for head
Created attachment 46030 [details] [review] updated patch Updated patch to fall back to xscreensaver if gnome-screensaver is not in the path. I left out changes to the display capplet since they shouldn't be necessary with gnome-screensaver. Does this look ok to commit?
Comment on attachment 46030 [details] [review] updated patch Thanks for the patch, some comments: >diff -p -u -r1.12 gnome-settings-multimedia-keys.c >--- gnome-settings-daemon/gnome-settings-multimedia-keys.c 17 Feb 2005 18:19:06 -0000 1.12 >+++ gnome-settings-daemon/gnome-settings-multimedia-keys.c 4 May 2005 20:26:06 -0000 >@@ -1,4 +1,5 @@ >-/* >+/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- >+ * > * Copyright (C) 2001,2002,2003 Bastien Nocera <hadess@hadess.net> > * > * gnome-settings-multimedia-keys.c >@@ -44,7 +45,7 @@ > > #define DIALOG_TIMEOUT 1000 /* dialog timeout in ms */ > #define VOLUME_STEP 6 /* percents for one volume button press */ >- >+ > /* we exclude shift, GDK_CONTROL_MASK and GDK_MOD1_MASK since we know what > these modifiers mean > these are the mods whose combinations are bound by the keygrabbing code */ Please don't mix such changes to the patch. >@@ -799,6 +800,25 @@ do_sound_action (Acme *acme, int type) > } > > static void >+do_screensaver_action (Acme *acme) >+{ >+ char *command; >+ char *path; >+ >+ path = g_find_program_in_path ("gnome-screensaver-command"); >+ if (path) { >+ command = g_strdup_printf ("%s --lock", path); >+ g_free (path); >+ } else { >+ command = g_strdup ("xscreensaver-command -lock"); >+ } >+ >+ execute (command, FALSE); >+ >+ g_free (command); >+} Any reason to use "Acme *acme" here? "You also prefer gnome-screensaver-command" than "xscreensaver-command", are they both installable in the same time? Any reason to not give the choice to the user? >diff -p -u -r1.2 gnome-settings-screensaver.c >--- gnome-settings-daemon/gnome-settings-screensaver.c 30 Apr 2002 09:45:05 -0000 1.2 >+++ gnome-settings-daemon/gnome-settings-screensaver.c 4 May 2005 20:26:06 -0000 >@@ -1,6 +1,6 @@ >-/* -*- mode: c; style: linux -*- */ >- >-/* gnome-settings-screensaver.c >+/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- >+ * >+ * gnome-settings-screensaver.c > * > * Copyright (C) 2002 Sun Microsystems, Inc. > * same comment as before >@@ -33,7 +33,6 @@ > > #define START_SCREENSAVER_KEY "/apps/gnome_settings_daemon/screensaver/start_screensaver" > #define SHOW_STARTUP_ERRORS_KEY "/apps/gnome_settings_daemon/screensaver/show_startup_errors" >-#define XSCREENSAVER_COMMAND "xscreensaver -nosplash" > > void > gnome_settings_screensaver_init (GConfClient *client) Any reason to remove this? >@@ -41,12 +40,6 @@ gnome_settings_screensaver_init (GConfCl > /* > * do nothing. > * >- * our settings only apply to startup, and the screensaver >- * settings are all in xscreensaver and not gconf. >- * >- * we could have xscreensaver-demo run gconftool-2 directly, >- * and start / stop xscreensaver here >- * > */ > } Any reason to drop these explanation rather than updating them to mention the new case?
Thanks for taking a look at the patch. Is adding the mode line really a problem? It helps maintain a consistent style. > Any reason to use "Acme *acme" here? "You also prefer > gnome-screensaver-command" than "xscreensaver-command", are they both > installable in the same time? Any reason to not give the choice to the user? The function prototype is such that it is consistent with all the other do_ functions. gnome-screensaver-command should be tried first since in order to be consistent with gnome-settings-screensaver.c. I don't think there is any need to make a preference for this. If gnome-screensaver is installed then it should be used. If not, then xscreensaver is the logical fallback. >>-#define XSCREENSAVER_COMMAND "xscreensaver -nosplash" > Any reason to remove this? Nope. Just that it isn't really that useful. >>@@ -41,12 +40,6 @@ gnome_settings_screensaver_init (GConfCl >> /* >> * do nothing. >> * >>- * our settings only apply to startup, and the screensaver >>- * settings are all in xscreensaver and not gconf. >>- * >>- * we could have xscreensaver-demo run gconftool-2 directly, >>- * and start / stop xscreensaver here >>- * >> */ >> } > Any reason to drop these explanation rather than updating them > to mention the new case? Nope. Just that it isn't that useful anyway.
Comment on attachment 46030 [details] [review] updated patch > static void >+do_screensaver_action (Acme *acme) Having the Acme struct being passed to the actual function is in line with what's done for the other commands. I would however prefer if you modified execute to return a boolean saying whether the command failed, have it check the path first, and try both commands using that. <snip> > #define START_SCREENSAVER_KEY "/apps/gnome_settings_daemon/screensaver/start_screensaver" > #define SHOW_STARTUP_ERRORS_KEY "/apps/gnome_settings_daemon/screensaver/show_startup_errors" >-#define XSCREENSAVER_COMMAND "xscreensaver -nosplash" Given the style, you should add GNOME_SCREENSAVER_COMMAND and use both of those in the code. > void > gnome_settings_screensaver_init (GConfClient *client) >@@ -41,12 +40,6 @@ gnome_settings_screensaver_init (GConfCl > /* > * do nothing. > * >- * our settings only apply to startup, and the screensaver >- * settings are all in xscreensaver and not gconf. >- * >- * we could have xscreensaver-demo run gconftool-2 directly, >- * and start / stop xscreensaver here >- * That should be updated indeed, as Sebastien said. Other than that the updates look pretty obvious.
Created attachment 47718 [details] [review] updated patch Bastien, this patch doesn't use execute to test the command availability since execute also handles errors internally. And also so that the logic used to lock the screen remains exactly the same as the logic used to start a screensaver. I can change the behavior of execute if you wish but it will touch a lot more code.
Created attachment 49464 [details] [review] My version of the patch Sorry, didn't know about this bug/patch, so wrote my own. It includes changes also in the display properties applet.
Comment on attachment 49464 [details] [review] My version of the patch > /* xscreensaver should handle this itself, but does not currently so we hack > * it. Ignore failures in case xscreensaver is not installed */ >- if (changed) >- g_spawn_command_line_async ("xscreensaver-command -restart", NULL); >+ if (changed) { >+ char *cmd; >+ >+ cmd = g_strdup_printf ("%s -restart", get_screensaver_command ()); The comment still mention xscreensaver here. According to the list of option and a quick grep, gnome-screensaver has no "-restart" > /* xscreensaver should handle this itself, but does not currently so we hack > * it. Ignore failures in case xscreensaver is not installed */ >- g_spawn_command_line_async ("xscreensaver-command -restart", NULL); >+ cmd = g_strdup_printf ("%s -restart", get_screensaver_command ()); same here Out of this the patch seems to be good. If you change that feel free to commit with a mail to i18n about the strings changes.
could you also update the comments as commented on William's patch, you can probably use this part of the patch from comment #6
William, thanks for the update. I'm marking your patch as needs-work since the new patch send to the bug does pretty the same and has some other changes. With a combinaison of both patches you should have something good to commit
Created attachment 49566 [details] [review] Updated patch This now includes the changes in the comments and changes the behaviour in display/main.c to just use restart for xscreensaver. gnome-screensaver does not need to be restarted, all its settings are in GConf
Created attachment 49567 [details] [review] Updated patch Sorry, the last one included some extra changes, this now is the correct one
Comment on attachment 49567 [details] [review] Updated patch >- g_spawn_command_line_async ("xscreensaver-command -restart", NULL); >+ if (changed) { >+ gchar *cmd; >+ >+ if ((cmd = g_find_program_in_path ("gnome-screensaver-command"))) >+ g_free (cmd); What is the interest to set cmd here? >- /* xscreensaver should handle this itself, but does not currently so we hack >- * it. Ignore failures in case xscreensaver is not installed */ >- g_spawn_command_line_async ("xscreensaver-command -restart", NULL); >- >+ if ((cmd = g_find_program_in_path ("gnome-screensaver-command"))) >+ g_free (cmd); same here. >- if (g_spawn_command_line_async (XSCREENSAVER_COMMAND, &gerr)) >+ if ((ss_command = g_find_program_in_path ("gnome-screensaver"))) >+ use_gscreensaver = TRUE; >+ else { >+ if (!(ss_command = g_find_program_in_path ("xscreensaver"))) >+ return; >+ } >+ >+ g_free (ss_command); >+ if (use_gscreensaver) >+ ss_command = GSCREENSAVER_COMMAND; >+ else >+ ss_command = XSCREENSAVER_COMMAND; >+ >+ if (g_spawn_command_line_async (ss_command, &gerr)) > return; Any reason to define 2 new variables. What about? if (g_find_program_in_path("gnome-screensaver")) if (g_spawn_command_line_async (GSCREENSAVER_COMMAND, $gerr)) return; else if (g_spawn_command_line_async (XSCREENSAVER_COMMAND, $gerr)) return; > <locale name="C"> >- <short>Start XScreenSaver</short> >- <long>Run XScreenSaver at login</long> >+ <short>Start ScreenSaver</short> >+ <long>Run screen saver at login</long> > </locale> > </schema> > <schema> >@@ -20,7 +20,7 @@ > <default>true</default> > <locale name="C"> > <short>Show Startup Errors</short> >- <long>Display a dialog when there are errors running XScreenSaver</long> >+ <long>Display a dialog when there are errors running the screen saver</long> > </locale> > </schema> > </schemalist> The HIG (http://developer.gnome.org/documents/style-guide/gnome-glossary-generic-terms. html) uses "screensaver" and not "screen saver"/"ScreenSaver", these are probably to change. Out of this the patch looks fine, thanks for working on that.
Sebastien, I think the variables are needed because these specific functions return a newly allocated a string which needs to be freed manually
right, the API description is not really coherent on that, sometimes it specifies that and other times not. The other points stand
Created attachment 49707 [details] [review] Updated patch This fixes the screen saver/Screen Saver thing. The other points, as g_find_program_in_path returns a newly allocated string, don't apply.
Thanks for the updates. Feel free to commit with a mail to i18n for the strings changes.
Committed to HEAD