From faa3daa58544fbbd6a2fb2d3a11d2dc02cc94f82 Mon Sep 17 00:00:00 2001 From: Aleix Pol Date: Fri, 11 Oct 2013 15:19:23 +0200 Subject: [PATCH] Code review Fixes/adds some comments. Proper use of some Qt API. REVIEW: 113195 --- kded/backends/lan/landevicelink.cpp | 7 ++ kded/device.cpp | 80 ++++++++----------- kded/device.h | 20 +++-- kded/networkpackagetypes.h | 16 ++-- kded/plugins/battery/batteryplugin.cpp | 6 ++ kded/plugins/battery/batteryplugin.h | 6 +- kded/plugins/clipboard/clipboardplugin.cpp | 8 +- kded/plugins/clipboard/clipboardplugin.h | 2 + kded/plugins/kdeconnectplugin.h | 11 ++- .../plugins/mpriscontrol/mpriscontrolplugin.h | 2 + .../notificationsdbusinterface.cpp | 12 ++- .../notifications/notificationsplugin.h | 1 + kded/plugins/pausemusic/pausemusicplugin.cpp | 30 +++---- .../sharereceiver/sharereceiverplugin.h | 2 + kded/plugins/telephony/telephonyplugin.cpp | 2 +- 15 files changed, 107 insertions(+), 98 deletions(-) diff --git a/kded/backends/lan/landevicelink.cpp b/kded/backends/lan/landevicelink.cpp index d2d268208..bf9bd756b 100644 --- a/kded/backends/lan/landevicelink.cpp +++ b/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); } diff --git a/kded/device.cpp b/kded/device.cpp index b7b015d8a..a7c501eef 100644 --- a/kded/device.cpp +++ b/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("deviceName", QString("unnamed")); const QString& key = data.readEntry("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("deviceId")) + , m_deviceName(identityPackage.get("deviceName")) + , m_pairStatus(Device::NotPaired) + , m_protocolVersion(identityPackage.get("protocolVersion")) { - m_deviceId = identityPackage.get("deviceId"); - m_deviceName = identityPackage.get("deviceName"); - m_protocolVersion = identityPackage.get("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/"; - QMap pluginStates = KSharedConfig::openConfig(path + id())->group("Plugins").entryMap(); + QString path = KGlobal::dirs()->findResource("config", "kdeconnect/"+id()); + 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()); - KPluginInfo info = loader->getPluginInfo(pluginName); - bool enabled = (value.isNull() ? info.isPluginEnabledByDefault() : QVariant(value).toBool()); + bool isPluginEnabled = (pluginStates.hasKey(enabledKey) ? pluginStates.readEntry(enabledKey, false) + : loader->getPluginInfo(pluginName).isPluginEnabledByDefault()); - if (enabled) { - - if (m_plugins.contains(pluginName)) { + if (isPluginEnabled) { + KdeConnectPlugin* reusedPluginInstance = m_plugins.take(pluginName); + if (reusedPluginInstance) { //Already loaded, reuse it - newPluginMap[pluginName] = m_plugins[pluginName]; - m_plugins.remove(pluginName); + newPluginMap[pluginName] = reusedPluginInstance; } else { KdeConnectPlugin* plugin = loader->instantiatePluginForDevice(pluginName, this); @@ -105,15 +99,13 @@ void Device::reloadPlugins() } } - //Erase all the plugins left in the original map (it means that we don't want - //them anymore, otherways they would have been moved to the newPluginMap) + //Erase all left plugins in the original map (meaning that we don't want + //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) { - plugin->connected(); + plugin->connected(); } Q_EMIT pluginsChanged(); @@ -122,17 +114,19 @@ void Device::reloadPlugins() void Device::requestPair() { - if (m_pairStatus == Device::Paired) { - Q_EMIT pairingFailed(i18n("Already paired")); - return; - } - if (m_pairStatus == Device::Requested) { - Q_EMIT pairingFailed(i18n("Pairing already requested for this device")); - return; - } - if (!isReachable()) { - Q_EMIT pairingFailed(i18n("Device not reachable")); - return; + switch(m_pairStatus) { + case Device::Paired: + Q_EMIT pairingFailed(i18n("Already paired")); + return; + case 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; } } diff --git a/kded/device.h b/kded/device.h index 7ac646b2c..196558bca 100644 --- a/kded/device.h +++ b/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; diff --git a/kded/networkpackagetypes.h b/kded/networkpackagetypes.h index 6fa9ff8f6..617d171e3 100644 --- a/kded/networkpackagetypes.h +++ b/kded/networkpackagetypes.h @@ -21,16 +21,10 @@ #ifndef NETWORKPACKAGETYPES_H #define NETWORKPACKAGETYPES_H -#define PACKAGE_TYPE_IDENTITY QString("kdeconnect.identity") -#define PACKAGE_TYPE_PAIR QString("kdeconnect.pair") -#define PACKAGE_TYPE_ENCRYPTED QString("kdeconnect.encrypted") -#define PACKAGE_TYPE_PING QString("kdeconnect.ping") -#define PACKAGE_TYPE_NOTIFICATION QString("kdeconnect.notification") -#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") - +#define PACKAGE_TYPE_IDENTITY QLatin1String("kdeconnect.identity") +#define PACKAGE_TYPE_PAIR QLatin1String("kdeconnect.pair") +#define PACKAGE_TYPE_ENCRYPTED QLatin1String("kdeconnect.encrypted") +#define PACKAGE_TYPE_TELEPHONY QLatin1String("kdeconnect.telephony") +#define PACKAGE_TYPE_PING QLatin1String("kdeconnect.ping") #endif // NETWORKPACKAGETYPES_H diff --git a/kded/plugins/battery/batteryplugin.cpp b/kded/plugins/battery/batteryplugin.cpp index 57bb2a64a..ecd247e5a 100644 --- a/kded/plugins/battery/batteryplugin.cpp +++ b/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)); diff --git a/kded/plugins/battery/batteryplugin.h b/kded/plugins/battery/batteryplugin.h index 95959b33d..3288980f3 100644 --- a/kded/plugins/battery/batteryplugin.h +++ b/kded/plugins/battery/batteryplugin.h @@ -21,12 +21,10 @@ #ifndef BATTERYPLUGIN_H #define BATTERYPLUGIN_H -#include - -#include - #include "../kdeconnectplugin.h" +#define PACKAGE_TYPE_BATTERY QLatin1String("kdeconnect.battery") + class BatteryDbusInterface; class BatteryPlugin diff --git a/kded/plugins/clipboard/clipboardplugin.cpp b/kded/plugins/clipboard/clipboardplugin.cpp index fe505d154..15f1dca21 100644 --- a/kded/plugins/clipboard/clipboardplugin.cpp +++ b/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(); - ignore_next_clipboard_change = false; - connect(clipboard,SIGNAL(changed(QClipboard::Mode)),this,SLOT(clipboardChanged(QClipboard::Mode))); + 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; diff --git a/kded/plugins/clipboard/clipboardplugin.h b/kded/plugins/clipboard/clipboardplugin.h index b87e3a095..9a7cab4f5 100644 --- a/kded/plugins/clipboard/clipboardplugin.h +++ b/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 { diff --git a/kded/plugins/kdeconnectplugin.h b/kded/plugins/kdeconnectplugin.h index b8c9c44e8..051fdc488 100644 --- a/kded/plugins/kdeconnectplugin.h +++ b/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: diff --git a/kded/plugins/mpriscontrol/mpriscontrolplugin.h b/kded/plugins/mpriscontrol/mpriscontrolplugin.h index 61a1f7820..abb4587ec 100644 --- a/kded/plugins/mpriscontrol/mpriscontrolplugin.h +++ b/kded/plugins/mpriscontrol/mpriscontrolplugin.h @@ -26,6 +26,8 @@ #include "../kdeconnectplugin.h" +#define PACKAGE_TYPE_MPRIS QLatin1String("kdeconnect.mpris") + class MprisControlPlugin : public KdeConnectPlugin { diff --git a/kded/plugins/notifications/notificationsdbusinterface.cpp b/kded/plugins/notifications/notificationsdbusinterface.cpp index 1248c52d9..cbaba5261 100644 --- a/kded/plugins/notifications/notificationsdbusinterface.cpp +++ b/kded/plugins/notifications/notificationsdbusinterface.cpp @@ -21,6 +21,7 @@ #include "notificationsdbusinterface.h" #include "../../filetransferjob.h" +#include #include #include @@ -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("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]; - mInternalIdToPublicId.remove(internalId); + QString publicId = mInternalIdToPublicId.take(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(); diff --git a/kded/plugins/notifications/notificationsplugin.h b/kded/plugins/notifications/notificationsplugin.h index 37549702f..8b800d4d7 100644 --- a/kded/plugins/notifications/notificationsplugin.h +++ b/kded/plugins/notifications/notificationsplugin.h @@ -24,6 +24,7 @@ #include #include "../kdeconnectplugin.h" +#define PACKAGE_TYPE_NOTIFICATION QLatin1String("kdeconnect.notification") /* * This class is just a proxy for NotificationsDbusInterface diff --git a/kded/plugins/pausemusic/pausemusicplugin.cpp b/kded/plugins/pausemusic/pausemusicplugin.cpp index 0c2de49cb..8f6d3aa1b 100644 --- a/kded/plugins/pausemusic/pausemusicplugin.cpp +++ b/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; } - - if (pauseWhen == PauseWhenRinging) { - - if (np.get("event") != "ringing" && np.get("event") != "talking") { - return true; - } - - } else if (pauseWhen == PauseWhenTalking) { - - if (np.get("event") != "talking") { - return true; - } - + switch(pauseWhen) { + case PauseWhenRinging: + if (np.get("event") != "ringing" && np.get("event") != "talking") { + return true; + } + break; + case PauseWhenTalking: + if (np.get("event") != "talking") { + return true; + } + break; } - bool pauseConditionFulfilled = !np.get("isCancel"); if (pauseConditionFulfilled) { diff --git a/kded/plugins/sharereceiver/sharereceiverplugin.h b/kded/plugins/sharereceiver/sharereceiverplugin.h index 2def1d872..16580132f 100644 --- a/kded/plugins/sharereceiver/sharereceiverplugin.h +++ b/kded/plugins/sharereceiver/sharereceiverplugin.h @@ -26,6 +26,8 @@ #include "../kdeconnectplugin.h" +#define PACKAGE_TYPE_SHARE QLatin1String("kdeconnect.share") + class ShareReceiverPlugin : public KdeConnectPlugin { diff --git a/kded/plugins/telephony/telephonyplugin.cpp b/kded/plugins/telephony/telephonyplugin.cpp index ab5c8075a..8386a219a 100644 --- a/kded/plugins/telephony/telephonyplugin.cpp +++ b/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);