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
TICS Quality Gate✔️ Passedmir
|
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?
| /// Tracks keyboard state shared among all keyboard event filters | ||
| class KeyboardStateTracker | ||
| { | ||
| public: | ||
| KeyboardStateTracker() = default; | ||
|
|
||
| /// Update pressed keysyms on key down | ||
| void on_key_down(uint32_t keysym); | ||
|
|
||
| /// Update pressed keysyms on key up | ||
| void on_key_up(uint32_t keysym); | ||
|
|
||
| /// Check if a keysym exists in the pressed set | ||
| auto keysym_is_pressed(uint32_t keysym, bool case_insensitive) const -> bool; | ||
|
|
||
| /// Check if protocol modifiers match event modifiers | ||
| static auto protocol_and_event_modifiers_match(uint32_t protocol_modifiers, MirInputEventModifiers event_mods) | ||
| -> bool; | ||
|
|
||
| private: | ||
| std::unordered_set<uint32_t> pressed_keysyms; |
There was a problem hiding this comment.
Not sure if the comments being in a different style is reason enough to change them...
|
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.
There was a problem hiding this comment.
This branch is called "a spike", and spike are intended for learning, not for production. In my experience (and that of others), it is usually better to start afresh with the knowledge learnt rather than try to remove all trace of the learning process. You usually end up with better results faster.
Practically speaking, 76 commits will be a PITA if ever bisecting a problem with these changes. Can we, at least, rework the history to remove dead experiments? (It doesn't really need more than three, and one wouldn't be terrible.)
There are no tests of any of this: There are clearly elements that can be tested in isolation (KeyboardStateTracker & BoundedQueue for a start).
PS
I didn't get my head fully around this yet, so don't assume I'm content with things I've not commented on
I may have gotten slightly attached to the code. But I believe that even if I started fresh, the final outcome would be very similar. (This is my inexperience speaking, I know)
That was always my intention once I got the thumbs up 👍, as usual, I stop rebasing once others start reviewing my code.
Fair point. Just making sure the overall picture is okay. I originally intended on testing this mainly in WLCS in an upcoming PR, but as you pointed out, there are things I could also test here.
Understandable. Will fix the things you pointed out and wait for more feedback. |
That's the difference in mindset between writing a spike and writing production code: a spike explores the solutions space and clarifies the picture, for production code we write tests of the behaviour required to complete that picture and ensure we write code to pass them. |
| /// Check if this trigger matches the current keyboard state | ||
| virtual auto matches(std::shared_ptr<KeyboardStateTracker> const& keyboard_state, MirInputEventModifiers event_mods) const -> bool = 0; | ||
|
|
||
| MirInputEventModifiers const modifiers; |
There was a problem hiding this comment.
Not sure if it's worth going through the code and making this keysym, and scancode private. They are const so I don't see a strong reason to make them private. Opinions welcome.
| keyboard_trigger->send_done_event(); | ||
| } | ||
|
|
||
| // TODO: Store the description string |
There was a problem hiding this comment.
I have a comment earlier here asking whether or not we actually want to store the description. So far I can't think of a use.
|
I wonder why this PR is failing |
tarek-y-ismail
left a comment
There was a problem hiding this comment.
Too much cruft. Will continue reviewing tomorrow.
src/server/frontend_wayland/input_triggers/input_trigger_action_v1.cpp
Outdated
Show resolved
Hide resolved
src/server/frontend_wayland/input_triggers/input_trigger_data.cpp
Outdated
Show resolved
Hide resolved
src/server/frontend_wayland/input_triggers/input_trigger_data.cpp
Outdated
Show resolved
Hide resolved
src/server/frontend_wayland/input_triggers/input_trigger_data.cpp
Outdated
Show resolved
Hide resolved
src/server/frontend_wayland/input_triggers/input_trigger_data.cpp
Outdated
Show resolved
Hide resolved
src/server/frontend_wayland/input_triggers/input_trigger_data.cpp
Outdated
Show resolved
Hide resolved
src/server/frontend_wayland/input_triggers/input_trigger_data.h
Outdated
Show resolved
Hide resolved
src/server/frontend_wayland/input_triggers/input_trigger_data.h
Outdated
Show resolved
Hide resolved
src/server/frontend_wayland/input_triggers/input_trigger_data.h
Outdated
Show resolved
Hide resolved
src/server/frontend_wayland/input_triggers/input_trigger_data.h
Outdated
Show resolved
Hide resolved
src/server/frontend_wayland/input_triggers/input_trigger_registration_v1.cpp
Outdated
Show resolved
Hide resolved
tests/unit-tests/frontend_wayland/test_protocol_modifier_match.cpp
Outdated
Show resolved
Hide resolved
tests/unit-tests/frontend_wayland/test_protocol_modifier_match.cpp
Outdated
Show resolved
Hide resolved
tests/unit-tests/frontend_wayland/test_protocol_modifier_match.cpp
Outdated
Show resolved
Hide resolved
tests/unit-tests/frontend_wayland/test_protocol_modifier_match.cpp
Outdated
Show resolved
Hide resolved
6090fca to
2cc77c9
Compare
|
I know why the tests fail. It's because of the protocol and mir modifiers being out of sync and being implicitly convertible between each other. Will push some code tomorrow to add new types to guard against this. Edit: I may have slipped and "accidentally" slapped it together real quick 🤫 |
ad39cbb to
61feb6e
Compare
61feb6e to
bcd5c5d
Compare
AlanGriffiths
left a comment
There was a problem hiding this comment.
Looking a lot cleaner. Some high level nitpicking, will look in more detail next week
| if (!case_insensitive) | ||
| return pressed_keysyms.contains(keysym); | ||
|
|
||
| // Only perform case mapping for ASCII letters. For other keysyms, fall back to exact match. |
There was a problem hiding this comment.
Do we actually know the character set is ASCII? What if we're running on EBCDIC or Unicode16?
Maybe the intent was "Latin 1"?
| # Override the parent's fallback component with our own | ||
| # Use COMPILE_OPTIONS to undefine first, then redefine | ||
| target_compile_options(mirfrontend-wayland-input-trigger | ||
| PRIVATE | ||
| -UMIR_LOG_COMPONENT_FALLBACK # Undefine the definition we get from mirserver | ||
| -DMIR_LOG_COMPONENT_FALLBACK="input-triggers" | ||
| ) |
There was a problem hiding this comment.
This seems very specific naming.
Elsewhere we have:
MIR_LOG_COMPONENT_FALLBACK="mircommon";MIR_LOG_COMPONENT_FALLBACK="mirplatform"; and,MIR_LOG_COMPONENT_FALLBACK="mirserver"
Why does this one protocol within the Wayland frontend qualify as a component in its own right?
I could see MIR_LOG_COMPONENT_FALLBACK="mirserver/frontend", or similar, for ALL the frontend code, but this seems unnecessary granularity.
There was a problem hiding this comment.
Do we need a separate folder for each protocol? We've not done that before
There was a problem hiding this comment.
[non blocking] I wonder why this shows as a file move? It is just a file delete & new.
| if (keysym >= XKB_KEY_A && keysym <= XKB_KEY_Z) | ||
| { | ||
| lower = keysym + (XKB_KEY_a - XKB_KEY_A); | ||
| } | ||
| else if (keysym >= XKB_KEY_a && keysym <= XKB_KEY_z) | ||
| { | ||
| upper = keysym - (XKB_KEY_a - XKB_KEY_A); | ||
| } |
There was a problem hiding this comment.
Arithmetic like this is heavily dependent on the character representation: e.g. in EBCDIC this will not work as intended
What's new?
Quick and dirty spike to feel out how the input trigger protocol would be to work with
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
Tests added and passWLCS tests require the addition of keyboard access. Probably for another PR.