feat(wayland): support multiple protocol extensions through plugin system
AbandonedPublic

Authored by romangg on Apr 20 2020, 8:44 PM.

Details

Reviewers
None
Group Reviewers
Plasma
Summary

Besides the KWayland output-management protocol there exist other protocol
extensions for output-management, for example wlroot's
wlr-output-management-unstable-v1 and kwinft_output_management_unstable_v1
protocols.

This patch refactors the Wayland backend such that alternative extensions can
be added in the future in an encaplusated way and are instantiated through a
plugin system.

All available Wayland backend plugins are fired up in parallel and the first to
return success will be used while all others are rejected instantly. In theory
this is racy if a compositor supports more than one of the supported output-
management protocols, but there is none out at the moment and it is unlikely to
happen in the future.

Test Plan

Tested with KWin's Wayland session.

Diff Detail

Repository
R110 KScreen Library
Branch
wayland-plugins
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25554
Build 25572: arc lint + arc unit
romangg created this revision.Apr 20 2020, 8:44 PM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 20 2020, 8:44 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
romangg requested review of this revision.Apr 20 2020, 8:44 PM
romangg added inline comments.Apr 20 2020, 8:49 PM
backends/kwayland/plugins/kwayland/CMakeLists.txt
28

That directory fine or are there other suggestions? I would like to centralize over time all plugins libkscreen seems to install at random locations in a single one so that's why there is the wayland subdirectory.

backends/kwayland/waylandconfig.cpp
155

