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

[Roman Gilg]

romangg added a comment.


  In D29024#669970 , @dvratil wrote:
  
  > 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.

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D29024

To: romangg, #plasma
Cc: dvratil, ngraham, davidedmundson, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

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

[Daniel Vrátil]

dvratil added a comment.


  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.

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D29024

To: romangg, #plasma
Cc: dvratil, ngraham, davidedmundson, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

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

[Roman Gilg]

romangg added a comment.


  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.

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D29024

To: romangg, #plasma
Cc: ngraham, davidedmundson, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Roman Gilg]

romangg added a comment.


  In D29024#669718 , @davidedmundson 
wrote:
  
  > > 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!

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D29024

To: romangg, #plasma
Cc: ngraham, davidedmundson, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[David Edmundson]

davidedmundson added a comment.


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

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D29024

To: romangg, #plasma
Cc: ngraham, davidedmundson, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Roman Gilg]

romangg added a comment.


  In D29024#668884 , @ngraham wrote:
  
  > In D29024#668383 , @romangg 
wrote:
  >
  > > 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.
  
  In D29024#668978 , @davidedmundson 
wrote:
  
  > > 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.

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D29024

To: romangg, #plasma
Cc: ngraham, 

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

[David Edmundson]

davidedmundson added a comment.


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

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D29024

To: romangg, #plasma
Cc: ngraham, davidedmundson, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Nathaniel Graham]

ngraham added a comment.


  In D29024#668383 , @romangg wrote:
  
  > 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.

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D29024

To: romangg, #plasma
Cc: ngraham, davidedmundson, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Roman Gilg]

romangg added a comment.


  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 
omit this and instead continue my work on libkscreen as a KDE project like I 
have worked on it in the last two years.

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D29024

To: romangg, #plasma
Cc: davidedmundson, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[David Edmundson]

davidedmundson added a comment.


  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.

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D29024

To: romangg, #plasma
Cc: davidedmundson, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Roman Gilg]

romangg added a comment.


  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.

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D29024

To: romangg, #plasma
Cc: davidedmundson, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Roman Gilg]

romangg added a comment.


  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.

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D29024

To: romangg, #plasma
Cc: davidedmundson, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Roman Gilg]

romangg updated this revision to Diff 82265.
romangg added a comment.


  - refactor: export Wayland plugins headers

REPOSITORY
  R110 KScreen Library

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29024?vs=80700=82265

BRANCH
  wayland-plugins-export

REVISION DETAIL
  https://phabricator.kde.org/D29024

AFFECTED FILES
  CONTRIBUTING.md
  backends/kwayland/CMakeLists.txt
  backends/kwayland/KScreenWaylandConfig.cmake.in
  backends/kwayland/plugins/CMakeLists.txt
  backends/kwayland/plugins/kwayland/CMakeLists.txt
  backends/kwayland/plugins/kwayland/kwayland.json
  backends/kwayland/plugins/kwayland/kwayland_interface.cpp
  backends/kwayland/plugins/kwayland/kwayland_interface.h
  backends/kwayland/plugins/kwayland/kwayland_output.cpp
  backends/kwayland/plugins/kwayland/kwayland_output.h
  backends/kwayland/wayland_interface.cpp
  backends/kwayland/wayland_interface.h
  backends/kwayland/waylandbackend.cpp
  backends/kwayland/waylandbackend.h
  backends/kwayland/waylandconfig.cpp
  backends/kwayland/waylandconfig.h
  backends/kwayland/waylandoutput.cpp
  backends/kwayland/waylandoutput.h
  backends/kwayland/waylandscreen.cpp

To: romangg, #plasma
Cc: davidedmundson, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Roman Gilg]

romangg added a comment.


  In D29024#653096 , @davidedmundson 
wrote:
  
  > 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.

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D29024

To: romangg, #plasma
Cc: davidedmundson, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[David Edmundson]

davidedmundson added a comment.


  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.

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D29024

To: romangg, #plasma
Cc: davidedmundson, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Roman Gilg]

romangg updated this revision to Diff 80700.
romangg added a comment.


  Cleanup

REPOSITORY
  R110 KScreen Library

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29024?vs=80698=80700

BRANCH
  wayland-plugins

REVISION DETAIL
  https://phabricator.kde.org/D29024

