feat(wayland): add Wrapland plugin
AbandonedPublic

Authored by romangg on Apr 20 2020, 9:18 PM.

Details

Reviewers
davidedmundson
apol
Group Reviewers
Plasma
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.

Diff Detail

Repository
R110 KScreen Library
Branch
wrapland-plugin
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25561
Build 25579: arc lint + arc unit
romangg created this revision.Apr 20 2020, 9:18 PM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 20 2020, 9:18 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
romangg requested review of this revision.Apr 20 2020, 9:18 PM
davidedmundson requested changes to this revision.Apr 20 2020, 10:03 PM
davidedmundson added a subscriber: davidedmundson.

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.

This revision now requires changes to proceed.Apr 20 2020, 10:03 PM
romangg added a comment.EditedApr 20 2020, 10:22 PM

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 satisfy? 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?

apol requested changes to this revision.Apr 20 2020, 11:55 PM
apol added a subscriber: apol.

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.

romangg added a comment.EditedApr 21 2020, 12:05 AM
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.

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.

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.

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

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

davidedmundson added a comment.EditedApr 22 2020, 10:36 AM

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.

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.

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.

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.

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

romangg abandoned this revision.May 8 2020, 3:50 PM

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