That's an easy fix, but it needs to be a preference, as some setups have the speakers powered by the screen, which means that the speakers would stop working when the screen went on power save.

Should it be default on or default off?

Should be a plugin. whether it's on or off by default is easy to switch.

I mean, it _is_ a plugin. But the UI for it should be in the main preferences.

So then should the UI code go into the plugin or the main totem.ui file?

The UI should be in the main interface. The code should only be responsible for changing a GConf entry though, and the GConf entry be under the screensaver plugins directory.

So what's the UI going to do if the screensaver plugin isn't loaded?

The screensaver is always loaded, it's builtin. If it's not loaded because of a broken installation, or any other errors, the setting will do absolutely nothing... But that's not supposed to happen :)

Note that if you find a nice way to have the tickbox in the plugin rather than directly in the main UI, please be my guest. It's fairly straight forward for actions, not so for other items.

I've got a working patch, but I can't figure out how to see if visual effects are running... I know the bvw knows, but I obviously can't get its priv data.

Created attachment 102735 [details] [review]
Working patch, except for that I can't get the schemas file to actually install.

Created attachment 102790 [details] [review]
updated patch

Thanks for the help on IRC, Philip!

Looks like you've forgotten to include the schema file in the patch?

+	plugins_list = totem_plugins_engine_get_plugins_list ();

You might want to put a comment for this block of code to explain what you're doing, as it's not immediately obvious. (Disabling then re-enabling the plugin to get it to pick up the config change?)

-	} else {
+	}
+	else
+	{

Braces on the same line as the statement, please. :-)

Looks good, apart from the indentation problems. Philip, feel free to commit this when it's fixed.

(In reply to comment #14)
> Looks like you've forgotten to include the schema file in the patch?

I can't commit until I've seen and reviewed this file. :-)

Created attachment 103179 [details] [review]
Fixed patch

Comment on attachment 103179 [details] [review]
Fixed patch

> totem_screensaver_update_from_state (TotemObject *totem,
> 				     TotemScreensaverPlugin *pi)
> {
>-	if (totem_is_playing (totem) != FALSE) {
>+	gboolean audio_lock = FALSE, visual_effects = TRUE, can_get_frames = TRUE;

Don't do assignments in declarations, especially when all those are assigned correctly later.

>Index: src/plugins/screensaver/totem-screensaver.schemas.in
>===================================================================
>--- src/plugins/screensaver/totem-screensaver.schemas.in	(revision 0)
>+++ src/plugins/screensaver/totem-screensaver.schemas.in	(revision 0)
>@@ -0,0 +1,19 @@
>+<gconfschemafile>
>+    <schemalist>    
>+
>+      <schema>
>+        <key>/schemas/apps/totem/plugins/screensaver/audio_lock</key>
>+	<applyto>/apps/totem/plugins/screensaver/audio_lock</applyto>
>+	<owner>totem</owner>
>+	<type>bool</type>
>+	<default>false</default>
>+	<locale name="C">
>+	  <short>Lock the screensaver when audio is playing</short>
>+	  <long>
>+	    Lock the screensaver when there is no video playing.
>+	  </long>
>+	</locale>
>+      </schema>

The description is completely wrong. It's about allowing the screensaver to kick in even when only audio is playing.

> void
>+checkbutton4_toggled_cb (GtkToggleButton *togglebutton, Totem *totem)
>+{
>+	gboolean value;
>+	guint i;
>+	GList *plugins_list;
>+
>+	value = gtk_toggle_button_get_active (togglebutton);
>+
>+	gconf_client_set_bool (totem->gc,
>+			       GCONF_PREFIX"/plugins/screensaver/audio_lock", 
>+			       value, NULL);
>+
>+	/* Disable and reenable the plugin to pick up config change */
>+	plugins_list = totem_plugins_engine_get_plugins_list ();
>+	for (i = 0; i < g_list_length (plugins_list); i++)
>+	{
>+		TotemPluginInfo *plugin_info = plugins_list->data;
>+		if (g_str_equal (totem_plugins_engine_get_plugin_name (plugin_info),
>+				 "screensaver") 
>+				 && totem_plugins_engine_plugin_is_active (plugin_info))
>+		{
>+			totem_plugins_engine_deactivate_plugin (plugin_info);
>+			totem_plugins_engine_activate_plugin (plugin_info);
>+		}
>+		plugins_list = g_list_next (plugins_list);
>+	}

That is just wrong. You shouldn't even be touching the plugins list.

I also noticed that the toggle in the prefs didn't change when the GConf setting did.

2008-01-25  Bastien Nocera  <hadess@hadess.net>

        * data/totem.schemas.in:
        * data/totem.ui:
        * src/plugins/screensaver/totem-screensaver.c:
        (totem_screensaver_update_from_state),
        (lock_screensaver_on_audio_changed_cb), (impl_activate),
        (impl_deactivate):
        * src/totem-preferences.c: (checkbutton4_toggled_cb),
        (lock_screensaver_on_audio_changed_cb), (totem_setup_preferences):
        Lock the screensaver when only audio is playing, but allow the
        user to override that, to avoid monitor-powered speakers being
        turned off

        Based on patch by Patrick Hulin <patrick.hulin@gmail.com>

        (Closes: #481080)

I've created a patch that cleans this a bit up (in my opinion). Some discussion might still remain as it could make more sense to use "Inhibit power management when playing audio file" or something like that instead of specifically referring to the screensaver.

See also: http://mail.gnome.org/archives/gnome-multimedia/2009-January/msg00002.html

This patch is to SVN r5906.

Created attachment 125988 [details] [review]
patch to rework screensaver inhibition when playing audio

to r5906.

Created attachment 125991 [details] [review]
newer version

Something is still wrong. The inhibit doesn't seem to work in some cases, have to look more at it.

This bug is fixed. If there's a problem with the code, file a new bug.