Make UserInput generic over the maximum number of chord inputs
#241
Conversation
alice-i-cecile
added
usability
|
Hmm, it looks like defaults for const generics are only allowed for |
| @@ -11,32 +11,41 @@ use crate::{ | |||
| buttonlike::{MouseMotionDirection, MouseWheelDirection}, | |||
| }; | |||
|
|
|||
| /// An [`UserInput::Chord`] could not be created because there were too many input elements | |||
| #[derive(Clone, Copy, PartialEq, Eq, Debug)] | |||
| pub struct ChordCreationError; | |||
There was a problem hiding this comment.
This should probably return the passed in iterator to make error recovery more feasible.
There was a problem hiding this comment.
Hmm, that does make sense, but at the point of error the iterator will already be consumed for the most part, so I'm not sure how useful it will be for recovery
| set.insert(button.into()); | ||
| } | ||
|
|
||
| let user_input = match length { |
There was a problem hiding this comment.
This matching behavior is inconsistent with the behavior in the infallible version. It's also surprising, as the user has explicitly tried to make a chord.
Might still be correct, but if so we'll want a different name.
There was a problem hiding this comment.
How is it inconsistent? I copied this logic from the infallible version
There was a problem hiding this comment.
Oh, I see. Never mind!
| /// | ||
| /// If `inputs` contains more than `CHORD_MAX_SIZE` elements, a [`ChordCreationError`] is returned. | ||
| /// If you want to silently ignore the rest of the elements, use the [`UserInput::chord`] function instead. | ||
| pub fn try_make_chord( |
There was a problem hiding this comment.
Maybe we can use something super generic here? Like new or try_create or something?
This feels like it should be the default method for configurable keybindings.
| @@ -11,6 +11,13 @@ | |||
| ### Usability | |||
|
|
|||
| - Implemented `Eq` for `Timing` and `InputMap`. | |||
| - `UserInput` is now generic over the maximum size of inputs for a `Chord` (the default is still `8`). | |||
There was a problem hiding this comment.
IMO we should probably lower the default to something much smaller, like 4, now that it can be configured, to save space for the 99% of use cases that don't want more.
This is quite annoying and I think blocks this PR. Is there an issue for this in the |
Looks like it's this issue: rust-lang/rust#98931. Turns out you can do something like: let chord: UserInput = UserInput::chord(...);but not let chord = UserInput::chord(...);...which is kind of weird. |
@TimJentzsch If possible could you post something on the rust-lang issue to explain your use-case and requirements for a default type feature. AFAICT your objections to the current situation is that it:
Would be great if we can gather a couple of use-cases to show that this is an important feature to add. |
|
I think this will be obsolete if we use traits instead of enums to represent the user input, which is probably the better move in the long run. @alice-i-cecile do we already have an issue for this change? I couldn't find one myself |
Not yet. Let me make one and close this out. |
Closes #21.
UserInputis now generic over the maximum number of inputs that fit in a chord. The default is still8.Furthermore,
UserInput::chorddoesn't panic anymore if the iterator contains more elements than the maximum size; instead, the rest of the inputs are ignored (breaking change).Additionally, a
UserInput::try_make_chordfunction does the same thing, but returns aResultand fails when too many inputs were provided. Not sure if this is the best name for this function :P