Skip to content

Change KeyState #53

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 3 commits into from
Mar 23, 2025
Merged

Change KeyState #53

merged 3 commits into from
Mar 23, 2025

Conversation

madsmtm
Copy link
Member

@madsmtm madsmtm commented Dec 2, 2024

Remove Display impl, rename variants and add helper methods, see each commit for details.

The motivation is to make this match winit::event::ElementState.

Unsure about the helper method names, whether is_pressed or pressed is cleaner? I went with is_pressed to match what's used in the wild for e.g. Bevy's ButtonState, and since it also more closely matches Rust conventions on enums (is_some, is_poisoned, is_ne, ...). But that's inconsistent with e.g. Modifiers::shift (so maybe we should change the latter too?).

@pyfisch
Copy link
Contributor

pyfisch commented Dec 4, 2024

Renaming the KeyState variants will require consumers of this library to update quite a few lines of code.
Since both the Web API and libxkbcommon use the names "up" and "down" many developers will be familiar with them.
I don't think the variants should be renamed.

@madsmtm
Copy link
Member Author

madsmtm commented Dec 6, 2024

Renaming the KeyState variants will require consumers of this library to update quite a few lines of code. Since both the Web API and libxkbcommon use the names "up" and "down" many developers will be familiar with them. I don't think the variants should be renamed.

Fair, I've removed the rename from the PR.

Still think there is value in the helper methods, but now I'm even more unsure what they should be called?

@pyfisch
Copy link
Contributor

pyfisch commented Dec 22, 2024

Still think there is value in the helper methods, but now I'm even more unsure what they should be called?

Either is_up and is_down or we postpone the decision.

@madsmtm
Copy link
Member Author

madsmtm commented Feb 13, 2025

I've changed it to use is_up and is_down.

madsmtm added 3 commits March 9, 2025 15:01
Use an inherent method instead, since that allows using in more contexts
(as the return type is now a static str), and because it's semantically
more correct (the key state isn't intrinsically tied to being an event
type).
These allow you to use KeyState without having to import it.
@pyfisch pyfisch merged commit 48b40a2 into rust-windowing:main Mar 23, 2025
3 checks passed
@madsmtm madsmtm deleted the key-state branch March 23, 2025 11:10
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