<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/126101/">https://git.reviewboard.kde.org/r/126101/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 17th, 2015, 9:55 p.m. UTC, <b>Daniel Vrátil</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Thanks a lot for the patch! I like the general approach and I just have two objections regarding the implementation: </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">1) I don't like the split of the ConfigOperations for In- and Out-Of-Process cases. Please just implement both ways in the existing Operation classes. The way you implemented it now requires that all code using Operations effectively needs to be ported to use the factory methods you added, which is not a desirable change. You can hide this equally well into the Operation implementations. (you can then ignore some of the review comments in the Operations below)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">2) BackendManager::loadBackend() vs. BackendManager::requestBackend() vs. BackendLoader::loadBackend() is confusing and all mixed up. What about moving the actual plugin loading code back to BackendLoader::loadBackend() and adding BackendLoader::unload() method to unload it when necessary? This way we can avoid having another QPluginLoader in BackendManager. Then BackendManager::loadBackend() can become BackendManager::requestInProcessBackend() and be nicely in pair with BackendManager::requestBackend(). It would make the API cleaner IMO and less mixed up.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">First, thanks for the review. :)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As to 2), I've had a look into that, but I'm not convinced it's a good idea to move the plugin loading logic back into BackendLoader, mostly for practical reasons: BackendLoader is currently not used or built in the libkscreen binary, it's only in the backendlauncher binary. Moving this (static) method into BackendLoader means that we have to build the BackendLoader and the DBus adapter into the libkscreen library(while ironically the goal here is to avoid this logic with the in-process method).
On the other hand, we would save the QPluginLoader<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;"> in BackendManager, but in exchange we have to carry a BackendLoader instance, which we'd only need for that one method and the contained QPluginLoader</em>.
I think that's a bad trade-off and a net loss.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'll have a look how I can make the loadBackend logic a bit clearer in the code, otherwise.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As to the other comments, they make sense to me. I'm working on it.</p></pre>
<br />
<p>- Sebastian</p>
<br />
<p>On November 17th, 2015, 8:35 p.m. UTC, Sebastian Kügler wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Plasma, Solid, Aleix Pol Gonzalez, Daniel Vrátil, and Martin Gräßlin.</div>
<div>By Sebastian Kügler.</div>
<p style="color: grey;"><i>Updated Nov. 17, 2015, 8:35 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
libkscreen
</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patchset adds in-process operation to libkscreen</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">purpose:
- allow easier debugging
- for some backends (qscreen, upcoming kwayland) the out of process operation is not necessary since these backends are well-shielded</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This implementation is backwards compatible and should mean only minimal changes to running setups.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If the user exports KSCREEN_BACKEND_INPROCESS=1 before running, all operations will be done in process. Otherwise, the out-of-process mode is used.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The idea is that we use
The changes in the clients to use the in-process mode are to use ConfigOperation::create() and ConfigOperation::setOperation() to retrieve the get or set config jobs. The rest will be handled inside libkscreen.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Autotests should cover all the cases (and actually a few currently unsupported ones, such as using different backends in the same process).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Details on performance, etc.: http://vizzzion.org/blog/2015/11/wayland-and-libkscreen-benchmarks/</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Added a ton of autotests, made sure all existing ones pass.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">tried "KSCREEN_BACKEND_INPROCESS=1 kcmshell5 kscreen", all working as expected.</p></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>CMakeLists.txt <span style="color: grey">(86a0965)</span></li>
<li>autotests/CMakeLists.txt <span style="color: grey">(69af7f0)</span></li>
<li>autotests/testconfigmonitor.cpp <span style="color: grey">(a051226)</span></li>
<li>autotests/testinprocess.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/testqscreenbackend.cpp <span style="color: grey">(da4dbae)</span></li>
<li>autotests/testscreenconfig.cpp <span style="color: grey">(ecbcedf)</span></li>
<li>backends/fake/fake.cpp <span style="color: grey">(60264dd)</span></li>
<li>src/CMakeLists.txt <span style="color: grey">(4b56b61)</span></li>
<li>src/backendlauncher/backendloader.cpp <span style="color: grey">(52051df)</span></li>
<li>src/backendmanager.cpp <span style="color: grey">(ca9c746)</span></li>
<li>src/backendmanager_p.h <span style="color: grey">(c6418e2)</span></li>
<li>src/config.cpp <span style="color: grey">(75d947d)</span></li>
<li>src/configmonitor.h <span style="color: grey">(b6f1189)</span></li>
<li>src/configmonitor.cpp <span style="color: grey">(a14bc70)</span></li>
<li>src/configoperation.h <span style="color: grey">(2405d79)</span></li>
<li>src/configoperation.cpp <span style="color: grey">(87fe141)</span></li>
<li>src/getconfigoperation.h <span style="color: grey">(c85bfaa)</span></li>
<li>src/inprocessconfigoperation.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/inprocessconfigoperation.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/output.cpp <span style="color: grey">(bd381fa)</span></li>
<li>src/setconfigoperation.cpp <span style="color: grey">(6ea944f)</span></li>
<li>src/setinprocessoperation.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/setinprocessoperation.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/126101/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>