Someone having an idea about that? In general the plugin system is kinda funky. The thing returned is a singleton and gets returned whenever instantiate is called (that's why there is a factory to then generate objects on the heap).

KScreen already is a plugin system.

We don't need to go all inception, and have a plugin load plugins. It doesn't look to bring anything other than complexity.

If there is any duplication, we can just add some extra base classes in the lib using the split you've done above that the plugin can inherit from.

KScreen already is a plugin system.

You mean libkscreen? It has a plugin system for differentiating between windowing systems, yes.

We don't need to go all inception, and have a plugin load plugins. It doesn't look to bring anything other than complexity.

If there is any duplication, we can just add some extra base classes in the lib using the split you've done above that the plugin can inherit from.

That's not a practical solution. The windowing system plugins are chosen on the basis of Qt platform name without running any async requests. But we need to check in async communication with the Wayland compositor which protocol is supported, select that one and cleanup the other ones. For that at least one Wayland event loop has to be launched. Trying to implement that on the higher level of the previously available plugins not needing such a runtime setup would be ill-advised.

romangg updated this revision to Diff 82265.May 8 2020, 1:40 PM
  • refactor: export Wayland plugins headers

A separate library KScreenWayland is installed additionally to the runtime plugin, so that library can be linked against to create nested plugins for a Wayland session. These plugins must be installed in a certain directory (usually`/usr/lib/plugins/org.kde.libkscreen.backends/wayland/`) so they can be discovered at runtime after the Wayland plugin has been loaded. For each plugin a separate connection is established to the Wayland compositor as before and depending on reply the plugin is used or discarded.

KWayland plugin is by default compiled with libkscreen.

Since this has now the structure with exported headers as requested in D29028#653150 and D29028#654526 I'll push later today if there are no more specific issues pointed out in regards to the code.

It's still got this pointless plugin within plugin situation.

For that at least one Wayland event loop has to be launched

It needs a roundrip after the registry is fetched. That doesn't need an event loop.

romangg added a comment.EditedMay 11 2020, 2:05 PM

These are not specific issues but some general complains about the overall concept chosen here without providing an alternative solution.

I chose this plugin system because it allows robust extension to the current system. Plugins are well contained and reuse already existing infrastructure. Besides moving the code around there are practically no logic changes to the KWayland backend.

Note that I need to have this plugin system or something similar in for 5.19 or I will be forced to fork libkscreen permanently for KWinFT. I would like to avoid this and instead continue my work on libkscreen as a KDE project like I have worked on it in the last two years.

Note that I need to have this plugin system or something similar in for 5.19 or I will be forced to fork libkscreen permanently for KWinFT. I would like to avoid this and instead continue my work on libkscreen as a KDE project like I have worked on it in the last two years.

Nobody's forcing you to do anything.

Note that Plasma 5.19 branches in three days and merging huge things like this right before branching has bitten us before and I'd like to avoid it if possible for 5.19. So if we do go with this patch in something like its current form (I have no technical opinions on it whatsoever), I would like it merged in after branching for 5.19, not right before.

These are not specific issues but some general complains about the overall concept chosen here without providing an alternative solution.

Well yes, it makes sense to do high level discussion first.

With the Plasma agreed "new approach" that we will be porting to we will be using QtWaylandClientExtension and removing the need for the connection thread - having that in the interface will hold up progress we want to do upstream.

here without providing an alternative solution.

static bool isValid() in the plugin. That is made synchronous by use of roundtrip. The existing plugin gets a very tiny refactor.

(also you could just qputenv("KSCREEN_BACKEND") from your fork before launching the shell which is even less invasive)

Note that I need to have this plugin system or something similar in for 5.19 or I will be forced to fork libkscreen permanently for KWinFT. I would like to avoid this and instead continue my work on libkscreen as a KDE project like I have worked on it in the last two years.

Nobody's forcing you to do anything.

How would you call it? I obviously need output management to work and if my integration patch to libkscreen is blocked I need to keep maintaining my libkscreen fork. For that I inevitably have to build up much more project infrastructure than I currently have for what I intended to provide only for a short intermediate period.

Note that Plasma 5.19 branches in three days and merging huge things like this right before branching has bitten us before and I'd like to avoid it if possible for 5.19. So if we do go with this patch in something like its current form (I have no technical opinions on it whatsoever), I would like it merged in after branching for 5.19, not right before.

This patch without header export but the very same plugin system was published for review on here already three weeks ago. I asked for feedback and later help in regards to the exported headers but received very little. The so called "high level discussion first" was David adding some arguments that are not worth repeating if one knows the internal libksceen structure, me pointing that out and him then ignoring it. He calls this high level discussion, I call it stalling tactic.

I uploaded three days ago an updated version incorporating changes that were requested in the other review. Again no feedback until finally I ask today one last time for specific issues but instead David comes with the very same high level discussion he had more than enough time to reply to but missed out on doing in time. If he wants to tell other people how to do their code he needs to stay on top with what they are doing or delegate it.

I have done large changes shortly before beta release in the past and fixed remaining issues in the beta phase without problems. That's what the beta phase is for. Since I'm online on most days there is no delay in fixing bugs if there are any.

These are not specific issues but some general complains about the overall concept chosen here without providing an alternative solution.

Well yes, it makes sense to do high level discussion first.

You had more than enough time for that but ignored it in the last three weeks.

With the Plasma agreed "new approach" that we will be porting to we will be using QtWaylandClientExtension and removing the need for the connection thread - having that in the interface will hold up progress we want to do upstream.

Show me where the documentation for this QtWaylandClientExtension is and where you documented that we "agreed" on using that in Plasma exclusively and we can talk about libkscreen using it in 5.20. Personally, I think with the current Qt situation instead of making us more dependent on Qt Company offerings we should become more independent but that's a different topic.

In any case the plugin system here is independent of some "connection thread". You only hand over a normal QThread.

here without providing an alternative solution.

static bool isValid() in the plugin. That is made synchronous by use of roundtrip. The existing plugin gets a very tiny refactor.

I know when you say "tiny refactor", "little change", it's in the end often a large one with major repercussions on the overall structure and internal logic.

Why would you want a synchronous design? I explicitly opted for an asynchronous one such that we can directly select the active plugin instead of waiting for potentially multiple other ones to return without success.

(also you could just qputenv("KSCREEN_BACKEND") from your fork before launching the shell which is even less invasive)

What would this help? I would still need to fork basically the whole libkscreen Wayland backend to create another plugin. And enforcing this environment variable is a hack for a problem I solved cleaner and in more generality with the plugin system presented here. How would you add a plugin for wlroots based compositors? Should they then set a different environment variable?

It's telling that you call providing infrastructure for clean integration with independent projects "invasive". Plasma is an unwelcoming inward-looking project thanks to this mindset.

davidedmundson added a comment.EditedMay 12 2020, 9:18 PM

Show me where the documentation for this QtWaylandClientExtension is and where you documented that we "agreed" on using that in Plasma exclusively and we can talk about libkscreen using it in 5.20

It was documented here https://phabricator.kde.org/T11903 at the end and it was discussed in the meeting on # kwin that you were in.

Show me where the documentation for this QtWaylandClientExtension is and where you documented that we "agreed" on using that in Plasma exclusively and we can talk about libkscreen using it in 5.20

It was documented here https://phabricator.kde.org/T11903 at the end and it was discussed in the meeting on # kwin that you were in.

That's not documentation of a decision process. That's you telling everybody else what you want them to do. That you call what happened in # kwin a discussion is laughable. You knew already from the start that you wanted these QtWaylandClientExtensions, don't tell me otherwise. Of the five people attending I am sure only one other person had the knowledge to form an informed opinion about it. And after all why does this "discussion" of only very few people determine the future direction of other Plasma projects like libkscreen these people haven't worked on for years? Because you want it that way?

By the way I didn't participate in your "meeting" because I wanted to play fair after KWinFT went public and not accidentally derail it with discussions about that. You know this because I told you before. Now you're misrepresenting it as if I was an active attendee of this meeting. If you look at your own link I am not even on the list of attendees you published. That you even have the guts to twist the truth like this!

Oh I nearly forgot: great work that you ignore everything else I wrote and only reply to the most unimportant statement in all of that. That's a great rhetorical trick. Every good politician must know about this one for distraction.

If I may add my two cents here, I agree with David that introducing a plugin for a plugin is a bit over the top. Whats the issue with contributing your backend into libkscreen upstream instead? If there's any code that could be shared between KWayland backend and your new backend, you can create a small static library (basically reusing most of what you've already done). Otherwise you are basically forcing libkscreen to accommodate a single usecase for your fork, and the community has the right to push back on that as it makes things more complicated for them just to make live easier for you.

