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

Trigger scroll on scrolled selection #5798

Merged
merged 1 commit into from
May 8, 2020

Conversation

carlos-zamora
Copy link
Member

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
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 ¯\_(ツ)_/¯

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

@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

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

@carlos-zamora
Copy link
Member Author

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

Excellent.

@DHowett-MSFT
Copy link
Contributor

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

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

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
## 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:

jelster pushed a commit to jelster/terminal that referenced this pull request May 28, 2020
## 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
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
Development

Successfully merging this pull request may close these issues.

Multiple Selections Rendered with Search Box
4 participants