Implement VT-based progress reporting #883

chausner · 2021-04-21T22:57:00Z

This implements #869: if VT is enabled, OSC 9;4 sequences are printed in addition to the existing progress spinner and progress bar.

Whenever an indefinite spinner is printed, a corresponding OSC sequence with state "Indeterminate" is output. When the spinner is hidden, an OSC sequence to clear the progress is written. Analogously, OSC sequences for showing/updating/hiding determinate progress are printed.

I noticed that the implementation of the progress OSC sequences in Windows Terminal does not exactly match the implementation in ConEmu. In particular, ConEmu allows the progress value to be left out (e.g. to switch progress states without having to specify the current progress value again) but Windows Terminal does not recognize these sequences at the moment. It is not really an issue at all, there are just a couple of lines with workaround logic.

I added the new logic to the existing IndefiniteSpinner and ProgressBar classes, as opposed to creating a new class derived from ProgressVisualizerBase. The latter should work easily for determinate progress but for the spinner case, IndefiniteSpinner contains some logic with Sleep that would have to be duplicated if we want to separate the VT-logic into its own class, which is probably not so nice.

@JohnMcPMS mentioned potential issues with clearing the progress when a user forces a process exit by hitting Ctrl+C twice. I am able to reproduce this issue by running winget source update and quickly hitting Ctrl+C twice. In this case, the progress indicator in the taskbar is indeed not cleared but there are also other issues like the console foreground color not getting reset (the spinner uses a different color). So any fix for this issue should ideally not only reset the taskbar progress bar but also any other terminal state like colors.

Microsoft Reviewers: Open in CodeFlow

yao-msft · 2021-04-21T23:04:42Z

/azp run

azure-pipelines · 2021-04-21T23:04:51Z

Azure Pipelines successfully started running 1 pipeline(s).

JohnMcPMS

I was promised this would take a week 😉

This is exactly what I would have done, and I think will work well. I do wonder though if we should actually set the indeterminate state as soon as we hit anything with progress, then not disable it until we exit. By which I mean, rather than None right now, we would write Indeterminate (or nothing if we already know that is the state we are in), and have set a flag somewhere that on destruction/exit we write None to close it out.

I'm not sure what it looks like right now when it flips back and forth, or maybe those happen so fast that it isn't really noticeable. That might just need to be something we give a chance and see if we need to improve on it.

src/AppInstallerCLICore/VTSupport.h

src/AppInstallerCLICore/VTSupport.cpp

src/AppInstallerCLICore/VTSupport.h

src/AppInstallerCLICore/VTSupport.cpp

Co-authored-by: JohnMcPMS <johnmcp@microsoft.com>

…eter to uint32_t

chausner · 2021-04-22T17:36:20Z

I do wonder though if we should actually set the indeterminate state as soon as we hit anything with progress, then not disable it until we exit. By which I mean, rather than None right now, we would write Indeterminate (or nothing if we already know that is the state we are in), and have set a flag somewhere that on destruction/exit we write None to close it out.

I'm not sure what it looks like right now when it flips back and forth, or maybe those happen so fast that it isn't really noticeable. That might just need to be something we give a chance and see if we need to improve on it.

Good point. It seems it can look slightly distracting when it switches states multiple times in quick succession but most people probably wouldn't notice. Your proposal of always staying in the Indeterminate state would probably work, it just makes the code slightly less nice or might lead to issues later on (suppose winget adds some form of user interaction/confirmation in the future, if there was a spinner displayed before, it would continuously show the Intermediate state while waiting for user input. That would probably feel distracting.).

So I suggest us to get a bit experience first how the current implementation feels in practice, then can decide if we need to tweak it or if it is fine as it is.

denelon · 2021-04-22T17:40:12Z

I'll defer to @JohnMcPMS on the initial implementation. I'd be happy to solicit feedback from the community, and I can likely get @cinnamon-msft to help get feedback from Windows Terminal users. Just let me know what scenarios we want to ask for the community to test.

jsoref · 2021-04-22T17:47:11Z

src/AppInstallerCLICore/ExecutionProgress.cpp

-            // Multi-terabyate installers should be fairly rare for the foreseeable future...
+            // Multi-terabyte installers should be fairly rare for the foreseeable future...


Oops. Thanks for catching this. Initial spell-checking passes are best effort, and a goal is for people to incrementally improve.

JohnMcPMS · 2021-04-22T17:48:37Z

/azp run

azure-pipelines · 2021-04-22T17:48:46Z

Azure Pipelines successfully started running 1 pipeline(s).

JohnMcPMS

Thanks, I look forward to trying it out.

denelon · 2021-04-22T17:59:25Z

@chausner I'm hoping to get this out in a build next week. I'll tweet about it as well. Would you like me to @mention you or not?

If you would just let me know your Twitter handle.

chausner · 2021-04-22T18:22:45Z

@chausner I'm hoping to get this out in a build next week. I'll tweet about it as well.

Cool 👍

If you would just let me know your Twitter handle.

Thanks but I am not on Twitter. But feel free to mention my GitHub name in the next blog post in case you mention the feature there. :)

JohnMcPMS · 2021-04-22T18:45:29Z

The only part of the build that failed was publishing the test result artifacts, so I'm going to go ahead and merge.

DHowett · 2021-04-26T16:52:55Z

