Review Request 127387: Refactor the backend loading code

[Martin Gräßlin]

-----------------------------------------------------------
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
> 
>

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