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

Set Progress ring color based on the status from the running process #15847

Closed
wants to merge 3 commits into from

Conversation

ocalvo
Copy link
Contributor

@ocalvo ocalvo commented Aug 17, 2023

Summary of the Pull Request

Currently Terminal ignores the state flag passed from the VT progress sequence.

Errors

image

Warning

image

Normal

image

References and Relevant Issues

#6700

Detailed Description of the Pull Request / Additional comments

This change changes the default color of the progress ring to green. If errors are sent then the color changes to red. If warnings are detected the color changes to orange.

Validation Steps Performed

Tested end to end with tools that sends status updates.

PR Checklist

  • Closes #xxx
  • Tests added/passed
  • Documentation updated
    • If checked, please file a pull request on our docs repo and link it here: #xxx
  • Schema updated (if necessary)

@zadjii-msft
Copy link
Member

(I'm just gonna mark this as a draft while you work on this 😝 )

@zadjii-msft zadjii-msft marked this pull request as draft August 17, 2023 21:56
@ocalvo ocalvo changed the title Try red foreground Set Progress ring color based on the status from the running process Aug 17, 2023
@ocalvo ocalvo marked this pull request as ready for review August 17, 2023 22:31
@zadjii-msft
Copy link
Member

my 2c even though I'm out today: I think the default should still be your accent color / whatever the default color is. The others look sweet though

@DHowett
Copy link
Member

DHowett commented Aug 18, 2023

my 2c even though I'm gonna be out this afternoon: I love it, but I am worried about how it looks in high contrast or in other conditions where the user has color overrides :)

Comment on lines +1041 to +1046
const auto color =
(taskbarState == 2) ?
winrt::Windows::UI::Colors::Red() :
((taskbarState == 4) ?
winrt::Windows::UI::Colors::Orange() :
winrt::Windows::UI::Colors::Green());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'd prefer a switch statement here for readability!

Also for tastbarState = 3 we should probably set it to the default color (it currently adopts the color based on the previous state, so it could be any of red/orange/green depending).

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.

Other notes:

  • Let's leave the default (0) and the indeterminate(3) state just the accent color.
  • We need to use ThemeResources here. I think InfoBar uses theme brushes for the background, but those are more muted colors. I dunno if InfoBadge has a good "Warning" color.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 31, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Sep 7, 2023
@microsoft-github-policy-service
Copy link
Contributor

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. zBugBash-Consider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants