Rework the battery plugin

Instead of having the DBus stuff in a separate class expose the plugin class itself like we do for the other plugins.

Replace the method-based API with properties.

Change the path to <device>/battery for consistency with other plugins

declarativeplugin/kdeconnectdeclarativeplugin.cpp

@@ -65,7 +65,7 @@ void KdeConnectDeclarativePlugin::registerTypes(const char* uri)
 #endif
 
     registerFactory<DeviceDbusInterface>(uri, "DeviceDbusInterfaceFactory");
-    registerFactory<DeviceBatteryDbusInterface>(uri, "DeviceBatteryDbusInterfaceFactory");
+    registerFactory<BatteryDbusInterface>(uri, "DeviceBatteryDbusInterfaceFactory");
     registerFactory<FindMyPhoneDeviceDbusInterface>(uri, "FindMyPhoneDbusInterfaceFactory");
     registerFactory<SftpDbusInterface>(uri, "SftpDbusInterfaceFactory");
     registerFactory<RemoteKeyboardDbusInterface>(uri, "RemoteKeyboardDbusInterfaceFactory");

indicator/deviceindicator.cpp

@@ -18,13 +18,15 @@ Q_OBJECT
 public:
     BatteryAction(DeviceDbusInterface* device)
         : QAction(nullptr)
-        , m_batteryIface(new DeviceBatteryDbusInterface(device->id(), this))
+        , m_batteryIface(device->id())
     {
-        setWhenAvailable(m_batteryIface->charge(), [this](int charge) { setCharge(charge); }, this);
-        setWhenAvailable(m_batteryIface->isCharging(), [this](bool charging) { setCharging(charging); }, this);
+        setCharge(m_batteryIface.charge());
+        setCharging(m_batteryIface.isCharging());
 
-        connect(m_batteryIface, SIGNAL(chargeChanged(int)), this, SLOT(setCharge(int)));
-        connect(m_batteryIface, SIGNAL(stateChanged(bool)), this, SLOT(setCharging(bool)));
+        connect(&m_batteryIface, &BatteryDbusInterface::refreshedProxy, this, [this]{
+            setCharge(m_batteryIface.charge());
+            setCharging(m_batteryIface.isCharging());
+        });
 
         setIcon(QIcon::fromTheme(QStringLiteral("battery")));
 
@@ -45,7 +47,7 @@ private Q_SLOTS:
     void setCharging(bool charging) { m_charging = charging; update(); }
 
 private:
-    DeviceBatteryDbusInterface* m_batteryIface;
+    BatteryDbusInterface m_batteryIface;
     int m_charge = -1;
     bool m_charging = false;
 };

interfaces/CMakeLists.txt

@@ -38,7 +38,7 @@ set(libkdeconnect_SRC
 
 geninterface(${PROJECT_SOURCE_DIR}/core/daemon.h daemoninterface)
 geninterface(${PROJECT_SOURCE_DIR}/core/device.h deviceinterface)
-geninterface(${PROJECT_SOURCE_DIR}/plugins/battery/batterydbusinterface.h devicebatteryinterface)
+geninterface(${PROJECT_SOURCE_DIR}/plugins/battery/batteryplugin.h batteryinterface)
 geninterface(${PROJECT_SOURCE_DIR}/plugins/sftp/sftpplugin.h devicesftpinterface)
 geninterface(${PROJECT_SOURCE_DIR}/plugins/notifications/notificationsdbusinterface.h devicenotificationsinterface)
 geninterface(${PROJECT_SOURCE_DIR}/plugins/findmyphone/findmyphoneplugin.h devicefindmyphoneinterface)

interfaces/dbusinterfaces.cpp

@@ -58,16 +58,13 @@ void DeviceDbusInterface::pluginCall(const QString& plugin, const QString& metho
     DBusHelper::sessionBus().asyncCall(msg);
 }
 
-DeviceBatteryDbusInterface::DeviceBatteryDbusInterface(const QString& id, QObject* parent)
-    : OrgKdeKdeconnectDeviceBatteryInterface(DaemonDbusInterface::activatedService(), QStringLiteral("/modules/kdeconnect/devices/") +id, DBusHelper::sessionBus(), parent)
+BatteryDbusInterface::BatteryDbusInterface(const QString& id, QObject* parent)
+    : OrgKdeKdeconnectDeviceBatteryInterface(DaemonDbusInterface::activatedService(), QStringLiteral("/modules/kdeconnect/devices/") + id + QStringLiteral("/battery"), DBusHelper::sessionBus(), parent)
 {
-
+    connect(this, &OrgKdeKdeconnectDeviceBatteryInterface::refreshed, this, &BatteryDbusInterface::refreshedProxy);
 }
 
-DeviceBatteryDbusInterface::~DeviceBatteryDbusInterface()
-{
-
-}
+BatteryDbusInterface::~BatteryDbusInterface() = default;
 
 DeviceNotificationsDbusInterface::DeviceNotificationsDbusInterface(const QString& id, QObject* parent)
     : OrgKdeKdeconnectDeviceNotificationsInterface(DaemonDbusInterface::activatedService(), QStringLiteral("/modules/kdeconnect/devices/") +id, DBusHelper::sessionBus(), parent)

interfaces/dbusinterfaces.h

@@ -11,7 +11,7 @@
 
 #include "daemoninterface.h"
 #include "deviceinterface.h"
-#include "devicebatteryinterface.h"
+#include "batteryinterface.h"
 #include "devicesftpinterface.h"
 #include "devicefindmyphoneinterface.h"
 #include "devicenotificationsinterface.h"
@@ -77,13 +77,18 @@ private:
     const QString m_id;
 };
 
