FakeInput: add support for pointer move with absolute coordinates
Needs ReviewPublic

Authored by jgrulich on Tue, Jan 8, 6:15 PM.

Details

Summary

For remote desktop support, we need to move with the pointer using absolute positin.

Test Plan

I tested this with xdg-desktop-portal-kde and krfb and it worked.

Diff Detail

Repository
R127 KWayland
Branch
master
Lint
Lint ErrorsExcuse: it's a Qt macro
SeverityLocationCodeMessage
Errorsrc/client/xdgshell.h:92CppcheckunknownMacro
Unit
No Unit Test Coverage
Build Status
Buildable 6897
Build 6915: arc lint + arc unit
jgrulich created this revision.Tue, Jan 8, 6:15 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptTue, Jan 8, 6:15 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
jgrulich requested review of this revision.Tue, Jan 8, 6:15 PM
zzag added a subscriber: zzag.Tue, Jan 8, 6:23 PM
zzag added inline comments.
src/client/protocols/fake-input.xml
19

Version of the interface has to be bumped.

zzag added inline comments.Tue, Jan 8, 6:29 PM
src/client/fakeinput.h
139

Wouldn't QPointF be a better choice?

graesslin requested changes to this revision.Wed, Jan 9, 5:28 AM
graesslin added a subscriber: graesslin.
graesslin added inline comments.
src/client/protocols/fake-input.xml
44

New requests can only be added at end. Since is missing.

src/server/fakeinput_interface.cpp
85

Must be last, new requests only at end.

This revision now requires changes to proceed.Wed, Jan 9, 5:28 AM
jgrulich updated this revision to Diff 49046.Wed, Jan 9, 6:45 AM

Use QPointF instead of QSizeF and bump interface version

jgrulich marked 4 inline comments as done.Wed, Jan 9, 7:59 AM
jgrulich updated this revision to Diff 49064.Wed, Jan 9, 10:08 AM

Revert unwanted changes in CMakeLists

zzag added a comment.Wed, Jan 9, 11:08 AM

maxVersion in src/client/registry.cpp should be updated as well.

src/client/fakeinput.h
138

Missing @since.

src/server/fakeinput_interface.h
118

Missing @since.

zzag added inline comments.Wed, Jan 9, 11:33 AM
src/server/fakeinput_interface.cpp
164

Please delete one extra empty line.

jgrulich updated this revision to Diff 49066.Wed, Jan 9, 11:37 AM

Add information when new methods were introduced and bump version of FakeInput in registry

jgrulich marked an inline comment as done.Wed, Jan 9, 11:37 AM
jgrulich marked 2 inline comments as done.Wed, Jan 9, 11:39 AM
zzag accepted this revision.Wed, Jan 9, 11:58 AM

Please wait for @davidedmundson and @graesslin. :-)

davidedmundson added inline comments.Wed, Jan 9, 12:14 PM
src/client/fakeinput.cpp
106

if (wl_proxy_get_version(d->manager) < ORG_KDE_SOMETHING_SOMETHING_FAKE_INPUT_VERSION) {
return;
}

jgrulich updated this revision to Diff 49069.EditedWed, Jan 9, 12:37 PM

Fix review comments

davidedmundson accepted this revision.Wed, Jan 9, 12:46 PM
zzag added inline comments.Wed, Jan 9, 12:47 PM
src/client/protocols/fake-input.xml
86

since="3"

zzag requested changes to this revision.Wed, Jan 9, 12:49 PM
This revision now requires changes to proceed.Wed, Jan 9, 12:49 PM
jgrulich updated this revision to Diff 49070.Wed, Jan 9, 12:50 PM

Fix review comments

zzag accepted this revision.Wed, Jan 9, 12:59 PM
zzag added inline comments.
src/client/fakeinput.cpp
106

Use curly braces even when the body of a conditional statement contains only one line.

jgrulich updated this revision to Diff 49071.Wed, Jan 9, 1:00 PM

Coding style

jgrulich marked an inline comment as done.Wed, Jan 9, 1:00 PM
zzag accepted this revision.Wed, Jan 9, 1:06 PM
jgrulich updated this revision to Diff 49081.Wed, Jan 9, 3:16 PM

Bump fake interface version

jgrulich updated this revision to Diff 49082.Wed, Jan 9, 3:37 PM

Add support for keyboard key press and release

jgrulich retitled this revision from FakeInput: add support for pointer move with absolute coordinates to FakeInput: add support for pointer move with absolute coordinates and keyboard key press and release.Wed, Jan 9, 3:38 PM
jgrulich edited the summary of this revision. (Show Details)

I didn't intent to push this together, but when creating a branch from master which already had first round of changes caused my second part of changes to be pushed here. I hope you don't mind that. I'm doing this in a hurry hoping to get exception from David Faure to include this with KDE Frameworks 5.54. I thought the tagging is this saturday and tarballs are made after that, but I was wrong. I would really like to push the remote desktop support into Plasma 5.15, I have almost complete support in xdg-desktop-portal-kde and krfb.

jgrulich updated this revision to Diff 49087.Wed, Jan 9, 4:08 PM

Fix documentation

I don't want to have key press/release in it. It has a reason that I didn't add key press/release from the start. The problem is the mismatch of key events and keysyms. Especially the kdeconnect use case for which this interface was developed has this problem. Android composes text, not key codes. Thus it would not be possible to inject key events.

Also from a security perspective I'm strictly against adding fake key events. That's one of the largest security problems on X11. I rather have no remote support than a gaping security hole.

I'm the author of this interface and I consider it nowadays as a mistake to have added it. If I would restart I would do the whole thing differently: input events need to be injected through a kernel module in a dedicated input device. It would need a user space component which processes can talk to and that would need to be secured though e.g. polkit. Thus we could properly authenticate. This protocol does not support it and KWin does not even try to authenticate. Overall from security perspective this is a mess.

jgrulich updated this revision to Diff 49133.Thu, Jan 10, 5:53 AM

Drop keyboard support from this review

jgrulich retitled this revision from FakeInput: add support for pointer move with absolute coordinates and keyboard key press and release to FakeInput: add support for pointer move with absolute coordinates.Thu, Jan 10, 5:54 AM
jgrulich edited the summary of this revision. (Show Details)

Hi, can I get please this re-approved? It's now just again about the additional mouse support.