Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Fix scrollbar doesn't update viewport after window resize (#3344)
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? --> ## Summary of the Pull Request Fixes a bug where scrolling up/down doesn't update the viewport after the window is resized and in other cases. Also changes other things, please read the detailed description. <!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> ## References <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist * [x] Closes #1494 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Requires documentation to be updated * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx <!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here --> ## Detailed Description of the Pull Request / Additional comments There are two ways scroll can happen: - the user scrolls using the scroll bar and the `Terminal` is notified - the `Terminal` changed the viewport and the scroll bar is updated to reflect the change The code to notify the `Terminal` that the user scrolled is in the event handler for when the scroll bar's value changes. However this poses a problem because it means that when the `Terminal` changes the viewport, the scroll bar is updated so it would then also notify the `Terminal` that the scroll changed. But it already knows because it's coming from itself! To fix this, the `TermControl` class had a member called `_lastScrollOffset` that would be set when the `Terminal` decides to change the viewport so that the event handler for the scroll bar could check the new scroll value against `_lastScrollOffset` and if it matches, then everything is fine and there is nothing to update. This is what happens when the `Terminal` changes the viewport: 1. set `_lastScrollOffset` 2. dispatch job on the UI thread: update the scrollbar which is going to call the event handler which is going to check for `_lastScrollOffset` and clear it There are two bugs introduced by this approach: 1. (I am not sure about this.) The dispatcher appears to store jobs in a LIFO stack so it sometimes reorders the "update the scrollbar" jobs when there are too many. When I run `1..10000` on PowerShell, then I get this from the event handler (format: `_lastScrollOffset newValue`): ``` 8988 8988 8989 8989 8990 8990 8992 8991 8993 8992 ... 9001 8997 9001 8998 9001 8999 9001 9000 9001 9001 9001 8985 9001 8968 9001 8953 ... 9001 7242 9001 7226 9001 7210 ``` This causes the following issues: 1. `_lastScrollOffset` wouldn't be reset because it wouldn't be equal to the current scroll bar value (see example above) so the next scrolls wouldn't do anything as the event handler would still be waiting for an event with the good scroll bar value which would never happen because it happened earlier 2. the `TermControl` would notify the `Terminal` about its own scroll 2. If the `Terminal` didn't actually changed its viewport but still called the `TermControl::_TerminalScrollPositionChanged` method, then it would set the `_lastScrollOffset` member as usual but the scroll bar value change event handler would not be called because it is only called when the value actually changes so the `_lastScrollOffset` member wouldn't be cleared and subsequent scroll bar value change events would be ignored because again the event handler would still be waiting for an event with the good scroll bar value which would never happen. This is actually the reason for #1494: when the window is resized, the `Terminal` will call `TermControl::_TerminalScrollPositionChanged` even if the scroll position didn't actually change (https://github.com/microsoft/terminal/blob/444de5b166d800b17c90510cde8664aefffcb236/src/cascadia/TerminalCore/Terminal.cpp#L183). Maybe this should also be fixed in another PR? I replaced `_lastScrollOffset` by a flag `_isTerminalInitiatedScroll`. I set the flag just before and unset it just after the terminal changes the scrollbar on the UI thread to eliminate the race conditions and the bug when the scroll bar's value doesn't actually change. Other changes: - I also fixed a potential bug where if the user scrolls just after the terminal updates the viewport, it would en up ignoring the user scroll. To do this, when the user scrolls, I cancel any update with `_willUpdateScrollBarToMatchViewport`. - I also removed the original `ScrollViewport` method because it was not used anywhere and I think it can potentially create confusion (and therefore bugs) because this method updates the viewport but not the scroll bar unlike `KeyboardScrollViewport` which functions as you would expect. I then renamed `KeyboardScrollViewport` into `ScrollViewport`. So, now, there is only one method to scroll the viewport from the `TermControl`. Please, tell me if this shouldn't be in this PR. - I also removed `_terminal->UserScrollViewport(viewTop);` in the `KeyboardScrollViewport` method because it will be updated later anyways in the scroll bar's value change event handler because of the `_scrollBar.Value(viewTop);`. <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> ## Validation Steps Performed I tested manually by doing this: - For bug 1: 1. Start the terminal 2. Run the `1..30000` command in PowerShell and wait for it to end (maybe more if you have a fast computer?) 3. Hold left click on the scrollbar slider and start moving it - For bug 2: 1. Start the terminal 2. Run the `1..100` command in PowerShell and wait for it to end 3. Resize the window horizontally 4. Hold left click on the scrollbar slider and start moving it Without this patch, the viewport doesn't update. With the patch, the viewport updates correctly.
- Loading branch information