-
-
Notifications
You must be signed in to change notification settings - Fork 130
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
Don't send key on release from niri actions #58
Conversation
What happens on a sequence like:
if Mod+r (but not Mod+Shift+r) is a compositor action? Is the app going to get its "r" release? |
44849fa
to
5c0cdae
Compare
Not anymore. |
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 think after merging, I will need to extract this filtering code to be testable and add tests for several of these input scenarios. This has certainly reached the complexity level where I'm no longer sure the code is correct.
src/input.rs
Outdated
if pressed { | ||
*key += 1; | ||
} else { | ||
*key -= 1; | ||
} |
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.
Can we legitimately go above 1 here? I'm thinking it might be better to use a simple bool
instead of a counter.
If we get a press without a release somehow (some bug for instance, for example of parent compositor when running windowed), a bool
would allow to "unstick" the key back by tapping it again, which is how we usually work around those kinds of bugs in other software. With a counter it instead goes to 2 and back to 1, remaining non-working.
Also, for the VT switch keys like Ctrl-Alt-F1, I'm not sure we ever see key release events. A counter will keep counting up unnecessarily, while a bool
will just stay true
.
I guess with multiple keyboards we can get a counter above 1? I'm not sure that is more important to handle correctly compared to the two cases I mentioned.
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.
Can we legitimately go above 1 here? I'm thinking it might be better to use a simple bool instead of a counter.
yes, with keyboard having duplicated keys.
Also, for the VT switch keys like Ctrl-Alt-F1, I'm not sure we ever see key release events. A counter will keep counting up unnecessarily, while a bool will just stay true.
I think the only solution for that is to clear
the HashMap
.
|
||
// Filter actions when the session is locked. | ||
if self.niri.is_locked() { |
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 think if you change the let Some(Some(
above to let Some(mut action) =
then you can leave this part of the code completely unchanged, save for the if !pressed { return; }
.
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.
Yeah, but why have the old logic with extra None
and increasing indention for the giant match
.
Thanks, seems to work fine. Is this good to merge? |
Some clients run logic on `Release`, thus don't send the key originally used for running `niri` actions. Fixes YaLTeR#28.
46601f0
to
0f4e0d2
Compare
Some clients run logic on
Release
, thus don't send the key originally used for runningniri
actions.Fixes #28.