Implements the necessary classes to have proper support for the tablet and pen.
Doesn't implement yet the ring/slider/totem bits.
Details
Details
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
Diff Detail
- Repository
- R127 KWayland
- Branch
- master
- Lint
Lint Errors Excuse: zucchini Severity Location Code Message Error src/server/tablet_interface.h:80 Cppcheck unknownMacro Error src/server/tablet_interface.h:80 Cppcheck unknownMacro Error src/server/tablet_interface.h:80 Cppcheck unknownMacro Error src/server/textinput_interface.h:160 Cppcheck unknownMacro - Unit
No Unit Test Coverage - Build Status
Buildable 21581 Build 21599: arc lint + arc unit
| 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. | |
| autotests/server/test_tablet_interface.cpp | ||
|---|---|---|
| 103–119 | Put a single space before *. | |
| 227 ↗ | (On Diff #74217) | Align pointers to right. |
| src/server/display.h | ||
| 328 ↗ | (On Diff #74217) | Wrong pointer alignment + missing @since. |
| src/server/tablet_interface.cpp | ||
| 53 ↗ | (On Diff #74217) | Align pointers to right. |
| 366 ↗ | (On Diff #74217) | Put a single space before *. Display * const m_display; |
| 64 | No auto. | |
| 237–238 | Add braces. | |
| src/server/tablet_interface.h | ||
| 52 ↗ | (On Diff #74217) | Wrong pointer alignment. |
| 121 ↗ | (On Diff #74217) | 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 ↗ | (On Diff #74217) | Put a single space before *. |
| 137–145 ↗ | (On Diff #74217) | Align pointers to right. |
Comment Actions
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.
Comment Actions
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 ↗ | (On Diff #74217) | The opening brace must be on its own line. |
| 371 ↗ | (On Diff #74217) | SeatInterface *seat = SeatInterface::get(seat_resource); |
Comment Actions
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
Comment Actions
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.
| src/server/tablet_interface.cpp | ||
|---|---|---|
| 347 ↗ | (On Diff #74217) | We're mixing up hardwareId and hardwareSerial this method is called toolByHardwareId calls an internal method called hardwareSerial Yet ultimately does check: comparing it against a serial |
| autotests/server/test_tablet_interface.cpp | ||
|---|---|---|
| 2–20 ↗ | (On Diff #74217) | Note that we've switched to SPDX markers. |
| src/server/tablet_interface.cpp | ||
|---|---|---|
| 59 ↗ | (On Diff #74217) | This is still potentially crashy: 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. |
| src/server/tablet_interface.cpp | ||
|---|---|---|
| 59 ↗ | (On Diff #74217) | or if we do want to keep the TabletInterface around till clients gracefully remove their side |