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
Branch
apol/tablet-unstable-v2
Lint
Lint ErrorsExcuse: cppcheck is dumb
SeverityLocationCodeMessage
Errorsrc/server/tablet_interface.h:78CppcheckunknownMacro
Errorsrc/server/tablet_interface.h:78CppcheckunknownMacro
Errorsrc/server/tablet_interface.h:78CppcheckunknownMacro
Unit
No Unit Test Coverage
Build Status
Buildable 21543
Build 21561: arc lint + arc unit
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
2

Haven't you written this autotest?

40

The opening has to be on its own line.

78–79

Put a single space before * and &.

205

No short names.

225
231

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
328 ↗(On Diff #74217)

Wrong pointer alignment + missing @since.

src/server/tablet_interface.cpp
53

Align pointers to right.

63

No auto.

236–237

Add braces.

366

Put a single space before *.

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

Wrong pointer alignment.

121

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.

126

Put a single space before *.

137–145

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
369

The opening brace must be on its own line.

370
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.Mar 17 2020, 2:33 PM
src/server/tablet_interface.cpp
346

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.Mar 17 2020, 3:31 PM

Address comment by David

zzag added inline comments.Mar 17 2020, 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.Mar 17 2020, 7:14 PM

Fix tablet removal

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

Only delete when all the subscribed resources are gone.

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

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.Mar 18 2020, 4:04 PM
src/server/tablet_interface.cpp
58

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.Mar 19 2020, 5:09 PM

Fix the refcounting
Expose teh set_cursor interface as well

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

rebase

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