----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127387/#review93591 -----------------------------------------------------------
logic looks good and it's mostly nitpicking on my side. autotests/testbackendloader.cpp (line 66) <https://git.reviewboard.kde.org/r/127387/#comment63813> instead of storing the env variable you could also just use: qputenv("KSCREEN_BACKEND", QByteArray()); as you always set it to empty in the ctor. src/backendmanager.cpp (line 74) <https://git.reviewboard.kde.org/r/127387/#comment63817> const src/backendmanager.cpp (line 75) <https://git.reviewboard.kde.org/r/127387/#comment63818> personally I find the parentethes around `_inprocess.isEmpty()` confusing. I would just do: if (!_inprocess.isEmpty()) src/backendmanager.cpp (line 77) <https://git.reviewboard.kde.org/r/127387/#comment63814> QByteArrayList falses({QByteArrayLiteral("0"), QByteArrayLiteral("false")}); this brings the following improvements: * two less appends * the general improvments of fooLiteral src/backendmanager.cpp (line 87) <https://git.reviewboard.kde.org/r/127387/#comment63815> qCDebug? src/backendmanager.cpp (line 90) <https://git.reviewboard.kde.org/r/127387/#comment63816> qCDebug src/backendmanager.cpp (line 149) <https://git.reviewboard.kde.org/r/127387/#comment63819> const src/backendmanager.cpp (line 164) <https://git.reviewboard.kde.org/r/127387/#comment63823> startsWith always goes with QLatin1String src/backendmanager.cpp (line 167) <https://git.reviewboard.kde.org/r/127387/#comment63824> same here: QLatin1String src/backendmanager.cpp (line 179) <https://git.reviewboard.kde.org/r/127387/#comment63820> this looks weird - maybe QStringLiteral? src/backendmanager.cpp (line 195) <https://git.reviewboard.kde.org/r/127387/#comment63821> const src/backendmanager.cpp (line 220) <https://git.reviewboard.kde.org/r/127387/#comment63822> nitpick: added empty new line - Martin Gräßlin On March 15, 2016, 6:57 p.m., Sebastian Kügler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127387/ > ----------------------------------------------------------- > > (Updated March 15, 2016, 6:57 p.m.) > > > Review request for Plasma, Solid, Daniel Vrátil, and Martin Gräßlin. > > > Repository: libkscreen > > > Description > ------- > > Refactor the backend loading code > > Untangle the large plugin loading logic in the BackendManager static > into separate bits. This makes the code clearer and easier to auto-test. > > - listBackends() compiles a list of backends from the plugin paths > - preferredBackend() picks the backend, in this priority: > - if KSCREEN_BACKEND is set in the environment, this is the only > used method to find the backend plugin > - if platform is X11, the XRandR backend is picked > - if platform is wayland, KWayland backend is picked > - if neither is the case, QScreen backend is picked > > It does introduce a slight behavioral change: The mechanism was based on > falling through, so it would consider another backend if the logically > picked on fails to load. This is undesired behavior, however, since the > backendloader may be able to load the plugin, but that doesn't mean that > the plugin actually work. > > Parsing of the KSCREEN_BACKEND variable is kept case-insensitive. > > > autotests for new backend loading logic > > - makes sure we find plugins installed > - pick plugins from env var > > We can't sensible test all the runtime cases yet, but this at least > covers the code paths around those few lines that do runtime detection > of x11 and wayland. > > > Diffs > ----- > > autotests/CMakeLists.txt 26c7952 > autotests/testbackendloader.cpp PRE-CREATION > src/backendmanager.cpp 382ae71 > src/backendmanager_p.h 150883b > > Diff: https://git.reviewboard.kde.org/r/127387/diff/ > > > Testing > ------- > > manual runtime tests and autotests pass > > > Thanks, > > Sebastian Kügler > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
