----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126101/#review88502 -----------------------------------------------------------
Thanks a lot for the patch! I like the general approach and I just have two objections regarding the implementation: 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) 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. autotests/testconfigmonitor.cpp (line 59) <https://git.reviewboard.kde.org/r/126101/#comment60633> I know the code wasn't previously consistent about this, but please use qputenv everywhere, unless there's a real reason not to. autotests/testconfigmonitor.cpp (line 100) <https://git.reviewboard.kde.org/r/126101/#comment60634> qputenv src/backendmanager.cpp (line 92) <https://git.reviewboard.kde.org/r/126101/#comment60635> Redundant? src/backendmanager.cpp (line 114) <https://git.reviewboard.kde.org/r/126101/#comment60655> Remove src/backendmanager.cpp (line 192) <https://git.reviewboard.kde.org/r/126101/#comment60662> This should move above the if(), see my comment in BackendManager::requestBackend() src/backendmanager.cpp (line 193) <https://git.reviewboard.kde.org/r/126101/#comment60636> No need to use .keys() here. Use m_inProcessBackends.constFind() to save one lookup on the next line. src/backendmanager.cpp (line 206) <https://git.reviewboard.kde.org/r/126101/#comment60637> Use qCDebug src/backendmanager.cpp (line 214) <https://git.reviewboard.kde.org/r/126101/#comment60661> Add a Q_ASSERT please. BackendManager is a private class, calling requestBackend() while in InProcess mode is a bug in our code. src/backendmanager.cpp (line 257) <https://git.reviewboard.kde.org/r/126101/#comment60638> qCDebug src/backendmanager.cpp (line 353) <https://git.reviewboard.kde.org/r/126101/#comment60639> qCDebug src/backendmanager.cpp (line 362) <https://git.reviewboard.kde.org/r/126101/#comment60640> Don't use range-based loops on Qt containers, they might cause detach. Use normal iterator instead - it also eliminates the need for temporary allocation of keys() and double lookup in the loop below. src/backendmanager_p.h (line 126) <https://git.reviewboard.kde.org/r/126101/#comment60660> Why is this even a QHash? We don't want to run multiple backends at once, or do we? Wouldn't just holding pointer to the current backend be enough? Maybe even without the arguments... src/config.cpp (line 198) <https://git.reviewboard.kde.org/r/126101/#comment60641> Remove src/configmonitor.cpp (line 67) <https://git.reviewboard.kde.org/r/126101/#comment60642> This should be on one line src/configmonitor.cpp (line 138) <https://git.reviewboard.kde.org/r/126101/#comment60643> One line src/configmonitor.cpp (line 210) <https://git.reviewboard.kde.org/r/126101/#comment60644> One line src/configmonitor.cpp (line 216) <https://git.reviewboard.kde.org/r/126101/#comment60645> qCDebug or remove src/configmonitor.cpp (line 249) <https://git.reviewboard.kde.org/r/126101/#comment60648> Remove src/configmonitor.cpp (line 253) <https://git.reviewboard.kde.org/r/126101/#comment60646> Remove src/configmonitor.cpp (line 255) <https://git.reviewboard.kde.org/r/126101/#comment60647> Remove src/configoperation.h (line 49) <https://git.reviewboard.kde.org/r/126101/#comment60659> The `create()` and `setOperation()` pair looks weird and inconsistent. Please rename it to `getConfig()` and `setConfig()` (or something among those lines, if you have better idea). src/getconfigoperation.h (line 41) <https://git.reviewboard.kde.org/r/126101/#comment60650> Should be virtual src/getconfigoperation.h (line 43) <https://git.reviewboard.kde.org/r/126101/#comment60649> Use Q_DECL_OVERRIDE src/inprocessconfigoperation.h (line 44) <https://git.reviewboard.kde.org/r/126101/#comment60651> Use Q_DECL_OVERRIDE src/inprocessconfigoperation.cpp (line 108) <https://git.reviewboard.kde.org/r/126101/#comment60656> Move this to InProcessConfigOperation::start() please, this has nothing to do with loading backend anymore. src/output.cpp (line 414) <https://git.reviewboard.kde.org/r/126101/#comment60652> Remove src/setconfigoperation.cpp (line 127) <https://git.reviewboard.kde.org/r/126101/#comment60653> qCWarning src/setinprocessoperation.cpp (line 81) <https://git.reviewboard.kde.org/r/126101/#comment60657> This code looks exactly like InProcessConfigOperation::loadBackend() - move this method one level up to ConfigOperationPrivate. src/setinprocessoperation.cpp (line 98) <https://git.reviewboard.kde.org/r/126101/#comment60658> Move this to SetInProcessOperation::start(), this has nothing to do with backend loading. src/setinprocessoperation.cpp (line 101) <https://git.reviewboard.kde.org/r/126101/#comment60654> Remove - Daniel Vrátil On Nov. 17, 2015, 9:35 p.m., Sebastian Kügler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126101/ > ----------------------------------------------------------- > > (Updated Nov. 17, 2015, 9:35 p.m.) > > > Review request for Plasma, Solid, 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 > ----- > > CMakeLists.txt 86a0965 > autotests/CMakeLists.txt 69af7f0 > autotests/testconfigmonitor.cpp a051226 > autotests/testinprocess.cpp PRE-CREATION > autotests/testqscreenbackend.cpp da4dbae > autotests/testscreenconfig.cpp ecbcedf > backends/fake/fake.cpp 60264dd > src/CMakeLists.txt 4b56b61 > src/backendlauncher/backendloader.cpp 52051df > src/backendmanager.cpp ca9c746 > src/backendmanager_p.h c6418e2 > src/config.cpp 75d947d > src/configmonitor.h b6f1189 > src/configmonitor.cpp a14bc70 > src/configoperation.h 2405d79 > src/configoperation.cpp 87fe141 > src/getconfigoperation.h c85bfaa > src/inprocessconfigoperation.h PRE-CREATION > src/inprocessconfigoperation.cpp PRE-CREATION > src/output.cpp bd381fa > src/setconfigoperation.cpp 6ea944f > src/setinprocessoperation.h PRE-CREATION > src/setinprocessoperation.cpp PRE-CREATION > > 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 > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
