Merge lp:~ken-vandine/ubuntu-system-settings/lp1337200 into lp:ubuntu-system-settings

[Ken VanDine]

Disconnect from upower on suspend and connect on resume

Disconnect from upower on suspend and connect on resume

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1203
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/1783/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/204
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-vivid/139
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-vivid-i386-ci/53
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/184
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/204
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/204/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15814
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-vivid/118
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-amd64/143
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-amd64/143/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/1783/rebuild

review: Approve (continuous-integration)

[lp:~ken-vandine/ubuntu-system-settings/lp1337200]

1204. By Ken VanDine on 2014-11-13

Moved up_client_enumerate_devices_sync to the constructor, we only need to call it once.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1204
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/1787/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/210
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-vivid/144
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-vivid-i386-ci/57
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/188
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/210
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/210/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15822
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-vivid/122
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-amd64/148
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-amd64/148/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/1787/rebuild

review: Approve (continuous-integration)

[Iain Lane (laney)]

One inline comment.

If we disconnect when suspended, what happens if a battery is added or removed then?

Can you add a test for this (how do you programatically suspend an application?) and fix it (call the callback on resuming) if it's broken?

review: Needs Information

[Sebastien Bacher (seb128)]

Thanks, can you explain why we need to disconnect from upower? With a suspend device not a lot should be happening, if there are events generated/queued while the device is sleeping and not getting any interaction then that seems to be the real bug to fix. Or do we plan to patch every single upower client to workaround the issue?

review: Needs Information

[Sebastien Bacher (seb128)]

(just read the bug comments, still it feels like wrong to have to workaround in every client)

[Ken VanDine (ken-vandine)]

It doesn't really fix the issue either, I think the only solution is to really fix upower.

[Sebastien Bacher (seb128)]

setting as rejected then

review: Disapprove

Unmerged revisions

1204. By Ken VanDine on 2014-11-13

Moved up_client_enumerate_devices_sync to the constructor, we only need to call it once.

1203. By Ken VanDine on 2014-11-13

Disconnect from upower on suspend and connect on resume

=== modified file 'plugins/battery/plugin/battery-plugin.cpp'
--- plugins/battery/plugin/battery-plugin.cpp	2014-09-12 15:42:58 +0000
+++ plugins/battery/plugin/battery-plugin.cpp	2014-11-14 02:59:39 +0000
@@ -20,6 +20,7 @@
 
 #include "battery-plugin.h"
 
+#include <QCoreApplication>
 #include <QDebug>
 #include <QStringList>
 #include <SystemSettings/ItemBase>
@@ -39,8 +40,14 @@
     ~BatteryItem();
 
 private:
+    void up_connect();
+    void up_disconnect();
     UpClient *m_client;
+    QCoreApplication *m_app;
     gulong m_addedHandler, m_removedHandler;
+
+private Q_SLOTS:
+    void onApplicationStateChanged(Qt::ApplicationState st);
 };
 
 void deviceChanged(UpClient *client,
@@ -48,44 +55,56 @@
                    gpointer user_data)
 {
     BatteryItem *item (static_cast<BatteryItem *> (user_data));
-
-#if !UP_CHECK_VERSION(0, 99, 0)
-    gboolean ret = up_client_enumerate_devices_sync (client, nullptr, nullptr);
-    if (!ret) {
-        item->setVisibility (false);
-    } else
-#endif
-    {
-        GPtrArray *devices = up_client_get_devices (client);
-        item->setVisibility (devices->len > 0);
-        g_ptr_array_unref (devices);
-    }
-
+    GPtrArray *devices = up_client_get_devices (client);
+    item->setVisibility (devices->len > 0);
+    g_ptr_array_unref (devices);
 }
 
 BatteryItem::BatteryItem(const QVariantMap &staticData, QObject *parent):
     ItemBase(staticData, parent),
     m_client(up_client_new()),
+    m_app(QCoreApplication::instance()),
     m_addedHandler(0),
     m_removedHandler(0)
 {
+#if !UP_CHECK_VERSION(0, 99, 0)
+    gboolean ret = up_client_enumerate_devices_sync (m_client, nullptr, nullptr);
+    if (!ret) {
+        setVisibility (false);
+    }
+#endif
+
     deviceChanged(m_client, nullptr, this);
-    m_addedHandler = g_signal_connect (m_client,
-                                      "device-added",
-                                      G_CALLBACK (::deviceChanged),
-                                      this /* user_data */);
-    m_removedHandler = g_signal_connect (m_client,
-                                        "device-removed",
-                                        G_CALLBACK (::deviceChanged),
-                                        this /* user_data */);
-}
-
-void BatteryItem::setVisibility(bool visible)
-{
-    setVisible(visible);
-}
-
-BatteryItem::~BatteryItem()
+    connect(m_app, SIGNAL(applicationStateChanged(Qt::ApplicationState)), this, SLOT(onApplicationStateChanged(Qt::ApplicationState)));
+    up_connect();
+}
+
+void BatteryItem::onApplicationStateChanged(Qt::ApplicationState st)
+{
+    if (st == Qt::ApplicationActive)
+        up_connect();
+    else
+        up_disconnect();
+}
+
+void BatteryItem::up_connect()
+{
+    if (!m_addedHandler) {
+        m_addedHandler = g_signal_connect (m_client,
+                                          "device-added",
+                                          G_CALLBACK (::deviceChanged),
+                                          this /* user_data */);
+    }
+
+    if (!m_removedHandler) {
+        m_removedHandler = g_signal_connect (m_client,
+                                            "device-removed",
+                                            G_CALLBACK (::deviceChanged),
+                                            this /* user_data */);
+    }
+}
+
+void BatteryItem::up_disconnect()
 {
     if (m_addedHandler) {
         g_signal_handler_disconnect (m_client, m_addedHandler);
@@ -96,7 +115,16 @@
         g_signal_handler_disconnect (m_client, m_removedHandler);
         m_removedHandler = 0;
     }
-
+}
+
+void BatteryItem::setVisibility(bool visible)
+{
+    setVisible(visible);
+}
+
+BatteryItem::~BatteryItem()
+{
+    up_disconnect();
     g_object_unref (m_client);
 }
 
@@ -105,7 +133,6 @@
 {
 }
 
-
 ItemBase *BatteryPlugin::createItem(const QVariantMap &staticData,
                                  QObject *parent)
 {