<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/118626/">https://git.reviewboard.kde.org/r/118626/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 9th, 2014, 3:21 p.m. UTC, <b>Aleix Pol Gonzalez</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On June 9th, 2014, 6:39 p.m. UTC, <b>Alexandr Akulich</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On June 10th, 2014, 3:52 p.m. UTC, <b>Aleix Pol Gonzalez</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Oh I see! They were building this inside the plugin, that looks pretty bad!
Ok, +1 here.</pre>
</blockquote>
<p>On June 10th, 2014, 4:09 p.m. UTC, <b>Alexandr Akulich</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Thank you! :)</pre>
</blockquote>
<p>On June 10th, 2014, 4:12 p.m. UTC, <b>Albert Vaca Cintora</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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 :/</pre>
</blockquote>
<p>On June 11th, 2014, 9:27 a.m. UTC, <b>Alexandr Akulich</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- Aleix</p>
<br />
<p>On June 9th, 2014, 1:59 p.m. UTC, Alexandr Akulich wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdeconnect and Albert Vaca Cintora.</div>
<div>By Alexandr Akulich.</div>
<p style="color: grey;"><i>Updated June 9, 2014, 1:59 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdeconnect-kde
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Done.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kcm/CMakeLists.txt <span style="color: grey">(505359b)</span></li>
<li>cli/CMakeLists.txt <span style="color: grey">(6bc525e)</span></li>
<li>kcm/kcm.h <span style="color: grey">(072c544)</span></li>
<li>kcm/kcm.cpp <span style="color: grey">(daaac3a)</span></li>
<li>kded/CMakeLists.txt <span style="color: grey">(e2dcb21)</span></li>
<li>kded/daemon.h <span style="color: grey">(686c393)</span></li>
<li>kded/daemon.cpp <span style="color: grey">(fb1c166)</span></li>
<li>kded/default_args.h <span style="color: grey">(ff1b7f7)</span></li>
<li>kded/device.h <span style="color: grey">(3f97b19)</span></li>
<li>kded/device.cpp <span style="color: grey">(ee13dcc)</span></li>
<li>kded/idevice.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>kded/plugins/battery/CMakeLists.txt <span style="color: grey">(d623141)</span></li>
<li>kded/plugins/clipboard/CMakeLists.txt <span style="color: grey">(426cfef)</span></li>
<li>kded/plugins/clipboard/clipboardplugin.h <span style="color: grey">(9a7cab4)</span></li>
<li>kded/plugins/clipboard/clipboardplugin.cpp <span style="color: grey">(9acfa76)</span></li>
<li>kded/plugins/kdeconnectplugin.h <span style="color: grey">(e8ff017)</span></li>
<li>kded/plugins/kdeconnectplugin.cpp <span style="color: grey">(4971c49)</span></li>
<li>kded/plugins/mpriscontrol/CMakeLists.txt <span style="color: grey">(dd2a2a3)</span></li>
<li>kded/plugins/notifications/CMakeLists.txt <span style="color: grey">(a6ed32a)</span></li>
<li>kded/plugins/notifications/notificationsdbusinterface.h <span style="color: grey">(9865ae7)</span></li>
<li>kded/plugins/notifications/notificationsdbusinterface.cpp <span style="color: grey">(c56a3d8)</span></li>
<li>kded/plugins/pluginloader.h <span style="color: grey">(1a03259)</span></li>
<li>kded/plugins/pluginloader.cpp <span style="color: grey">(737edae)</span></li>
<li>kded/plugins/sftp/CMakeLists.txt <span style="color: grey">(fa8f082)</span></li>
<li>kded/plugins/sftp/mounter.h <span style="color: grey">(91b6bad)</span></li>
<li>kded/plugins/share/CMakeLists.txt <span style="color: grey">(ae4e18f)</span></li>
<li>kio/CMakeLists.txt <span style="color: grey">(0d58ced)</span></li>
<li>libkdeconnect/CMakeLists.txt <span style="color: grey">(b65da06)</span></li>
<li>libkdeconnect/KDEConnect/DevicesModel <span style="color: grey">(PRE-CREATION)</span></li>
<li>libkdeconnect/KDEConnect/NotificationsModel <span style="color: grey">(PRE-CREATION)</span></li>
<li>libkdeconnect/KDEConnectConfig.cmake.in <span style="color: grey">(PRE-CREATION)</span></li>
<li>libkdeconnect/dbusinterfaces.h <span style="color: grey">(c5b4905)</span></li>
<li>libkdeconnect/devicesmodel.h <span style="color: grey">(a84c84d)</span></li>
<li>libkdeconnect/devicesmodel.cpp <span style="color: grey">(7b7a749)</span></li>
<li>libkdeconnect/kdeconnect_export.h <span style="color: grey">(56c2459)</span></li>
<li>libkdeconnect/notificationsmodel.h <span style="color: grey">(a79c063)</span></li>
<li>libkdeconnect/notificationsmodel.cpp <span style="color: grey">(46dc8ee)</span></li>
<li>plasmoid/declarativeplugin/CMakeLists.txt <span style="color: grey">(4919d47)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/118626/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>