Add support for touch events in fakeinput protocol and interface.
ClosedPublic

Authored by bdhruve on May 24 2016, 12:56 PM.

Details

Summary

This adds support for following in fakeinput interface:

  • touch_down
  • touch_motion
  • touch_up
  • touch_cancel
  • touch_frame
Test Plan

added autotests passes

Diff Detail

Repository
R127 KWayland
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bdhruve updated this revision to Diff 3965.May 24 2016, 12:56 PM
bdhruve retitled this revision from to Add support for touch events in fakeinput protocol and interface..
bdhruve updated this object.
bdhruve edited the test plan for this revision. (Show Details)
bdhruve added a reviewer: Plasma.
Restricted Application added a project: Plasma. · View Herald TranscriptMay 24 2016, 12:56 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
graesslin added inline comments.
src/client/protocols/fake-input.xml
19

when you add new requests to a protocol you need to increase the version number. This requires changes at a few more places. For reference on what needs to be changed check D1417

bshah requested changes to this revision.May 25 2016, 3:51 AM
bshah added a reviewer: bshah.
bshah added a subscriber: bshah.
bshah added inline comments.
autotests/client/test_fake_input.cpp
328

In addition to what @graesslin said, one point about coding style,

KWayland is part of KF5 which follows KDE Frameworks 5 Coding style guide for its code.

You seem to have used inconsistent coding style for code here and as well other places. Please go over all the code you have added and correct it.

src/server/fakeinput_interface.h
128–132

Given this will be part of public API, please add the documentation for this methods like other pointer methods.

This revision now requires changes to proceed.May 25 2016, 3:51 AM
bdhruve updated this revision to Diff 3982.May 25 2016, 8:58 AM
bdhruve edited edge metadata.

Fixed issues mentioned by @graesslin and @bshah

graesslin requested changes to this revision.May 25 2016, 9:34 AM
graesslin added a reviewer: graesslin.
graesslin added inline comments.
src/client/fakeinput.h
133

why a delta? In KWin all touchMotion related code takes the new position.

src/client/protocols/fake-input.xml
52–68

if you have a new enough wayland-scanner installed this should generate errors because it expects documentation. I'm aware that the existing interface is missing documentation, but I think for new code we should make sure that wayland-scanner is happy.

src/server/fakeinput_interface.cpp
196

nitpick: empty line

src/server/fakeinput_interface.h
139

same here

This revision now requires changes to proceed.May 25 2016, 9:34 AM
bdhruve updated this revision to Diff 3992.May 25 2016, 11:56 AM
bdhruve edited edge metadata.

Use position instead of delta in touch motion, and added docs.

I'm wondering: should we ensure that the ids are correct. E.g. a touchUp for id 1 doesn't make sense if we never got a touchDown for id 1. This would require tracking the used ids in FakeInputInterface. But it must be done somewhere - either in the library or by the user of the library.

src/server/fakeinput_interface.cpp
198

The isAuthenticated check is not verified in the autotest. That's the same with the other new methods.

I'm wondering: should we ensure that the ids are correct. E.g. a touchUp for id 1 doesn't make sense if we never got a touchDown for id 1. This would require tracking the used ids in FakeInputInterface. But it must be done somewhere - either in the library or by the user of the library.

IMO client ideally will not send bogus event. And in any case if it does, compositor itself should ignore bogus events because it is the one which is processing in end..

In D1672#31094, @bshah wrote:

I'm wondering: should we ensure that the ids are correct. E.g. a touchUp for id 1 doesn't make sense if we never got a touchDown for id 1. This would require tracking the used ids in FakeInputInterface. But it must be done somewhere - either in the library or by the user of the library.

IMO client ideally will not send bogus event. And in any case if it does, compositor itself should ignore bogus events because it is the one which is processing in end..

a client could also use this to attack (crash) the Compositor by sending bogus events. That's security relevant from my point of view.

bdhruve updated this revision to Diff 4000.May 25 2016, 2:02 PM
bdhruve edited edge metadata.

Improved autotest

I'm wondering: should we ensure that the ids are correct. E.g. a touchUp for id 1 doesn't make sense if we never got a touchDown for id 1. This would require tracking the used ids in FakeInputInterface. But it must be done somewhere - either in the library or by the user of the library.

Martin, do you prefer this change to be done in this review only or in separate review.

I'm wondering: should we ensure that the ids are correct. E.g. a touchUp for id 1 doesn't make sense if we never got a touchDown for id 1. This would require tracking the used ids in FakeInputInterface. But it must be done somewhere - either in the library or by the user of the library.

Martin, do you prefer this change to be done in this review only or in separate review.

I think it would be better to directly integrate it here.

I'm wondering: should we ensure that the ids are correct. E.g. a touchUp for id 1 doesn't make sense if we never got a touchDown for id 1. This would require tracking the used ids in FakeInputInterface. But it must be done somewhere - either in the library or by the user of the library.

Martin, do you prefer this change to be done in this review only or in separate review.

I think it would be better to directly integrate it here.

Hello, I've understood the problem and how to fix it in theory, but i am not sure on implementation details? For instance, I am not sure on where to track ids? On server side FakeInputInterface, FakeInputDevice or client side in FakeInput?
So, can you please help me on this?

On server side FakeInputInterface, FakeInputDevice or client side in FakeInput?

Needs to be done on server side. The thought is that the Client might send incorrect data (the interface is the protocol, not your client code), so the server needs to do input validation.

bdhruve updated this revision to Diff 4063.May 30 2016, 11:54 AM

Track touchIds and update autotests.
(I am not sure if i did correctly or not.)

graesslin requested changes to this revision.May 31 2016, 9:32 AM
graesslin edited edge metadata.

Looks good, but please fix the coding style.

src/server/fakeinput_interface.cpp
201–202

please watch coding style - same in all other areas you just adjusted.

This revision now requires changes to proceed.May 31 2016, 9:32 AM
bdhruve updated this revision to Diff 4089.May 31 2016, 10:07 AM
bdhruve edited edge metadata.

Fixed the coding style issue.

bshah accepted this revision.May 31 2016, 10:25 AM
bshah edited edge metadata.

Looks good.

In D1672#31870, @bshah wrote:

Looks good.

Output of arc land fake-input-touch:
TARGET Landing onto "master", the default target under git.
REMOTE Using remote "origin", the default remote under git.
FETCH Fetching origin/master...
This commit will be landed:

  • 2d5d5c2 Add support for touch events in fakeinput protocol and interface.

    Revision 'D1672: Add support for touch events in fakeinput protocol and interface.' has not been accepted. Continue anyway? [y/N]

Can i still submit it?

graesslin accepted this revision.May 31 2016, 11:06 AM
graesslin edited edge metadata.
This revision is now accepted and ready to land.May 31 2016, 11:06 AM
This revision was automatically updated to reflect the committed changes.