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

Update pattern locations again after scrolling #11059

Merged
1 commit merged into from
Aug 26, 2021

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Aug 26, 2021

This is on me. When I got rid of the _updatePatternLocations ThrottledFunc in the TermControl, I didn't add a matching call to _updatePatternLocations->Run() in this method.

In #9820, in TermControl::_ScrollPositionChanged, there was still a call to _updatePatternLocations->Run();. (TermControl.cpp:1655 on the right) https://github.com/microsoft/terminal/pull/9820/files#diff-c10bb023995e88dac6c1d786129284c454c2df739ea547ce462129dc86dc2697R1654

#10051 didn't change this

In #10187 I moved the _updatePatternLocations throttled func from termcontrol to controlcore. Places it existed before:

  • TermControl::_coreReceivedOutput: already matched by ControlCore::_connectionOutputHandler
  • TermControl::_ScrollbarChangeHandler -> added in c20eb9d
  • TermControl::_ScrollPositionChanged -> ControlCore::_terminalScrollPositionChanged

Validation Steps Performed

Print a URL, scroll the wheel: it still works.

Closes #11055

@ghost ghost added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree. labels Aug 26, 2021
@carlos-zamora carlos-zamora added the Needs-Second It's a PR that needs another sign-off label Aug 26, 2021
@DHowett
Copy link
Member

DHowett commented Aug 26, 2021

@msftbot merge this in 1 minute

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 26, 2021
@ghost
Copy link

ghost commented Aug 26, 2021

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Thu, 26 Aug 2021 18:37:40 GMT, which is in 1 minute

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@ghost ghost merged commit 7423734 into main Aug 26, 2021
@ghost ghost deleted the dev/migrie/b/11055-urls-no-worky branch August 26, 2021 18:57
DHowett pushed a commit that referenced this pull request Aug 26, 2021
This is on me. When I got rid of the `_updatePatternLocations` `ThrottledFunc` in the `TermControl`, I didn't add a matching call to `_updatePatternLocations->Run()` in this method.

In #9820, in `TermControl::_ScrollPositionChanged`, there was still a call to `_updatePatternLocations->Run();`. (TermControl.cpp:1655 on the right) https://github.com/microsoft/terminal/pull/9820/files#diff-c10bb023995e88dac6c1d786129284c454c2df739ea547ce462129dc86dc2697R1654

#10051 didn't change this

In #10187 I moved the `_updatePatternLocations` throttled func from termcontrol to controlcore. Places it existed before:
* [x] `TermControl::_coreReceivedOutput`: already matched by ControlCore::_connectionOutputHandler
* [x] `TermControl::_ScrollbarChangeHandler` -> added in c20eb9d
* [x] `TermControl::_ScrollPositionChanged` -> `ControlCore::_terminalScrollPositionChanged`

## Validation Steps Performed
Print a URL, scroll the wheel: it still works.

Closes #11055

(cherry picked from commit 7423734)
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1.11: scrolling at all breaks the URL pattern buffer (no URLs work)
3 participants