Skip to content

Trigger scroll on scrolled selection#5798

Merged
DHowett-MSFT merged 1 commit intomasterfrom
dev/cazamor/bugfix-search-selection
May 8, 2020
Merged

Trigger scroll on scrolled selection#5798
DHowett-MSFT merged 1 commit intomasterfrom
dev/cazamor/bugfix-search-selection

Conversation

@carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented May 7, 2020

Summary of the Pull Request

We accidentally missed switching one TriggerRedrawAll to TriggerScroll. This does that.

References

#5185 - applies logic from this PR

PR Checklist

Validation Steps Performed

Followed bug repro steps.

@carlos-zamora carlos-zamora added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Product-Terminal The new Windows Terminal. labels May 7, 2020
@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. labels May 7, 2020
zadjii-msft
zadjii-msft approved these changes May 7, 2020
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kinda shocked that this fixes #5756 but ¯\_(ツ)_/¯

DHowett-MSFT
DHowett-MSFT suggested changes May 7, 2020
View reviewed changes
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Investigate to make sure that callers of SelectNewRegion call it under lock.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 7, 2020
@carlos-zamora
Copy link
Member Author

carlos-zamora commented May 7, 2020

@DHowett-MSFT
Trace for SelectNewRegion:

  TermControl::_Search
  --> LockForWriting()
  --> Search::Select()
      --> Terminal::SelectNewRegion()
          --> // Scroll Logic
          --> TriggerScroll

The only other time SelectNewRegion is called is via UiaTextRange::Select, where we start off by doing...

    _pData->LockConsole();
    auto Unlock = wil::scope_exit([&]() noexcept {
        _pData->UnlockConsole();
    });

So, I think that covers it. Though I do agree with @zadjii-msft that this doesn't seem like the complete solution. hmm.

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 7, 2020
@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented May 7, 2020

Sorry, I'm missing something here. Does Search::Select() end up taking the lock?

@carlos-zamora
Copy link
Member Author

carlos-zamora commented May 8, 2020

Sorry, I'm missing something here. Does Search::Select() end up taking the lock?

We lock in TermControl::Select() before calling Search::Select. So yes?

@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented May 8, 2020

Excellent.

@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented May 8, 2020

Just to be sure: you had a 100% surefire repro before this, and now you don't? Wow

miniksa
miniksa approved these changes May 8, 2020
View reviewed changes
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep.

@carlos-zamora
Copy link
Member Author

carlos-zamora commented May 8, 2020

Just to be sure: you had a 100% surefire repro before this, and now you don't? Wow

Yep. I tested it on 0.11 and this branch side-by-side. I'm surprised too haha.

@DHowett-MSFT DHowett-MSFT merged commit 75752ae into master May 8, 2020
@DHowett-MSFT DHowett-MSFT deleted the dev/cazamor/bugfix-search-selection branch May 8, 2020 21:48
DHowett-MSFT pushed a commit that referenced this pull request May 8, 2020
@carlos-zamora @DHowett
Trigger scroll on scrolled selection (#5798)
8ece137 
## Summary of the Pull Request
We accidentally missed switching one `TriggerRedrawAll` to `TriggerScroll`. This does that.

## References
#5185 - applies logic from this PR

## PR Checklist
* [X] Closes #5756

## Validation Steps Performed
Followed bug repro steps.

(cherry picked from commit 75752ae)
@ghost
Copy link

ghost commented May 13, 2020

🎉Windows Terminal Release Candidate v0.11.1333.0 (1.0rc2) has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request May 13, 2020
Closed
jelster pushed a commit to jelster/terminal that referenced this pull request May 28, 2020
@carlos-zamora @jelster
Trigger scroll on scrolled selection (microsoft#5798)
f3612d9 
## Summary of the Pull Request
We accidentally missed switching one `TriggerRedrawAll` to `TriggerScroll`. This does that.

## References
microsoft#5185 - applies logic from this PR

## PR Checklist
* [X] Closes microsoft#5756

## Validation Steps Performed
Followed bug repro steps.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miniksa miniksa miniksa approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

+1 more reviewer

@DHowett-MSFT DHowett-MSFT DHowett-MSFT approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Multiple Selections Rendered with Search Box

4 participants

@carlos-zamora @DHowett-MSFT @miniksa @zadjii-msft