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

Lock when changing selection endpoint on wheel/auto-scroll #5551

Merged
merged 1 commit into from
Apr 24, 2020

Conversation

miniksa
Copy link
Member

@miniksa miniksa commented Apr 24, 2020

Summary of the Pull Request

Takes the lock inside two routines in TermControl that were changing the selection endpoint while a rendering frame was still drawing, resulting in several variants of graphical glitches from double-struck selection boxes to duplicated line text.

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

The renderer base and specific renderer engine do a lot of work to remember the previous selection and compensate for scrolling regions and deltas between frames. However, all that work doesn't quite match up when the endpoints are changed out from under it. Unfortunately, TermControl doesn't have a robust history of locking correctly in step with the renderer nor does the renderer's IRenderData currently provide any way of 'snapping' state at the beginning of a frame so it could work without a full lock. So the solution for now is for the methods that scroll the display in TermControl to take the lock that is shared with the renderer's frame painter so they can't change out of sync.

Validation Steps Performed

  • - Opened terminal with Powershell core. Did ls a bunch of times. Clicked to make selection and held mouse button while wheeling around.
  • - Opened terminal with Powershell core. Did ls a bunch of times. Clicked to make selection and dragged mouse outside the window to make auto scroll happen.
  • - Opened terminal with Powershell core. Did ls a bunch of times. Clicked to make selection and released. Wheeled around like a crazy person to make sure I didn't regress that.

@miniksa miniksa added 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. labels Apr 24, 2020
@miniksa miniksa self-assigned this Apr 24, 2020
@DHowett-MSFT
Copy link
Contributor

Don’t auto merge because it will delete the body of the message

@DHowett-MSFT DHowett-MSFT changed the title Lock while changing selection endpoint on wheel & auto-scroll to not confuse renderer Lock when changing selection endpoint on wheel/auto-scroll Apr 24, 2020
@DHowett-MSFT DHowett-MSFT merged commit 98ac196 into master Apr 24, 2020
@DHowett-MSFT DHowett-MSFT deleted the dev/miniksa/racey_mcgee branch April 24, 2020 22:14
@ghost
Copy link

ghost commented May 5, 2020

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

Handy links:

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.

differential: selection gets struck multiple times while scrolling
3 participants