Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Ui.input_mut & InputState.ignore_key #1212
Add Ui.input_mut & InputState.ignore_key #1212
Changes from 9 commits
ab3cbca
3c61075
b118aa0
18defca
7a4c751
7b89764
a7b394c
d0d8856
1ef87ad
1aa5f45
48dac3a
11050bd
f195c20
6a9f1ec
cb4fcd4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can make this even nicer by switching the argument order and adding some helpers to
Modifiers
:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, i added it as a builder api but lmk if you prefer classmethods + operator overloading instead
vs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way to do this more generally could be to have a
consume_event
+ some helper to easily make the event from key and modifier combo likeEvent::from_key(modifier, key)
ormodifier.event_with_key(key)
or viceversa. Not sure if that would need a bunch of refactoring of code that makes the convenience properties from the events to keep the invariants maintainedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the docs of
consume_key
, it will return true both when the key is pressed and when it is released, so this should toggle twice, which is not intentional. This is why I thinkconsume_key
should only detect presses and ignore releases (or the other way around)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It only returns true when the key was pressed but it will remove the release event too, so triggering only once. Also in the built app it seems to only trigger once (on Windows & Ubuntu) - do you see differently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't run in since last review :)
But then the docs for
consume_key
should be changed to indicate it is only triggered on press, and also match forpressed: true
.