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

feat: add mouse wheel events to defsrc #592

Merged
merged 16 commits into from
Nov 4, 2023
Merged

Conversation

rszyma
Copy link
Contributor

@rszyma rszyma commented Oct 22, 2023

Describe your changes. Use imperative present tense.

Closes #553

  • For now, implemented only for Linux
  • Allows adding scroll events to defsrc
  • (Currently) adding any of the new scroll labels to deflayer or anywhere else don't error and just outputs nothing.
  • Tested manually, appears functonal.

Checklist

  • Add documentation to docs/config.adoc
    • Yes or N/A
  • Add example and basic docs to cfg_samples/kanata.kbd
    • Yes or N/A
  • Update error messages
    • Yes or N/A
  • Added tests, or did manual testing
    • Yes

@jtroo
Copy link
Owner

jtroo commented Oct 22, 2023

Thanks for the PR!

The thing that stands out to me most for my initial review is that I don't know that it's valuable to have the SupportedInputEvent type be sent to the processing loop. The processing loop has no way to deal with events other than plain button presses, and this PR doesn't change that from my reading. So it seems that it should be the event loop's responsibility to translate the wheel input events into some symbolic "key press", and to inject a corresponding release. Then the event loop could also do defrsc-based filtering on those events like it does with keyboard keys.

@rszyma
Copy link
Contributor Author

rszyma commented Oct 22, 2023

While thinking of a good reason we should keep SupportedInputEvent I just realized that remapping mwu to _ currently doesn't work at all.

I haven't delved into how _ labels work yet, but I'm guessing that it maps a given label to itself.

So maybe we should allow adding the new mouse scroll labels to deflayer? Just a guess, but this may fix mwu -> _ remapping? (what do you think?). As to where SupportedInputEvent comes to usefullness: mwu (and other wheel labels) could dynamically adjust distance of scroll based on the last registered scroll distance. And to set that dynamic distance we would read SupportedInputEvent.distance in processing loop.

As to why I think this kind of dynamic adjustment is necessary: users might remap scroll distance so we can't know what is the scroll distance value until we read it directly from input.

@jtroo
Copy link
Owner

jtroo commented Oct 24, 2023

So maybe we should allow adding the new mouse scroll labels to deflayer? Just a guess, but this may fix mwu -> _ remapping? (what do you think?). As to where SupportedInputEvent comes to usefullness: mwu (and other wheel labels) could dynamically adjust distance of scroll based on the last registered scroll distance. And to set that dynamic distance we would read SupportedInputEvent.distance in processing loop.

That seems like it would be a reasonable solution to the issue. Potentially might need to add a new CustomAction enum member to signify "user-input scroll" which the parser would put into the _ or mw* slots.

@rszyma
Copy link
Contributor Author

rszyma commented Oct 25, 2023

Now this works as nothing were remapped:

(defsrc mwu mwl)
(deflayer base _ _)

Well, actually there is a small difference. Currently kanata assumes that all scroll events have value 1 or -1 (120 and -120 for hi-res). If a mouse sends scroll event with value other these e.g. 3, it's still treated as 1. However this problem won't happen if user doesn't map any of mw*.

Previously I proposed we do this:

mwu (and other wheel labels) could dynamically adjust distance of scroll based on the last registered scroll distance

But there are some downsides:

  1. I user is scrolling very fast, a device instead sending 1 would send 2 or 3. If we were to register this value as a base scroll distance it would be incorrect.
  2. It requires at least 1 scroll input from user.

However this "dynamically adjust distance" thing is probably not worth it anyway, because IMO users having remapped scroll to send 2 instead of 1 is niche enough to not care. Again, like above, this problem is not-existent if mouse wheel is not mapped.

@rszyma
Copy link
Contributor Author

rszyma commented Oct 25, 2023

This also works:

(defsrc a b)
(deflayer base mwu mwl)

mw* just scrolls one notch, without repeating (that's what mwheel-* actions are for)

@rszyma
Copy link
Contributor Author

rszyma commented Oct 25, 2023

I'm not sure why build-test-clippy-linux don't pass. It works for me locally.

Edit: Oh, it's because of 9516cbb

@rszyma
Copy link
Contributor Author

rszyma commented Oct 26, 2023

The thing that stands out to me most for my initial review is that I don't know that it's valuable to have the SupportedInputEvent type be sent to the processing loop. The processing loop has no way to deal with events other than plain button presses

Reverted some changes as per the above comment. However I deem it arguable if it's better this way, since it requires adding an "imaginary" Tap member to KeyValue enum.

src/kanata/linux.rs Outdated Show resolved Hide resolved
src/kanata/linux.rs Outdated Show resolved Hide resolved
src/kanata/mod.rs Show resolved Hide resolved
src/kanata/linux.rs Outdated Show resolved Hide resolved
src/kanata/linux.rs Outdated Show resolved Hide resolved
src/kanata/linux.rs Outdated Show resolved Hide resolved
src/oskbd/mod.rs Outdated Show resolved Hide resolved
parser/src/keys/mod.rs Outdated Show resolved Hide resolved
src/kanata/linux.rs Outdated Show resolved Hide resolved
@rszyma
Copy link
Contributor Author

rszyma commented Oct 30, 2023

Tested on linux. One time when I ran with this config

(defsrc mwu)
(deflayer base a)

when scrolling up it both scrolled and outputted a. It fixed itself after I restarted kanata and can't reproduce since no matter how many times I try. Weird.

@jtroo jtroo merged commit 28bed11 into jtroo:main Nov 4, 2023
3 checks passed
@rszyma rszyma deleted the defsrc-scroll branch November 4, 2023 06:01
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.

Feature request: add mouse wheel events to defsrc in kanata wintercept
2 participants