Initial support for tablets on Wayland
ClosedPublic

Authored by apol on Sun, Dec 1, 6:46 PM.

Details

Summary

This includes support for them on libinput and turns it into fake
pointer actions.
This doesn't implement zwp_tablet, this will have to happen in an
iteration later.

Test Plan

Been playing around with it, see video.
https://www.youtube.com/watch?v=GF1WbO8FVvU

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a project: KWin. · View Herald TranscriptSun, Dec 1, 6:46 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
apol requested review of this revision.Sun, Dec 1, 6:46 PM
apol edited the test plan for this revision. (Show Details)Sun, Dec 1, 6:50 PM
romangg requested changes to this revision.Sun, Dec 1, 10:50 PM
romangg added a subscriber: romangg.
  • I suppose this is related to T11660? If so relate the diff and the object.
  • Improve commit message, explain your future steps.
  • For touch events we already have a fallback to pointer events in KWayland in case the client does not support touch events. I would prefer that we follow the same pattern with tablet events. Otherwise argue why a input event filter is better suited in this case.
  • Adhere to styling rules. From what I have seen:
    • Line length
    • Brackets
    • Enum definition
  • libinput interfacing code looks good
input.cpp
1540

unused

1563

There is a input()->pointer() getter. Remove member variable.

libinput/connection.cpp
471

Do not use single letter variable names. Minimum two letters. Same below with e.

libinput/events.h
220

Implement or add a notice why these are commented out / a TODO.

pointer_input.h
60 ↗(On Diff #70691)

Change to hash and getter in separate patch. Under the assumption we keep the new input event filter this needs to go into input.h/input.cpp or some other device-independent file and not anymore in pointer_input.

This revision now requires changes to proceed.Sun, Dec 1, 10:50 PM
apol marked 5 inline comments as done.Mon, Dec 2, 12:13 PM
apol updated this revision to Diff 70738.Mon, Dec 2, 12:27 PM

Address roman's comments

apol edited the summary of this revision. (Show Details)Mon, Dec 2, 2:04 PM
apol updated this revision to Diff 70960.Thu, Dec 5, 2:52 PM

split

romangg requested changes to this revision.Thu, Dec 5, 7:30 PM
  • Still needs several styling corrections.
  • I would say as with touch and pointer input there should be a focus update when the pen touches down (update() function)
tablet_input.cpp
93

No inline comments, make it mutliple lines, put comment as // comments behind arguments.

tablet_input.h
77

Implement in cpp file. There use Q_UNUSED macro.

This revision now requires changes to proceed.Thu, Dec 5, 7:30 PM
apol marked 2 inline comments as done.Mon, Dec 9, 10:24 AM

I would say as with touch and pointer input there should be a focus update when the pen touches down (update() function)

It doesn't make much sense that we need to update() focus from input collection, especially in this patch that is about exposing the data. I'm not sure though.

apol updated this revision to Diff 71113.Mon, Dec 9, 10:25 AM

address comments by roman

apol updated this revision to Diff 71114.Mon, Dec 9, 10:32 AM

Fix build

romangg requested changes to this revision.Tue, Dec 10, 1:51 PM
In D25663#574166, @apol wrote:

I would say as with touch and pointer input there should be a focus update when the pen touches down (update() function)

It doesn't make much sense that we need to update() focus from input collection, especially in this patch that is about exposing the data. I'm not sure though.

Ok, we can look at that later.

See remaining style comments.

input.cpp
1975

These lines are still longer than 100 chars.

libinput/events.h
227

These lines are still longer than 100 chars.

tablet_input.cpp
113

These lines are still longer than 100 chars.

tablet_input.h
56

These lines are still longer than 100 chars.

This revision now requires changes to proceed.Tue, Dec 10, 1:51 PM
apol updated this revision to Diff 71194.Tue, Dec 10, 2:27 PM
apol marked an inline comment as done.

Run clang-format

romangg requested changes to this revision.Tue, Dec 10, 2:44 PM

Now it's fucked up all over the place.

debug_console.cpp
494

better before. + in front

510

brackets

523

brackets

527

+ in front

536

+ in front

546

+ in front

input.cpp
1975

clang-format stupid

input.h
98

clang-format stupid

libinput/connection.h
134

unnecessary many lines. group xTilt, yTilt etc.

libinput/events.h
200

clang-format stupid

This revision now requires changes to proceed.Tue, Dec 10, 2:44 PM
apol updated this revision to Diff 71200.Tue, Dec 10, 3:03 PM
apol marked 9 inline comments as done.

Spaces

I don't care about any whitespace stuff, but code-wise this looks fine.

+1 when Roman's happy.

romangg requested changes to this revision.Tue, Dec 10, 4:33 PM
romangg added inline comments.
input.h
233

bracket

libinput/events.h
200

If you define member functions in the class itself these brackets must be behind the function name, i.e.:

bool xHasChanged() const {
....
}
281

For code comments please use proper punctuation and capitalization.

tablet_input.cpp
125

Split like this:

input()->processSpies(std::bind(&InputEventSpy::tabletToolButtonEvent, std::placeholders::_1,
                                m_toolPressedButtons));

Below as well.

132

Brackets!

This revision now requires changes to proceed.Tue, Dec 10, 4:33 PM
zzag added a subscriber: zzag.Tue, Dec 10, 4:53 PM
zzag added inline comments.
input.h
233

From Frameworks coding style wiki page

Exception: Function implementations, class, struct and namespace declarations always have the opening brace on the start of a line.

So, the opening brace must be on the next line.

romangg added inline comments.Tue, Dec 10, 4:58 PM
input.h
233

Are we doing it somewhere in KWin this way?

zzag added inline comments.Tue, Dec 10, 5:00 PM
input.h
233

Only in a few places... but I'd like to point out that putting the opening brace and the method prototype on the same line goes against the coding style.

romangg added inline comments.Tue, Dec 10, 5:09 PM
input.h
233

I haven't seen these places. I always see in KWin code opening brackets on same line in this case. So I would prefer to be consistent in deviating from coding style here. Note also that getters keyboard(), pointers(),... directly above this new getter have opening brackets on the same line.

In the end best would be to always have member function definitions in the cpp file. But for small getters it's really a hassle...

apol marked 6 inline comments as done.Tue, Dec 10, 5:35 PM
apol updated this revision to Diff 71218.Tue, Dec 10, 5:35 PM

more coding style dance

romangg accepted this revision.Tue, Dec 10, 6:31 PM

Looks good now. Some last style details to change, please fix them on commit.

libinput/connection.cpp
497

unnecessary many lines. group xTilt, yTilt etc.

502

Line still too long.

tablet_input.cpp
70

unnecessary many lines. group xTilt, yTilt etc.

107

unnecessary many lines. group xTilt, yTilt etc.

This revision is now accepted and ready to land.Tue, Dec 10, 6:31 PM
apol marked 6 inline comments as done.Tue, Dec 10, 8:55 PM
This revision was automatically updated to reflect the committed changes.
zzag added inline comments.Tue, Dec 10, 10:03 PM
input.h
233

There are places where we use the other style, but not that many. Hopefully, we'll fix these annoying style issues one day with a little help from beloved clang-format. ;-)

In the end best would be to always have member function definitions in the cpp file.

++