Review Request 126101: allow loading backends in-process

[Daniel Vrátil]

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



src/backendlauncher/backendloader.cpp (line 122)
<https://git.reviewboard.kde.org/r/126101/#comment60688>

    Use return directly?



autotests/testconfigmonitor.cpp (line 120)
<https://git.reviewboard.kde.org/r/126101/#comment60689>

    I think you want to setop->exec() first, then check for error ;) Or you can just QVERIFY(setop->exec()), because exec() returns value of hasError().



autotests/testconfigmonitor.cpp (line 130)
<https://git.reviewboard.kde.org/r/126101/#comment60690>

    Same here - QVERIFY(setop2->exec()) will do.



autotests/testinprocess.cpp (line 91)
<https://git.reviewboard.kde.org/r/126101/#comment60691>

    QVERIFY



src/configoperation.cpp (line 25)
<https://git.reviewboard.kde.org/r/126101/#comment60686>

    Not needed anymore?



src/configoperation.cpp (line 141)
<https://git.reviewboard.kde.org/r/126101/#comment60697>

    I am not a big fan of being able to reload a different backend just by changing the env variable. This can lead to all kinds of funny situations that we don't want.
    
    IMO we should require that shutdownBackend() is called explicitly before loading a different backend, enforcing that QPluginLoader::unload() is called first, m_backend is cleared etc.
    
    Same goes with changing the KSCREEN_BACKEND_ARGS - while calling init() to re-init works for the simple Fake backend, it might not be enough for more complex backends.
    
    Basically the intended behaviour is that you call get(InProcess)Backend() (or something like that), and you fall back to parsing the env variables and calling loadBackendInProcess() if getBackend() returned null pointer. Or you can just move the env parsing to loadBackendInProcess() when loading a new plugin, always returning the cached backend unless it's null, then loading based on current env variables.



src/configoperation_p.h (line 43)
<https://git.reviewboard.kde.org/r/126101/#comment60687>

    Maybe we don't even need to store it? Looking at the implementation in both Operations, it might just fine if loadBackend() returned the backend pointer...


- Daniel Vrátil


On Nov. 18, 2015, 3:36 a.m., Sebastian Kügler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126101/
> -----------------------------------------------------------
> 
> (Updated Nov. 18, 2015, 3:36 a.m.)
> 
> 
> Review request for Plasma, Solid, Àlex Fiestas, Aleix Pol Gonzalez, Daniel Vrátil, and Martin Gräßlin.
> 
> 
> Repository: libkscreen
> 
> 
> Description
> -------
> 
> This patchset adds in-process operation to libkscreen
> 
> purpose:
> - allow easier debugging
> - for some backends (qscreen, upcoming kwayland) the out of process operation is not necessary since these backends are well-shielded
> 
> This implementation is backwards compatible and should mean only minimal changes to running setups.
> 
> 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.
> 
> 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.
> 
> Autotests should cover all the cases (and actually a few currently unsupported ones, such as using different backends in the same process).
> 
> Details on performance, etc.: http://vizzzion.org/blog/2015/11/wayland-and-libkscreen-benchmarks/
> 
> 
> Diffs
> -----
> 
>   src/configoperation.h 2405d79 
>   autotests/testconfigmonitor.cpp a051226 
>   backends/fake/fake.cpp 60264dd 
>   src/configmonitor.cpp a14bc70 
>   src/backendlauncher/backendloader.cpp 52051df 
>   src/backendmanager.cpp ca9c746 
>   src/backendmanager_p.h c6418e2 
>   src/config.cpp 75d947d 
>   src/configmonitor.h b6f1189 
>   src/configoperation.cpp 87fe141 
>   CMakeLists.txt 86a0965 
>   autotests/CMakeLists.txt 69af7f0 
>   autotests/testinprocess.cpp PRE-CREATION 
>   autotests/testqscreenbackend.cpp da4dbae 
>   autotests/testscreenconfig.cpp ecbcedf 
>   autotests/testxrandr.cpp b9b838a 
>   src/configoperation_p.h afdc462 
>   src/getconfigoperation.h c85bfaa 
>   src/getconfigoperation.cpp 22f92b4 
>   src/output.cpp bd381fa 
>   src/setconfigoperation.h 020a990 
>   src/setconfigoperation.cpp 6ea944f 
> 
> Diff: https://git.reviewboard.kde.org/r/126101/diff/
> 
> 
> Testing
> -------
> 
> Added a ton of autotests, made sure all existing ones pass.
> 
> tried "KSCREEN_BACKEND_INPROCESS=1 kcmshell5 kscreen", all working as expected.
> 
> 
> Thanks,
> 
> Sebastian Kügler
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20151118/7df18235/attachment-0001.html>