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.

Sign up for GitHub

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

Jump to bottom

1.11: scrolling at all breaks the URL pattern buffer (no URLs work) #11055

Closed
DHowett opened this issue Aug 26, 2021 · 3 comments · Fixed by #11059
Closed

1.11: scrolling at all breaks the URL pattern buffer (no URLs work) #11055

DHowett opened this issue Aug 26, 2021 · 3 comments · Fixed by #11059
Assignees
@zadjii-msft
Labels
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. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Blocking We won't ship a release like this! No-siree.
Milestone
Terminal v1.11

Comments

@DHowett
Copy link
Member

DHowett commented Aug 26, 2021

Windows Terminal version (or Windows build number)

1.11

Other Software

No response

Steps to reproduce

Emit a URL
Scroll -- even downwards off the bottom of the screen where you can't scroll

Expected Behavior

No response

Actual Behavior

URLs stop hovering!
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Aug 26, 2021
@zadjii-msft zadjii-msft self-assigned this Aug 26, 2021
@zadjii-msft zadjii-msft 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
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Aug 26, 2021
@zadjii-msft zadjii-msft added this to the Terminal v1.11 milestone Aug 26, 2021
@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Aug 26, 2021
@zadjii-msft
Copy link
Member

I knew there was no way that #9955 was totally done 😋

@zadjii-msft
Copy link
Member

zadjii-msft commented Aug 26, 2021 •
edited
Loading

guh Even just scrolling the wheel (even if there's nowhere to scroll) will break this!

EDIT: Investigation notes:

  • The wheel will get us into this state without ever firing Terminal::UpdatePatternsUnderLock

c20eb9d has most of a fix in it

zadjii-msft added a commit that referenced this issue Aug 26, 2021
@zadjii-msft
This fixes the root cause of #11055, but the hover popup doesn't work…
c20eb9d 
… after scrolling 😕
@zadjii-msft
Copy link
Member

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
  • TermControl::_ScrollPositionChanged

There's an existing call in ControlCore::_terminalScrollPositionChanged.

I added a call to Run() to ControlCore::UserScrollViewport in c20eb9d.

I believe TermControl::_ScrollbarChangeHandler -> ControlCore::UserScrollViewport, and TermControl::_ScrollPositionChanged -> ControlCore::_terminalScrollPositionChanged

@zadjii-msft zadjii-msft mentioned this issue Aug 26, 2021
Merged
3 tasks
@ghost ghost added the In-PR This issue has a related PR label Aug 26, 2021
@ghost ghost closed this as completed in #11059 Aug 26, 2021
@ghost ghost removed the In-PR This issue has a related PR label Aug 26, 2021
ghost pushed a commit that referenced this issue Aug 26, 2021
@zadjii-msft
Update pattern locations again after scrolling (#11059)
7423734 
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
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Aug 26, 2021
DHowett pushed a commit that referenced this issue Aug 26, 2021
@zadjii-msft @DHowett
Update pattern locations again after scrolling (#11059)
3052ff7 
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 issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zadjii-msft zadjii-msft

Labels
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. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Blocking We won't ship a release like this! No-siree.
Projects
None yet
Milestone
Terminal v1.11
Development

Successfully merging a pull request may close this issue.

Update pattern locations again after scrolling microsoft/terminal
2 participants
@DHowett @zadjii-msft