AFFECTED FILES
  CONTRIBUTING.md
  backends/kwayland/CMakeLists.txt
  backends/kwayland/plugins/CMakeLists.txt
  backends/kwayland/plugins/kwayland/CMakeLists.txt
  backends/kwayland/plugins/kwayland/kwayland.json
  backends/kwayland/plugins/kwayland/kwayland_interface.cpp
  backends/kwayland/plugins/kwayland/kwayland_interface.h
  backends/kwayland/plugins/kwayland/kwayland_output.cpp
  backends/kwayland/plugins/kwayland/kwayland_output.h
  backends/kwayland/wayland_interface.cpp
  backends/kwayland/wayland_interface.h
  backends/kwayland/waylandbackend.cpp
  backends/kwayland/waylandbackend.h
  backends/kwayland/waylandconfig.cpp
  backends/kwayland/waylandconfig.h
  backends/kwayland/waylandoutput.cpp
  backends/kwayland/waylandoutput.h
  backends/kwayland/waylandscreen.cpp

To: romangg, #plasma
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Roman Gilg]

romangg added inline comments.

INLINE COMMENTS

> CMakeLists.txt:28
> +TARGETS LibkscreenWaylandPluginKWayland
> +DESTINATION ${PLUGIN_INSTALL_DIR}/org.kde.libkscreen.backends/wayland/
> +)

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.

> waylandconfig.cpp:155
>  
> -const auto features = Config::Feature::Writable | 
> Config::Feature::PerOutputScaling
> -| Config::Feature::AutoRotation | 
> Config::Feature::TabletMode;
> -m_kscreenConfig->setSupportedFeatures(features);
> -m_kscreenConfig->setValid(m_connection->display());
> -
> -KScreen::ScreenPtr screen = m_kscreenConfig->screen();
> -m_screen->updateKScreenScreen(screen);
> -
> -//Removing removed outputs
> -const KScreen::OutputList outputs = m_kscreenConfig->outputs();
> -for (const auto  : outputs) {
> -if (!m_outputMap.contains(output->id())) {
> -m_kscreenConfig->removeOutput(output->id());
> -}
> +// TODO: qobject_cast not working here. Why?
> +auto *factory = dynamic_cast(plugin->instantiate());

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

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D29024

To: romangg, #plasma
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Roman Gilg]

romangg updated this revision to Diff 80698.
romangg added a comment.


  Cleanup

REPOSITORY
  R110 KScreen Library

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29024?vs=80695=80698

BRANCH
  wayland-plugins

REVISION DETAIL
  https://phabricator.kde.org/D29024

AFFECTED FILES
  CONTRIBUTING.md
  backends/kwayland/CMakeLists.txt
  backends/kwayland/plugins/CMakeLists.txt
  backends/kwayland/plugins/kwayland/CMakeLists.txt
  backends/kwayland/plugins/kwayland/kwayland.json
  backends/kwayland/plugins/kwayland/kwayland_interface.cpp
  backends/kwayland/plugins/kwayland/kwayland_interface.h
  backends/kwayland/plugins/kwayland/kwayland_output.cpp
  backends/kwayland/plugins/kwayland/kwayland_output.h
  backends/kwayland/wayland_interface.cpp
  backends/kwayland/wayland_interface.h
  backends/kwayland/waylandbackend.cpp
  backends/kwayland/waylandbackend.h
  backends/kwayland/waylandconfig.cpp
  backends/kwayland/waylandconfig.h
  backends/kwayland/waylandoutput.cpp
  backends/kwayland/waylandoutput.h
  backends/kwayland/waylandscreen.cpp

To: romangg, #plasma
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Roman Gilg]

romangg created this revision.
romangg added a reviewer: Plasma.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
romangg requested review of this revision.

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

REPOSITORY
  R110 KScreen Library

BRANCH
  wayland-plugins

REVISION DETAIL
  https://phabricator.kde.org/D29024

AFFECTED FILES
  CONTRIBUTING.md
  backends/kwayland/CMakeLists.txt
  backends/kwayland/plugins/CMakeLists.txt
  backends/kwayland/plugins/kwayland/CMakeLists.txt
  backends/kwayland/plugins/kwayland/kwayland.json
  backends/kwayland/plugins/kwayland/kwayland_interface.cpp
  backends/kwayland/plugins/kwayland/kwayland_interface.h
  backends/kwayland/plugins/kwayland/kwayland_output.cpp
  backends/kwayland/plugins/kwayland/kwayland_output.h
  backends/kwayland/wayland_interface.cpp
  backends/kwayland/wayland_interface.h
  backends/kwayland/waylandbackend.cpp
  backends/kwayland/waylandbackend.h
  backends/kwayland/waylandconfig.cpp
  backends/kwayland/waylandconfig.h
  backends/kwayland/waylandoutput.cpp
  backends/kwayland/waylandoutput.h
  backends/kwayland/waylandscreen.cpp

To: romangg, #plasma
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart