D29028: feat(wayland): add Wrapland plugin

[Roman Gilg]

romangg abandoned this revision.
romangg added a comment.


  Will become an external plugin with libkscreen Wayland headers being exported 
now.

REPOSITORY
  R110 KScreen Library

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

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

D29028: feat(wayland): add Wrapland plugin

[Roman Gilg]

romangg added a comment.


  In D29028#654542 , @davidedmundson 
wrote:
  
  > A new dependency also needs to actually solve an actual problem.
  >
  > If we say we should support wl_roots' protocol for wlroots users. Fair 
enough. There are some parts of Plasma used by 3rd parties. 
  >  I'd certainly be very happy for us both to switch to a new standard given 
they're upstreaming some stuff currently.
  >
  > But we then have to answer the technical question of why does that require 
a library with a different implementation of ConnectionThread/Registry and 
every client protocol in order to do so? Compared to using one technology 
throughout. Otherwise you're not really solving the original problem of having 
it available to users.
  
  
  For wlroots I have done in Wrapland a quick protocol implementation in two or 
three days. I have not done much testing and hope for more data from first use 
cases in the future to iterate it. Since KWayland is API/ABI-stable that won't 
be possible there. When at some point the implementation has become well tested 
why not copy-paste it too KWayland?
  
  But this is not only about support for the wlroots protocol. In Wrapland I 
have rewritten KWayland's org_kde_kwin_outputdevice 
 
protocol as kwinft_output_device_unstable_v1 
.
 It is mostly the same but I cleaned up some stuff, used semantics as it is 
standard nowadays and replaced the scale factor with a new way of defining the 
logical size of an output (basically D26311 
). So for libkscreen to work with KWinFT it 
needs to support this protocol. And I assume it isn't something that should go 
into KWayland. For API/ABI reasons again and because it already has one 
protocol, even if it's sub-optimal nowadays.
  
  Of course when the protocol is stabilized and shows its merit it can always 
go into KWayland or whatever KDE Framework there will be in the future. Or we 
go for a common protocol with wlroots. I talked with Simon some time ago about 
their protocol and they have a mechanism with custom modes, but I find defining 
the logical size better. But I might be wrong on that and the 
kwinft_output_device_unstable_v1 protocol will tell me so in the future

REPOSITORY
  R110 KScreen Library

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

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

D29028: feat(wayland): add Wrapland plugin

[Roman Gilg]

romangg added a comment.


  In D29028#654535 , @anthonyfieroni 
wrote:
  
  > In D29028#653194 , @romangg 
wrote:
  >
  > > In D29028#653192 , @apol wrote:
  > >
  > > > 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.
  > >
  > >
  > > It is offering in the same way ABI stability as most other components of 
Plasma, i.e. until a new minor Plasma release.
  >
  >
  > That's not true. Always use pimpl in library code, say KWin can break 
backward compatibility, KWayland, KScreen, etc. does not. If you do it as it 
should be then it may be accepted (i cannot guaranteed but will be step in 
right direction, at least).
  
  
  First, how is pimpl relevant to the discussion here? Second, KWin and 
libkscreen are both Plasma projects, so the library functionality they both 
provide to a certain degree are offered on the same stability premise. KWayland 
on the other hand is a KDE Framework and has a different stability guarantee. 
To my knowledge there is no equivalent guarantee in Plasma, not written down at 
least.

REPOSITORY
  R110 KScreen Library

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

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

D29028: feat(wayland): add Wrapland plugin

[Roman Gilg]

romangg added a comment.


  In D29028#654526 , @tcanabrava 
wrote:
  
  > I understand David's point: Wrapland project has one developer and we don't 
know how successful it will be, while the other backends have developers. What 
would happen if you suddenly quit / disappear and the project dies? Then 
kscreen will have a folder of dead code.
  >
  > This is a Plugin, it can live in any folder / project, you already forked 
KWinFT, create a new project to put this KScreen plugin, we can't scale to N 
projects adding code as plugins that will need to be maintained for quite a 
while in the future.
  >
  > > Come on man, do you really want to make this ugly? I thought we would 
still treat each other with respect David. :(
  >
  > Your words. His words are technical, Please don't distort things and make 
it about you.
  
  
  I haven't noticed much technical words in what was said. I did ask about the 
headers-only proposal that was of technical nature. Also the tone made it seem 
to me personal, but that sadly might have been just what is often the default 
tone in Plasma reviews (no finger pointing intended, I have been guilty of that 
in the past too).
  
  >> Which "established practices" does this not satisfy?
  > 
  > It's an external library that does not solve any problem within the plasma, 
nor adds value to plasma users. It's an experimental project, with only one 
developer, not stable, not ready, not core. You can see on this picture that we 
are not blocking your project here because of $excuse, but this is something 
that we do if the project is not core, please check the date.
  >  F8253102: image.png 
  > 
  > There's more information on the full phabricator ticket:
  >  https://phabricator.kde.org/D20265
  
  I looked at that and suspected to see a hard dependency there. But it also is 
only an optional dependency with a check if the package is available. So I'm 
wondering what are the rules if something can become an //optional// dependency 
for a Plasma project? Is this written down somewhere?
  
  >> How would that look in detail?
  > 
  > That will give you the possibility to create plugins for KScreen that are 
not bound to KScreen code, then you can create your project in gitlab and have 
different release schedules, in a way that:
  >  1 - The plugin code is independent to KDE Releases
  >  2 - We have a clear separation on project responsabilities
  >  3 - Being independent means that it can also test with experimental 
libraries.
  > 
  >> Why is this even relevant?
  > 
  > We need to be sure that the code of the project will always be buildable 
through the lifespan of Plasma 5.19, and right now we can't guarantee it.
  
  Since the dependency is optional you are guaranteed to be able to build 
libkscreen by removing Wrapland or not installing it in the first place. Also 
Wrapland will have a 0.519.0 release as it has now a 0.518.0 release. Releases 
will be synced up with Plasma releases. Otherwise the KWinFT-Plasma integration 
would not work anyway.
  
  > Create this code outside of KSCreen as a plugin. We will gladly accept code 
that install the Interface Headers to make that possible. :)
  
  I think that this is unnecessary complicated because releases will be synced, 
but I can live with it. Would this mean only the first patch of this patch 
series would go in and I export the `wayland_interface.h` and `waylandoutput.h` 
headers (they are being used in the plugins)?
  
  Thanks for taking the time Tomaz.

REPOSITORY
  R110 KScreen Library

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

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

D29028: feat(wayland): add Wrapland plugin

[David Edmundson]

davidedmundson added a comment.


  A new dependency also needs to actually solve an actual problem.
  
  If we say we should support wl_roots' protocol for wlroots users. Fair 
enough. There are some parts of Plasma used by 3rd parties. 
  I'd certainly be very happy for us both to switch to a new standard given 
they're upstreaming some stuff currently.
  
  But we then have to answer the technical question of why does that require a 
library with a different implementation of ConnectionThread/Registry and every 
client protocol in order to do so? Compared to using one technology throughout.

REPOSITORY
  R110 KScreen Library

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

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

D29028: feat(wayland): add Wrapland plugin

[Anthony Fieroni]

anthonyfieroni added a comment.


  In D29028#653194 , @romangg wrote:
  
  > In D29028#653192 , @apol wrote:
  >
  > > 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.
  >
  >
  > It is offering in the same way ABI stability as most other components of 
Plasma, i.e. until a new minor Plasma release.
  
  
  That's not true. Always use pimpl in library code, say KWin can break 
backward compatibility, KWayland, KScreen, etc. does not. If you do it as it 
should be then it may be accepted (i cannot guaranteed but will be step in 
right direction, at least).

REPOSITORY
  R110 KScreen Library

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

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

D29028: feat(wayland): add Wrapland plugin

[Tomaz Canabrava]

tcanabrava added a comment.


  I understand David's point: Wrapland project has one developer and we don't 
know how successful it will be, while the other backends have developers. What 
would happen if you suddenly quit / disappear and the project dies? Then 
kscreen will have a folder of dead code.
  
  This is a Plugin, it can live in any folder / project, you already forked 
KWinFT, create a new project to put this KScreen plugin, we can't scale to N 
projects adding code as plugins that will need to be maintained for quite a 
while in the future.
  
  > Come on man, do you really want to make this ugly? I thought we would still 
