Review Request 119822: QScreen backend for libkscreen

Wed Aug 20 16:32:54 UTC 2014

> On Aug. 18, 2014, 11:55 a.m., Àlex Fiestas wrote:
> > autotests/testqscreenbackend.cpp, line 90
> > <https://git.reviewboard.kde.org/r/119822/diff/1/?file=306029#file306029line90>
> >
> >     Q_FOREACH
> 
> Martin Gräßlin wrote:
>     or instead of using Qt macros one could use what the language provides: for (const KScreen::Output *op : m_config->outputs())

Turns out that this (range-based for) is only available in GCC 4.6, so not usable for Frameworks (yet). It won't compile on my machine, anyway. (Undeclared output on the next line inside the block.)

> On Aug. 18, 2014, 11:55 a.m., Àlex Fiestas wrote:
> > autotests/testqscreenbackend.cpp, line 98
> > <https://git.reviewboard.kde.org/r/119822/diff/1/?file=306029#file306029line98>
> >
> >     Shouldn't it be qscreen? also, why the conditional at all?

Good catch. The conditional is there since when using the xrandr backend (which I've used to test the test with :D) lists also disconnected outputs, while QScreen only knows live outputs, so the test would miserably fail every time, then. I'll add a note there.

> On Aug. 18, 2014, 11:55 a.m., Àlex Fiestas wrote:
> > autotests/testqscreenbackend.cpp, line 66
> > <https://git.reviewboard.kde.org/r/119822/diff/1/?file=306029#file306029line66>
> >
> >     If we don't have a config even though we correctly configured the backend, should not we fail? a QSKIP might be unnoticed (specially in jenkins).
> 
> Sebastian Kügler wrote:
>     Until we can be sure to have a config running (i.e. either an X or a Wayland server), this would mean we'd have failing tests. I'm happy to remove the SKIP once we have the CI / tooling caught up to bring up a server for us, until then, no use in having it not SKIP.
> 
> Ben Cooksley wrote:
>     The CI system already starts an Xvfb server - although this doesn't support xrandr...
>     Plain X itself can't be run headless from what I understand.

How can I start an Xvfb from the test there? 
I'm trying right now to use Xvfb for the test, in the future it would also be nice to be able to start a Wayland compositor for the test. 

As the sam (SKIP) is done in the xrandrbackend test as well, I'll drop this issue for now, as to not block the code from going in.

- Sebastian

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

On Aug. 18, 2014, 9:31 a.m., Sebastian Kügler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119822/
> -----------------------------------------------------------
> 
> (Updated Aug. 18, 2014, 9:31 a.m.)
> 
> 
> Review request for Plasma and Solid.
> 
> 
> Repository: libkscreen
> 
> 
> Description
> -------
> 
> This patch adds a QScreen backend to libkscreen.
> 
> This is useful to avoid a dependency on XRandR (at build time) and a running X server at runtime.
> 
> The backend itself is read-only and kept simple. It only reports the basic necessities (which is what QScreen provides).
> 
> The changes are kept to the backends/qscreen directory, so no API has been touched. The changes outside of that directory are autotests, tests, and a fallback to the qscreen backend non non-X11 platforms. The latter will automatically make libkscreen work on Wayland (as far as QScreen allows us to, so r-o). This case otherwise just crashes, and the XRandR backend can't work. If the user specifies the backend using the KSCREEN_BACKEND env var, it will be respected, the automatism only triggers when no backend is explicitely specified. I've also added apidocs in some files, but again, no functional changes.
> 
> The plan is to augment this also with a native Wayland backend, which will take a bit longer to complete (more complex, it's r-w, I have to learn Wayland APIs). That backend is work-in-progress in the sebas/wayland branch. The QScreen backend allows us to test and run our code under Wayland, without crashing, so we can continue the port while a native Wayland backend is conceived.
> 
> You can find the code for this QScreen backend in sebas/qscreen if you'd like to give it a try.
> 
> 
> Diffs
> -----
> 
>   autotests/CMakeLists.txt 18b93c0 
>   autotests/testqscreenbackend.cpp PRE-CREATION 
>   backends/CMakeLists.txt a827ee8 
>   backends/abstractbackend.h 7ffe627 
>   backends/qscreen/CMakeLists.txt PRE-CREATION 
>   backends/qscreen/qscreenbackend.h PRE-CREATION 
>   backends/qscreen/qscreenbackend.cpp PRE-CREATION 
>   backends/qscreen/qscreenconfig.h PRE-CREATION 
>   backends/qscreen/qscreenconfig.cpp PRE-CREATION 
>   backends/qscreen/qscreenoutput.h PRE-CREATION 
>   backends/qscreen/qscreenoutput.cpp PRE-CREATION 
>   backends/qscreen/qscreenscreen.h PRE-CREATION 
>   backends/qscreen/qscreenscreen.cpp PRE-CREATION 
>   src/CMakeLists.txt 4606862 
>   src/backendloader.cpp d6ccdff 
>   src/config.h 10a8f1e 
>   tests/CMakeLists.txt 86efedc 
>   tests/testplugandplay.cpp PRE-CREATION 
>   tests/testpnp.h PRE-CREATION 
>   tests/testpnp.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/119822/diff/
> 
> 
> Testing
> -------
> 
> * Ran autotest "testqscreenbackend" under X11 and Wayland -- all pass
> * Tested hotplugging (using included testpnp app) under X11
> * Started plasmashell with KSCREEN_BACKEND=QScreen under X11
> * Started kcmshell5 kcm_kscreen
> 
> All of these work correctly in my tests, no strange behaviour observed.
> 
> 
> Thanks,
> 
> Sebastian Kügler
> 
>

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