Support tablet-v2 #489
Draft
Support tablet-v2 #489
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Missing: pad support.
I’ve stubbed enough that it shouldn’t crash if one is added
(I feel like it should be possible to stub less,
but I didn’t know enough to do that!),
but since I don’t have such a thing to test or benefit from,
I’ve skipped it for now.
I have the feeling they’re not so common anyway?
My tablet has ten buttons, but they come through as keyboard, not pad.
And OpenTabletDriver doesn’t look to be able to produce pads.
There are still some improvements to make:
• I haven’t played with zwp_tablet_tool_v2.set_cursor yet;
it may warrant something extra.
• I want to add a drawing mode in the example, a trivial sketching app.
• I like the metadata and frame accumulators I made in the example.
I want to shift them into sctk itself as handlers or whatever,
because I think that’s going to be a pretty common desire.
• There are some places where I’m passing, say, a wl_seat,
because it was done in the code I was basing it off,
and I’m not sure if it’s useful or not.
Need to whittle the *Data things down, where possible.
• I’ve got a few TODOs from things that I just don’t know about.
Each and every one should be checked.
I want something *like* this, partly because it’s a more interesting demo for tablets, and partly because I want something showing overdrawing. The implementation is terrible in various ways that I know, and probably terrible in various ways that I don’t yet know. But it works… mostly. And it *normally* doesn’t segfault. I’m just committing it to get *something* out there for now, not because it’s finished. Pretty sure I’m going to ditch the mode stuff and put at least the tablet tool info, state and circle thing into a cursor. That’s cool.
This lets the user consume it in either way.
I’m not *certain* why tablet-v2 uses a burst of events for descriptions.
For frames it makes inherent sense,
as that’s often the way you want to view it,
and for some things close to necessary, if you want it to be meaningful,
but for the static device descriptions it doesn’t make *inherent* sense.
My guesses are:
(1) The information isn’t all available at once.
I don’t believe this is correct, or meaningfully so.
(2) Wayland protocol wire format limitations.
I don’t know if it can express a list of strings¹, like paths?
(3) Forwards-compatibility measure.
Is adding a new event to the burst easier than
adding a new argument to an existing event?
It would be good to know whether it’s just vagaries of Wayland,
in which case reconstructing the pieces is preferrable,
or whether there is enduring purpose to the fragmentation.
Effects of this change:
• Ergonomics: significant improvement for direct usage,
much of a muchness for abstraction layers wanting to use their own struct.
• Idle memory usage: reduced by 40 bytes per tablet.
• Runtime: will typically make one more allocation than before.
Fixable by changing paths from Vec<String> to SmallVec<[String; 1]>,
in which case runtime should overall be slightly better than before.
But I’m not sure if we want to expose SmallVec like that.
—⁂—
¹ <https://wayland.freedesktop.org/docs/html/ch04.html#sect-Protocol-Wire-Format:~:text=array>,
it’s just called “array” and it’s not clear to me what it can contain.
This stuff is known by diverse names in the spec:
• static information;
• static characteristics;
• static description;
And there may be more names I didn’t notice.
I think you could reasonably call it “metadata”, “info”, “description”,
maybe even “characteristics”; no need for the “static” part of the name.
In tablet tools, there’s state as well as the static information,
and access both at once (e.g. pressure and whether pressure-capable).
The name “state” is easy there, but since you’re touching both at once,
the name for this bit becomes a little more important.
My thoughts:
• “info” feels just a *little* too vague;
• “metadata” feels a *little* too confusable with sctk’s “data”,
though on the other hand that’s largely invisible to users;
• “characteristics” is too long, not elegantly shortenable,
and the English word only really feels appropriate for capabilities,
not names, paths and serial numbers;
• “description” is longer than I’d like,
and if you shorten it it becomes more confusable with “descriptor”.
I’m not fond of some of these names.
I’m not fond of deep paths.
Yet I think that overall this restructuring of access is an improvement.
A little.
Maybe it needs to go further.
The trouble starts with tablet-v2’s own names for things.
Everything is zwp_tablet_*_v2… except zwp_tablet_v2.
If you viewed “tablet” as the namespace,
it’d mean you have types Manager, Seat, Tool, Pad, PadGroup, PadRing and PadStrip…
and a type that’s not called Tablet because it actually just has no name.
A part of me wishes it was actually named zwp_tablet_tablet_v2.
It gets worse when you have this notion of a “tablet seat”.
At present I have modules named sctk::seat::tablet::seat and sctk::seat::tablet::tablet.
I find this rather amusing, but freely admit it to be suboptimal.
Maybe we need a different convention.
As a general rule in Rust, you import types and traits individually,
functions by module, constants vary,
and you mostly avoid duplication of the module name in type names.
Across a large code base you may get many things called “Error”,
and if you want to deal with more than one, you rename the import.
But there are some places where a different convention holds:
you don’t import each thing from std::fmt, but just import the module,
and `impl fmt::Display`, referring to fmt::Formatter and fmt::Result.
If I controlled *all* the naming, I’d probably structure things so:
sctk::seat::{
TabletManager (mismatching sctk’s *State custom,
but matching zwp_tablet_manager_v2,
and notably not exposed in a submodule),
tablet_seat::{Data (hidden), Handler},
tablet::{
Data (hidden),
Description (accumulator for the initial burst of events),
Event,
Handler,
},
tablet_tool::{
Capability (reexported enum),
Data (hidden),
Description (accumulator for the initial burst of events),
Event,
EventList (maybe gone?),
Frame,
Handler,
InitEvent,
InitEventList (maybe gone?),
State (optional accumulator of frames,
nothing to do with sctk’s *State convention),
Type (reexported enum),
},
tablet_pad::{
I dunno, not so comfortable with the further namespacing here,
we have pad, pad group, pad ring and pad strip.
Might even rename the module just pad.
},
}
And users would generally be expected to use it like this:
use sctk::seat::{TabletManager, tablet_seat, tablet, tablet_tool, tablet_pad};
impl tablet_tool::Handler for … {
fn …(
…,
events: Vec<tablet_tool::Event>,
) {
if self.tools.get(tablet).type == tablet_tool::Type::Pen {
Another possibility would be putting everything inside sctk::seat::tablet:
sctk::seat::tablet::{
Manager, (spelling: tablet::Manager)
seat::{Data, Handler}, (spelling: tablet::seat::Handler)
tool::{…}, (spelling: tablet::tool::Handler)
// Tablet itself is messy.
// One possibility (spelling: tablet::tablet::Handler):
tablet::{…},
// Another possibility (spelling: tablet::Event):
// Not so fond of this, especially if it led to tablet::State!
…,
// One possibility:
pad::{…},
pad_group::{…},
pad_ring::{…},
pad_strip::{…},
// Another possibility:
pad::{
…,
group::{…},
ring::{…},
strip::{…},
},
}
But tablet::tool::Handler is feeling a little icky,
and then we’re back with sctk::seat::tablet::seat.
There are at least three sets of customs: general Rust, Wayland, sctk.
It’s not possible to satisfy all three,
and at places it may be a struggle to satisfy even two.
I think we’re going to be breaking someone’s custom, somewhere,
however we do it.
This matches Wayland conventions a little better, Rust conventions better in some ways and a little worse in others, and sctk conventions worse. I won’t describe the module shifts, they’re clear enough, and were messy because I wasn’t sure about flat or nested before. The in-module renames are mostly obvious X* → x::* changes. • A few Tool* (most were TabletTool*) have become tablet_tool::*. • TabletState → TabletManager (and it’s exported from sctk::seat). • TabletToolEventFrame → tablet_tool::Frame (dropped “Event”).
This started because I didn’t like tool_type, and contemplated r#type. I’m fed up with naming obvious single-field variants. So much pointless duplication. Type and Capability should clearly be tuple variants. Then I realised I don’t like the hardware_serial_ and hardware_id_ duplication. I kept the “hi” and “lo” names rather than going straight to (u32, u32); the names are probably just sufficiently meaningful to retain them.
Rather than giving the list of events, do the accumulation ourselves. This will normally be faster. I didn’t just shift it from the example back into sctk, I also got a bit carried away with stupid microoptimisation. Without benchmarking, of course. Ah well, it’s pretty self-contained, so it shouldn’t be too bad.
OK, OK, you win, possibly-saner side of self. I dunno, though, makes things heavier for dubious gain.
1. Improved the example a lot:
• Draws for all stylus positions, not just one per rendered frame;
• Can finally reuse buffers, as I figured out what was going on,
why the buffers were still active.
(Answer: accidentally make N frame requests within a frame,
get N callbacks. I was rendering three times per frame.)
Computers are fast.
My code was doing such stupid things,
and yet there was no lag because of iit.
2. Changed tablet_tool quite a bit:
• Now just using the upstream Event enum for state events.
Less code, potentially more future-compatible,
• Guidance on stylus buttons now present.
• Started moving a couple more bits from the example into sctk,
with some modifications and fixes: the state accumulator,
and the tools info/state collection.
Not yet entirely sure how to handle it, but getting somewhere.
This should have been deleted a while ago, but I can’t be bothered editing the history because of restructurings.
• description → info, because I found another part of sctk using that,
which tips the balance enough for me
(wasn’t fond of “description”’s length, anyway).
• init_done → info, for feeling consistent, maybe
• tablet_tool_frame → frame, for feeling consistent, maybe
Add a little, remove a little.
Honestly I’m not sure if even the seat is worthwhile. If the user cares, they *can* map tablet_seat → seat themselves.
Not sure if this is a good idea or not. I would like advice. I’m quite content to revert it. It’s perfectly *possible* for the user to map tablet seat to seat, as they have both directly after the get_tablet_seat call.
I think this is pretty close to what people will want now.
I came via passing around &mut Option<State> and such,
but realised it was better to stop blatting *all* the state,
and just shift the proximity_in information into an optional substruct.
This lets things be just methods on State, for much greater consistency,
and the scoping makes it easier to ignore State if you don’t want to use it, too.
I didn’t like the Tools map, it was too inflexible.
So, ditched it in favour of making the user choose the map and struct.
It’s much of a muchness on boilerplate, and simpler, which is a virtue.
Now you see a Tool struct which can have more per-tool fields added.
This is good.
It’s funny how you can come back to earlier designs like this.
This is very similar to what I had a few days ago,
but it was just a *little* different then, in bad ways.
Now I’ve got the best of all worlds I’ve thought of so far.
—⁂—
I’m still mulling over the udata pattern,
which would probably be more consistent with the rest of sctk.
I’m not entirely sold on shifting things into there,
for a couple of initial reasons:
• I’m not sure if State can be optional quite so well—
in order to avoid overhead if it’s not being used, it wants
relaxed coherence, negative bounds, specialisation or similar.
• If Info remains in the data, it must be in a mutex.
Mutex guards are nasty to deal with, and compromise API.
Only a custom cursor, haven’t tried a themed cursor yet. Reaching the stage where I’m completely happy with this as a demo.
Is it better? Dunno. But it feels *maybe* warranted. One thing I’m feeling is that argument ordering is inconsistent: in some places qh is at the start, in other places at the end. Am I missing a pattern, or do we need to go revise methodically?
Pointer has CSM all wrapped up tight, but tablet doesn’t yet. Exposing CSM is the easiest approach for now.
Its logging is comparatively important for grasping what’s going on, and wanted a little more care put into it.
This should make it possible (untested), but it’s clearly not finished yet.
Almost forgot! Important for forwards-compatibility, e.g. this new bus type thing on tablet which I haven’t added yet.
wash2
reviewed
Apr 2, 2025
There was a problem hiding this comment.
- I’ve got a different pattern going on from the normal udata, and I think it’s noticeably nicer in some regards, but it’s also probably a bad idea. Some remarks are present in commit messages and maybe comments.
If I've understood correctly, I guess the main drawback I think of is that the info is only available in the event really, instead of via a method on the Data? So, users would have to add this to their state if they want to use it after the info method. I don't think it is too bad.
I’ve named things somewhat differently from upstream, broadly speaking foo::{X, Y, Z} because foo::{FooX, FooY, FooZ} gets tedious here. I think it’s an improvement, see how it looks in the example. Honestly I’d suggest doing this more generally, but I think it’s justifiable even as a tablet-local convention, because the names get unwieldy.
Ya, for now it would be fine as just a tablet-local convention.
Tablet pads are not supported (but are plumbed just enough that their presence shouldn’t crash it).
This looks good to me, but unfortunately I don't have anything to test it with, or really the rest of the PR, to be honest.
src/seat/tablet_manager.rs
Outdated
| { | ||
| let udata = super::tablet_seat::Data { wl_seat: seat.clone() }; |
There was a problem hiding this comment.
This is fine in my opinion. As you say, it is possible for the user to map tablet set to seat.
| /// Sent when the tablet has been removed from the system. | ||
| /// When a tablet is removed, some tools may be removed. | ||
| /// | ||
| /// This method is responsible for running `tablet.destroy()`. ← TODO: true or not? |
There was a problem hiding this comment.
It would be a good time to destroy the object, but I suppose it isn't strictly responsible for it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is still a work in progress. Some remarks:
foo::{X, Y, Z}becausefoo::{FooX, FooY, FooZ}gets tedious here. I think it’s an improvement, see how it looks in the example. Honestly I’d suggest doing this more generally, but I think it’s justifiable even as a tablet-local convention, because the names get unwieldy.Took me a while to figure out various things, and on account of working various things out I went back and changed lots more, but it’s reached the stage where I’m mostly happy with it. The commit messages will at times be useful, but the history is inevitably a bit of a mess, for such an exploratory thing (this was my first tangle with programming against Wayland).
Things work reliably for me on my XP-Pen Artist 16 (2nd Gen).
Fixes #139.
Now I can get on with making my own app that uses this…