Regarding the asynchronous plugin loading. You still need to block the constructor of the KWayland backend in order to figure out which plugin you want to load, because the KScreen backend loading code synchronously calls isValid() just after constructing the backend. So it's nice you try loading your plugins asynchronously, but it doesn't solve much since all you do is blocking-waiting for a bunch o threads to finish, rather than just blocking-waiting for the calls to the registry to finish... The current backend loading code in libkscreen is of course simple and stupid because it was first designed only with XRandR and Fake backends in mind. Later on QScreen was added which also works synchronously. When Wayland support was introduced, Sebas rewrote lot of the code, but some of the assumptions were kept, like plugins initializing synchronously. It's not too hard to fix the backend loading code to not only make assumptions about backends based on Qt platform and to allow for true asynchronous initialization.

If I may add my two cents here,

Hi Daniel, sorry for the late reply. But I was busy as I had to kick things off now with the libkscreen fork.

I agree with David that introducing a plugin for a plugin is a bit over the top.

Is it though? Just because it's normally not done must not mean it should never be done. The Wayland platform is pretty special and pretty new so why not try something unusual? That said if a one-level plugin-infrastructure is in a similar way possible and somebody has implemented it: sure, let's go with that then.

Whats the issue with contributing your backend into libkscreen upstream instead?

I would have been fine with that. But I anticipated there will be opposition to that. And I believe this was a correct assessment if you read the comments in D29028:

> I don't see why we should have that in KDE code.

and

> I don't really see why we'd want to support something that is not offering ABI stability and doesn't push Plasma in any direction.

These comments show pretty clearly that there is political opposition to any integration effort with KWinFT. I don't want to comment on this any further but I think this is a really sad state of affairs and it shines a very unflattering light on the KDE Plasma project. Anyways, at least on a personal level I explain this with initial resentment and will stay open to reconciliation.

If there's any code that could be shared between KWayland backend and your new backend, you can create a small static library (basically reusing most of what you've already done). Otherwise you are basically forcing libkscreen to accommodate a single usecase for your fork, and the community has the right to push back on that as it makes things more complicated for them just to make live easier for you.

It was not my intention to make things complicated for other people. And since I was basically the only person with larger contributions to libkscreen and KScreen in the last 1.5 years, I was its last maintainer and I definitely planned to do more work on it in the future the one person who would have needed to endure additional complexity in particular would have been me. ;)

Regarding the asynchronous plugin loading. You still need to block the constructor of the KWayland backend in order to figure out which plugin you want to load, because the KScreen backend loading code synchronously calls isValid() just after constructing the backend.

But I think that's fine, otherwise the consumer gets incorrect or incomplete information about the current/initial setup. You think otherwise?

So it's nice you try loading your plugins asynchronously, but it doesn't solve much since all you do is blocking-waiting for a bunch o threads to finish, rather than just blocking-waiting for the calls to the registry to finish... The current backend loading code in libkscreen is of course simple and stupid because it was first designed only with XRandR and Fake backends in mind. Later on QScreen was added which also works synchronously. When Wayland support was introduced, Sebas rewrote lot of the code, but some of the assumptions were kept, like plugins initializing synchronously. It's not too hard to fix the backend loading code to not only make assumptions about backends based on Qt platform and to allow for true asynchronous initialization.

I don't know if it's hard or not. When I look at the current backend loading code it looks pretty complicated because of the out-of-process loading so I didn't want to manipulate that directly. Since I have now forked libkscreen to Disman I might look into that through some bigger design changes. But I will likely rather try to slime the library down than packing stuff on top.

I created some tasks for that. Your opinion as former KScreen maintainer and expert is of course very welcome: https://gitlab.com/kwinft/disman/-/issues

I will close this review now.

romangg abandoned this revision.May 14 2020, 11:16 PM