Skip to content

Separate Key into Character and NamedKey #50

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 23, 2025

Conversation

madsmtm
Copy link
Member

@madsmtm madsmtm commented Dec 1, 2024

This makes most of the key Copy, and allows other crates like Winit to more easily use their own string types.

See rust-windowing/winit#2995 and rust-windowing/winit#3143 for discussion on a similar change in Winit.

Part of #19.

@pyfisch
Copy link
Contributor

pyfisch commented Dec 22, 2024

Please rebase the PR.

I wonder if it is possible to have a Key<T=String> which implements Copy if T is Copy as well? I don't really like having separate types for keys and named keys.

@madsmtm madsmtm force-pushed the named_key branch 2 times, most recently from 0b833c9 to 4d2a4db Compare February 13, 2025 17:31
@madsmtm
Copy link
Member Author

madsmtm commented Feb 13, 2025

I don't really like having separate types for keys and named keys.

My main argument here is that Character is not in spec, and that even the key attribute in the spec is split into two categories, namely "key string" and "named key".

There's also a question about normalization; technically Key can currently contain a lot of invalid/unexpected states, such as Key::Character("Shift") or Key::Character("\t"), by explicitly splitting it up into Key and NamedKey, we alleviate some of those concerns (see also #59).

I wonder if it is possible to have a Key<T=String> which implements Copy if T is Copy as well?

Would definitely be technically possible, but wouldn't really help Winit which also wants extra variants, such as Dead and Native.

@madsmtm madsmtm marked this pull request as ready for review February 13, 2025 19:03
@madsmtm madsmtm mentioned this pull request Mar 10, 2025
9 tasks
Copy link
Contributor

@pyfisch pyfisch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I will merge it with the nit addressed.

@madsmtm
Copy link
Member Author

madsmtm commented Mar 23, 2025

Done!

@madsmtm madsmtm requested a review from pyfisch March 23, 2025 11:17
@pyfisch
Copy link
Contributor

pyfisch commented Mar 23, 2025

@madsmtm The docs need to used named key.

74 | ...es to the phone’s main screen), use [`GoHome`][Key::GoHome].
   |                                                   ^^^^^^^^^^^ no item named `Key` in scope

This makes most of the key Copy, and allows other crates like Winit to
more easily use their own string types.
@pyfisch pyfisch merged commit 5f9239e into rust-windowing:main Mar 23, 2025
4 checks passed
@madsmtm madsmtm deleted the named_key branch March 23, 2025 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants