Implement the tablet wayland protocol in kwin
ClosedPublic

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

Details

Summary

Uses the tablet classes introduced in kwayland.
Depends on D26858

Test Plan

Scratched my tablet with a magic stick and it did things depending on the pressure.
https://youtu.be/GGx0TlNJlzs

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.
apol created this revision.Jan 23 2020, 12:59 AM
Restricted Application added a project: KWin. · View Herald TranscriptJan 23 2020, 12:59 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
apol requested review of this revision.Jan 23 2020, 12:59 AM
zzag requested changes to this revision.Jan 23 2020, 9:15 AM
zzag added a subscriber: zzag.

Good stuff! There are a couple of coding style issues.

autotests/libinput/mock_libinput.h
108

I know that you want to match libinput_device_tablet_pad_get_{value}, but please follow coding style.

uint32_t buttonCount = 0;
input.cpp
1563
1774

No short names.

Toplevel *toplevel = ...
1775–1776

Should have braces.

input_event.h
183

It's off by a single space.

input_event_spy.h
88

Align pointers and reference to right.

libinput/connection.cpp
555–557

Please don't submit commented code unless there is a reason.

This revision now requires changes to proceed.Jan 23 2020, 9:15 AM
apol marked 5 inline comments as done.Jan 23 2020, 11:50 AM
apol updated this revision to Diff 74220.Jan 23 2020, 11:51 AM

address zzag's comments

zzag requested changes to this revision.Jan 23 2020, 12:30 PM
zzag added inline comments.
input.cpp
1572

Use explicit type

const char *devnode = udev_device_get_devnode(udev_device);
1634

Align pointers to right.

1636–1639

No short names.

Toplevel *toplevel = input()->findToplevel(event->globalPos());
if (!toplevel || !toplevel->surface()) {
    return false;
}
1641

Align pointers to right.

1644–1645

Add missing braces.

2099–2100

Please put a single space between for and the opening parenthesis, add missing braces and use explicit type.

input_event_spy.cpp
124

Align pointers to right.

This revision now requires changes to proceed.Jan 23 2020, 12:30 PM
apol marked 6 inline comments as done.Jan 23 2020, 2:29 PM

Addressed the comments. I hope that all that really is a problem with the patch is styling issues...

apol updated this revision to Diff 74232.Jan 23 2020, 2:29 PM

zzag's styling

apol updated this revision to Diff 74233.Jan 23 2020, 2:30 PM

forgot to save

apol updated this revision to Diff 74235.Jan 23 2020, 2:36 PM

fix build

apol updated this revision to Diff 74240.Jan 23 2020, 2:54 PM

Adapt to changes in kwayland

apol updated this revision to Diff 74471.Jan 28 2020, 2:56 AM

clang format

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

clang format

zzag accepted this revision.Jan 28 2020, 11:01 AM
zzag added a subscriber: davidedmundson.

Code-wise, +1

Please wait for a +1 from @davidedmundson

input.cpp
1577

Naming nitpick: maybe addDevice?

input_event.h
193

Make it const?

This revision is now accepted and ready to land.Jan 28 2020, 11:01 AM
davidedmundson added inline comments.Feb 4 2020, 11:01 AM
input.cpp
1599

I think this should be event->uniqueId()

apol updated this revision to Diff 75560.Feb 12 2020, 4:05 PM

rebase on master

apol added inline comments.Feb 19 2020, 12:02 AM
input.cpp
1599

We are using the same value as the second argument of addTablet() (se below). If we change here the serialId, we'd have to change it there too and we'd have the same behaviour.

davidedmundson requested changes to this revision.Tue, Mar 17, 11:05 AM

We are using the same value as the second argument of addTablet() (se below). If we change here the serialId, we'd have to change it there too and we'd have the same behaviour.

You might be right about serialId having read the docs, but I'm still seeing waaaaaay too many:

zwp_tablet_seat_v2.tool_added()

one for every move.

I have kwin on master + this patch, and master of kwayland.

I'll try and debug and find out.

This revision now requires changes to proceed.Tue, Mar 17, 11:05 AM

There's also a crash on tablet disconnect

#0  0x00007f9962c49ce5 in raise () at /usr/lib/libc.so.6
#1  0x00007f9962c33857 in abort () at /usr/lib/libc.so.6
#2  0x00007f9963357faf in qt_message_fatal(QtMsgType, QMessageLogContext const&, QString const&) (context=..., message=...)
    at /home/david/projects/qt/src/qtbase/src/corelib/global/qlogging.cpp:1914
#3  0x00007f99633545e6 in QMessageLogger::fatal(char const*, ...) const
    (this=0x7ffdb72ca9f0, msg=0x7f9963719ef0 "ASSERT failure in %s: \"%s\", file %s, line %d")
    at /home/david/projects/qt/src/qtbase/src/corelib/global/qlogging.cpp:893
#4  0x00007f996334adf3 in qt_assert_x(char const*, char const*, char const*, int)
    (where=0x7f9965176763 "zwp_tablet_v2::removed", what=0x7f99651756ba "Uninitialised resource", file=0x7f9965175653 "src/server/qwayland-server-tablet-unstable-v2.cpp", line=1229) at /home/david/projects/qt/src/qtbase/src/corelib/global/qglobal.cpp:3362
#5  0x00007f9965156770 in QtWaylandServer::zwp_tablet_v2::send_removed() (this=0x56487ab837c0)
    at src/server/qwayland-server-tablet-unstable-v2.cpp:1229
#6  0x00007f996511c79d in KWayland::Server::TabletSeatInterface::removeTablet(QString const&) (this=0x56487ab9cdb0, sysname=...)
    at /home/david/projects/kde5/src/frameworks/kwayland/src/server/tablet_interface.cpp:341
#7  0x00007f9967e1c3a4 in KWin::TabletInputFilter::removeDevice(QString const&) (this=0x56487acc7f50, sysname=...)
    at /home/david/projects/kde5/src/kde/workspace/kwin/input.cpp:1578
#8  0x00007f9967e270a3 in QtPrivate::FunctorCall<QtPrivate::IndexesList<0>, QtPrivate::List<QString>, void, void (KWin::TabletInputFilter::*)(QString const&)>::call(void (KWin::TabletInputFilter::*)(QString const&), KWin::TabletInputFilter*, void**) (f=
    (void (KWin::TabletInputFilter::*)(KWin::TabletInputFilter * const, const QString &)) 0x7f9967e1c378 <KWin::TabletInputFilter::removeDevice(QString const&)>, o=0x56487acc7f50, arg=0x7f99540179c8) at /opt/qt5/include/QtCore/qobjectdefs_impl.h:152
apol updated this revision to Diff 77840.Tue, Mar 17, 3:32 PM

Rebase and address issue commented by david

One minor comment, but I think this is good to go.

input.cpp
1664

Double check with vlad, but I think we want

toplevel->bufferGeometry().topLeft()

as input is relative to the buffer not the frame

modifier_only_shortcuts.cpp
62

seems unrelated but I assume this is fixing a teardown crash?

zzag added inline comments.Wed, Mar 18, 2:06 PM
input.cpp
1664

Yes, x and y in the motion event must be in the surface-local coordinates, i.e. relative to the upper left corner of the main surface.

zzag added inline comments.Wed, Mar 18, 2:17 PM
input.cpp
1664

btw, maybe it might be worth to introduce a method to map points from the global screen coordinates to surface-local coordinates, e.g.

QPoint Toplevel::mapToLocal(const QPoint &point) const
{
    return point - bufferGeometry().topLeft();
}

QPointF Toplevel::mapToLocal(const QPointF &point) const
{
    return point - QPointF(bufferGeometry().topLeft());
}
apol updated this revision to Diff 77927.Wed, Mar 18, 2:53 PM

rebase

apol updated this revision to Diff 77944.Wed, Mar 18, 5:21 PM

Address position translation

apol updated this revision to Diff 77968.Thu, Mar 19, 2:37 AM

Don't use libinput when it's null

davidedmundson accepted this revision.Thu, Mar 19, 12:27 PM
This revision is now accepted and ready to land.Thu, Mar 19, 12:27 PM
This revision was automatically updated to reflect the committed changes.