add public api to create Keys from application logic#11138
add public api to create Keys from application logic#11138manushT wants to merge 1 commit intoslint-ui:masterfrom
Conversation
fd3971d to
e410b6f
Compare
LeonMatthes
left a comment
There was a problem hiding this comment.
Overall, I think this is going in a decent direction.
The biggest issue we may run into with this approach is that it is harder for us to differentiate between named keys and string literals. But I think this is manageable, as key literals are only a single grapheme cluster, so have few conflicts with the key namespace.
The benefit of this approach is that we don't have to deal with custom string-literal and separator parsing in core.
And this API is open enough that we can easily adapt it to use a emacs-style keyboard shortcuts or alternate shortcuts.
e.g. if this was supported:
@keys(A | B) // alternative shortcuts
@keys(A => B) // emacs-stylewe could easily support this:
Keys::from(["A", "|", "B"]);
Keys::from(["A", "=>", "B"])@ogoffart @tronical What are your thoughts on this?
Is it okay if we continue in this direction?
| let mut ignore_shift = false; | ||
| let mut ignore_alt = false; | ||
| let mut key_part: Option<&str> = None; | ||
| let mut has_any = false; |
There was a problem hiding this comment.
This is not necessary, Keys::from([]) should be valid, just like @keys() is valid. (Same as Keys::default)
| let key_name = key_part.ok_or(KeysParseError::NoKey)?; | ||
|
|
||
| // First: try named-key lookup (case-sensitive, like the @keys macro) | ||
| if let Some(key_char) = lookup_key_name(key_name) { |
There was a problem hiding this comment.
This should also look up whether to automatically ignore_shift or not, like the compiler does in resolving.rs .
| "Control" => modifiers.control = true, | ||
| "Alt" => modifiers.alt = true, | ||
| "Shift" => modifiers.shift = true, | ||
| "Meta" => modifiers.meta = true, | ||
| "Shift?" => ignore_shift = true, | ||
| "Alt?" => ignore_alt = true, |
There was a problem hiding this comment.
These need to be checked for compatibility.
Shift? is not compatible with Shift, etc.
| assert_eq!( | ||
| k, | ||
| make_keys( | ||
| "z".into(), | ||
| KeyboardModifiers { control: true, ..Default::default() }, | ||
| true, | ||
| false, | ||
| ) | ||
| ); |
There was a problem hiding this comment.
| assert_eq!( | |
| k, | |
| make_keys( | |
| "z".into(), | |
| KeyboardModifiers { control: true, ..Default::default() }, | |
| true, | |
| false, | |
| ) | |
| ); | |
| Keys { | |
| key: "z".into() | |
| modifiers: KeyboardModifiers { control: true, ..Default::default() }, | |
| ignore_shift: true | |
| ignore_alt: false | |
| } |
I think this would be much more readable.
| fn test_from_parts_ignore_alt() { | ||
| let k = Keys::try_from(["Control", "Alt?", "A"].as_slice()).unwrap(); | ||
| assert_eq!( | ||
| k, | ||
| make_keys( | ||
| "a".into(), | ||
| KeyboardModifiers { control: true, ..Default::default() }, | ||
| false, | ||
| true, | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_from_parts_named_key() { | ||
| let k = Keys::try_from(["F5"].as_slice()).unwrap(); | ||
| let f5_char: char = key_codes::Key::F5.into(); | ||
| assert_eq!( | ||
| k, | ||
| make_keys( | ||
| alloc::string::String::from(f5_char).into(), | ||
| KeyboardModifiers::default(), | ||
| false, | ||
| false, | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_from_parts_return() { | ||
| let k = Keys::try_from(["Return"].as_slice()).unwrap(); | ||
| let return_char: char = key_codes::Key::Return.into(); | ||
| assert_eq!( | ||
| k, | ||
| make_keys( | ||
| alloc::string::String::from(return_char).into(), | ||
| KeyboardModifiers::default(), | ||
| false, | ||
| false, | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_from_parts_plus_key() { | ||
| // "Plus" is a named key in the Keys namespace | ||
| let k1 = Keys::try_from(["Control", "Plus"].as_slice()).unwrap(); | ||
| assert_eq!( | ||
| k1, | ||
| make_keys( | ||
| "+".into(), | ||
| KeyboardModifiers { control: true, ..Default::default() }, | ||
| false, | ||
| false | ||
| ) | ||
| ); | ||
| // "+" also works as a string literal fallback (single grapheme, lowercase) | ||
| let k2 = Keys::try_from(["Control", "+"].as_slice()).unwrap(); | ||
| assert_eq!(k1, k2); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_from_parts_all_modifiers() { | ||
| let k = Keys::try_from(["Control", "Shift", "Alt", "A"].as_slice()).unwrap(); | ||
| assert_eq!( | ||
| k, | ||
| make_keys( | ||
| "a".into(), | ||
| KeyboardModifiers { control: true, shift: true, alt: true, ..Default::default() }, | ||
| false, | ||
| false, | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_from_parts_error_empty() { | ||
| let result = Keys::try_from([].as_slice()); | ||
| assert_eq!(result, Err(KeysParseError::Empty)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_from_parts_error_no_key() { | ||
| let result = Keys::try_from(["Control", "Shift"].as_slice()); | ||
| assert_eq!(result, Err(KeysParseError::NoKey)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_from_parts_error_multiple_keys() { | ||
| let result = Keys::try_from(["A", "B"].as_slice()); | ||
| assert_eq!(result, Err(KeysParseError::MultipleKeys)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_from_parts_error_unknown() { | ||
| // Multi-char strings that are not named keys and not single grapheme clusters | ||
| let result = Keys::try_from(["Control", "Foobar"].as_slice()); | ||
| assert!(matches!(result, Err(KeysParseError::MultipleGraphemeClusters(_)))); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_from_parts_string_literal_fallback() { | ||
| // Unicode characters that aren't in the named-key namespace | ||
| let k = Keys::try_from(["Control", "€"].as_slice()).unwrap(); | ||
| assert_eq!( | ||
| k, | ||
| make_keys( | ||
| "€".into(), | ||
| KeyboardModifiers { control: true, ..Default::default() }, | ||
| false, | ||
| false, | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_from_parts_string_literal_must_be_lowercase() { | ||
| let result = Keys::try_from(["Control", "É"].as_slice()); | ||
| assert!(matches!(result, Err(KeysParseError::NotLowercase(_)))); | ||
| let k = Keys::try_from(["Control", "é"].as_slice()).unwrap(); | ||
| assert_eq!( | ||
| k, | ||
| make_keys( | ||
| "é".into(), | ||
| KeyboardModifiers { control: true, ..Default::default() }, | ||
| false, | ||
| false, | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_from_parts_multiple_grapheme_clusters() { | ||
| let result = Keys::try_from(["Control", "ab"].as_slice()); | ||
| assert!(matches!(result, Err(KeysParseError::MultipleGraphemeClusters(_)))); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_from_parts_case_sensitive_key_name() { | ||
| // Named keys are case-sensitive (like the @keys macro) | ||
| // "A" is in the namespace, "a" is not — falls through to string literal | ||
| let k1 = Keys::try_from(["A"].as_slice()).unwrap(); | ||
| let k2 = Keys::try_from(["a"].as_slice()).unwrap(); | ||
| // Both should produce the same Keys ("a" key) | ||
| assert_eq!(k1, k2); | ||
| // But "Return" works while "return" falls to literal fallback and fails (multi-grapheme) | ||
| let k3 = Keys::try_from(["Return"].as_slice()).unwrap(); | ||
| assert!(k3.matches(&KeyEvent { | ||
| text: SharedString::from(alloc::string::String::from(char::from( | ||
| key_codes::Key::Return | ||
| ))), | ||
| modifiers: KeyboardModifiers::default(), | ||
| ..Default::default() | ||
| })); | ||
| let result = Keys::try_from(["return"].as_slice()); | ||
| assert!(matches!(result, Err(KeysParseError::MultipleGraphemeClusters(_)))); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_from_parts_matches_event() { | ||
| let k = Keys::try_from(["Control", "A"].as_slice()).unwrap(); | ||
| let event = KeyEvent { | ||
| text: "a".into(), | ||
| modifiers: KeyboardModifiers { control: true, ..Default::default() }, | ||
| ..Default::default() | ||
| }; | ||
| assert!(k.matches(&event)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_from_parts_matches_with_ignore_shift() { | ||
| let k = Keys::try_from(["Control", "Shift?", "Z"].as_slice()).unwrap(); | ||
| // Should match with Shift pressed | ||
| let with_shift = KeyEvent { | ||
| text: "z".into(), | ||
| modifiers: KeyboardModifiers { control: true, shift: true, ..Default::default() }, | ||
| ..Default::default() | ||
| }; | ||
| assert!(k.matches(&with_shift)); | ||
| // Should also match without Shift | ||
| let without_shift = KeyEvent { | ||
| text: "z".into(), | ||
| modifiers: KeyboardModifiers { control: true, ..Default::default() }, | ||
| ..Default::default() | ||
| }; | ||
| assert!(k.matches(&without_shift)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_from_parts_no_match_wrong_key() { | ||
| let k = Keys::try_from(["Control", "A"].as_slice()).unwrap(); | ||
| let event = KeyEvent { | ||
| text: "b".into(), | ||
| modifiers: KeyboardModifiers { control: true, ..Default::default() }, | ||
| ..Default::default() | ||
| }; | ||
| assert!(!k.matches(&event)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_from_parts_no_match_wrong_modifier() { | ||
| let k = Keys::try_from(["Control", "A"].as_slice()).unwrap(); | ||
| let event = KeyEvent { | ||
| text: "a".into(), | ||
| modifiers: KeyboardModifiers { alt: true, ..Default::default() }, | ||
| ..Default::default() | ||
| }; | ||
| assert!(!k.matches(&event)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_from_parts_equals_make_keys() { | ||
| let from_parts = Keys::try_from(["Control", "A"].as_slice()).unwrap(); | ||
| let made = make_keys( | ||
| "a".into(), | ||
| KeyboardModifiers { control: true, ..Default::default() }, | ||
| false, | ||
| false, | ||
| ); | ||
| assert_eq!(from_parts, made); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_from_parts_special_key_matches_event() { | ||
| let k = Keys::try_from(["Return"].as_slice()).unwrap(); | ||
| let return_char: char = key_codes::Key::Return.into(); | ||
| let event = KeyEvent { | ||
| text: SharedString::from(alloc::string::String::from(return_char)), | ||
| modifiers: KeyboardModifiers::default(), | ||
| ..Default::default() | ||
| }; | ||
| assert!(k.matches(&event)); | ||
| } |
There was a problem hiding this comment.
These shouldn't be dozens of separate tests.
You can just use one keys valid and keys invalid test.
These can then both just contain a plain list of data for valid and invalid arguments respectively.
then iterate over the list and make your assertions.
| impl core::fmt::Display for KeysParseError { | ||
| fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { | ||
| match self { | ||
| Self::Empty => write!(f, "empty parts list"), |
| /// Error type returned when constructing a [`Keys`] from string parts. | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| #[non_exhaustive] | ||
| pub enum KeysParseError { |
There was a problem hiding this comment.
This should implement the error trait.
| Ok(Keys { key, modifiers, ignore_shift, ignore_alt }) | ||
| } | ||
|
|
||
| impl<'a> TryFrom<&'a [&'a str]> for Keys { |
There was a problem hiding this comment.
| impl<'a> TryFrom<&'a [&'a str]> for Keys { | |
| impl<'a, Iter: IntoIterator<Item='a str>> TryFrom<Iter> for Keys { |
For an easier-to-use API, prefer using IntoIterator instead of a string slice.
Same for the internal function.
| /// Create a `Keys` from a slice of string parts (matching `@keys` macro syntax). | ||
| /// | ||
| /// Each element is either a modifier (`Control`, `Shift`, `Alt`, `Meta`, `Shift?`, `Alt?`) | ||
| /// or a key. Keys are first looked up by name (case-sensitive) in the Keys namespace; |
There was a problem hiding this comment.
"Keys namespace" -> "Key namespace"
| impl<'a> TryFrom<&'a [&'a str]> for Keys { | ||
| type Error = KeysParseError; | ||
|
|
||
| /// Create a `Keys` from a slice of string parts (matching `@keys` macro syntax). |
There was a problem hiding this comment.
This should probably link to the documentation that we have on key bindings.
I'm not a fan of using |
e410b6f to
acd41e6
Compare
issue #11117