treat each other with respect David. :(
  
  Your words. His words are technical, Please don't distort things and make it 
about you.
  
  > Which "established practices" does this not satisfy?
  
  It's an external library that does not solve any problem within the plasma, 
nor adds value to plasma users. It's an experimental project, with only one 
developer, not stable, not ready, not core. You can see on this picture that we 
are not blocking your project here because of $excuse, but this is something 
that we do if the project is not core, please check the date.
  F8253102: image.png 
  
  There's more information on the full phabricator ticket:
  https://phabricator.kde.org/D20265
  
  > How would that look in detail?
  
  That will give you the possibility to create plugins for KScreen that are not 
bound to KScreen code, then you can create your project in gitlab and have 
different release schedules, in a way that:
  1 - The plugin code is independent to KDE Releases
  2 - We have a clear separation on project responsabilities
  3 - Being independent means that it can also test with experimental libraries.
  
  > Why is this even relevant?
  
  We need to be sure that the code of the project will always be buildable 
through the lifespan of Plasma 5.19, and right now we can't guarantee it.
  
  Create this code outside of KSCreen as a plugin. We will gladly accept code 
that install the Interface Headers to make that possible. :)

REPOSITORY
  R110 KScreen Library

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

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

D29028: feat(wayland): add Wrapland plugin

[Roman Gilg]

romangg added a comment.


  In D29028#653192 , @apol wrote:
  
  > 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.
  
  
  It is offering in the same way ABI stability as most other components of 
Plasma, i.e. until a new major Plasma release.
  
  Why is this even relevant? The build is optional. Distros not providing 
Wrapland won't be affected by it.
  
  If this rejection is for political reasons just say so directly. You have 
already hinted at that with "doesn't push Plasma in any direction". Please 
think about what you are doing here.

REPOSITORY
  R110 KScreen Library

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

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

D29028: feat(wayland): add Wrapland plugin

[Aleix Pol Gonzalez]

apol requested changes to this revision.
apol added a comment.


  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.

REPOSITORY
  R110 KScreen Library

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

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

D29028: feat(wayland): add Wrapland plugin

[Roman Gilg]

romangg added a comment.


  In D29028#653150 , @davidedmundson 
wrote:
  
  > I don't see why we should have that in KDE code.
  
  
  Come on man, do you really want to make this ugly? I thought  we would still 
treat each other with respect David. :(
  
  > As per manifesto
  > 
  >> The project stays true to established practices common to similar KDE 
projects
  
  What do you mean by that? Which "established practices" does this not 
fulfill? Wrapland is free software and it is not required for the build of 
libkscreen. The default is still KWayland.
  
  > As a compromise we can install the headers of the plugin interface.
  
  How would that look in detail?

REPOSITORY
  R110 KScreen Library

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

To: romangg, #plasma, davidedmundson
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

D29028: feat(wayland): add Wrapland plugin

[David Edmundson]

davidedmundson requested changes to this revision.
davidedmundson added a comment.
This revision now requires changes to proceed.


  I don't see why we should have that in KDE code.
  
  As per manifesto
  
  > The project stays true to established practices common to similar KDE 
projects
  
  As a compromise we can install the headers of the plugin interface.

REPOSITORY
  R110 KScreen Library

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

To: romangg, #plasma, davidedmundson
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

D29028: feat(wayland): add Wrapland plugin

[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
  Adds a plugin that uses the Wrapland library to interact with compositors
  supporting the kwinft_output_management_unstable_v1 protocol.
  
  If the backend plugin is available at runtime the interface is instantiated in
  parallel with the KWayland interface and whichever interface has success in
  retrieving a management global first is used while the other one is rejected
  for this execution.
  
  Building this plugin is optionally and depends on Wrapland being available or
  not.

TEST PLAN
  Tested in KWin and KWinFT sessions.

REPOSITORY
  R110 KScreen Library

BRANCH
  wrapland-plugin

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

AFFECTED FILES
  backends/kwayland/plugins/CMakeLists.txt
  backends/kwayland/plugins/wrapland/CMakeLists.txt
  backends/kwayland/plugins/wrapland/wrapland.json
  backends/kwayland/plugins/wrapland/wrapland_interface.cpp
  backends/kwayland/plugins/wrapland/wrapland_interface.h
  backends/kwayland/plugins/wrapland/wrapland_output.cpp
  backends/kwayland/plugins/wrapland/wrapland_output.h

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