-
Notifications
You must be signed in to change notification settings - Fork 119
Draft ext-input-triggers #4328
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: main
Are you sure you want to change the base?
Draft ext-input-triggers #4328
Conversation
Saviq
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.
I see this is still a draft, but just leaving some notes on it.
47bbff0 to
7ef9f66
Compare
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.
Activation part is mostly fine except handling oneshot/continuous actions, but I think registration still needs more discussion
627266b to
b36df35
Compare
tarek-y-ismail
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 few minor nits, but I think it might be good enough for a quick spike.
759ea0c to
6489d90
Compare
f1620c2 to
f60b0cf
Compare
Addresses feedback from #4328 where the protocol specification stated the compositor "should raise a protocol error" without defining which error to raise. **Changes:** - Added `invalid_token` error enum to `ext_input_trigger_action_manager_v1` interface - Updated `get_input_trigger_action` request description to prescriptively specify when the `invalid_token` error must be raised The error is raised when a client provides a token that was not issued by the compositor, following the pattern established in other Wayland protocols like `ext-data-control-v1` and `ext-image-copy-capture-v1`.
|
So I used Copilot as a rubber duck, here's a quick "summary" of the points it brought up about the protocol and my opinions/responses. Apologies in advance for the wall of text.
It's worth discussing whether we want to allow just one modifier in this request, or multiple ones. The use case I recall from when we discussed this was tapping both the left and right shifts to toggle mute, which would require us to specify two modifiers. High priority semantic and security issues
Tokens are mostly meant to be opaque strings similar to tokens used in ext_activation_v1.
This is true. Do we want to take this into account for v1?
I think we discussed this before. We settled on this being compositor-defined.
We should clear this up. From memory, we decided that all inputs will be delivered to the focused client as usual. Once a registered trigger is invoked, that input will be consumed by the compositor and diverted to the listening client. Implementation details: As @RAOF mentioned before, the modifier state will need to be reset by synthesizing events to clear the pressed modifiers so the focused client is not confused when focus returns to it. Example,
Is this even possible? Unless you have multiple input devices (keyboards / mice), I don't think it is.
I think we should specify this. For us, our clocks are usually relative to the UNIX epoch, right? I don't think we need to worry about wrap-around. And ordering of the events should match the Wayland event order so that's also a non-issue.
We should clear up the usage of the token. From the API, it's obviously not nullable/optional, so that's not a concern. We should specify that tokens are issued by the compositor and expire after a certain (compositor-defined) period of time. Medium priority design omissions / missing error handling
Maybe we want to explore this a bit. I believe we discussed the conflict case before and settled on leaving it up to the DE maintainers to handle conflicts between the various previliged clients externally. Though, this doesn't rule out the user modifying a trigger and accidentally using one that's already taken. Which in this case would just call
Again, I believe we discussed this before and agreed that it should be up to the compositor to define these to ensure a consistent experience.
Compositor defined, again.
I think Copilot got a bit confused, but this probably means we need to clarify things a bit. Code triggers should not change with the layout, but sym triggers should. Though the more I think about it, the more I see sym triggers as superfluous, what was their use case again?
Fair point. I think this should also be compositor-defined to ensure consistency.
I agree with this. I confuse all three quite a bit. Lower priority / niceties / improvements
I just had to handle this in the spike. I don't have a use case in mind where repeat would be that useful, so I believe we should specify that it's should not emit
Fair point, my opinion at the moment is that 1.0 must be sent.
Again, I remember that we agreed that this should not be a part of these two protocols.
This might be a minor issue for applications that want to show button icons instead of
I don't think virtual devices or OSKs should trigger global actions.
We talked about this before as well. "Priviliged clients" can mean clients launched explicitly by the compositor. In which case we would advertise the globals. Otherwise, we don't. |
`ext_input_trigger_action_control_v1.done`
only be used for comparisons/deltas.
Thinking about this again, maybe this isn't the best idea as it might introduce some unwanted jank. For example, if a user is moving to the right workspace via a gesture, the DE might have a threshold of 0.5 progress after which the move is done whether the gesture is cancelled or not. Requiring compositors to send 1.0 will just introduce complexity as they'll have to handle and ignore sudden jumps in progress. |
41c94a9 to
3599e06
Compare
| <event name="activated"> | ||
| <description summary="activate the input action"> | ||
| The input trigger has been activated. This can be on the leading or | ||
| trailing edge of the input being pressed. It's up to the user to | ||
| configure this as they see fit. | ||
|
|
||
| The app may interpret this as a request to perform the action it | ||
| associates with the trigger. | ||
|
|
||
| The time argument is a timestamp with millisecond granularity, with an | ||
| undefined base. clients must not assume any relation to wall-clock | ||
| time; timestamps are only valid for comparison/deltas. | ||
|
|
||
| The activation token argument is a short-lived opaque token that may be | ||
| used with `xdg_activation_v1` to request to focus another surface. | ||
| </description> | ||
| <arg name="time" type="uint" summary="timestamp with millisecond granularity"/> | ||
| <arg name="activation_token" type="string" summary="an xdg_activation token"/> | ||
| </event> | ||
|
|
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 we decided against introducing that (yet)?
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.
Yeah that was shorty after I updated the PR. Haven't gotten around to it again, sorry
TICS Quality Gate✔️ PassedNo changed files applicable for TICS analysis quality gating. TICS / TICS / Run TICS analysis |
39740d9 to
43ac49a
Compare
No description provided.