Expand keysym values sufficiently to represent an AT keyboard.#8
Expand keysym values sufficiently to represent an AT keyboard.#8jordanhendricks merged 4 commits intomainfrom
Conversation
|
In 934fcc7, I added a default implementation of the |
src/keysym.rs
Outdated
| KEYSYM_KP_PERIOD => Ok(KeypadPeriod), | ||
| KEYSYM_KP_DELETE => Ok(KeypadDelete), | ||
|
|
||
| _ => Ok(Unknown(value)), |
There was a problem hiding this comment.
Why is this a try-from if we unconditionally succeed?
There was a problem hiding this comment.
We chatted a bit about this offline, but -- yeah, you're right, this doesn't make a lot of sense. I had wanted to distinguish between keysyms that aren't mapped in this crate (because there are a lot of them) and invalid keysyms, hence the Unknown variant and making the conversion function a try_from. I used the variant a lot for debugging while wiring things up with the keyboard, but it's pretty useless in this interface for consumers. So I'll just convert the unknown case to an error.
| pub enum Keysym { | ||
| Unknown(u32), | ||
| Utf32(char), | ||
| Ascii(ascii::AsciiChar), |
There was a problem hiding this comment.
You could potentially pub use ascii:AsciiChar and perhaps obviate the need for consumers to pull ascii in via their Cargo.toml (unless they're using other pieces of it?)
There was a problem hiding this comment.
Oh, that's a great idea -- I don't think I've ever seen that done before.
gjcolombo
left a comment
gjcolombo
left a comment
There was a problem hiding this comment.
LGTM with the caveat that I didn't carefully check the keysym constants (I did look at the TryFrom impl, though, and everything matched up there, AFAICT).
luqmana
left a comment
luqmana
left a comment
There was a problem hiding this comment.
LGTM pending a few small nits and the stuff patrick already pointed out.
src/keysym.rs
Outdated
| fn try_from(value: u32) -> Result<Self, Self::Error> { | ||
| match value { | ||
| v if v <= ASCII_MAX => { | ||
| let ac = v.to_ascii_char().unwrap(); |
There was a problem hiding this comment.
Would have probably been fine to keep the unwrap, considering you're already testing against ASCII_MAX
4 participants
In support of oxidecomputer/propolis#211, this PR expands the keysyms available from this crate.