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.

Sign up for GitHub

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

Jump to bottom

Fix a race condition in pane animation completion that broke sizing #8241

Merged
1 commit merged into from
Nov 18, 2020
Merged

Fix a race condition in pane animation completion that broke sizing #8241

1 commit merged into from
Nov 18, 2020

Conversation

Don-Vito
Copy link
Contributor

@Don-Vito Don-Vito commented Nov 12, 2020 •
edited by DHowett
Loading

Fixes a race in pane entrance animation, where the animation
completion routine processes wrong children.

Validated w/ manual testing

Closes #8230

@ghost ghost added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. labels Nov 12, 2020
@Don-Vito
Copy link
Contributor Author

@zadjii-msft - please review

@Don-Vito
Copy link
Contributor Author

Don-Vito commented Nov 12, 2020 •
edited
Loading

I think there is also a race in Close animation. But suggest to handle it separately

Hmmm... it seems you already have discussion about it here: #7942

zadjii-msft
zadjii-msft approved these changes Nov 12, 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.

Well this code just looks better IMO, even without the locking thing. Let's do this to fix #8230, and try the locking thing to fix #7942

@Don-Vito
Copy link
Contributor Author

Don-Vito commented Nov 12, 2020 •
edited
Loading

@zadjii-msft - without locking there will still be a race - since the control is reattached to other panes (upon close / split). Yet I hope this fix reduces the severity (to potential performance degradation due to reflows rather than potentially non-stretching terminal)

@Don-Vito
Copy link
Contributor Author

@DHowett - WDYT about this one? It doesn't fully solve the issue, but can it be a mitigation for 1.5?

@Don-Vito
Copy link
Contributor Author

Bumping this - at is marked as Priority 1 and almost a week-old 😊.
If this solution is sufficient as a mitigation? If not I could work on a more complete one.

@Don-Vito
Copy link
Contributor Author

@DHowett - Now that I know that you are somewhere here, just let me know if you like this one as a mitigation, or should we abandon in it until the fully fledged solution is found? 😊

@DHowett
Copy link
Member

DHowett commented Nov 18, 2020

Slowly working through my list 😄

@Don-Vito
Copy link
Contributor Author

Slowly working through my list 😄

Slowly? 😆 I wish I had 5 minutes of your "slow" attention once in few days, and I could hopefully contribute much more.

@DHowett
Copy link
Member

DHowett commented Nov 18, 2020

This looks way way better
Thanks 😄

DHowett
DHowett approved these changes Nov 18, 2020
View reviewed changes
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.

love it

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 18, 2020
@ghost
Copy link

ghost commented Nov 18, 2020

Hello @DHowett!

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.

@DHowett DHowett changed the title 8230: Some panes stop resizing after other panes are closed Fix a race condition in pane animation completion that broke sizing Nov 18, 2020
@DHowett DHowett added AutoMerge Marked for automatic merge by the bot when requirements are met and removed AutoMerge Marked for automatic merge by the bot when requirements are met labels Nov 18, 2020
@ghost ghost merged commit c41e078 into microsoft:main Nov 18, 2020
@Don-Vito Don-Vito deleted the fet/8230-restore-stretch-animation branch December 3, 2020 17:02
DHowett pushed a commit that referenced this pull request Jan 25, 2021
@Don-Vito @DHowett
Fix a race condition in pane animation completion that broke sizing (#…
c5e1bbd 
…8241)

Fixes a race in pane entrance animation, where the animation
completion routine processes wrong children.

Validated w/ manual testing

Closes #8230

(cherry picked from commit c41e078)
@ghost
Copy link

ghost commented Jan 28, 2021

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

Handy links:

@ghost ghost mentioned this pull request Jan 28, 2021
Closed
@ghost
Copy link

ghost commented Jan 28, 2021

🎉Windows Terminal Preview v1.6.10272.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
Reviewers

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

@DHowett DHowett DHowett approved these changes

Assignees
No one assigned
Labels
Area-UserInterface Issues pertaining to the user interface of the Console or Terminal 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-1 A description (P1) Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Some panes stop resizing after other panes are closed
3 participants
@Don-Vito @DHowett @zadjii-msft