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

Proposed by Phillip Susi on 2011-03-19
Status: Rejected
Rejected by: James Westby on 2011-05-17
Proposed branch: lp:~psusi/ubuntu/natty/upower/sleep
Merge into: lp:ubuntu/natty/upower
Diff against target: 153 lines (+124/-0)
4 files modified
debian/changelog (+8/-0)
debian/control (+1/-0)
debian/patches/add-signal-argument.patch (+114/-0)
debian/patches/series (+1/-0)
To merge this branch: bzr merge lp:~psusi/ubuntu/natty/upower/sleep
Reviewer Review Type Date Requested Status
Chris Coulson (community) Disapprove on 2011-04-04
Evan Broder (community) Disapprove on 2011-03-30
Phillip Susi (community) Resubmit on 2011-03-29
Chris Halse Rogers Needs Fixing on 2011-03-29
Ubuntu branches 2011-03-19 Pending
Review via email: mp+54093@code.launchpad.net

Description of the change

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

To post a comment you must log in.
Chris Halse Rogers (raof) wrote :

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) wrote :

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: Resubmit
lp:~psusi/ubuntu/natty/upower/sleep updated on 2011-03-30
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) wrote :

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) wrote :

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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/changelog'
2--- debian/changelog 2011-02-23 17:46:34 +0000
3+++ debian/changelog 2011-03-30 00:29:25 +0000
4@@ -1,3 +1,11 @@
5+upower (0.9.8-3ubuntu1) natty; urgency=low
6+
7+ * Add add-signal-argument.patch: Added missing argument to the
8+ dbus Suspending/Resuming signal so g-p-m can apply the correct
9+ policy. (LP: 578542)
10+
11+ -- Phillip Susi <psusi@cfl.rr.com> Fri, 18 Mar 2011 21:18:32 -0400
12+
13 upower (0.9.8-3) experimental; urgency=low
14
15 [ Frédéric Péters ]
16
17=== modified file 'debian/control'
18--- debian/control 2011-02-23 17:46:34 +0000
19+++ debian/control 2011-03-30 00:29:25 +0000
20@@ -28,6 +28,7 @@
21 Package: upower
22 Architecture: any
23 Depends: ${shlibs:Depends}, ${misc:Depends}, udev [linux-any], dbus
24+Breaks: gnome-power-manager (<<2.32.0-2ubuntu3), indicator-session (<<0.2.15-0ubuntu2)
25 Recommends: pm-utils, policykit-1
26 Conflicts: devicekit-power
27 Replaces: devicekit-power
28
29=== added file 'debian/patches/add-signal-argument.patch'
30--- debian/patches/add-signal-argument.patch 1970-01-01 00:00:00 +0000
31+++ debian/patches/add-signal-argument.patch 2011-03-30 00:29:25 +0000
32@@ -0,0 +1,114 @@
33+From: Phillip Susi <psusi@cfl.rr.com>
34+Last-Updated: 2011-03-18
35+Description: Upower took over suspend/hibernate functionality from
36+ g-p-m. When it did so, it omitted the argument to the Sleeping and
37+ Resuming signals indicating whether the sleep is a suspend or
38+ hibernate. This patch adds that argument back. The reason for this
39+ is so that g-p-m can properly lock the screen and other activities it
40+ takes in response to suspending or hibernating.
41+Index: b/src/org.freedesktop.UPower.xml
42+===================================================================
43+--- a/src/org.freedesktop.UPower.xml
44++++ b/src/org.freedesktop.UPower.xml
45+@@ -123,8 +123,12 @@
46+ before the sleep action is taken (such as sending out Avahi or
47+ Jabber messages).
48+ </doc:para>
49++ <doc:para>
50++ The argument type specifies either 0 for Suspend or 1 for Hibernate
51++ </doc:para>
52+ </doc:description>
53+ </doc:doc>
54++ <arg type='u' name='type' />
55+ </signal>
56+
57+ <!-- ************************************************************ -->
58+@@ -138,8 +142,12 @@
59+ Session and system programs can then do anything required (such as
60+ sending out Avahi or Jabber messages).
61+ </doc:para>
62++ <doc:para>
63++ The argument type specifies either 0 for Suspend or 1 for Hibernate
64++ </doc:para>
65+ </doc:description>
66+ </doc:doc>
67++ <arg type='u' name='type' />
68+ </signal>
69+
70+ <!-- ************************************************************ -->
71+Index: b/src/up-daemon.c
72+===================================================================
73+--- a/src/up-daemon.c
74++++ b/src/up-daemon.c
75+@@ -91,6 +91,7 @@
76+ guint about_to_sleep_id;
77+ guint conf_sleep_timeout;
78+ gboolean conf_allow_hibernate_encrypted_swap;
79++ guint sleeptype;
80+ };
81+
82+ static void up_daemon_finalize (GObject *object);
83+@@ -337,7 +338,7 @@
84+
85+ /* we've told the clients we're going down */
86+ g_debug ("emitting sleeping");
87+- g_signal_emit (daemon, signals[SIGNAL_SLEEPING], 0);
88++ g_signal_emit (daemon, signals[SIGNAL_SLEEPING], 0, priv->sleeptype);
89+ g_timer_start (priv->about_to_sleep_timer);
90+ daemon->priv->sent_sleeping_signal = TRUE;
91+
92+@@ -383,7 +384,7 @@
93+
94+ /* emit signal for session components */
95+ g_debug ("emitting resuming");
96+- g_signal_emit (daemon, signals[SIGNAL_RESUMING], 0);
97++ g_signal_emit (daemon, signals[SIGNAL_RESUMING], 0, priv->sleeptype);
98+
99+ /* reset the about-to-sleep logic */
100+ g_timer_reset (priv->about_to_sleep_timer);
101+@@ -427,7 +428,7 @@
102+ /* we didn't use AboutToSleep() so send the signal for clients now */
103+ if (!priv->sent_sleeping_signal) {
104+ g_debug ("no AboutToSleep(), so emitting ::Sleeping()");
105+- g_signal_emit (daemon, signals[SIGNAL_SLEEPING], 0);
106++ g_signal_emit (daemon, signals[SIGNAL_SLEEPING], 0, priv->sleeptype);
107+ priv->about_to_sleep_id = g_timeout_add (priv->conf_sleep_timeout,
108+ (GSourceFunc) up_daemon_deferred_sleep_cb, sleep);
109+ #if GLIB_CHECK_VERSION(2,25,8)
110+@@ -466,6 +467,7 @@
111+ const gchar *command;
112+ UpDaemonPrivate *priv = daemon->priv;
113+
114++ priv->sleeptype = 0;
115+ /* no kernel support */
116+ if (!priv->kernel_can_suspend) {
117+ error = g_error_new (UP_DAEMON_ERROR,
118+@@ -569,6 +571,7 @@
119+ const gchar *command;
120+ UpDaemonPrivate *priv = daemon->priv;
121+
122++ priv->sleeptype = 1;
123+ /* no kernel support */
124+ if (!priv->kernel_can_hibernate) {
125+ error = g_error_new (UP_DAEMON_ERROR,
126+@@ -1222,16 +1225,16 @@
127+ G_OBJECT_CLASS_TYPE (klass),
128+ G_SIGNAL_RUN_LAST | G_SIGNAL_DETAILED,
129+ 0, NULL, NULL,
130+- g_cclosure_marshal_VOID__VOID,
131+- G_TYPE_NONE, 0);
132++ g_cclosure_marshal_VOID__UINT,
133++ G_TYPE_NONE, 1, G_TYPE_UINT);
134+
135+ signals[SIGNAL_RESUMING] =
136+ g_signal_new ("resuming",
137+ G_OBJECT_CLASS_TYPE (klass),
138+ G_SIGNAL_RUN_LAST | G_SIGNAL_DETAILED,
139+ 0, NULL, NULL,
140+- g_cclosure_marshal_VOID__VOID,
141+- G_TYPE_NONE, 0);
142++ g_cclosure_marshal_VOID__UINT,
143++ G_TYPE_NONE, 1, G_TYPE_UINT);
144+
145+ g_object_class_install_property (object_class,
146+ PROP_DAEMON_VERSION,
147
148=== modified file 'debian/patches/series'
149--- debian/patches/series 2010-11-04 11:46:22 +0000
150+++ debian/patches/series 2011-03-30 00:29:25 +0000
151@@ -1,1 +1,2 @@
152 # Debian patches for upower
153+add-signal-argument.patch

Subscribers

People subscribed via source and target branches

to all changes: