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

Fix ATS tab status indicators #9076

Merged
6 commits merged into from Feb 10, 2021
Merged

Fix ATS tab status indicators #9076

6 commits merged into from Feb 10, 2021

Conversation

Don-Vito
Copy link
Contributor

@Don-Vito Don-Vito commented Feb 8, 2021

  1. Fix progress value not updated
  2. Introduce TabStatus object and bind both TabHeaderControl and CommandPalette to it
  3. Add support for read-only mode indicator

@Don-Vito
Copy link
Contributor Author

Don-Vito commented Feb 8, 2021

@zadjii-msft - I missed the progress value in the original PR 😢

Instead of an interface I creates a TabStatus object and bind all controls to it.

Marked as draft, as for some reason the ATS bindings are sometimes not updated.

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 so far I'm cool with what's here.

src/cascadia/TerminalApp/TerminalTab.cpp Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalTab.cpp Outdated Show resolved Hide resolved
@Don-Vito
Copy link
Contributor Author

Don-Vito commented Feb 9, 2021

@zadjii-msft - for some reasons nested bindings were not updating to me.. so I added a code in the TabPaletteItem that notifies changes on the entire status if one of the indicators changes. This should have no implications, but it is definitely annoying and feels like voodoo.

@Don-Vito Don-Vito marked this pull request as ready for review February 9, 2021 20:03
@Don-Vito
Copy link
Contributor Author

Don-Vito commented Feb 9, 2021

@carlos-zamora - please take a look as well. There are some gaps I missed originally + I did something simpler to avoid properties duplication by now.

@Don-Vito Don-Vito removed their assignment Feb 9, 2021
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

omg this is so much cleaner. Thank you!

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.

:shipit:

@zadjii-msft
Copy link
Member

Thanks for hoping on this so quickly. What a fun bit of polish ☺️

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

ghost commented Feb 10, 2021

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 8b2cdfd into microsoft:main Feb 10, 2021
@ghost
Copy link

ghost commented Mar 1, 2021

🎉Windows Terminal Preview v1.7.572.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
AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants