-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Added basic Pen support for X11 #19910
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
MrJul
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple nit but the logic looks good, thank you!
However, I have no pen to currently test this.
src/Avalonia.X11/XI2Manager.cs
Outdated
| UpdateKnownValuator(); | ||
| } | ||
|
|
||
| public bool HasPenEvaluators() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this also works for touch. I understand that it's used only to distinguish between mouse and pen but this isn't obvious at first glance. Rename to something like HasPressureValuator (or remove completely and check for PressureXIValuatorClassInfo directly since it's public).
| { | ||
| if (xiValuatorClassInfo.Label == pressureAtom) | ||
| if (xiValuatorClassInfo.Label == pressureAtom || | ||
| xiValuatorClassInfo.Label == pressureAtomPen) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for a device to have both Abs Pressure and Abs MT Pressure? I don't think so, but if that's the case we should probably prefer the second one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Abs MT Pressure is specifically for multitouch. The master device can't have both at the same time, it will issue a DeviceChange event when switching between touch and pen.
i. e. this is the event sequence for my pen-aware touchscreen (press finger, press with pen without releasing finger, release pen without releasing finger):
Slave switch: Wacom Pen and multitouch sensor Finger touch
TouchBegin
TouchEnd
Slave switch: Wacom Pen and multitouch sensor Pen stylus
LeftButtonDown
LeftButtonUp
Slave switch: Wacom Pen and multitouch sensor Finger touch
TouchBegin
TouchEnd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably need somebody to test with a separate tablet device, since TouchEnd is actually issued by the device driver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in general it still should do a slave switch like it does with touchpad/trackpoint/mouse
src/Avalonia.X11/XI2Manager.cs
Outdated
| XiEventType.XI_ButtonPress, | ||
| XiEventType.XI_ButtonRelease, | ||
| XiEventType.XI_Leave, | ||
| XiEventType.XI_Motion, XiEventType.XI_ButtonPress, XiEventType.XI_ButtonRelease, XiEventType.XI_Leave, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert the formatting changes here and on the field below.
|
Tested the PR, eraser detection doesn't seem to be implemented. I think you need to monitor the |
|
abe0f9b - this seems to work for eraser detection. |
|
I merged your eraser commit @kekekeks, I don't have eraser pen so I can't really test it, if it works, then great. I also did not implement Twist, as this is something my tablet also doesn't provide, so if yours do, I'd appreciate if you check the valuator name and values it provide. |
|
You can test this PR using the following package version. |
What does the pull request do?
This PR adds support for
PressureTwistXandTwistYfor X11 platform inside Move, Press and Release events.What is the current behavior?
Avalonia on Linux never reports Pen as pointer type and does not set
PressureandTwistproperties in mentioned pointer events.How was the solution implemented (if it's not obvious)?
Pressure and Twist events are simple atomics, I only added a few fields and read from evaluators. Pointer Type is determined based on presence of these evaluators.
I couldn't find information about Twist anywhere, my tablet (Huion H950P) doesn't seem to report it, or at least not without any additional drivers (no evaluator with relevant data is present).
While this PR does not provide full API (back eraser detection, twist), it's already better than having nothing on Linux. I'd be more than happy to implement missing elements.
Twist: I'd need an information about Twist evaluator name and value range provided by their tablet.
Back Eraser: I'd need a small hint where should I implement it, it's not part of RawPointerEvent, but processed PointerEventProperties, not sure where it is handled. I can then try implementing detection based on device name as mentioned in [X11] Some WACOM tablets are not supported #18873. My hardware doesn't support it, but the principle is simple enough to implement dry, then someone could confirm if it works properly.
Barrel buttons: Also, I'd use a hint where it is implemented too.
Checklist
Breaking changes
Obsoletions / Deprecations
Fixed issues
#18873