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

Don't send key on release from niri actions #58

Merged
merged 1 commit into from
Oct 29, 2023

Conversation

kchibisov
Copy link
Contributor

Some clients run logic on Release, thus don't send the key originally used for running niri actions.

Fixes #28.

src/input.rs Outdated Show resolved Hide resolved
@YaLTeR
Copy link
Owner

YaLTeR commented Oct 27, 2023

What happens on a sequence like:

  1. Press Mod
  2. Press Shift
  3. Press r
  4. Release Shift
  5. Release r
  6. Release Mod

if Mod+r (but not Mod+Shift+r) is a compositor action? Is the app going to get its "r" release?

@kchibisov
Copy link
Contributor Author

if Mod+r (but not Mod+Shift+r) is a compositor action? Is the app going to get its "r" release?

Not anymore.

Copy link
Owner

@YaLTeR YaLTeR left a 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
Comment on lines 148 to 152
if pressed {
*key += 1;
} else {
*key -= 1;
}
Copy link
Owner

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.

Copy link
Contributor Author

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() {
Copy link
Owner

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; }.

Copy link
Contributor Author

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.

@kchibisov kchibisov requested a review from YaLTeR October 28, 2023 10:16
src/input.rs Outdated Show resolved Hide resolved
src/input.rs Outdated Show resolved Hide resolved
@YaLTeR
Copy link
Owner

YaLTeR commented Oct 29, 2023

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.
@kchibisov kchibisov requested a review from YaLTeR October 29, 2023 08:49
@YaLTeR YaLTeR merged commit 5c24754 into YaLTeR:main Oct 29, 2023
4 checks passed
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.

Don't set Key::Release on hotkey end
2 participants