Patch looks ok to me, please put that in our rawhide rhythmbox packages. For committing this upstream, we probably want to figure out the proper xid to pass...

See shell/rb-missing-plugins.c line 210 for one example of finding the xid for the main window.

What exactly gets inhibited in this case?  From what I remember of previous discusisons, we want to stop the machine suspending due to idle time, but not due to the lid closing or any explicit user action.

(In reply to comment #2)
> we want to stop the machine suspending due to idle time, but not
> due to the lid closing or any explicit user action.

That's exactly what the new behaviour does. I'll update the patch to pass the xid in a couple of hours. Thanks for the pointer.

Created attachment 144177 [details] [review]
new patch

New patch, please review. Thanks.

OK, great, that seems to work properly.. it seems to inhibit the screensaver too here (on F11); is that intentional?

This generally looks good, but I'm a bit curious about this:

-				 G_TYPE_STRING, _("Music Player"),
+				 G_TYPE_STRING, "rhythmbox",

is this no longer displayed to the user?

That parameter is described as follows in the gnome-session dbus api:

     <arg type="s" name="app_id" direction="in">
        <doc:doc>
          <doc:summary>The application identifier</doc:summary>
        </doc:doc>
      </arg>

And I assume it should be 'rhythmbox.desktop' but I don't see any app_id for rhythmbox when poking at gnome-session in d-feet. Not sure how that is supposed to be set via EggSMClient. And not sure if you are supposed to be able to use Inhibit without calling RegisterClient first.

Hmm, does that mean we're supposed to switch to the dbus EggSMClient backend?

*** Bug 588876 has been marked as a duplicate of this bug. ***

I guess that it will probably inhibit the logout, which is probably not something you'd want to do.

Having one big inhibit that is then used for logout+screensaver+g-p-m doesn't seem to work well in this case.

This doesn't inhibit any explicit user actions, including logging out or locking the screen. It only stops things happening because the session is idle.

Comment on attachment 144177 [details] [review]
new patch

commit f987b3aec9d61bc58e0471069e6c84b747d1cac2
Author: Richard Hughes <richard@hughsie.com>
Date:   Wed Oct 7 10:20:58 2009 +0100

    Do not use gnome-power-manager for inhibits, instead use gnome-session
    
    In this patch the session is prevented from going idle, but all other
    activities (user switching, suspend, etc) are allowed. This will prevent
    gnome-power-manager from suspending the computer due to inactivity.
    
    See http://git.gnome.org/cgit/gnome-session/tree/gnome-session/org.gnome.SessionManager.xml
    for explaination of the flags value we use to inhibit gnome-session

diff --git a/plugins/power-manager/rb-power-manager-plugin.c b/plugins/power-manager/rb-power-manager-plugin.c
index c8dea1d..b6912e9 100644
--- a/plugins/power-manager/rb-power-manager-plugin.c
+++ b/plugins/power-manager/rb-power-manager-plugin.c
@@ -27,7 +27,7 @@
  */
 
 /*
- * gnome-power-manager integration.
+ * gnome-session integration.
  * currently consists of inhibiting suspend while playing.
  */
 
@@ -35,6 +35,7 @@
 
 #include <glib/gi18n.h>
 #include <dbus/dbus-glib.h>
+#include <gdk/gdkx.h>
 
 #include "rb-plugin.h"
 #include "rb-debug.h"
@@ -55,6 +56,7 @@ typedef struct
 	guint32 cookie;
 	gint handler_id;
 	gint timeout_id;
+	RBShell *shell;
 } RBGPMPlugin;
 
 typedef struct
