Code review

Fixes/adds some comments.
Proper use of some Qt API.

REVIEW: 113195

kded/backends/lan/landevicelink.cpp

@@ -64,6 +64,10 @@ bool LanDeviceLink::sendPackageEncrypted(QCA::PublicKey& key, NetworkPackage& np
     np.encrypt(key);
 
     int written = mSocket->write(np.serialize());
+
+    //TODO: Actually detect if a package is received or not, now we keep TCP
+    //"ESTABLISHED" connections that look legit (return true when we use them),
+    //but that are actually broken
     return (written != -1);
 }
 
@@ -76,6 +80,9 @@ bool LanDeviceLink::sendPackage(NetworkPackage& np)
     }
 
     int written = mSocket->write(np.serialize());
+    //TODO: Actually detect if a package is received or not, now we keep TCP
+    //"ESTABLISHED" connections that look legit (return true when we use them),
+    //but that are actually broken
     return (written != -1);
 }
 

kded/device.cpp

@@ -20,36 +20,31 @@
 #include "networkpackage.h"
 
 Device::Device(const QString& id)
+    : m_deviceId(id)
+    , m_pairStatus(Device::Paired)
+    , m_protocolVersion(NetworkPackage::ProtocolVersion) //We don't know it yet
 {
-
-    m_deviceId = id;
-
     KSharedConfigPtr config = KSharedConfig::openConfig("kdeconnectrc");
-    const KConfigGroup& data = config->group("trusted_devices").group(id);
+    KConfigGroup data = config->group("trusted_devices").group(id);
 
     m_deviceName = data.readEntry<QString>("deviceName", QString("unnamed"));
 
     const QString& key = data.readEntry<QString>("publicKey",QString());
     m_publicKey = QCA::RSAPublicKey::fromPEM(key);
 
-    m_pairStatus = Device::Paired;
-
-    m_protocolVersion = NetworkPackage::ProtocolVersion; //We don't know it yet
-
     //Register in bus
     QDBusConnection::sessionBus().registerObject(dbusPath(), this, QDBusConnection::ExportScriptableContents | QDBusConnection::ExportAdaptors);
 
 }
 
 Device::Device(const NetworkPackage& identityPackage, DeviceLink* dl)
+    : m_deviceId(identityPackage.get<QString>("deviceId"))
+    , m_deviceName(identityPackage.get<QString>("deviceName"))
+    , m_pairStatus(Device::NotPaired)
+    , m_protocolVersion(identityPackage.get<int>("protocolVersion"))
 {
-    m_deviceId = identityPackage.get<QString>("deviceId");
-    m_deviceName = identityPackage.get<QString>("deviceName");
-    m_protocolVersion = identityPackage.get<int>("protocolVersion");
     addLink(identityPackage, dl);
 
-    m_pairStatus = Device::NotPaired;
-
     //Register in bus
     QDBusConnection::sessionBus().registerObject(dbusPath(), this, QDBusConnection::ExportScriptableContents | QDBusConnection::ExportAdaptors);
 }
