Provide an implementation for the tablet interface
ClosedPublic

Authored by apol on Jan 23 2020, 12:46 AM.

Details

Summary

Implements the necessary classes to have proper support for the tablet and pen.
Doesn't implement yet the ring/slider/totem bits.

Test Plan

Used it with Plasma and GTK clients, together with the kwin patch I'll submit soon.
https://youtu.be/GGx0TlNJlzs

Also I added a test.

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.
apol created this revision.Jan 23 2020, 12:46 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 23 2020, 12:46 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
apol requested review of this revision.Jan 23 2020, 12:46 AM
zzag requested changes to this revision.Jan 23 2020, 9:38 AM
zzag added a subscriber: zzag.
zzag added inline comments.
autotests/server/test_tablet_interface.cpp
3

Haven't you written this autotest?

41

The opening has to be on its own line.

79–80

Put a single space before * and &.

206

No short names.

226
232

What's n?

src/server/tablet_interface.h
46

This revision seems to be incomplete. I don't see where TabletManagerInterface was introduced.

This revision now requires changes to proceed.Jan 23 2020, 9:38 AM
apol marked 6 inline comments as done.Jan 23 2020, 11:17 AM
apol updated this revision to Diff 74212.Jan 23 2020, 11:20 AM

Addressed zzag's comments, fixed the patch overall.

apol updated this revision to Diff 74213.Jan 23 2020, 11:22 AM

remove unrelated bits

This revision was not accepted when it landed; it landed in state Needs Review.Jan 23 2020, 11:23 AM
This revision was automatically updated to reflect the committed changes.
apol reopened this revision.Jan 23 2020, 11:27 AM

Phabricator chokes on git branches...

apol updated this revision to Diff 74217.Jan 23 2020, 11:27 AM

ehm......

zzag requested changes to this revision.Jan 23 2020, 12:58 PM
zzag added inline comments.
autotests/server/test_tablet_interface.cpp
103–119

Put a single space before *.

228

Align pointers to right.

src/server/display.h
314

Wrong pointer alignment + missing @since.

src/server/tablet_interface.cpp
54

Align pointers to right.

64

No auto.

237–238

Add braces.

367

Put a single space before *.

Display * const m_display;
src/server/tablet_interface.h
53

Wrong pointer alignment.

122

This method lacks documentation. At first, I thought that it returns the wl_resource for a wl_surface. However, the best thing would be not to leak wl_resource to the public API at all.

127

Put a single space before *.

138–146

Align pointers to right.

This revision now requires changes to proceed.Jan 23 2020, 12:58 PM
apol marked 11 inline comments as done.Jan 23 2020, 2:54 PM
apol updated this revision to Diff 74241.Jan 23 2020, 2:56 PM

Address comments

zzag added a comment.EditedJan 27 2020, 2:07 PM

Looks good to me in general. There are still many coding style issues. I suggest to run clang-format over individual files in this patch and then adjust the most problematic places (I don't think there will be that many).

You could borrow a .clang-format file from https://github.com/KDE/extra-cmake-modules/blob/master/kde-modules/clang-format.cmake. Just make sure that ColumnLimit is set to 0.

apol updated this revision to Diff 74473.Jan 28 2020, 3:03 AM

clang format

zzag accepted this revision.Jan 28 2020, 10:53 AM
zzag added a subscriber: davidedmundson.

Code-wise, this change looks good. I don't have a tablet device to test this patch so you may want to wait for a +1 from @davidedmundson.

src/server/tablet_interface.cpp
370

The opening brace must be on its own line.

371
SeatInterface *seat = SeatInterface::get(seat_resource);
This revision is now accepted and ready to land.Jan 28 2020, 10:53 AM

Feedback from some testing:

  • we're adding the same tool a bunch of times when a new client is created
  • sometimes we fail to enter the surface leaving the cursor "stuck" (with gtk3-demo)

I'll see if I can spot why.

FWIW, Qt have a tablet test at qtbase/examples/widgets/widgets/tablet though that means compiling 5.15 to get the qtwayland tablet integration

apol updated this revision to Diff 75561.Feb 12 2020, 4:07 PM

rebase on master

apol added a comment.Feb 12 2020, 4:39 PM

Feedback from some testing:

  • we're adding the same tool a bunch of times when a new client is created
  • sometimes we fail to enter the surface leaving the cursor "stuck" (with gtk3-demo)

    I'll see if I can spot why.

FWIW, Qt have a tablet test at qtbase/examples/widgets/widgets/tablet though that means compiling 5.15 to get the qtwayland tablet integration

It looks stuck because we're not emulating mouse over them.

We'll need to set up some infrastructure in KWin to do that properly.
Before you look into it, we can't use Cursor::setPos() because it actually moves the mouse pointer and spawns a ton of unwanted events.

apol updated this revision to Diff 75564.Feb 12 2020, 4:42 PM

Only call destruction if it was an owned tablet, this way we don't crash.

davidedmundson added inline comments.Tue, Mar 17, 2:33 PM
src/server/tablet_interface.cpp
347

We're mixing up hardwareId and hardwareSerial

this method is called toolByHardwareId calls an internal method called hardwareSerial

Yet ultimately does check:
m_hardwareIdHigh

comparing it against a serial

apol updated this revision to Diff 77839.Tue, Mar 17, 3:31 PM

Address comment by David

zzag added inline comments.Tue, Mar 17, 3:36 PM
autotests/server/test_tablet_interface.cpp
2–20

Note that we've switched to SPDX markers.

apol updated this revision to Diff 77861.Tue, Mar 17, 7:14 PM

Fix tablet removal

apol updated this revision to Diff 77926.Wed, Mar 18, 2:49 PM

Only delete when all the subscribed resources are gone.

davidedmundson added inline comments.Wed, Mar 18, 3:56 PM
src/server/tablet_interface.cpp
59

This is still potentially crashy:
if all clients remove their tablet support we would have a dangling pointer in m_tablets.

Personally I would remove this delete q, and have the lifespan of TabletInterface managed up to TabletSeatInterface::removeTablet or TabletSeatInterface::~TabletSeatInterface()

even if it has no resources.

davidedmundson added inline comments.Wed, Mar 18, 4:04 PM
src/server/tablet_interface.cpp
59

or if we do want to keep the TabletInterface around till clients gracefully remove their side
we should have some sort of if (resourcesMap.isEmpty() && isRemoved) {delete q}

apol updated this revision to Diff 78027.Thu, Mar 19, 5:09 PM

Fix the refcounting
Expose teh set_cursor interface as well

apol updated this revision to Diff 78028.Thu, Mar 19, 5:10 PM

rebase

davidedmundson accepted this revision.Thu, Mar 19, 5:10 PM
This revision was automatically updated to reflect the committed changes.