From f90d6a019ff288c4a16f91c2d7baa617ce793077 Mon Sep 17 00:00:00 2001 From: Nicolas Fella Date: Sat, 22 Aug 2020 22:37:18 +0200 Subject: [PATCH] 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 /battery for consistency with other plugins --- .../kdeconnectdeclarativeplugin.cpp | 2 +- indicator/deviceindicator.cpp | 14 ++--- interfaces/CMakeLists.txt | 2 +- interfaces/dbusinterfaces.cpp | 11 ++-- interfaces/dbusinterfaces.h | 13 +++-- plasmoid/package/contents/ui/Battery.qml | 26 ++-------- plugins/battery/CMakeLists.txt | 1 - plugins/battery/batterydbusinterface.cpp | 50 ------------------ plugins/battery/batterydbusinterface.h | 41 --------------- plugins/battery/batteryplugin.cpp | 51 +++++++++---------- plugins/battery/batteryplugin.h | 21 +++++--- 11 files changed, 63 insertions(+), 169 deletions(-) delete mode 100644 plugins/battery/batterydbusinterface.cpp delete mode 100644 plugins/battery/batterydbusinterface.h diff --git a/declarativeplugin/kdeconnectdeclarativeplugin.cpp b/declarativeplugin/kdeconnectdeclarativeplugin.cpp index f5849b751..12e44c92b 100644 --- a/declarativeplugin/kdeconnectdeclarativeplugin.cpp +++ b/declarativeplugin/kdeconnectdeclarativeplugin.cpp @@ -65,7 +65,7 @@ void KdeConnectDeclarativePlugin::registerTypes(const char* uri) #endif registerFactory(uri, "DeviceDbusInterfaceFactory"); - registerFactory(uri, "DeviceBatteryDbusInterfaceFactory"); + registerFactory(uri, "DeviceBatteryDbusInterfaceFactory"); registerFactory(uri, "FindMyPhoneDbusInterfaceFactory"); registerFactory(uri, "SftpDbusInterfaceFactory"); registerFactory(uri, "RemoteKeyboardDbusInterfaceFactory"); diff --git a/indicator/deviceindicator.cpp b/indicator/deviceindicator.cpp index 91f7d061e..349fa5a6b 100644 --- a/indicator/deviceindicator.cpp +++ b/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; }; diff --git a/interfaces/CMakeLists.txt b/interfaces/CMakeLists.txt index 7912a6dd0..8f47b4eb2 100644 --- a/interfaces/CMakeLists.txt +++ b/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) diff --git a/interfaces/dbusinterfaces.cpp b/interfaces/dbusinterfaces.cpp index 228a6b866..a32f9038c 100644 --- a/interfaces/dbusinterfaces.cpp +++ b/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) diff --git a/interfaces/dbusinterfaces.h b/interfaces/dbusinterfaces.h index 683cbd82e..370d7951c 100644 --- a/interfaces/dbusinterfaces.h +++ b/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 diff --git a/plasmoid/package/contents/ui/Battery.qml b/plasmoid/package/contents/ui/Battery.qml index a2f83fb9c..e9533a55e 100644 --- a/plasmoid/package/contents/ui/Battery.qml +++ b/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 } diff --git a/plugins/battery/CMakeLists.txt b/plugins/battery/CMakeLists.txt index e4b947b7a..a77c67d45 100644 --- a/plugins/battery/CMakeLists.txt +++ b/plugins/battery/CMakeLists.txt @@ -7,7 +7,6 @@ ecm_qt_declare_logging_category( set(kdeconnect_battery_SRCS batteryplugin.cpp - batterydbusinterface.cpp ${debug_file_SRCS} ) diff --git a/plugins/battery/batterydbusinterface.cpp b/plugins/battery/batterydbusinterface.cpp deleted file mode 100644 index e2f148a30..000000000 --- a/plugins/battery/batterydbusinterface.cpp +++ /dev/null @@ -1,50 +0,0 @@ -/** - * SPDX-FileCopyrightText: 2013 Albert Vaca - * - * SPDX-License-Identifier: GPL-2.0-only OR GPL-3.0-only OR LicenseRef-KDE-Accepted-GPL - */ - -#include "batterydbusinterface.h" -#include "batteryplugin.h" - -#include -#include -#include "plugin_battery_debug.h" - -QMap BatteryDbusInterface::s_dbusInterfaces; - -BatteryDbusInterface::BatteryDbusInterface(const Device* device) - : QDBusAbstractAdaptor(const_cast(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::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); -} - - diff --git a/plugins/battery/batterydbusinterface.h b/plugins/battery/batterydbusinterface.h deleted file mode 100644 index 69000296e..000000000 --- a/plugins/battery/batterydbusinterface.h +++ /dev/null @@ -1,41 +0,0 @@ -/** - * SPDX-FileCopyrightText: 2013 Albert Vaca - * - * SPDX-License-Identifier: GPL-2.0-only OR GPL-3.0-only OR LicenseRef-KDE-Accepted-GPL - */ - -#ifndef BATTERYDBUSINTERFACE_H -#define BATTERYDBUSINTERFACE_H - -#include - -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 s_dbusInterfaces; -}; - -#endif diff --git a/plugins/battery/batteryplugin.cpp b/plugins/battery/batteryplugin.cpp index 5e7a0acd4..5bbc616d1 100644 --- a/plugins/battery/batteryplugin.cpp +++ b/plugins/battery/batteryplugin.cpp @@ -15,16 +15,23 @@ #include -#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(); - 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(QStringLiteral("isCharging"), false); - int currentCharge = np.get(QStringLiteral("currentCharge"), -1); - int thresholdEvent = np.get(QStringLiteral("thresholdEvent"), (int)ThresholdNone); + m_isCharging = np.get(QStringLiteral("isCharging"), false); + m_charge = np.get(QStringLiteral("currentCharge"), -1); + const int thresholdEvent = np.get(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" diff --git a/plugins/battery/batteryplugin.h b/plugins/battery/batteryplugin.h index dc3030b8a..72fde5f18 100644 --- a/plugins/battery/batteryplugin.h +++ b/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