Merge lp:~psusi/ubuntu/natty/upower/sleep into lp:ubuntu/natty/upower

[Phillip Susi]

See linked bug report and related branches that all need merged together.

[Chris Halse Rogers (raof)]

You seem to have accidentally changed the copyright header in src/up-daemon.c; apart from that, looks good.

review: Needs Fixing

[Phillip Susi (psusi)]

Oops, good catch. Looks like my editor got a little excited about tabifying whitespace. I also had forgotten to add -pab --no-timestamps when freshing the quilt patch. Corrected.

review: Needs Resubmitting

[lp:~psusi/ubuntu/natty/upower/sleep]

20. By Phillip Susi on 2011-03-30

Add add-signal-argument.patch: Added missing argument to the
dbus Suspending/Resuming signal so g-p-m can apply the correct
policy. (LP: 578542)

[Evan Broder (broder)]

I disagree with changing an established and cross-distro D-Bus interface here.

And even though you fix g-p-m, I don't see patches for kdebase-workspace-bin or xfce4-power-manager. And even if you patched all of the utilities in the Ubuntu archive that used UPower, there could be any number of third-party programs that are expecting UPower to conform to its cross-distro definition (http://upower.freedesktop.org/docs/UPower.html).

The better approach here is to add a new signal that includes the argument, but leave the old signal alone, and that seems to be the conclusion that upstream is coming to as well.

review: Disapprove

[Chris Coulson (chrisccoulson)]

I agree with Evan here, and it's not clear whether hughsie agrees with this approach either (see http://lists.freedesktop.org/archives/devkit-devel/2011-March/001056.html). At this stage in the cycle, we shouldn't be breaking public dbus API's like this.

In addition to this, bypassing the upower client library reduces traceability (which is a big advantage of using client libraries to wrap dbus interfaces), and that traceability is required for problems exactly like this (ie, it's easy to find consumers of libupower and we have processes for library versioning if you want to break the API, but it's much more difficult to trace consumers of the raw dbus interface or properly version it if you want to break it).

review: Disapprove

Unmerged revisions

20. By Phillip Susi on 2011-03-30

Add add-signal-argument.patch: Added missing argument to the
dbus Suspending/Resuming signal so g-p-m can apply the correct
policy. (LP: 578542)

=== modified file 'debian/changelog'
--- debian/changelog	2011-02-23 17:46:34 +0000
+++ debian/changelog	2011-03-30 00:29:25 +0000
@@ -1,3 +1,11 @@
+upower (0.9.8-3ubuntu1) natty; urgency=low
+
+  * Add add-signal-argument.patch: Added missing argument to the
+    dbus Suspending/Resuming signal so g-p-m can apply the correct
+    policy. (LP: 578542)
+
+ -- Phillip Susi <[e-mail]>  Fri, 18 Mar 2011 21:18:32 -0400
+
 upower (0.9.8-3) experimental; urgency=low
 
   [ Frédéric Péters ]

=== modified file 'debian/control'
--- debian/control	2011-02-23 17:46:34 +0000
+++ debian/control	2011-03-30 00:29:25 +0000
@@ -28,6 +28,7 @@
 Package: upower
 Architecture: any
 Depends: ${shlibs:Depends}, ${misc:Depends}, udev [linux-any], dbus
+Breaks: gnome-power-manager (<<2.32.0-2ubuntu3), indicator-session (<<0.2.15-0ubuntu2)
 Recommends: pm-utils, policykit-1
 Conflicts: devicekit-power
 Replaces: devicekit-power

