In T11601#231487, @kleag wrote:

I see that @gladhorn has not commited since last september. Is anyone still working on this feature? @romangg , any insights on that subject?

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.

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.

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.

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.

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

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.

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

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

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.

• refactor: export Wayland plugins headers

Docs explicitly say this.

In D29272#659619, @apol wrote:

Are we ok to let Meta key opening menu even if inhibitor is active ?

Wouldn't this do exactly what this patch is trying to prevent?

I would recommend to create an overview where XTime is actually used in KWin and how other compositors deal with it. Then try to create a theoretic solution and present that.

Please confirm if that still stands.

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.

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

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

I like that. KCMs should be more inter-connected anyway. The message could just be shown all the time instead of making it dependent on the font size selected but no need to change that now.

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.

In D29028#653150, @davidedmundson wrote:

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

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.

Test on sway.

Cleanup

Cleanup

Looks good to me. Also happy @davidedmundson ?

Yes, please put the m_timer removal in a separate patch. And (in this separate patch) if you removed the m_timer the QTimer include in the header file can be removed as well I assume. Thanks.

Thanks. Some minor remaining nitpicks you can change on push.

I changed up the description a bit more (line length!). Please make sure to push with arc so it fetches this description for the commit message.

Conceptually makes sense. +1 form me on that. Good solution. Thanks for outlining it in the summary.

Header subject and commit message body must have shorter line length in line with message guideline. Also note regarding capitalization Angular's subject guideline.

Please explain your solution in the summary. I don't directly understand how this change fixes the bug. Thanks.

Message guideline must be adhered. Summary must be more extensive.

Message guideline must be adhered. Summary must be more extensive.

Add the const and push.

I'm not sure about the renaming. the lock... implies that the rotation stays the way it is at the moment (what it does). But I'm fine either way.

Yes, makes sense. I checked back on the protocol to see if a Wayland client can request the same offer multiple times and of course it can to receive multiple mime types.

If you want to you can also add a ⚠️ (:warning:) emoticon to the deprecated text and in both some headings with #, ##, etc.

README.md is common for viewing in modern code hosting solutions with markdown support.

Oh, I just noticed: Please absolutely add a commit message body to this commit on push! Just a single line explaining what you do is enough.

Sorry for late review. I didn't get notified by this bullshit platform (which might be because I disabled all notifications at one point to not get spammed constantly).

In D10042#629261, @dvratil wrote:

Agreed. deviceId is not actually used (it's stored in KDED config files, but the value doesn't seem to be used anywhere). I'll fix it in a separate commit (since it also needs adjusting a bunch of unit-tests in libkscreen and kscreen).

Include the cpp files

Thank you @zzag.

Shall I put the connects in the constructor to move this forward?

Compiles again on Neon.

In D28115#632489, @davidedmundson wrote:

How does mobile retrieve the current lockAutoRotate value?

Please call the functions AutoRotate. I failed to name it consistently in the code until now but let's go with AutoRotate from now on (and I will try to rename it where I accidentally said AutoRotation in some form). Reasons are that it's shorter and when I googled it I found only one technical document. Was for Android and called the feature that way.