Scrollfix #4554
Closed
Scrollfix #4554
Conversation
This file contains 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
Directly apply fractional delta values to scrollbar instead of rounding them first before applying.
Directly apply fractional delta values to scrollbar instead of rounding them first before applying.
leonMSFT
approved these changes
Feb 14, 2020
ghost
requested review from
zadjii-msft,
miniksa,
carlos-zamora and
DHowett-MSFT
carlos-zamora
requested changes
Feb 14, 2020
There was a problem hiding this comment.
@miniksa or @DHowett-MSFT I think we don't have Chromium Math in this repo. Is it ok to #include that at the top of this or was there a particular reason we didn't do that.
@jamestut Just in case you're not familiar with it, for reference on how to use this Chromium Math, here's the doc or just search our repo for base::Clamp and you should find a decent amount of examples.
Works great! 😊
| const auto currentOffset = this->GetScrollOffset(); | ||
| const double newValue = (numRows) + (currentOffset); | ||
| const auto currentOffset = _scrollBar.Value(); | ||
| const double newValue = numRows + currentOffset; |
There was a problem hiding this comment.
use safemath here to force a clamped double.
There was a problem hiding this comment.
I used the chromium's safemath here for the clamped addition on the 3rd commit of this fork.
ghost
added
the
Needs-Author-Feedback
|
#4179 is awaiting someone to pick up before we use chromium math in the repo. |
ghost
removed
the
Needs-Author-Feedback
|
Argh: I'm really sorry about the conflict. You will need to save and commit TermControl as CRLF, then merge in master. I was not aware that we had messed up the line endings so badly. :/ |
|
FWIW: i did push a fixed version here; 9dc594b |
DHowett-MSFT
approved these changes
Feb 19, 2020
|
I pushed this commit to master after doing a build test myself. Consider this merged, and congrats on your first contribution! |
DHowett
added a commit
that referenced
this pull request
Jul 13, 2020
This parameter was added as a workaround for our fast trackpad scrolling. Since that was fixed before 1.0 shipped, in #4554, it has been largely vestigial. There is no reason for us to keep it around any longer. It was also the only "logic" in TerminalSettings, which is otherwise a library that only transits data between two other libraries. I have not removed it from the schema, as I do not want to mark folks' settings files invalid to a strict schema parser. While I was in the area, I added support for "scroll one screen at a time" (which is represented by the API returning -1), fixing #5610. Fixes #5610
DHowett
added a commit
that referenced
this pull request
Jul 13, 2020
This parameter was added as a workaround for our fast trackpad scrolling. Since that was fixed before 1.0 shipped, in #4554, it has been largely vestigial. There is no reason for us to keep it around any longer. It was also the only "logic" in TerminalSettings, which is otherwise a library that only transits data between two other libraries. I have not removed it from the schema, as I do not want to mark folks' settings files invalid to a strict schema parser. While I was in the area, I added support for "scroll one screen at a time" (which is represented by the API returning WHEEL_PAGESCROLL), fixing #5610. We were also storing it in an int (whoops) instead of a uint. Fixes #5610
ghost
pushed a commit
that referenced
this pull request
Jul 14, 2020
#6891) This parameter was added as a workaround for our fast trackpad scrolling. Since that was fixed before 1.0 shipped, in #4554, it has been largely vestigial. There is no reason for us to keep it around any longer. It was also the only "logic" in TerminalSettings, which is otherwise a library that only transits data between two other libraries. I have not removed it from the schema, as I do not want to mark folks' settings files invalid to a strict schema parser. While I was in the area, I added support for "scroll one screen at a time" (which is represented by the API returning WHEEL_PAGESCROLL), fixing #5610. We were also storing it in an int (whoops) instead of a uint. Fixes #5610
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.
Summary of the Pull Request
Fixed inconsistent scrolling when using both touchscreen and precision touchpad.
References
#1066 and #4542
PR Checklist
Detailed Description of the Pull Request / Additional comments
Scrolling jumpiness is caused by rounding errors. Instead of retrieving the current scrolling value from
GetScrollOffset, which is already rounded inint, let the scrolling operation to operate on_scrollBar'sValuedirectly (which usesdouble) inTermControl.cpp.Validation Steps Performed
Testing scrolling on the following scenario manually:
nanoandmorein WSL.has the same behavior between using touchscreen or precision trackpad and regular mouse wheel.