=== added file 'debian/patches/add-signal-argument.patch'
--- debian/patches/add-signal-argument.patch	1970-01-01 00:00:00 +0000
+++ debian/patches/add-signal-argument.patch	2011-03-30 00:29:25 +0000
@@ -0,0 +1,114 @@
+From: Phillip Susi <[e-mail]>
+Last-Updated: 2011-03-18
+Description: Upower took over suspend/hibernate functionality from
+ g-p-m.  When it did so, it omitted the argument to the Sleeping and
+ Resuming signals indicating whether the sleep is a suspend or
+ hibernate.  This patch adds that argument back.  The reason for this
+ is so that g-p-m can properly lock the screen and other activities it
+ takes in response to suspending or hibernating.
+Index: b/src/org.freedesktop.UPower.xml
+===================================================================
+--- a/src/org.freedesktop.UPower.xml
++++ b/src/org.freedesktop.UPower.xml
+@@ -123,8 +123,12 @@
+             before the sleep action is taken (such as sending out Avahi or
+             Jabber messages).
+           </doc:para>
++	  <doc:para>
++	    The argument type specifies either 0 for Suspend or 1 for Hibernate
++	  </doc:para>
+         </doc:description>
+       </doc:doc>
++      <arg type='u' name='type' />
+     </signal>
+ 
+     <!-- ************************************************************ -->
+@@ -138,8 +142,12 @@
+             Session and system programs can then do anything required (such as
+             sending out Avahi or Jabber messages).
+           </doc:para>
++	  <doc:para>
++	    The argument type specifies either 0 for Suspend or 1 for Hibernate
++	  </doc:para>
+         </doc:description>
+       </doc:doc>
++      <arg type='u' name='type' />
+     </signal>
+ 
+     <!-- ************************************************************ -->
+Index: b/src/up-daemon.c
+===================================================================
+--- a/src/up-daemon.c
++++ b/src/up-daemon.c
+@@ -91,6 +91,7 @@
+ 	guint			 about_to_sleep_id;
+ 	guint			 conf_sleep_timeout;
+ 	gboolean		 conf_allow_hibernate_encrypted_swap;
++	guint			 sleeptype;
+ };
+ 
+ static void	up_daemon_finalize		(GObject	*object);
+@@ -337,7 +338,7 @@
+ 
+ 	/* we've told the clients we're going down */
+ 	g_debug ("emitting sleeping");
+-	g_signal_emit (daemon, signals[SIGNAL_SLEEPING], 0);
++	g_signal_emit (daemon, signals[SIGNAL_SLEEPING], 0, priv->sleeptype);
+ 	g_timer_start (priv->about_to_sleep_timer);
+ 	daemon->priv->sent_sleeping_signal = TRUE;
+ 
+@@ -383,7 +384,7 @@
+ 
+ 	/* emit signal for session components */
+ 	g_debug ("emitting resuming");
+-	g_signal_emit (daemon, signals[SIGNAL_RESUMING], 0);
++	g_signal_emit (daemon, signals[SIGNAL_RESUMING], 0, priv->sleeptype);
+ 
+ 	/* reset the about-to-sleep logic */
+ 	g_timer_reset (priv->about_to_sleep_timer);
+@@ -427,7 +428,7 @@
+ 	/* we didn't use AboutToSleep() so send the signal for clients now */
+ 	if (!priv->sent_sleeping_signal) {
+ 		g_debug ("no AboutToSleep(), so emitting ::Sleeping()");
+-		g_signal_emit (daemon, signals[SIGNAL_SLEEPING], 0);
++		g_signal_emit (daemon, signals[SIGNAL_SLEEPING], 0, priv->sleeptype);
+ 		priv->about_to_sleep_id = g_timeout_add (priv->conf_sleep_timeout,
+ 							 (GSourceFunc) up_daemon_deferred_sleep_cb, sleep);
+ #if GLIB_CHECK_VERSION(2,25,8)
+@@ -466,6 +467,7 @@
+ 	const gchar *command;
+ 	UpDaemonPrivate *priv = daemon->priv;
+ 
++	priv->sleeptype = 0;
+ 	/* no kernel support */
+ 	if (!priv->kernel_can_suspend) {
+ 		error = g_error_new (UP_DAEMON_ERROR,
+@@ -569,6 +571,7 @@
+ 	const gchar *command;
+ 	UpDaemonPrivate *priv = daemon->priv;
+ 
++	priv->sleeptype = 1;
+ 	/* no kernel support */
+ 	if (!priv->kernel_can_hibernate) {
+ 		error = g_error_new (UP_DAEMON_ERROR,
+@@ -1222,16 +1225,16 @@
+ 			      G_OBJECT_CLASS_TYPE (klass),
+ 			      G_SIGNAL_RUN_LAST | G_SIGNAL_DETAILED,
+ 			      0, NULL, NULL,
+-			      g_cclosure_marshal_VOID__VOID,
+-			      G_TYPE_NONE, 0);
++			      g_cclosure_marshal_VOID__UINT,
++			      G_TYPE_NONE, 1, G_TYPE_UINT);
+ 
+ 	signals[SIGNAL_RESUMING] =
+ 		g_signal_new ("resuming",
+ 			      G_OBJECT_CLASS_TYPE (klass),
+ 			      G_SIGNAL_RUN_LAST | G_SIGNAL_DETAILED,
+ 			      0, NULL, NULL,
+-			      g_cclosure_marshal_VOID__VOID,
+-			      G_TYPE_NONE, 0);
++			      g_cclosure_marshal_VOID__UINT,
++			      G_TYPE_NONE, 1, G_TYPE_UINT);
+ 
+ 	g_object_class_install_property (object_class,
+ 					 PROP_DAEMON_VERSION,

=== modified file 'debian/patches/series'
--- debian/patches/series	2010-11-04 11:46:22 +0000
+++ debian/patches/series	2011-03-30 00:29:25 +0000
@@ -1,1 +1,2 @@
 # Debian patches for upower
+add-signal-argument.patch