Conversation
76a9c52 to
29f6da5
Compare
We could solve this in two ways: Explicitly require users to press the alpha key at the end, or manually keep track of the pressed keysyms, consuming only the event where all the keys the user specified are down, which seems to be what i3 does on my machine. For example:
Edit: I'm opting to implement the second solution |
4d9958a to
60cd1ab
Compare
41c94a9 to
3599e06
Compare
60cd1ab to
99780c8
Compare
f488d7b to
5e01560
Compare
|
Big TODO: The token authority should be modified to create a new token type ( Right now, tokens are used to store the relationship between a trigger, the input filter, and the corresponding action (that's what I remember, the architecture is a bit hazy in my head right now). So having tokens that are invalidated after a short time would require replacing this with something else. The first idea that pops up into my head is using the token to just establish the relationship between the three objects, but not holding onto it as I do right now. |
5531782 to
d880af4
Compare
7c58613 to
90f9c87
Compare
1cf3f74 to
f8c1c47
Compare
| input_trigger_registration_v1.cpp input_trigger_registration_v1.h | ||
| input_trigger_action_v1.cpp input_trigger_action_v1.h |
There was a problem hiding this comment.
Ugly indentation. But probably something for another PR
There was a problem hiding this comment.
Pull request overview
This PR implements a spike/prototype for an input trigger protocol that allows Wayland clients to register global keyboard shortcuts. The protocol consists of two parts: trigger registration (defining shortcuts) and action management (handling shortcut events).
Changes:
- Implements Wayland protocol extensions for input trigger registration and action management
- Adds support infrastructure including token-based action tracking and event filtering
- Includes example client and Python-based gatekeeper for testing the protocol
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/server/frontend_wayland/wayland_default_configuration.cpp | Registers the two new input trigger protocol extensions in the Wayland extension system |
| src/server/frontend_wayland/wayland_connector.h | Adds InputTriggerData and Clock dependencies to WaylandExtensions context |
| src/server/frontend_wayland/wayland_connector.cpp | Instantiates InputTriggerData and passes it to extension context |
| src/server/frontend_wayland/input_trigger_registration_v1.h | Defines protocol interfaces for trigger registration and KeyboardSymTrigger class |
| src/server/frontend_wayland/input_trigger_registration_v1.cpp | Implements registration manager, action controls, and modifier mapping logic |
| src/server/frontend_wayland/input_trigger_data.h | Defines shared data structure for tracking pending trigger actions |
| src/server/frontend_wayland/input_trigger_action_v1.h | Defines protocol interfaces for action management |
| src/server/frontend_wayland/input_trigger_action_v1.cpp | Implements action manager, keyboard event filtering, and modifier matching |
| src/server/frontend_wayland/CMakeLists.txt | Adds new source files to build system |
| src/common/input/xkb_mapper.cpp | Moves expand_modifiers function to namespace scope for external use |
| include/common/mir/input/xkb_mapper.h | Exports expand_modifiers function for use in trigger protocol |
| examples/trigger_client/main.cpp | Implements standalone C++ Wayland client to test trigger registration |
| examples/trigger_client/CMakeLists.txt | Build configuration for trigger_client example |
| examples/gatekeeper/test_client.py | Python test client using D-Bus portal interface |
| examples/gatekeeper/generate_protocols.py | Script to generate Python bindings for Wayland protocols |
| examples/gatekeeper/gatekeeper.py | Python-based UI for managing global shortcuts via D-Bus portal |
| examples/CMakeLists.txt | Includes trigger_client subdirectory in build |
| debian/mir-demos.install | Adds trigger_client to package install list |
| rpm/mir.spec | Adds trigger_client to RPM package |
| CMakeLists.txt | Adds coverage build configuration and fixes platform path in LGPL test |
f8c1c47 to
5b72aeb
Compare
tarek-y-ismail
left a comment
There was a problem hiding this comment.
Quick review for the gatekeeper and test client
Gatekeeper is a bit spaghettified, test client is alright.
examples/gatekeeper/gatekeeper.py
Outdated
| # Add current directory to path to find generated protocols | ||
| sys.path.append(os.path.dirname(os.path.abspath(__file__))) |
There was a problem hiding this comment.
Is this necessary?
examples/gatekeeper/gatekeeper.py
Outdated
| # Minimal Keysym definitions (XKB) | ||
| KEY_A = 0x0061 | ||
| # ... add more as needed | ||
|
|
||
| MOD_SHIFT = 0x08 | ||
| MOD_CTRL = 0x100 | ||
| MOD_ALT = 0x01 | ||
| MOD_META = 0x800 |
There was a problem hiding this comment.
Hmmm, maybe there's a library to handle this?
examples/gatekeeper/gatekeeper.py
Outdated
| lbl = Gtk.Label(label="An application wants to register the following shortcuts:") | ||
| box.append(lbl) |
There was a problem hiding this comment.
Unnecessary variable.
| lbl = Gtk.Label(label="An application wants to register the following shortcuts:") | |
| box.append(lbl) | |
| box.append(Gtk.Label(label="An application wants to register the following shortcuts:")) |
examples/gatekeeper/gatekeeper.py
Outdated
| class GlobalShortcutsPortal: | ||
| def __init__(self, app): | ||
| self.app = app | ||
| self.sessions = {} # session_handle -> { 'app_id': str, 'shortcuts': dict, 'sender': str } |
There was a problem hiding this comment.
If we need a comment with the structure, might as well use type hints.
examples/gatekeeper/gatekeeper.py
Outdated
| # [CHANGE] Added sender argument | ||
| def CreateSession(self, request_handle, session_handle, app_id, options, sender): | ||
| logger.info(f"CreateSession: {session_handle} for {app_id} (Sender: {sender})") | ||
| self.sessions[session_handle] = { | ||
| 'app_id': app_id, | ||
| 'shortcuts': {}, | ||
| 'sender': sender # [CHANGE] Store the sender |
There was a problem hiding this comment.
Leftovers from when I was vibing this
| # [CHANGE] Added sender argument | |
| def CreateSession(self, request_handle, session_handle, app_id, options, sender): | |
| logger.info(f"CreateSession: {session_handle} for {app_id} (Sender: {sender})") | |
| self.sessions[session_handle] = { | |
| 'app_id': app_id, | |
| 'shortcuts': {}, | |
| 'sender': sender # [CHANGE] Store the sender | |
| def CreateSession(self, request_handle, session_handle, app_id, options, sender): | |
| logger.info(f"CreateSession: {session_handle} for {app_id} (Sender: {sender})") | |
| self.sessions[session_handle] = { | |
| 'app_id': app_id, | |
| 'shortcuts': {}, | |
| 'sender': sender |
examples/gatekeeper/test_client.py
Outdated
| from gi.repository import Gio, GLib | ||
|
|
||
| # XML for the interface we (the client) will expose to receive events | ||
| # [FIXED] Removed session_handle argument to match Gatekeeper's call signature (sua{sv}) |
There was a problem hiding this comment.
| # [FIXED] Removed session_handle argument to match Gatekeeper's call signature (sua{sv}) |
examples/gatekeeper/test_client.py
Outdated
| def on_client_method_call(self, connection, sender, object_path, interface_name, method_name, parameters, invocation): | ||
| # Handle the callbacks from Gatekeeper | ||
| if method_name == "Activated": | ||
| # [FIXED] Unpack 3 arguments: shortcut_id, timestamp, options |
There was a problem hiding this comment.
| # [FIXED] Unpack 3 arguments: shortcut_id, timestamp, options |
examples/gatekeeper/test_client.py
Outdated
| invocation.return_value(None) | ||
|
|
||
| elif method_name == "Deactivated": | ||
| # [FIXED] Unpack 3 arguments |
There was a problem hiding this comment.
| # [FIXED] Unpack 3 arguments |
examples/gatekeeper/test_client.py
Outdated
| def setup_client_listener(self): | ||
| # We simply register the object. Gatekeeper knows our bus unique name (e.g. :1.99) | ||
| # because it captured it when we called CreateSession. | ||
| session_path = "/org/freedesktop/portal/desktop/session/test_client/1" |
There was a problem hiding this comment.
This probably means we can't have multiple test clients?
|
Ok, I think this is ready to review now. One thing that could be improved is that the data inside |
AlanGriffiths
left a comment
There was a problem hiding this comment.
OK as a throwaway spike: it works without too much pain.
But for maintainable code that we want to land:
- There are apparently spurious changes to xkb_mapper;
- poor encapsulation of logic; and,
- a failure to leverage the threading mode.
0c183a1 to
ca48108
Compare
69fa308 to
3b07faa
Compare
validity and availability Co-authored-by: Alan Griffiths <alan@octopull.co.uk>
mattkae
left a comment
There was a problem hiding this comment.
I only have a few comments, but looks sensible!
|
Bug I just thought. Throwing it here before I forget. If key release order is not the same as key press order, clients will see inconsistent key states. For example with Will probably have to synthesize events in this case. Edit: After some contemplation, I think the following is in order:
The high level overview is that Edit: handled in 70355cf |
|
Got some commits stuck. Can't push right now. Will see to it later. |
| void InputTriggerActionControlV1::cancel() | ||
| { | ||
| action_group->cancel(); | ||
| delete this; |
There was a problem hiding this comment.
I wasn't originally sure if this was the right call or not. The thunk code does look like it calls wl_resource_destroy which I will assume handles freeing everything. Will leave it unresolved if anyone wants to chime in.
| delete this; | ||
| } | ||
|
|
||
| void InputTriggerActionControlV1::destroy() | ||
| { | ||
| delete this; |
| bool mf::InputTriggerRegistry::register_trigger(Trigger* trigger) | ||
| { | ||
| // Housekeeping | ||
| std::erase_if(triggers, [](auto const& weak_trigger) { return !weak_trigger; }); | ||
|
|
||
| auto const already_registered = sr::any_of( | ||
| triggers, [trigger](auto const& other_trigger) { return trigger->is_same_trigger(&other_trigger.value()); }); | ||
|
|
||
| if (already_registered) | ||
| return false; | ||
|
|
||
| triggers.push_back(wayland::make_weak(trigger)); | ||
| return true; |
There was a problem hiding this comment.
This depends on whether we should handle generic + sided modifiers being specified, which is currently being discussed in #4410 (comment)
mattkae
left a comment
There was a problem hiding this comment.
Please consider my feedback, but I will not block :)
TICS Quality Gate✔️ Passedmir
|
What's new?
Initial implementation of the input triggers protocols. Allows privileged clients to reserve a key combination (modifiers + symkey/keycode) and be notified whenever that combination is pressed/released, regardless of which client has focus.
How to test
Direct communication with privileged client:
--add-wayland-extensions=ext_input_trigger_action_manager_v1:ext_input_trigger_registration_manager_v1./build/bin/trigger_clientCtrl + Shift + CandALT + xfor key syms, andAlt + zfor scan codes. If you press any of them. "Hey from " will be printedTo test
Alt + z, run mir with--keymap fr. This should use the azerty layout and make thezkey printw, and theqkey printz. Even in the azerty layout,Alt + zwith the azerty z will not work, but the same combination with the qwerty z will. Note: Right alt is used for special letters, so use left alt :)Gatekeeper:
--add-wayland-extensions=ext_input_trigger_action_manager_v1:ext_input_trigger_registration_manager_v1python3 examples/gatekeeper/gatekeeper.py --show-uipython3 examples/gatekeeper/test_client.pybeginandendare received.Checklist