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

Initialize cursorOn to match focus state #12094

Merged
1 commit merged into from
Jan 10, 2022
Merged

Conversation

zadjii-msft
Copy link
Member

I'm not 100% sure that this is the right solution, but it does seem to work well enough. This is unfortunately a classic heisenbug. It was already hard enough to repro originally, but attaching a debugger made it totally impossible to hit.

My theory is that it's possible for the GotFocus event to fire before the LayoutUpdated event does. If that were to occur, then we'd try to turn on the cursor timer before it exists, gracefully do nothing, then create the timer. In that case, we'd never get a subsequent message to start the blinking.

I tested that theory by just initializing the cursor blinker to our _focused state. In that case, if the control has already been focused at the time of the LayoutUpdated event, then we can init the cursor to the correct state. Testing that out, I couldn't once get this to repro, which makes me think this works. I've opened some 900 (hyperbole) tabs now, so I'm pretty confident I'd have seen it by now.

@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. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Jan 5, 2022
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

It doesn't seem like a bad fix

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.

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 10, 2022
@ghost
Copy link

ghost commented Jan 10, 2022

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 55aea08 into main Jan 10, 2022
@ghost ghost deleted the dev/migrie/b/11411-cursor-visibility branch January 10, 2022 20:24
@DHowett DHowett added the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label Jan 21, 2022
DHowett pushed a commit that referenced this pull request Jan 21, 2022
I'm not 100% sure that this is the right solution, but it does seem to work well enough. This is unfortunately a classic heisenbug. It was already hard enough to repro originally, but attaching a debugger made it totally impossible to hit.

My theory is that it's possible for the GotFocus event to fire before the LayoutUpdated event does. If that were to occur, then we'd try to turn on the cursor timer before it exists, gracefully do nothing, then create the timer. In that case, we'd never get a subsequent message to start the blinking.

I tested that theory by just initializing the cursor blinker to our `_focused` state. In that case, if the control has already been focused at the time of the LayoutUpdated event, then we can init the cursor to the correct state. Testing that out, I couldn't once get this to repro, which makes me think this works. I've opened some 900 (<sup>hyperbole</sup>) tabs now, so I'm pretty confident I'd have seen it by now.

* Regressed in #10978
* [x] fixes #11411
* [x] I made sure I didn't regress #6586
* [x] I work here

(cherry picked from commit 55aea08)
@DHowett DHowett removed the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label Jan 26, 2022
@ghost
Copy link

ghost commented Feb 3, 2022

🎉Windows Terminal Preview v1.13.10336.0 has been released which incorporates this pull request.:tada:

Handy links:

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. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No cursor (randomly) on profile startup
3 participants