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 (444de5b166/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.
This commit is contained in:
greg904 2020-02-11 00:15:30 +01:00 committed by GitHub
parent 8f08fe15dc
commit 4f6916c2da
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 2263 additions and 2287 deletions

View file

@ -244,7 +244,7 @@ namespace winrt::TerminalApp::implementation
co_await winrt::resume_foreground(control.Dispatcher());
const auto currentOffset = control.GetScrollOffset();
control.KeyboardScrollViewport(currentOffset + delta);
control.ScrollViewport(currentOffset + delta);
}
// Method Description:

File diff suppressed because it is too large Load diff

View file

@ -70,7 +70,6 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
float SnapDimensionToGrid(const bool widthOrHeight, const float dimension) const;
void ScrollViewport(int viewTop);
void KeyboardScrollViewport(int viewTop);
int GetScrollOffset();
int GetViewHeight() const;
@ -135,9 +134,10 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
FontInfoDesired _desiredFont;
FontInfo _actualFont;
int _rowsToScroll;
bool _isTerminalInitiatedScroll;
std::atomic<bool> _willUpdateScrollBarToMatchViewport;
std::optional<int> _lastScrollOffset;
int _rowsToScroll;
// Auto scroll occurs when user, while selecting, drags cursor outside viewport. View is then scrolled to 'follow' the cursor.
double _autoScrollVelocity;

View file

@ -49,7 +49,6 @@ namespace Microsoft.Terminal.TerminalControl
Single SnapDimensionToGrid(Boolean widthOrHeight, Single dimension);
void ScrollViewport(Int32 viewTop);
void KeyboardScrollViewport(Int32 viewTop);
Int32 GetScrollOffset();
Int32 GetViewHeight();
event ScrollPositionChangedEventArgs ScrollPositionChanged;