src/AppInstallerCLICore/VTSupport.cpp

+            case State::Normal:
+            case State::Error:
+            case State::Paused:
+                // Windows Terminal does not support switching progress states without also setting a progress value at the same time,


Somebody shoulda filed this bug on us! 😉

DHowett · 2021-04-26T16:53:50Z

src/AppInstallerCLICore/VTSupport.cpp

+
+            THROW_HR_IF(E_BOUNDS, percentage.has_value() && percentage > 100u);
+
+            // Workaround some quirks in the Windows Terminal implementation of the progress OSC sequence


Y'all, we work at the same company. If I keep finding "quirk for Windows Terminal" comments I'm gonna have to glare at you from across the lunch room. 😁

I will keep that in mind @DHowett , although this one is more of an external report through us, so maybe we can still be friends?

I am to blame for writing that mean comment, sorry. 😁 I wanted to check whether this was a known issue/limitation (I think I saw an issue with some follow-up tasks related to the initial implementation in Windows Terminal) but forgot about it in the end.

I see you haven't yet created an issue report so let me do it.

I created an issue for this in the Windows Terminal repo: microsoft/terminal#9960

Thanks a ton!

(sorry -- my joke probably came across very poorly over the medium of text. Thanks for being understanding! 😁)

chausner added 2 commits April 22, 2021 00:25

Implement VT-based progress reporting

27fb1c6

Update GitHub spelling checker dictionary

143cfc5

chausner requested a review from a team as a code owner April 21, 2021 22:57

JohnMcPMS reviewed Apr 21, 2021

View reviewed changes

src/AppInstallerCLICore/VTSupport.h Outdated Show resolved Hide resolved

src/AppInstallerCLICore/VTSupport.cpp Outdated Show resolved Hide resolved

src/AppInstallerCLICore/VTSupport.h Outdated Show resolved Hide resolved

src/AppInstallerCLICore/VTSupport.cpp Show resolved Hide resolved

denelon linked an issue Apr 22, 2021 that may be closed by this pull request

Provide Taskbar progress indicator in Terminal #869

Closed

chausner and others added 2 commits April 22, 2021 18:36

Throw E_INVALIDARG instead of E_UNEXPECTED

3355daf

Co-authored-by: JohnMcPMS <johnmcp@microsoft.com>

Rename ProgressState -> State, ProgressBar -> Construct, change param…

c4d5c48

…eter to uint32_t

jsoref reviewed Apr 22, 2021

View reviewed changes

JohnMcPMS approved these changes Apr 22, 2021

View reviewed changes

JohnMcPMS merged commit 43e70fa into microsoft:master Apr 22, 2021

DHowett reviewed Apr 26, 2021

View reviewed changes

chausner deleted the vt-progress-bar branch April 26, 2021 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement VT-based progress reporting #883

Implement VT-based progress reporting #883

chausner commented Apr 21, 2021 •

edited by ghost

Loading

yao-msft commented Apr 21, 2021

azure-pipelines bot commented Apr 21, 2021

JohnMcPMS left a comment

chausner commented Apr 22, 2021

denelon commented Apr 22, 2021

jsoref Apr 22, 2021

JohnMcPMS commented Apr 22, 2021

azure-pipelines bot commented Apr 22, 2021

JohnMcPMS left a comment

denelon commented Apr 22, 2021

chausner commented Apr 22, 2021

JohnMcPMS commented Apr 22, 2021

DHowett Apr 26, 2021

DHowett Apr 26, 2021

JohnMcPMS Apr 26, 2021

chausner Apr 26, 2021 •

edited

Loading

chausner Apr 26, 2021

DHowett Apr 26, 2021

		// Multi-terabyate installers should be fairly rare for the foreseeable future...
		// Multi-terabyte installers should be fairly rare for the foreseeable future...


		THROW_HR_IF(E_BOUNDS, percentage.has_value() && percentage > 100u);

		// Workaround some quirks in the Windows Terminal implementation of the progress OSC sequence

Implement VT-based progress reporting #883

Implement VT-based progress reporting #883

Conversation

chausner commented Apr 21, 2021 • edited by ghost Loading

Microsoft Reviewers: Open in CodeFlow

yao-msft commented Apr 21, 2021

azure-pipelines bot commented Apr 21, 2021

JohnMcPMS left a comment

Choose a reason for hiding this comment

chausner commented Apr 22, 2021

denelon commented Apr 22, 2021

jsoref Apr 22, 2021

Choose a reason for hiding this comment

JohnMcPMS commented Apr 22, 2021

azure-pipelines bot commented Apr 22, 2021

JohnMcPMS left a comment

Choose a reason for hiding this comment

denelon commented Apr 22, 2021

chausner commented Apr 22, 2021

JohnMcPMS commented Apr 22, 2021

DHowett Apr 26, 2021

Choose a reason for hiding this comment

DHowett Apr 26, 2021

Choose a reason for hiding this comment

JohnMcPMS Apr 26, 2021

Choose a reason for hiding this comment

chausner Apr 26, 2021 • edited Loading

Choose a reason for hiding this comment

chausner Apr 26, 2021

Choose a reason for hiding this comment

DHowett Apr 26, 2021

Choose a reason for hiding this comment

chausner commented Apr 21, 2021 •

edited by ghost

Loading

chausner Apr 26, 2021 •

edited

Loading