Re: Review Request 127387: Refactor the backend loading code

[Sebastian Kügler]

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

(Updated Feb. 4, 2017, 11:18 a.m.)


Status
--

This change has been marked as submitted.


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

Re: Review Request 127387: Refactor the backend loading code

[Sebastian Kügler]

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

(Updated March 16, 2016, 1:54 p.m.)


Review request for Plasma, Solid, Daniel Vrátil, and Martin Gräßlin.


Changes
---

Also address Kai's change requests. :)


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 (updated)
-

  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

Re: Review Request 127387: Refactor the backend loading code

[Sebastian Kügler]

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

(Updated March 16, 2016, 1:50 p.m.)


Review request for Plasma, Solid, Daniel Vrátil, and Martin Gräßlin.


Changes
---

Addressed feedback, thanks David and Martin!


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 (updated)
-

  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

Re: Review Request 127387: Refactor the backend loading code

[Sebastian Kügler]

> On March 15, 2016, 11:42 p.m., David Edmundson wrote:
> > src/backendmanager.cpp, line 216
> > 
> >
> > is the name argument still used?
> > 
> > It looks like that used to be the name of the backend to load, which 
> > you now determine yourself inside the function, rendering it useless.

Yes, I want to kill "name", but it's used from a couple of places in the API 
(but doesn't get used in the end, it's not needed since we share all the logic 
now with in- and out-of-process loading. I'll remove the remainders of name in 
a separate patch.


- Sebastian


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


On March 15, 2016, 5: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, 5: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

Re: 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)


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)


const



src/backendmanager.cpp (line 75)


personally I find the parentethes around `_inprocess.isEmpty()` confusing. 
I would just do:
if (!_inprocess.isEmpty())



src/backendmanager.cpp (line 77)


QByteArrayList falses({QByteArrayLiteral("0"), QByteArrayLiteral("false")});

this brings the following improvements:
* two less appends
* the general improvments of fooLiteral



src/backendmanager.cpp (line 87)


qCDebug?



src/backendmanager.cpp (line 90)


qCDebug



src/backendmanager.cpp (line 149)


const



src/backendmanager.cpp (line 164)


startsWith always goes with QLatin1String



src/backendmanager.cpp (line 167)


same here: QLatin1String



src/backendmanager.cpp (line 179)


this looks weird - maybe QStringLiteral?



src/backendmanager.cpp (line 195)


const



src/backendmanager.cpp (line 220)


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

Re: Review Request 127387: Refactor the backend loading code

[Sebastian Kügler]

> On March 16, 2016, 11:45 a.m., Martin Gräßlin wrote:
> > src/backendmanager.cpp, line 165
> > 
> >
> > startsWith always goes with QLatin1String

QString() is needed for .arg() (which QLatin1String doesn't have).


- Sebastian


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


On March 15, 2016, 5: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, 5: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

Re: 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/#review93598
---




autotests/testbackendloader.cpp (line 46)


variable is no longer used. It's written to but never read.



src/backendmanager.cpp (line 77)


Now we can even make the list a const \o/



src/backendmanager.cpp (line 161)


QStringLiteral


- Martin Gräßlin


On March 16, 2016, 2:54 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 16, 2016, 2:54 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

Re: Review Request 127387: Refactor the backend loading code

[Kai Uwe Broulik]

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




src/backendmanager.cpp (line 86)


startsWith(QLatin1String(...))



src/backendmanager.cpp (line 182)


given paths is const, use range-for


- Kai Uwe Broulik


On März 15, 2016, 5:57 nachm., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127387/
> ---
> 
> (Updated März 15, 2016, 5:57 nachm.)
> 
> 
> 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

Re: Review Request 127387: Refactor the backend loading code

[Daniel Vrátil]

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



+1, looks good to me.

- Daniel Vrátil


On March 16, 2016, 3:04 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 16, 2016, 3:04 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

Re: 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/#review93639
---


Ship it!




Ship It!

- Martin Gräßlin


On March 16, 2016, 3:04 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 16, 2016, 3:04 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

Re: Review Request 127387: Refactor the backend loading code

[Sebastian Kügler]

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

(Updated March 16, 2016, 2:04 p.m.)


Review request for Plasma, Solid, Daniel Vrátil, and Martin Gräßlin.


Changes
---

Addressed Martin's comments.


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 (updated)
-

  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

Re: Review Request 127387: Refactor the backend loading code

[David Edmundson]

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



looks generally good.


autotests/testbackendloader.cpp (line 60)


this won't do want you want, you're  shadowing into a new local variable 
called _b, rather than the member variable



src/backendmanager.cpp (line 192)


is the name argument still used?

It looks like that used to be the name of the backend to load, which you 
now determine yourself inside the function, rendering it useless.


- David Edmundson


On March 15, 2016, 5: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, 5: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