-class KDECONNECTINTERFACES_EXPORT DeviceBatteryDbusInterface
+class KDECONNECTINTERFACES_EXPORT BatteryDbusInterface
     : public OrgKdeKdeconnectDeviceBatteryInterface
 {
     Q_OBJECT
+    Q_PROPERTY(int charge READ charge NOTIFY refreshedProxy)
+    Q_PROPERTY(bool isCharging READ isCharging NOTIFY refreshedProxy)
 public:
-    explicit DeviceBatteryDbusInterface(const QString& deviceId, QObject* parent = nullptr);
-    ~DeviceBatteryDbusInterface() override;
+    explicit BatteryDbusInterface(const QString& deviceId, QObject* parent = nullptr);
+    ~BatteryDbusInterface() override;
+
+Q_SIGNALS:
+    void refreshedProxy();
 };
 
 class KDECONNECTINTERFACES_EXPORT DeviceNotificationsDbusInterface

plasmoid/package/contents/ui/Battery.qml

@@ -10,9 +10,9 @@ import org.kde.plasma.components 2.0 as PlasmaComponents
 import org.kde.kdeconnect 1.0
 
 QtObject {
-  
+
     id: root
-    
+
     property alias device: checker.device
     readonly property alias available: checker.available
 
@@ -21,32 +21,14 @@ QtObject {
         pluginName: "battery"
     }
 
-    property bool charging: false
-    property int charge: -1
+    property bool charging: battery ? battery.isCharging : false
+    property int charge: battery ? battery.charge : -1
     property string displayString: (available && charge > -1) ? ((charging) ? (i18n("%1% charging", charge)) : (i18n("%1%", charge))) : i18n("No info")
     property variant battery: null
 
-    property variant nested1: DBusAsyncResponse {
-        id: startupCheck1
-        autoDelete: false
-        onSuccess: root.charging = result
-    }
-    
-    property variant nested2: DBusAsyncResponse {
-        id: startupCheck2
-        autoDelete: false
-        onSuccess: root.charge = result
-    }
-    
     onAvailableChanged: {
         if (available) {
             battery = DeviceBatteryDbusInterfaceFactory.create(device.id())
-            
-            battery.stateChanged.connect(function(c) {charging = c})
-            battery.chargeChanged.connect(function(c) {charge = c})
-
-            startupCheck1.setPendingCall(battery.isCharging())
-            startupCheck2.setPendingCall(battery.charge())
         } else {
             battery = null
         }

plugins/battery/CMakeLists.txt

@@ -7,7 +7,6 @@ ecm_qt_declare_logging_category(
 
 set(kdeconnect_battery_SRCS
     batteryplugin.cpp
-    batterydbusinterface.cpp
     ${debug_file_SRCS}
 )
 

plugins/battery/batterydbusinterface.cpp

@@ -1,50 +0,0 @@
-/**
- * SPDX-FileCopyrightText: 2013 Albert Vaca <albertvaka@gmail.com>
- *
- * SPDX-License-Identifier: GPL-2.0-only OR GPL-3.0-only OR LicenseRef-KDE-Accepted-GPL
- */
-
-#include "batterydbusinterface.h"
-#include "batteryplugin.h"
-
-#include <QDebug>
-#include <core/device.h>
-#include "plugin_battery_debug.h"
-
-QMap<QString, BatteryDbusInterface *> BatteryDbusInterface::s_dbusInterfaces;
-
-BatteryDbusInterface::BatteryDbusInterface(const Device* device)
-    : QDBusAbstractAdaptor(const_cast<Device*>(device))
-	, m_charge(-1)
-	, m_isCharging(false)
-{
-    // FIXME: Workaround to prevent memory leak.
-    // This makes the old BatteryDdbusInterface be deleted only after the new one is
-    // fully operational. That seems to prevent the crash mentioned in BatteryPlugin's
-    // destructor.
-    QMap<QString, BatteryDbusInterface *>::iterator oldInterfaceIter = s_dbusInterfaces.find(device->id());
-    if (oldInterfaceIter != s_dbusInterfaces.end()) {
-        qCDebug(KDECONNECT_PLUGIN_BATTERY) << "Deleting stale BatteryDbusInterface for" << device->name();
-        //FIXME: This still crashes sometimes even after the workaround made in 38aa970, commented out by now
-        //oldInterfaceIter.value()->deleteLater();
-        s_dbusInterfaces.erase(oldInterfaceIter);
-    }
-
-    s_dbusInterfaces[device->id()] = this;
-}
-
-BatteryDbusInterface::~BatteryDbusInterface()
-{
-    qCDebug(KDECONNECT_PLUGIN_BATTERY) << "Destroying BatteryDbusInterface";
-}
-
-void BatteryDbusInterface::updateValues(bool isCharging, int currentCharge)
-{
-    m_isCharging = isCharging;
-    m_charge = currentCharge;
-
-    Q_EMIT stateChanged(m_isCharging);
-    Q_EMIT chargeChanged(m_charge);
-}
-
-

plugins/battery/batterydbusinterface.h

@@ -1,41 +0,0 @@
-/**
- * SPDX-FileCopyrightText: 2013 Albert Vaca <albertvaka@gmail.com>
- *
- * SPDX-License-Identifier: GPL-2.0-only OR GPL-3.0-only OR LicenseRef-KDE-Accepted-GPL
- */
-
-#ifndef BATTERYDBUSINTERFACE_H
-#define BATTERYDBUSINTERFACE_H
-
-#include <QDBusAbstractAdaptor>
-
-class Device;
-
-class BatteryDbusInterface
-    : public QDBusAbstractAdaptor
-{
-    Q_OBJECT
-    Q_CLASSINFO("D-Bus Interface", "org.kde.kdeconnect.device.battery")
-
-public:
-    explicit BatteryDbusInterface(const Device* device);
-    ~BatteryDbusInterface() override;
-    
-    Q_SCRIPTABLE int charge() const { return m_charge; }
-    Q_SCRIPTABLE bool isCharging() const { return m_isCharging; }
-
-    void updateValues(bool isCharging, int currentCharge);
-
-Q_SIGNALS:
-    Q_SCRIPTABLE void stateChanged(bool charging);  
-    Q_SCRIPTABLE void chargeChanged(int charge);
-
-private:
-    int m_charge;
-    bool m_isCharging;
-
-    // Map to save current BatteryDbusInterface for each device.
-    static QMap<QString, BatteryDbusInterface *> s_dbusInterfaces;
-};
-
-#endif

plugins/battery/batteryplugin.cpp

@@ -15,16 +15,23 @@
 
 #include <core/daemon.h>
 
-#include "batterydbusinterface.h"
 #include "plugin_battery_debug.h"
 
 K_PLUGIN_CLASS_WITH_JSON(BatteryPlugin, "kdeconnect_battery.json")
 
 BatteryPlugin::BatteryPlugin(QObject* parent, const QVariantList& args)
     : KdeConnectPlugin(parent, args)
-    , batteryDbusInterface(new BatteryDbusInterface(device()))
 {
+}
 
+int BatteryPlugin::charge() const
+{
+    return m_charge;
+}
+
+bool BatteryPlugin::isCharging() const
+{
+    return m_isCharging;
 }
 
 void BatteryPlugin::connected()
@@ -50,16 +57,15 @@ void BatteryPlugin::connected()
     // and desktops, this is a safe assumption).
     const Solid::Battery* chosen = batteries.first().as<Solid::Battery>();
 
-    connect(chosen, &Solid::Battery::chargeStateChanged, this, &BatteryPlugin::chargeChanged);
-    connect(chosen, &Solid::Battery::chargePercentChanged, this, &BatteryPlugin::chargeChanged);
+    connect(chosen, &Solid::Battery::chargeStateChanged, this, &BatteryPlugin::slotChargeChanged);
+    connect(chosen, &Solid::Battery::chargePercentChanged, this, &BatteryPlugin::slotChargeChanged);
 
     // Explicitly send the current charge
-    chargeChanged();
+    slotChargeChanged();
 }
 
-void BatteryPlugin::chargeChanged()
+void BatteryPlugin::slotChargeChanged()
 {
-
     // Note: the NetworkPacket sent at the end of this method can reflect MULTIPLE batteries.
     // We average the total charge against the total number of batteries, which in practice
     // seems to work out ok.
@@ -111,35 +117,24 @@ void BatteryPlugin::chargeChanged()
     sendPacket(status);
 }
 
-BatteryPlugin::~BatteryPlugin()
-{
-    //FIXME: Qt dbus does not allow to remove an adaptor! (it causes a crash in
-    // the next dbus access to its parent). The implication of not deleting this
-    // is that disabling the plugin does not remove the interface (that will
-    // return outdated values) and that enabling it again instantiates a second
-    // adaptor. This is also a memory leak until the entire device is destroyed.
-
-    //batteryDbusInterface->deleteLater();
-}
-
 bool BatteryPlugin::receivePacket(const NetworkPacket& np)
 {
-    bool isCharging = np.get<bool>(QStringLiteral("isCharging"), false);
-    int currentCharge = np.get<int>(QStringLiteral("currentCharge"), -1);
-    int thresholdEvent = np.get<int>(QStringLiteral("thresholdEvent"), (int)ThresholdNone);
+    m_isCharging = np.get<bool>(QStringLiteral("isCharging"), false);
+    m_charge = np.get<int>(QStringLiteral("currentCharge"), -1);
+    const int thresholdEvent = np.get<int>(QStringLiteral("thresholdEvent"), (int)ThresholdNone);
 
-    if (batteryDbusInterface->charge() != currentCharge
-        || batteryDbusInterface->isCharging() != isCharging
-    ) {
-        batteryDbusInterface->updateValues(isCharging, currentCharge);
-    }
+    Q_EMIT refreshed();
 
-    if ( thresholdEvent == ThresholdBatteryLow && !isCharging ) {
-        Daemon::instance()->sendSimpleNotification(QStringLiteral("batteryLow"), i18nc("device name: low battery", "%1: Low Battery", device()->name()), i18n("Battery at %1%", currentCharge), QStringLiteral("battery-040"));
+    if (thresholdEvent == ThresholdBatteryLow && !m_isCharging) {
+        Daemon::instance()->sendSimpleNotification(QStringLiteral("batteryLow"), i18nc("device name: low battery", "%1: Low Battery", device()->name()), i18n("Battery at %1%", m_charge), QStringLiteral("battery-040"));
     }
 
     return true;
+}
 
+QString BatteryPlugin::dbusPath() const
+{
+    return QStringLiteral("/modules/kdeconnect/devices/") + device()->id() + QStringLiteral("/battery");
 }
 
 #include "batteryplugin.moc"

plugins/battery/batteryplugin.h

@@ -12,26 +12,30 @@
 #define PACKET_TYPE_BATTERY QStringLiteral("kdeconnect.battery")
 #define PACKET_TYPE_BATTERY_REQUEST QStringLiteral("kdeconnect.battery.request")
 
-class BatteryDbusInterface;
-
 class BatteryPlugin
     : public KdeConnectPlugin
 {
     Q_OBJECT
+    Q_CLASSINFO("D-Bus Interface", "org.kde.kdeconnect.device.battery")
+    Q_PROPERTY(int charge READ charge NOTIFY refreshed)
+    Q_PROPERTY(bool isCharging READ isCharging NOTIFY refreshed)
 
 public:
     explicit BatteryPlugin(QObject* parent, const QVariantList& args);
-    ~BatteryPlugin() override;
 
     bool receivePacket(const NetworkPacket& np) override;
     void connected() override;
+    QString dbusPath() const override;
 
-    // NB: This may be connected to zero or two signals in Solid::Battery -
-    // chargePercentageChanged and chargeStatusChanged.
-    // See inline comments for further details
-    void chargeChanged();
+    int charge() const;
+    bool isCharging() const;
+
+Q_SIGNALS:
+    Q_SCRIPTABLE void refreshed();
 
 private:
+    void slotChargeChanged();
+
     // Keep these values in sync with THRESHOLD* constants in
     // kdeconnect-android:BatteryPlugin.java
     // see README for their meaning
@@ -40,7 +44,8 @@ private:
         ThresholdBatteryLow = 1
     };
 
-    BatteryDbusInterface* batteryDbusInterface;
+    int m_charge;
+    bool m_isCharging;
 };
 
 #endif