@@ -75,24 +70,23 @@ void Device::reloadPlugins()
 
     if (isPaired() && isReachable()) { //Do not load any plugin for unpaired devices, nor useless loading them for unreachable devices
 
-        QString path = KStandardDirs().resourceDirs("config").first()+"kdeconnect/";
+        QString path = KGlobal::dirs()->findResource("config", "kdeconnect/"+id());
-        QMap<QString,QString> pluginStates = KSharedConfig::openConfig(path + id())->group("Plugins").entryMap();
+        KConfigGroup pluginStates = KSharedConfig::openConfig(path)->group("Plugins");
 
         PluginLoader* loader = PluginLoader::instance();
 
         //Code borrowed from KWin
         foreach (const QString& pluginName, loader->getPluginList()) {
+            QString enabledKey = pluginName + QString::fromLatin1("Enabled");
 
-            const QString value = pluginStates.value(pluginName + QString::fromLatin1("Enabled"), QString());
+            bool isPluginEnabled = (pluginStates.hasKey(enabledKey) ? pluginStates.readEntry(enabledKey, false)
-            KPluginInfo info = loader->getPluginInfo(pluginName);
+                                                            : loader->getPluginInfo(pluginName).isPluginEnabledByDefault());
-            bool enabled = (value.isNull() ? info.isPluginEnabledByDefault() : QVariant(value).toBool());
 
-            if (enabled) {
+            if (isPluginEnabled) {
-
+                KdeConnectPlugin* reusedPluginInstance = m_plugins.take(pluginName);
-                if (m_plugins.contains(pluginName)) {
+                if (reusedPluginInstance) {
                     //Already loaded, reuse it
-                    newPluginMap[pluginName] = m_plugins[pluginName];
+                    newPluginMap[pluginName] = reusedPluginInstance;
-                    m_plugins.remove(pluginName);
                 } else {
                     KdeConnectPlugin* plugin = loader->instantiatePluginForDevice(pluginName, this);
 
@@ -105,11 +99,9 @@ void Device::reloadPlugins()
         }
     }
 
-    //Erase all the plugins left in the original map (it means that we don't want
+    //Erase all left plugins in the original map (meaning that we don't want
-    //them anymore, otherways they would have been moved to the newPluginMap)
+    //them anymore, otherwise they would have been moved to the newPluginMap)
     qDeleteAll(m_plugins);
-    m_plugins.clear();
-
     m_plugins = newPluginMap;
 
     Q_FOREACH(KdeConnectPlugin* plugin, m_plugins) {
@@ -122,18 +114,20 @@ void Device::reloadPlugins()
 
 void Device::requestPair()
 {
-    if (m_pairStatus == Device::Paired) {
+    switch(m_pairStatus) {
+        case Device::Paired:
             Q_EMIT pairingFailed(i18n("Already paired"));
             return;
-    }
+        case Device::Requested:
-    if (m_pairStatus == Device::Requested) {
             Q_EMIT pairingFailed(i18n("Pairing already requested for this device"));
             return;
-    }
+        default:
             if (!isReachable()) {
                 Q_EMIT pairingFailed(i18n("Device not reachable"));
                 return;
             }
+            break;
+    }
 
     m_pairStatus = Device::Requested;
 
@@ -242,7 +236,7 @@ void Device::removeLink(DeviceLink* link)
 
     //qDebug() << "RemoveLink" << m_deviceLinks.size() << "links remaining";
 
-    if (m_deviceLinks.empty()) {
+    if (m_deviceLinks.isEmpty()) {
         reloadPlugins();
         Q_EMIT reachableStatusChanged();
     }
@@ -252,17 +246,11 @@ bool Device::sendPackage(NetworkPackage& np)
 {
     if (np.type() != PACKAGE_TYPE_PAIR && isPaired()) {
         Q_FOREACH(DeviceLink* dl, m_deviceLinks) {
-            //TODO: Actually detect if a package is received or not, now we keep TCP
-            //"ESTABLISHED" connections that look legit (return true when we use them),
-            //but that are actually broken
             if (dl->sendPackageEncrypted(m_publicKey, np)) return true;
         }
     } else {
         //Maybe we could block here any package that is not an identity or a pairing package to prevent sending non encrypted data
         Q_FOREACH(DeviceLink* dl, m_deviceLinks) {
-            //TODO: Actually detect if a package is received or not, now we keep TCP
-            //"ESTABLISHED" connections that look legit (return true when we use them),
-            //but that are actually broken
             if (dl->sendPackage(np)) return true;
         }
     }

kded/device.h

@@ -49,10 +49,18 @@ class Device
     };
 
 public:
-    //Read device from KConfig, we already know it but we need to wait for a incoming devicelink to communicate
+    /**
+     * Reads the @p device from KConfig
+     *
+     * We already know it but we need to wait for an incoming DeviceLink to communicate
+     */
     Device(const QString& id);
 
-    //Device known via an incoming connection sent to us via a devicelink, we know everything but we don't trust it yet
+    /**
+     * Device known via an incoming connection sent to us via a devicelink.
+     *
+     * We know everything but we don't trust it yet
+     */
     Device(const NetworkPackage& np, DeviceLink* dl);
 
     virtual ~Device();
@@ -69,15 +77,17 @@ public:
     Q_SCRIPTABLE bool pairRequested() const { return m_pairStatus==Device::Requested; }
 
     Q_SCRIPTABLE QStringList availableLinks() const;
-    Q_SCRIPTABLE bool isReachable() const { return !m_deviceLinks.empty(); }
+    Q_SCRIPTABLE bool isReachable() const { return !m_deviceLinks.isEmpty(); }
 
     Q_SCRIPTABLE QStringList loadedPlugins() const;
     Q_SCRIPTABLE bool hasPlugin(const QString& name) const;
 
-    //Send and receive
 Q_SIGNALS:
+    ///notifies about a @p np package that has just been received from the device
     void receivedPackage(const NetworkPackage& np) const;
+
 public Q_SLOTS:
+    ///sends a @p np package to the device
     virtual bool sendPackage(NetworkPackage& np);
 
     //Dbus operations
@@ -103,7 +113,7 @@ Q_SIGNALS:
 
 private:
     //TODO: Replace device id by public key
-    QString m_deviceId;
+    const QString m_deviceId;
     QString m_deviceName;
     QCA::PublicKey m_publicKey;
     PairStatus m_pairStatus;

kded/networkpackagetypes.h

@@ -21,16 +21,10 @@
 #ifndef NETWORKPACKAGETYPES_H
 #define NETWORKPACKAGETYPES_H
 
-#define PACKAGE_TYPE_IDENTITY QString("kdeconnect.identity")
+#define PACKAGE_TYPE_IDENTITY QLatin1String("kdeconnect.identity")
-#define PACKAGE_TYPE_PAIR QString("kdeconnect.pair")
+#define PACKAGE_TYPE_PAIR QLatin1String("kdeconnect.pair")
-#define PACKAGE_TYPE_ENCRYPTED QString("kdeconnect.encrypted")
+#define PACKAGE_TYPE_ENCRYPTED QLatin1String("kdeconnect.encrypted")
-#define PACKAGE_TYPE_PING QString("kdeconnect.ping")
+#define PACKAGE_TYPE_TELEPHONY QLatin1String("kdeconnect.telephony")
-#define PACKAGE_TYPE_NOTIFICATION QString("kdeconnect.notification")
+#define PACKAGE_TYPE_PING QLatin1String("kdeconnect.ping")
-#define PACKAGE_TYPE_BATTERY QString("kdeconnect.battery")
-#define PACKAGE_TYPE_TELEPHONY QString("kdeconnect.telephony")
-#define PACKAGE_TYPE_CLIPBOARD QString("kdeconnect.clipboard")
-#define PACKAGE_TYPE_MPRIS QString("kdeconnect.mpris")
-#define PACKAGE_TYPE_SHARE QString("kdeconnect.share")
-
 
 #endif // NETWORKPACKAGETYPES_H

kded/plugins/battery/batteryplugin.cpp

@@ -69,6 +69,12 @@ bool BatteryPlugin::receivePackage(const NetworkPackage& np)
 
         batteryDbusInterface->updateValues(isCharging, currentCharge);
 
+        //FIXME: Where's that 14 coming from?
+        //TODO: Decouple the protocol from Android
+        /*TODO: Look into the following android interfaces
+                    android.intent.action.BATTERY_LOW
+                    android.intent.action.BATTERY_OKAY
+        */
         if (currentCharge == 14 && !isCharging) {
             KNotification* notification = new KNotification("batteryLow");
             notification->setPixmap(KIcon("battery-040").pixmap(48, 48));

kded/plugins/battery/batteryplugin.h

@@ -21,12 +21,10 @@
 #ifndef BATTERYPLUGIN_H
 #define BATTERYPLUGIN_H
 
-#include <QDBusAbstractAdaptor>
-
-#include <KNotification>
-
 #include "../kdeconnectplugin.h"
 
+#define PACKAGE_TYPE_BATTERY QLatin1String("kdeconnect.battery")
+
 class BatteryDbusInterface;
 
 class BatteryPlugin

kded/plugins/clipboard/clipboardplugin.cpp

@@ -29,15 +29,15 @@ K_EXPORT_PLUGIN( KdeConnectPluginFactory("kdeconnect_clipboard", "kdeconnect_cli
 
 ClipboardPlugin::ClipboardPlugin(QObject *parent, const QVariantList &args)
     : KdeConnectPlugin(parent, args)
+    , clipboard(QApplication::clipboard())
+    , ignore_next_clipboard_change(false)
 {
-    clipboard = QApplication::clipboard();
+    connect(clipboard, SIGNAL(changed(QClipboard::Mode)), this, SLOT(clipboardChanged(QClipboard::Mode)));
-    ignore_next_clipboard_change = false;
-    connect(clipboard,SIGNAL(changed(QClipboard::Mode)),this,SLOT(clipboardChanged(QClipboard::Mode)));
 }
 
 void ClipboardPlugin::clipboardChanged(QClipboard::Mode mode)
 {
-    if (mode != QClipboard::QClipboard::Clipboard) return;
+    if (mode != QClipboard::Clipboard) return;
 
     if (ignore_next_clipboard_change) {
         ignore_next_clipboard_change = false;

kded/plugins/clipboard/clipboardplugin.h

@@ -28,6 +28,8 @@
 #include "../../networkpackage.h"
 #include "../../device.h"
 
+#define PACKAGE_TYPE_CLIPBOARD QLatin1String("kdeconnect.clipboard")
+
 class ClipboardPlugin
     : public KdeConnectPlugin
 {

kded/plugins/kdeconnectplugin.h

@@ -41,9 +41,16 @@ public:
     Device* device();
 
 public Q_SLOTS:
-    //Returns true if it has handled the package in some way
+    /**
-    //device.sendPackage can be used to send an answer back to the device
+     * Returns true if it has handled the package in some way
+     * device.sendPackage can be used to send an answer back to the device
+     */
     virtual bool receivePackage(const NetworkPackage& np) = 0;
+
+    /**
+     * This method will be called when a device is connected to this computer.
+     * The plugin could be loaded already, but there is no guarantee we will be able to reach the device until this is called.
+     */
     virtual void connected() = 0;
 
 private:

kded/plugins/mpriscontrol/mpriscontrolplugin.h

@@ -26,6 +26,8 @@
 
 #include "../kdeconnectplugin.h"
 
+#define PACKAGE_TYPE_MPRIS QLatin1String("kdeconnect.mpris")
+
 class MprisControlPlugin
     : public KdeConnectPlugin
 {

kded/plugins/notifications/notificationsdbusinterface.cpp

@@ -21,6 +21,7 @@
 #include "notificationsdbusinterface.h"
 
 #include "../../filetransferjob.h"
+#include <notificationsplugin.h>
 
 #include <QDebug>
 #include <QDBusConnection>
@@ -67,7 +68,7 @@ void NotificationsDbusInterface::processPackage(const NetworkPackage& np)
 
         //Do not show updates to existent notification nor answers to a initialization request
         if (!mInternalIdToPublicId.contains(noti->internalId()) && !np.get<bool>("requestAnswer", false)) {
-            KNotification* notification = new KNotification("notification");
+            KNotification* notification = new KNotification("notification", KNotification::CloseOnTimeout, this);
             notification->setPixmap(KIcon("preferences-desktop-notification").pixmap(48, 48));
             notification->setComponentData(KComponentData("kdeconnect", "kdeconnect"));
             notification->setTitle(mDevice->name());
@@ -107,17 +108,14 @@ void NotificationsDbusInterface::removeNotification(const QString& internalId)
         return;
     }
 
-    QString publicId = mInternalIdToPublicId[internalId];
+    QString publicId = mInternalIdToPublicId.take(internalId);
-    mInternalIdToPublicId.remove(internalId);
 
-    if (!mNotifications.contains(publicId)) {
+    Notification* noti = mNotifications.take(publicId);
+    if (!noti) {
         qDebug() << "Not found";
         return;
     }
 
-    Notification* noti = mNotifications[publicId];
-    mNotifications.remove(publicId);
-
     //Deleting the notification will unregister it automatically
     //QDBusConnection::sessionBus().unregisterObject(mDevice->dbusPath()+"/notifications/"+publicId);
     noti->deleteLater();

kded/plugins/notifications/notificationsplugin.h

@@ -24,6 +24,7 @@
 #include <knotification.h>
 
 #include "../kdeconnectplugin.h"
+#define PACKAGE_TYPE_NOTIFICATION QLatin1String("kdeconnect.notification")
 
 /*
  * This class is just a proxy for NotificationsDbusInterface

kded/plugins/pausemusic/pausemusicplugin.cpp

@@ -32,11 +32,8 @@ K_EXPORT_PLUGIN( KdeConnectPluginFactory("kdeconnect_pausemusic", "kdeconnect_pa
 
 PauseMusicPlugin::PauseMusicPlugin(QObject* parent, const QVariantList& args)
     : KdeConnectPlugin(parent, args)
+    , pauseWhen(PauseWhenRinging) //TODO: Be able to change this from plugin settings
 {
-
-    //TODO: Be able to change this from plugin settings
-    pauseWhen = PauseWhenRinging;
-
 }
 
 bool PauseMusicPlugin::receivePackage(const NetworkPackage& np)
@@ -46,22 +43,19 @@ bool PauseMusicPlugin::receivePackage(const NetworkPackage& np)
         return false;
     }
 
-
+    switch(pauseWhen) {
-    if (pauseWhen == PauseWhenRinging) {
+        case PauseWhenRinging:
-
             if (np.get<QString>("event") != "ringing" && np.get<QString>("event") != "talking") {
                 return true;
             }
-
+            break;
-    } else if (pauseWhen == PauseWhenTalking) {
+        case PauseWhenTalking:
-
             if (np.get<QString>("event") != "talking") {
                 return true;
             }
-
+            break;
     }
 
-
     bool pauseConditionFulfilled = !np.get<bool>("isCancel");
 
     if (pauseConditionFulfilled) {

kded/plugins/sharereceiver/sharereceiverplugin.h

@@ -26,6 +26,8 @@
 
 #include "../kdeconnectplugin.h"
 
+#define PACKAGE_TYPE_SHARE QLatin1String("kdeconnect.share")
+
 class ShareReceiverPlugin
     : public KdeConnectPlugin
 {

kded/plugins/telephony/telephonyplugin.cpp

@@ -68,7 +68,7 @@ KNotification* TelephonyPlugin::createNotification(const NetworkPackage& np)
 
     qDebug() << "Creating notification with type:" << type;
 
-    KNotification* notification = new KNotification(type); //, KNotification::Persistent
+    KNotification* notification = new KNotification(type, KNotification::CloseOnTimeout, this); //, KNotification::Persistent
     notification->setPixmap(KIcon(icon).pixmap(48, 48));
     notification->setComponentData(KComponentData("kdeconnect", "kdeconnect"));
     notification->setTitle(title);