Remove the reset_scroll_progress action and keybinding #7990
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Marked as draft so i can test with a mouse scroll wheel and not just trackpad. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7990 +/- ##
==========================================
+ Coverage 93.01% 93.05% +0.04%
==========================================
Files 648 648
Lines 61463 61448 -15
==========================================
+ Hits 57171 57182 +11
+ Misses 4292 4266 -26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Behavior on scroll wheel is unaffected. |
TimMonko
approved these changes
Jun 20, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References and relevant issues
Closes: #7232
Description
There is a strange
Reset scrollviewer keybinding, which isn't clear what it does, but it is modifiable. It turns out it's connected to a hard-coded Control/Cmd-Scroll action to scroll through the dims.When scrolling, enough scroll needs to accumulate to trigger a slider movement. Then it's reset back to 0 and repeats. The Control/Cmd key needs to be held while scrolling, so what the
Reset scrollkeybinding is doing is reseting this accumulated amount back to zero, if someone stopped scrolling midway through a slice and then starts scrolling again.There is an issue though: The user can't change the actual modifier associated with the scroll action, so having only the reset be a changeable keybinding is confusing. If you change it, it effectively does nothing because you won't trigger it when trying to scroll the dims, which still triggers off Control/Cmd.
Further, doing this demonstrates that there is no issue, even with the scroll action having a "memory" of past fractional scrolling. After every step a reset already happens, so at most a fraction of a step when starting again is just not noticeable. If you were last scrolling forward, the next time you go to scroll you are slightly closer to the next slice--or further away from the previous one--sure, but in practice you don't notice it.
So I would argue that this confusing binding can just be removed. Which is what I do here.
This means I have to update the settings test that was using the keybinding because it was the first in the list, so I also did that.
Edit: using the first keybinding turned out to be in a lot of tests! And it wasn't always obvious. I added comments to make it easier to follow for the next time.