@@ -120,33 +122,15 @@ create_dbus_proxy (RBGPMPlugin *plugin)
 
 	/* try new name first */
 	plugin->proxy = dbus_g_proxy_new_for_name_owner (plugin->bus,
-						   "org.freedesktop.PowerManagement",
-						   "/org/freedesktop/PowerManagement/Inhibit",
-						   "org.freedesktop.PowerManagement.Inhibit",
+						   "org.gnome.SessionManager",
+						   "/org/gnome/SessionManager",
+						   "org.gnome.SessionManager",
 						   &error);
 	if (error != NULL && ignore_error (error) == FALSE) {
-		g_warning ("Failed to create dbus proxy for org.gnome.PowerManager: %s",
+		g_warning ("Failed to create dbus proxy for org.gnome.SessionManager: %s",
 			   error->message);
 		g_error_free (error);
 		return FALSE;
-	} else if (error != NULL) {
-		g_error_free (error);
-		error = NULL;
-
-		/* fall back to original name */
-		plugin->proxy = dbus_g_proxy_new_for_name_owner (plugin->bus,
-							   "org.gnome.PowerManager",
-							   "/org/gnome/PowerManager",
-							   "org.gnome.PowerManager",
-							   &error);
-		if (error != NULL) {
-			if (ignore_error (error) == FALSE) {
-				g_warning ("Failed to create dbus proxy for org.gnome.PowerManager: %s",
-					   error->message);
-			}
-			g_error_free (error);
-			return FALSE;
-		}
 	}
 
 	g_signal_connect_object (plugin->proxy,
@@ -187,10 +171,12 @@ inhibit_cb (DBusGProxy *proxy,
 static gboolean
 inhibit (RBGPMPlugin *plugin)
 {
+	GtkWindow *window;
 	plugin->timeout_id = 0;
+	gulong xid = 0;
 
 	if (plugin->cookie != 0) {
-		rb_debug ("Was going to inhibit gnome-power-manager, but we already have done");
+		rb_debug ("Was going to inhibit gnome-session, but we already have done");
 		return FALSE;
 	}
 
@@ -200,12 +186,16 @@ inhibit (RBGPMPlugin *plugin)
 
 	rb_debug ("inhibiting");
 	g_object_ref (plugin);
+	g_object_get (plugin->shell, "window", &window, NULL);
+	xid = GDK_WINDOW_XWINDOW (GTK_WIDGET (window)->window);
 	dbus_g_proxy_begin_call (plugin->proxy, "Inhibit",
 				 (DBusGProxyCallNotify) inhibit_cb,
 				 plugin,
 				 NULL,
-				 G_TYPE_STRING, _("Music Player"),
+				 G_TYPE_STRING, "rhythmbox",
+				 G_TYPE_UINT, xid,
 				 G_TYPE_STRING, _("Playing"),
+				 G_TYPE_UINT, 8, /* flags */
 				 G_TYPE_INVALID);
 
 	return FALSE;
@@ -245,7 +235,7 @@ uninhibit (RBGPMPlugin *plugin)
 	plugin->timeout_id = 0;
 
 	if (plugin->cookie == 0) {
-		rb_debug ("Was going to uninhibit power manager, but we haven't inhibited it");
+		rb_debug ("Was going to uninhibit session manager, but we haven't inhibited it");
 		return FALSE;
 	}
 
@@ -255,7 +245,7 @@ uninhibit (RBGPMPlugin *plugin)
 
 	rb_debug ("uninhibiting; cookie = %u", plugin->cookie);
 	g_object_ref (plugin);
-	dbus_g_proxy_begin_call (plugin->proxy, "UnInhibit",
+	dbus_g_proxy_begin_call (plugin->proxy, "Uninhibit",
 				 (DBusGProxyCallNotify) uninhibit_cb,
 				 plugin,
 				 NULL,
@@ -291,6 +281,7 @@ impl_activate (RBPlugin *rbplugin,
 
 	plugin = RB_GPM_PLUGIN (rbplugin);
 
+	plugin->shell = g_object_ref (shell);
 	plugin->bus = dbus_g_bus_get (DBUS_BUS_SESSION, &error);
 	if (plugin->bus == NULL) {
 		g_warning ("Couldn't connect to system bus: %s", (error) ? error->message : "(null)");
@@ -338,6 +329,7 @@ impl_deactivate (RBPlugin *rbplugin,
 		plugin->handler_id = 0;
 	}
 
+	g_object_unref (plugin->shell);
 	g_object_unref (shell_player);
 
 	if (plugin->proxy != NULL) {

*** Bug 402863 has been marked as a duplicate of this bug. ***

Long comment ensues (sorry for that, I'm hoping for it to be as useful feedback as possible). If I should better open a new bug report for it instead of commenting on a closed bug, please let me know.
-----------

From what I understand (and tested with ubuntu 9.10's package which includes this patch), this new behavior inhibits session idle and does not inhibit suspend itself (because there is no mechanism to distinguish between soft-triggered suspend vs hard-triggered "yes I REALLY want to suspend" actions; i.e. lid close vs sleep/power button press).

I would like to point out a few fairly irritating problems I have noticed when testing the new behavior over the past few days:

1) It inhibits turning off the screen automatically. Urgh. It's not a movie player, it shouldn't cause such energy waste. Example use cases:
-- A desktop computer in a living room shouldn't have its screen turned on just because it's playing music
-- A laptop on battery, providing music to a car sound system, shouldn't waste its energy on lighting the screen when nobody is even looking at the screen

2) Session idle also controls other things such as instant messaging status. Therefore, if you have background music playing but are not attending the computer, everybody in your contact list will expect you to be available to answer their inquiries.


3) Users annoyed by the previous suspend inhibition approach could simply... pause the music player if they wanted to suspend (or disable the power management plugin if they don't use autosuspend). Which kinda makes sense to me. Users such as me that relied on the previous behavior are now left a bit in the cold with schemes that waste power. Want to have music playing with the screen turned off while still enjoying the ability to (in regular, non music-playing situations) suspend the computer by just closing the lid? With the new behavior, you can't.

4) The plugin does not have a configuration dialog allowing the user to specify that closing the lid shouldn't make it go to sleep. This is pretty much a dealbreaker for many of us who use our laptops as portable music players (unless you want to walk around in the public transportation with the laptop open in your arms at all times).



Recapping the situation on an "effort by users" basis:
======================================================
BEFORE:

* Users of the inhibit-idle approach are annoyed that their laptop doesn't suspend when music is playing. Solutions: a) pause/exit the music player b) deactivate the music player's power management plugin.
* Users of the inhibit-suspend approach are happy

Either solution for group #1 still leaves group #2 happy.



NEW BEHAVIOR:

* Users of the inhibit-idle approach are happy
* Users of the inhibit-suspend approach are annoyed that
-- their computer never goes idle
-- their screen never turns off unless 1) not set to suspend on lid close 2) the lid is manually closed or the screen manually turned off or locked
-- they can't use their laptop as a portable music player while still being able to suspend on lid close in other scenarios.

Workarounds for group #2: none, unless you consider giving up on "suspend on lid close" a solution.

If gnome-session were to provide an inhibit setting that only inhibited suspend-on-idle but not the screensaver, then we would most likely use it.  It currently doesn't.  If you want to have this added, file a bug against gnome-session.