Rebase Physical Keys Shortcuts - #12570
Conversation
Signed-off-by: Amirhossein Akhlaghpour <m9.akhlaghpoor@gmail.com>
Signed-off-by: Amirhossein Akhlaghpour <m9.akhlaghpoor@gmail.com>
Native menu bars interpret shortcuts as logical keys. Emit a compiler warning for @physical-keys() in MenuItem shortcuts, log a one-time runtime warning in the winit+muda backend, and map supported physical codes to a best-effort logical key. Add parser/resolver syntax tests for invalid @physical-keys(), and update the physical-keys runtime test to reuse testing backend helpers.
Replace WindowEvent::Key { event_type, event } with WindowEvent::Key(WindowKeyEvent) where WindowKeyEvent bundles the existing KeyEvent struct plus the event type. This keeps the public key event payload consistent with the rest of the runtime and avoids duplicating fields.
Signed-off-by: Amirhossein Akhlaghpour <m9.akhlaghpoor@gmail.com>
After the rebase, some imports were unresolved and some API shapes had changed, which needed adjustment.
Also improve physical_key_codes documentation
2630a9a to
faeae5c
Compare
| } | ||
| ``` | ||
|
|
||
| Physical key names reuse familiar `Key` names such as `A`, `Digit1`, `BackQuote`, or `LeftArrow`, but they refer to a physical key location instead of logical text. Today these names refer to positions on a U.S. keyboard layout. |
There was a problem hiding this comment.
Is that really just today and a contract that we might break in the future, or will it remain that way?
Generally, I think the use-case and the explanation what the mapping is should come first in the docs, before explaining the syntax. (see style guide, "Write actionable" and "Offer direct advice")
There was a problem hiding this comment.
The keys we're adding now will remain to be based on the position of a U.S. keyboard layout.
However, if we ever add keys that just don't exist on a U.S. layout, those of course won't refer to U.S. layout positions.
Or if we ever add things like "PowerButton"/"VolumeUp/Down" and so on. Those aren't really on the U.S. keyboard.
So this is a bit defensively phrased. Should I still remove it?
(Moved the paragraph above the code example).
There was a problem hiding this comment.
As an additional note on docs/discoverability here: The PR currently does not introduce a PhysicalKey namespace and Rust enum, like we have for Key.
I think this would be valuable to have as well, but we can/should do that as a follow-up PR so that this PR doesn't become stale again.
(#12212 also depends on this PR and would then be unblocked).
faeae5c to
df16e1a
Compare
597b083 to
a0dd0e6
Compare
For some reason the doc-tests picked up this comment as a doc test: //! Slint # Winit # Xkb # Win # Mac Fixed by removing the indentation 🤷
a0dd0e6 to
6502f4e
Compare
tronical
left a comment
There was a problem hiding this comment.
Looks generally good. I feel we're discussing details ;)
|
|
||
| ### Physical Key Bindings | ||
|
|
||
| If you want to match a physical key position instead of the character produced on the current keyboard layout, use `@physical-keys(..)`. |
There was a problem hiding this comment.
Suggest to flip it around: "Use @physical-keys(..) to match ..." . Also saves you the "want" :)
Could you include an example (use-case, not code) as well, perhaps? (games?)
There was a problem hiding this comment.
Done (Added WASD as example)
| If you want to match a physical key position instead of the character produced on the current keyboard layout, use `@physical-keys(..)`. | ||
|
|
||
| Physical key names reuse familiar `Key` names such as `A`, `Digit1`, `BackQuote`, or `LeftArrow`, but they refer to a physical key location instead of logical text. | ||
| Today these names refer to positions on a U.S. keyboard layout. |
There was a problem hiding this comment.
After yesterday's discussion I feel even more that "Today" should be omitted.
There was a problem hiding this comment.
Okay, and reworded it to make it shorter.
| /// This uses the same names as `@physical-keys(...)`, for example `A`, | ||
| /// `Digit1`, or `LeftArrow`. It is empty when the backend doesn't provide | ||
| /// physical key information. | ||
| physical_key: SharedString, |
There was a problem hiding this comment.
A question, just for my understanding: The name suggests that it's a single key, but the field is a string. Would you use a char in a perfect world?
There was a problem hiding this comment.
For performance we could just use an int. This is just an identifier for a physical key, we should not even be thinking in terms of "characters" here really.
The reason this is a string is so that you can easily name your own keys. Like if you have your own embedded device which has a physical key to "start the reactor", you could just have a "StartReactor" key.
At the moment, this is not supported yet, but that is a limitation we should lift in the future so backends can implement their own custom physical keys.
This is somewhat nicer to do with a string than some kind of integer, but an integer would definitely be more performant. So maybe we should talk about this again during API review.
Also, it matches what the Keys type already stores, so we don't have to make that a tagged union.
| } | ||
|
|
||
| #[inline(never)] | ||
| fn compile_keys_literal(expr: &Expression) -> TokenStream { |
There was a problem hiding this comment.
This looks like an unnecessary re-indentation? :)
There was a problem hiding this comment.
The indentation didn't change, the type of the function argument changed ;)
(The previous function was weirdly unpacking the expression enum for no reason).
| && let Some(binding) = element.borrow().bindings.get("shortcut") | ||
| && matches!(&binding.borrow().expression, Expression::Keys(k) if k.is_physical) | ||
| { | ||
| diag.push_warning( |
There was a problem hiding this comment.
Wouldn't it be better to make this an error?
There was a problem hiding this comment.
Hm, yes, probably better to make it an error, we can always relax it later.
In theory they do actually work in menus due to our fallback logic, but we cannot show them in the MenuItem correctly (they show up as and will match like logical keys and will also match the physical key).
This error also doesn't prevent you from sneaking a physical key onto a MenuItem shortcut through a property binding. So it's not a perfect defense.
| }; | ||
| } | ||
|
|
||
| thread_local! { |
There was a problem hiding this comment.
Instead of "duplicating" this in each thread, wouldn't it be better to put this behind a LazyLock?
There was a problem hiding this comment.
Hm, our compiler currently only runs in one thread, but sure, we can make that a lazylock for when olivier finishes the LLR interpreter :)
| /// Always reported as [`Accepted`](crate::api::WindowEventDispatchResult::Accepted). | ||
| PointerExited, | ||
| /// A key was pressed or released. | ||
| Key(WindowKeyEvent), |
There was a problem hiding this comment.
I suggest to split out making the new key event public into a separate PR. Reduces the surface of this and makes it easier to discuss/review just the API. I don't think it's needed strictly for this functionality, right?
There was a problem hiding this comment.
Ah, I just ported the backends to use this 🙈
They would all have to use InternalKeyEvent then for physical keys support.
Can we just decide during API review whether we want it public or not and add a PR to make it private again if needed?
| static WARN_ONCE: std::sync::Once = std::sync::Once::new(); | ||
| WARN_ONCE.call_once(|| { | ||
| i_slint_core::debug_log!( | ||
| "Warning: Physical keys used in menu shortcuts are interpreted as logical keys based on a US keyboard layout. Use @keys(...) for menu shortcuts." |
There was a problem hiding this comment.
Isn't this caught at compile time? (especially if it were an error)
There was a problem hiding this comment.
You could still set this via a property from Rust, so this is a last layer of defense.
|
|
||
| As another example, `@keys(Control + Plus)` is equivalent to `@keys(Control + Shift? + "+")`. | ||
|
|
||
| ### Physical Key Bindings |
There was a problem hiding this comment.
Perhaps here it would also be worthwhile to mention the menu limitation? (or is that overdoing it?)
There was a problem hiding this comment.
Added that onto the docs of the MenuItem.shortcut property now. I think it's unnecessary here, but definitely makes sense there.
de41e6e to
9e85f40
Compare
This PR rebases #11474 by @amirHdev on to the current master.
It also closes the backend gap so that now all backends support physical key bindings 🥳
Closes #11474
Closes #11247