Review Request 118626: KDEConnect-KDE: 16 commits pack.

Aleix Pol Gonzalez aleixpol at kde.org
Wed Jun 11 22:27:23 UTC 2014 


> On June 9, 2014, 3:21 p.m., Aleix Pol Gonzalez wrote:
> > I'm sorry if I missed some discussion, but why is the change from Device -> IDevice?
> > 
> > This patch is changing many unrelated things, it's a bit misleading.
> 
> Alexandr Akulich wrote:
>     I think you missed description. :)
>     
>     As you can see at http://quickgit.kde.org/?p=scratch%2Fakulichalexandr%2Fkdeconnect-kde.git,
>     in commit "Plugins ported to IDevice API. Removed Device/PluginLoader dependency." (next after
>     "Introduced IDevice interface to be used in plugins"), it is used to avoid useless
>     pluginloader.cpp (and, by the way, device.cpp) object linking into each plugin.
> 
> Aleix Pol Gonzalez wrote:
>     Oh I see! They were building this inside the plugin, that looks pretty bad!
>     
>     Ok, +1 here.
> 
> Alexandr Akulich wrote:
>     Thank you! :)
> 
> Albert Vaca Cintora wrote:
>     I don't know which is the correct way to do this, do you mind if we discuss it here? The only option I see besides compiling the Device class in every plugin (which, I agree, is a bad idea) or making this "virtual" trick (that, however, adds some overhead in every call) would be to create a "libkdeconnectplugin" that contains the classes that the plugins need so they can link to it. What do you think is the best way to accomplish it? No solutions seems to be perfect :/
> 
> Alexandr Akulich wrote:
>     What classes will be in libkdeconnectplugin? Without IDevice interface, we have to link against PluginLoader, which is bad practice, no matter, static or dynamic that link will be.
>     I'm proposing to move encryption and serialization methods out of NetworkPackage, as they're used only in kded/backends. That will let us to not link plugins against QCA2 and QJSON as well. Currently I'm busy on some Telegram stuff and can't make such patch, but that is not a big problem, as I see.
>     
>     So, instead of link everything, I'm proposing to split everything. Instead of having (actually useless) library for plugins, we can have library for backends.
> 
> Aleix Pol Gonzalez wrote:
>     Well, I think there's a use-case for a library. It's not about device.* anyway, this is from the notifications plugin:
>         ../kdeconnectplugin.cpp
>         ../pluginloader.cpp
>         ../../networkpackage.cpp
>         ../../filetransferjob.cpp
>         ../../device.cpp
>         ../../kdebugnamespace.cpp
>     
>     All these are being compiled again into the plugin just for referencing.
>     
>     Albert, let's meet this saturday and figure out the problem.
> 
> Alexandr Akulich wrote:
>     So, there will be a lib with kdeconnectplugin (20 lines) and kdebugnamespace (5 lines), right? May be, there will be FileTransferJob (170 lines), which is used only and exacly in one plugin.
>     
>     Is it really needed? Is it objections against my patches? And do you joke about virtual calls overhead? Can you really measure it? We have to rewrite encryption. It is real brake. You can seen it on contacts transferring. Encryption takes billion times more, that that virtual calls.

That's why I say this must be discussed. having virtuals without 2 children and reason to ever have more is not good either.


- Aleix


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118626/#review59617
-----------------------------------------------------------


