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

Fix: Scroll thresholds not applying to mouse debouncer #1440

Merged
merged 7 commits into from
Feb 6, 2024

Conversation

ImTheSquid
Copy link
Contributor

I have problems on my laptop where I switch panels when I meant to scroll through the tabs in a panel, so I added a threshold value that should take care of it. It defaults to 0 which is the original behavior.

@mbnuqw
Copy link
Owner

mbnuqw commented Feb 1, 2024

Thanks. Have you tried using "Set threshold for switching between panels and tabs..." > "Horizontal scrolling"? If so, and it didn't help, maybe we could try to improve these settings instead of creating a new one?

@ImTheSquid
Copy link
Contributor Author

I did try that originally to fix my problem, but it seems they are specifically hooked up to the mouse wheel debouncer getter that is more for an initial start on the wheel than setting a threshold for the deltaX for panel switching. I'm not sure if it makes sense to combine these settings, since the horizontal scrolling has the ability to change everything that uses a mouse listener, whereas my setting is very specific to switching panels.

@mbnuqw
Copy link
Owner

mbnuqw commented Feb 1, 2024

Hm... The purpose of "Set threshold for switching between panels and tabs..." was the exact same as in this PR - to prevent accidental switching in perpendicular direction on touchpads. IIRC, I even initially did this with similar code, but then started using a different approach with accumulative scrolled distance (in getWheelDebouncer).

If you set enough length-threshold for horizontal scrolling you won't get accidental panel switching because it will require you to scroll horizontally that distance - vertical scrolling won't be blocked so if you continue moving your fingers up/down tabs will be switched. And vice versa with vertical threshold.

It might be some bug if it doesn't work as I described.

@ImTheSquid
Copy link
Contributor Author

I set the two thresholds to 5000 each, and it still didn't have any effect, so I'm gonna guess that the debouncer code is broken. I'll try to figure out what's going on, but I'm not really sure what I'm looking at. My guess is that accumulating is probably breaking that feature, since if I scroll 10 px, it probably accumulates 10 + 9 + 8 + 7 + ... px. I might try disabling that and using a strict threshold without accumulation.

@ImTheSquid
Copy link
Contributor Author

Update: That fixed it!

@ImTheSquid ImTheSquid changed the title Feat: Panel switch threshold Fix: Scroll thresholds not applying to mouse debouncer Feb 1, 2024
@mbnuqw
Copy link
Owner

mbnuqw commented Feb 1, 2024

Sorry, I can't merge it since I can't reproduce the issue. Current "Set threshold for switching between panels and tabs..." settings works fine, preventing unintended switches. The algorithm accumulates delta distance from each event - if I scroll to 50px right, getWheelDebouncer will receive the sequence of events with deltaX, let's say: 10, 20, 20. And if the horizontal threshold set to 40px getWheelDebouncer will pass only a third event.

Setting 5000px for both thresholds completely blocks switching tabs and panels on my machine. Could you try to set that thresholds and reload sidebar, maybe the settings are updating incorrectly?

@ImTheSquid
Copy link
Contributor Author

This may be because I have an inertial trackpad. Using your configuration I was able to get vertical scrolling working fine, but now the limit to only switching one panel at once is completely broken, skipping panels immediately due to the accumulation. I have attached a video with the effect. I could make my changes a configuration setting so that users can turn it on and off, but this is not usable in its current state.

sideberySkipping.mp4

@mbnuqw
Copy link
Owner

mbnuqw commented Feb 5, 2024

Do your latest commits fix the problem with horizontal scrolling and I can merge it, or is the issue still there? Maybe instead add two "Scroll accumulation" options per scroll trajectory (horizontal/vertical) and put them right after each threshold input (Vertical scrolling (px) / Horizontal scrolling (px))? e.g.:

  • Vertical scrolling (px): 123
    • Accumulation mode: on/off
  • Horizontal scrolling (px): 123
    • Accumulation mode: on/off

@ImTheSquid
Copy link
Contributor Author

This does fix my problem, but I'll split the horizontal and vertical accumulation.

@ImTheSquid
Copy link
Contributor Author

I added both, but I put them in a separate subsection because custom thresholds are independent from accumulation behavior.

@mbnuqw
Copy link
Owner

mbnuqw commented Feb 6, 2024

Thank you!

custom thresholds are independent from accumulation behavior

They're independent, but related. Though, I went overboard in proposing a child-parent structure, it's true. I'll move them under the wheelThreshold, because both thresholds values and accumulation toggles only used in getWheelDebouncer which is active only if the wheelThreshold ("Set threshold for switching...") option is enabled.

Anyway, thank you again.

@mbnuqw mbnuqw merged commit 9b9ef17 into mbnuqw:v5 Feb 6, 2024
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.

2 participants