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? |
kchibisov
force-pushed
the
suppress-action
branch
from
44849fa to
5c0cdae
Compare
Not anymore. |
YaLTeR
left a comment
YaLTeR
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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 niri-wm#28.
kchibisov
force-pushed
the
suppress-action
branch
from
46601f0 to
0f4e0d2
Compare
2 participants
Some clients run logic on
Release, thus don't send the key originally used for runningniriactions.Fixes #28.