On June 9, 2014, 1:59 p.m., Alexandr Akulich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118626/
> -----------------------------------------------------------
> 
> (Updated June 9, 2014, 1:59 p.m.)
> 
> 
> Review request for kdeconnect and Albert Vaca Cintora.
> 
> 
> Repository: kdeconnect-kde
> 
> 
> Description
> -------
> 
> Easier to view at http://quickgit.kde.org/?p=scratch%2Fakulichalexandr%2Fkdeconnect-kde.git (master branch)
> 
> c67ba08 libkdeconnect: Fixed some style issues.
> 7da4d5b Added some more const declaration.
> c8e5f73 NotificationsModel: isAnyDimissable() marked as invokable method.
> 0657dbb libkdeconnect: KDECONNECT_EXPORT turned to be import declaration outside of lib.
> 8f27d03 kcm: Added DevicesModel declaration to move it's definition to implementation.
> 62c760a Get rid "libkdeconnect" path from includes in lib headers.
> e818d29 Added headers installation.
> 185e3ba DevicesModel: Updated StatusReachable value.
> ab8606e libkdeconnect: CMake files reworked to follow packaging guilde recommendations.
> 3eb44b0 libkdeconnect: Implemented CMake package configuration files.
> a4b9ca9 KDED/PluginLoader: Implemented standalone plugins support.
> 2f197f3 KDED/PluginLoader: Fixed multiply package-type support.
> 8e1172a KDED/Daemon: Implemented standalone plugins loading on construction.
> 725f071 KDED/Default_Args: Added template for QStringList.
> e25ea19 KDED: Introduced IDevice interface to be used in plugins.
> 2a67291 KDED: Plugins ported to IDevice API. Removed Device/PluginLoader dependency.
> 
> 
> Diffs
> -----
> 
>   kcm/CMakeLists.txt 505359b 
>   cli/CMakeLists.txt 6bc525e 
>   kcm/kcm.h 072c544 
>   kcm/kcm.cpp daaac3a 
>   kded/CMakeLists.txt e2dcb21 
>   kded/daemon.h 686c393 
>   kded/daemon.cpp fb1c166 
>   kded/default_args.h ff1b7f7 
>   kded/device.h 3f97b19 
>   kded/device.cpp ee13dcc 
>   kded/idevice.h PRE-CREATION 
>   kded/plugins/battery/CMakeLists.txt d623141 
>   kded/plugins/clipboard/CMakeLists.txt 426cfef 
>   kded/plugins/clipboard/clipboardplugin.h 9a7cab4 
>   kded/plugins/clipboard/clipboardplugin.cpp 9acfa76 
>   kded/plugins/kdeconnectplugin.h e8ff017 
>   kded/plugins/kdeconnectplugin.cpp 4971c49 
>   kded/plugins/mpriscontrol/CMakeLists.txt dd2a2a3 
>   kded/plugins/notifications/CMakeLists.txt a6ed32a 
>   kded/plugins/notifications/notificationsdbusinterface.h 9865ae7 
>   kded/plugins/notifications/notificationsdbusinterface.cpp c56a3d8 
>   kded/plugins/pluginloader.h 1a03259 
>   kded/plugins/pluginloader.cpp 737edae 
>   kded/plugins/sftp/CMakeLists.txt fa8f082 
>   kded/plugins/sftp/mounter.h 91b6bad 
>   kded/plugins/share/CMakeLists.txt ae4e18f 
>   kio/CMakeLists.txt 0d58ced 
>   libkdeconnect/CMakeLists.txt b65da06 
>   libkdeconnect/KDEConnect/DevicesModel PRE-CREATION 
>   libkdeconnect/KDEConnect/NotificationsModel PRE-CREATION 
>   libkdeconnect/KDEConnectConfig.cmake.in PRE-CREATION 
>   libkdeconnect/dbusinterfaces.h c5b4905 
>   libkdeconnect/devicesmodel.h a84c84d 
>   libkdeconnect/devicesmodel.cpp 7b7a749 
>   libkdeconnect/kdeconnect_export.h 56c2459 
>   libkdeconnect/notificationsmodel.h a79c063 
>   libkdeconnect/notificationsmodel.cpp 46dc8ee 
>   plasmoid/declarativeplugin/CMakeLists.txt 4919d47 
> 
> Diff: https://git.reviewboard.kde.org/r/118626/diff/
> 
> 
> Testing
> -------
> 
> Done.
> 
> 
> Thanks,
> 
> Alexandr Akulich
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20140611/83213da9/attachment.html>

More information about the KDEConnect mailing list