pyfisch / keyboard-types

Types to define keyboard related events.
Apache License 2.0
53 stars 9 forks source link

Modifiers ergonomics improvements #7

Open raphlinus opened 4 years ago

raphlinus commented 4 years ago

First, a heads-up, we're in the process of adopting this crate as our standard keyboard event type in druid, see https://github.com/linebender/druid/pull/1049 in particular. We appreciate having the crate are hopeful that it's a small step towards convergence in low-level platform integration the ecosystem.

There are a number of things we've run into as part of the integration. One ergonomics regression is that .shift (the old KeyModifiers was a struct of pub bools) has become .contains(Modifiers::SHIFT). Would you be open to an additional impl block with shift() and friends?

As a much lower priority, we also have an IntoKey trait that will convert any string into Key::Character(...), and this is used primarily to construct hotkeys. This has different semantics than your FromStr because it's not designed to parse strings like "Enter". It would be slightly more convenient for us to upgrade this to an Into, but I completely understand if you wouldn't want that.

There may be other work that makes sense to upstream, and if you're interested I can create additional issues.

pyfisch commented 4 years ago

We appreciate having the crate are hopeful that it's a small step towards convergence in low-level platform integration the ecosystem.

I am happy if my crate can help a bit with this!

One ergonomics regression is that .shift (the old KeyModifiers was a struct of pub bools) has become .contains(Modifiers::SHIFT). Would you be open to an additional impl block with shift() and friends?

Yes, we can add this impl block. I'd suggest to add only the most common modifiers like in JS KeyboardEvent:

The other modifiers are rarely used in keyboard shortcuts and would IMHO clutter the documentation.

By the way, would it make sense to add these convenient getters to the KeyboardEvent instead of the Modifiers enum?

As a much lower priority, we also have an IntoKey trait that will convert any string into Key::Character(...), and this is used primarily to construct hotkeys. This has different semantics than your FromStr because it's not designed to parse strings like "Enter". It would be slightly more convenient for us to upgrade this to an Into, but I completely understand if you wouldn't want that.

Right now strings are checked before they are converted to Key::Character(...) by the is_key_string method. I'd rather not implement Into as this method would either have to panic or accept bad strings. Maybe panicking is fine for constructing hotkeys. But in this case I'd add a regular method to construct a key from a string which panics on bad strings.

Did you see the ShortcutMatcher? Have a look at servo for how it can be used.

There may be other work that makes sense to upstream, and if you're interested I can create additional issues.

Sure, please create issues for anything that comes up.

raphlinus commented 4 years ago

Thanks for the thoughtful and timely response.

The impl block for the four basic modifiers sounds great, that's what we have in our draft PR.

I understand your concerns about an Into impl. I think ultimately the goals are different - for us, it's mostly to make the construction of HotKey more ergonomic, specifically we do not want to parse string values as you would in JavaScript environment, for those we want people to use the enum directly.

We did see ShortcutMatcher but felt it did not quite meet our needs. One of the missing features is the ability to specify the higher-level semantic concept of "command key" and have that map to Meta on macOS and Ctrl on other platforms. Overall, after some discussion, we've decided to keep using our own KeyEvent so we can control the fine details, and will be able to add other features such as timestamps, but we are encouraged by the fact that by using the component types it will be very easy to convert between the two if needed, and that this will encourage convergence in the ecosystem.

pyfisch commented 4 years ago

I understand your concerns about an Into impl. I think ultimately the goals are different - for us, it's mostly to make the construction of HotKey more ergonomic, specifically we do not want to parse string values as you would in JavaScript environment, for those we want people to use the enum directly.

I suggest to create a trait for matching keys, chars and strings in hotkeys similar to MatchKeyin this crate (or keep KeyCompare used by druid). It will be ergonomic and you are free to tweak the trait to fit your needs.

One of the missing features is the ability to specify the higher-level semantic concept of "command key" and have that map to Meta on macOS and Ctrl on other platforms.

Servo has the CMD_OR_CONTROL constant for this. How do you decide which key to use on the web?

Overall, after some discussion, we've decided to keep using our own KeyEvent so we can control the fine details, and will be able to add other features such as timestamps, but we are encouraged by the fact that by using the component types it will be very easy to convert between the two if needed, and that this will encourage convergence in the ecosystem.

I agree, with the custom event you can modify the type so it works best for druid.

chris-morgan commented 4 years ago

Servo has the CMD_OR_CONTROL constant for this. How do you decide which key to use on the web?

Detect whether the user is on an Apple platform with the right checks on navigator.platform and navigator.userAgent and use metaKey if so, or else use ctrlKey. Yes, this will mean invoking a JavaScript function; I dunno, maybe a lazy static or equivalent.

This is what Fastmail does in its Overture library, used primarily for its webmail. Relevant code: resolution of “Cmd-” to “Ctrl-” or “Meta-” depending on platform; determination of whether the user is on an Apple platform.

Also potentially relevant is formatting of keyboard shortcuts; for when you take into account platform conventions, you could get a display label of ⇧⌘⌫ or of Ctrl+Shift+Backspace. Once you’re on to the topic of keyboard shortcuts there are plenty of other interesting differences, of which I think the most important is Apple using Cmd-Backspace where other platforms use Delete (and as part of that, the way that Apple calls Backspace “Delete” and Delete “Forward Delete”, which is most conveniently side-stepped by always referring to them in UIs as ⌫ and ⌦). Of course, most of that lot should… probably not be addressed in this crate (potentially as much as all). But maybe some parts of it should go here rather than Druid. I haven’t thought too hard about it. But I figure it won’t hurt to mention it.