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

Relax locking around the swapchain #5225

Merged
merged 1 commit into from
Apr 7, 2020
Merged

Conversation

DHowett-MSFT
Copy link
Contributor

Summary of the Pull Request

The terminal lock is really only for the terminal; since the renderer is
fully owned by the control, not the Terminal, and we'll only be
receiving swap chain events after we register them during
initialization, we don't need to lock before or after firing off the
coroutine.

PR Checklist

Validation Steps Performed

Manual validation.

The terminal lock is really only for the terminal; since the renderer is
fully owned by the control, not the Terminal, and we'll only be
receiving swap chain events after we register them during
initialization, we don't need to lock before _or_ after firing off the
coroutine.

Fixes #5203.
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.

Yea I'm okay with this but pls make sure to add the GH# comment :)

}
}

// This event is only registered during terminal initialization,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// This event is only registered during terminal initialization,
// See GH#5203
// This event is only registered during terminal initialization,

Because otherwise I'd forget why this is okay

@DHowett-MSFT
Copy link
Contributor Author

This may not be entirely correct.

@DHowett-MSFT
Copy link
Contributor Author

I was wrong. It is not the cause of the crash in 5185.

@carlos-zamora
Copy link
Member

I was wrong. It is not the cause of the crash in 5185.

You mean #5203?

@DHowett-MSFT
Copy link
Contributor Author

@carlos-zamora no, I mean #5185.

@DHowett-MSFT DHowett-MSFT merged commit e44dc2b into master Apr 7, 2020
@DHowett-MSFT DHowett-MSFT deleted the dev/duhowett/retro-resize branch April 7, 2020 00:15
@ghost
Copy link

ghost commented Apr 22, 2020

🎉Windows Terminal Preview v0.11.1121.0 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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resizing a Terminal with retro terminal effects hangs the Terminal
4 participants