Review Request 119822: QScreen backend for libkscreen

Mon Aug 18 12:16:12 UTC 2014

> On Aug. 18, 2014, 11:55 a.m., Àlex Fiestas wrote:
> > Besides the nitpits, here is a concern I have.
> > 
> > KScreen is a library that should be used only by components of the shell (kwin, plasma, kscreen), the idea is to have a library that manipulates lowlevel stuff directly so we can control how we do things (events and what not). Because of this having a qscreen backend enabled by default (even if it is only in platforms where we don't have a backend) imho is a no-go. We can't relay on abstractions.
> > 
> > That said, I do think that having a qscreen backend is an excellent fallback to test the shell in non supported platforms while we develop a proper backend, but we have to find a way of doing so qscreen backend is not used by accident.
> > 
> > To archieve this I see 2 options:
> > 1-Load qscreen backend only via KSCREEN_BACKEND env
> > 2-Improve backendloader and backends to make sure we use the proper backend in every case.
> > 
> > I think I will go for 1 at the moment, and we can figure out how to do the backend loading later on.
> > 
> > What do you think?
> 
> Martin Gräßlin wrote:
>     1 sounds like a sane approach to me especially if it's supposed to be for early testing.

Thanks for the reviews so far, much appreciated. I'll wait for more comments to trickle in and will then address them all in one go - so no updated diff just yet. I think a few things also warrant further discussion, such as:

The alternative to automatically picking the screen backend is that it crashes. To me, that's about the worst possible thing our code could do. It manifests itself by running anything using libkscreen on Wayland not working until you find out about the KSCREEN_BACKEND env var (which is documented exactly nowhere). That means the screen KCM and also plasmashell will just crash. A warning about this sounds much better to me, since it will actually point out what's wrong, instead of hiding it in backtraces one needs to understand first.

I'd much prefer to not having it crash. We can improve the backend picking logic more once we have a proper Wayland backend, in the meantime, I'd want to it not explode by default. It will just make it harder to test anything Wayland.

That's my opinion, and I'll go with whatever the gods that maintain decide, but it's a pretty strong disagreement. :)

> On Aug. 18, 2014, 11:55 a.m., Àlex Fiestas wrote:
> > tests/testpnp.cpp, lines 26-38
> > <https://git.reviewboard.kde.org/r/119822/diff/1/?file=306047#file306047line26>
> >
> >     Why is this tool needed at all? Also the copy pasted code is a no-go we should find a better solution, for example implementing streams on each object so we can simply do qDebug() << config

It's taken from printconfig, with the difference that this also monitors changes (printconfig, which is also in tests, outputs the current configuration and exits). I'd modify it to not exit, and have both cases covered. Would that be acceptable?

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

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.

- 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/20140818/7cafdf6c/attachment-0001.html>