Skip to content
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

feat: Derive Ord for Event and sub-types #951

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nick42d
Copy link

@nick42d nick42d commented Dec 4, 2024

Might not be the best practice, but I wanted to use KeyCode in a BTreeMap and noticed it implements PartialOrd but not Ord. From what I understand from the Rust docs, the Ord derive is exactly the same as the PartialOrd derive, so I'm proposing that we add it in this PR.

I propagated the change to Event and all it's children for consistency.

This change should be non-breaking.

@nick42d nick42d requested a review from TimonPost as a code owner December 4, 2024 13:46
@joshka
Copy link
Collaborator

joshka commented Dec 4, 2024

I questioned the rationale for ordering a while back in #909 (comment). Mostly my concern was that there's nothing that seems obvious to me why this would be needed. I'm curious if you can expand on the use case for this a bit more.

I'd be inclined to instead remove even the PartialOrd impls from these types.

KeyCode seems like the only one that really has an obvious reason to be ordered, and that's mainly so that it would be reasonable to show the keys sorted in UIs. I suspect that the current default top-to-bottom ordering might not be the best there however. Any thoughts on that?

@nick42d
Copy link
Author

nick42d commented Dec 4, 2024

I questioned the rationale for ordering a while back in #909 (comment). Mostly my concern was that there's nothing that seems obvious to me why this would be needed. I'm curious if you can expand on the use case for this a bit more.

I'd be inclined to instead remove even the PartialOrd impls from these types.

KeyCode seems like the only one that really has an obvious reason to be ordered, and that's mainly so that it would be reasonable to show the keys sorted in UIs. I suspect that the current default top-to-bottom ordering might not be the best there however. Any thoughts on that?

Got it, I guess to expand on this a little;
I am storing a Map of keybinds (specifically, a struct containing KeyCode + KeyModifiers) and I indeed do want to display them in a UI. For this the actual sort order is not a priority for me, more that the ordering is consistent. For that, the derived ordering Rust provides is ideal, as it is automatically updated and generating my own version would require a lot of boilerplate.

I am wondering if you'd be more amenable to this change if it was limited to just KeyCode and KeyModifiers, perhaps with the addition of some documentation similar to here - as actually using comparison operators on these structs doesn't make much sense.

For the other structs I could personally take it or leave it, but it still doesn't make much sense for them to derive PartialOrd but not Ord. I wonder if anyone downstream is actually using the PartialOrd impls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants