Merge KeyEventExtra into KeyEvent
#4029
Conversation
To make the fields easier to use, and to allow constructing KeyEvent in user test code.
madsmtm
added
S - enhancement
madsmtm
requested review from
jackpot51,
notgull,
daxpedda,
kchibisov and
MarijnS95
as code owners
| /// - **Android:** Unimplemented, this field is always the same value as `logical_key`. | ||
| /// - **iOS:** Unimplemented, this field is always the same value as `logical_key`. | ||
| /// - **Web:** Unsupported, this field is always the same value as `logical_key`. | ||
| pub key_without_modifiers: keyboard::Key, |
There was a problem hiding this comment.
Another option would be to make this an Option<keyboard::Key>, I'm not familiar enough with it to say what would be best.
| /// - **Android:** Unimplemented, this field is always the same value as `text`. | ||
| /// - **iOS:** Unimplemented, this field is always the same value as `text`. | ||
| /// - **Web:** Unsupported, this field is always the same value as `text`. |
There was a problem hiding this comment.
Another option would be to always set None here on these platforms, but I felt this choice was more consistent with key_without_modifiers (unless we change that to be an Option).
There was a problem hiding this comment.
Could we have it the same way as before in a struct on a KeyboardEvent which you can not match, but can e.g. default init or set separately?
I don't really want to have an option to match on those, since they are not cross platform. Other than that removing the extension is fine and it's not that big of a breaking change.
Hmm, I can, but I kinda feel like that'd just be unnecessary indirection? And I guess it we really want users to be able to test their crates, they'll need the ability to set the fields in this extra struct anyhow?
Am I understanding correctly that your worry is that users may be more aware of a method not being cross-platform, as opposed to a field being so? |
|
Yes, so it's an |
To make the
text_with_all_modifiersandkey_without_modifiersfields easier to use, and to allow constructingKeyEventin user test code. This also removes the need forKeyEventExtModifierSupplement.Fixes #3606 (which is desired by Xilem/Masonry).
I went with adding the fields on all platforms, and just defining their behaviour as the same as
text/logical_keyon the unsupported platforms. This is similar to how we have allWindowEvents available on all the platforms, but not all are emitted by all of them, so I felt this was a good trade-off.changelogmodule if knowledge of this change could be valuable to users