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

Smooth progress uses characters not available by default #2099

Open
JohnMcPMS opened this issue Apr 14, 2022 · 10 comments
Open

Smooth progress uses characters not available by default #2099

JohnMcPMS opened this issue Apr 14, 2022 · 10 comments
Labels
Area-Output Issue related to CLI output Issue-Bug It either shouldn't be doing this or needs an investigation.

Comments

@JohnMcPMS
Copy link
Member

Brief description of your issue

The smooth progress implemented in #2046 uses partial block characters to make progress bars able to show 8x as much granularity. Unfortunately, those characters are not available in Consolas, the default font used by connhost on Windows. And without fallback, connhost displays the "I don't have that glyph" character (box with a question mark inside).

Steps to reproduce

Make a progress bar appear in the dev build, while using Consolas in connhost.

Expected behavior

I would prefer that we could detect this and not use the partial block characters.
I would accept having a user setting to control it.

Actual behavior

Progress bar pushes a question mark block across until smashing it at 100%.

Environment

Not yet released winget
@ghost ghost added the Needs-Triage Issue need to be triaged label Apr 14, 2022
@JohnMcPMS JohnMcPMS added Issue-Bug It either shouldn't be doing this or needs an investigation. and removed Needs-Triage Issue need to be triaged labels Apr 14, 2022
@sba923
Copy link

sba923 commented Apr 15, 2022

I don't think there can be a way to detect which font is being used.

I would opt for a winget setting under visual to enable the smooth progress, with a default value of false.

@sba923
Copy link

sba923 commented Apr 16, 2022

There must be a way to force winget list (or another non-install-related command) to produce a (slow-growing) progress bar for testing, right?

@Trenly
Copy link
Contributor

Trenly commented Apr 19, 2022

I don't think there can be a way to detect which font is being used.

I would opt for a winget setting under visual to enable the smooth progress, with a default value of false.

https://stackoverflow.com/questions/34278340/easy-way-to-tell-if-unicode-character-is-supported-in-current-font

@sba923
Copy link

sba923 commented Apr 19, 2022

I don't think there can be a way to detect which font is being used.
I would opt for a winget setting under visual to enable the smooth progress, with a default value of false.

https://stackoverflow.com/questions/34278340/easy-way-to-tell-if-unicode-character-is-supported-in-current-font

I would find it rather weird that winget, a CLI application, would have to determine information based on whatever terminal / console host it happens to be running within...

@denelon what's your feeling about this?

@denelon
Copy link
Contributor

denelon commented Apr 19, 2022

I like the improvement to the progress bar, so it would be nice to be able to use it. If we can correctly determine if the appropriate glyphs are supported, we could gracefully fail back. I think we should have this as an option as an argument on the command-line as well as a setting. As this was detected with someone who was automating something against the progress bar, we should treat this as a breaking change 😔.

The default should be the current released stable progress bar, and when we release 2.0 and can take breaking change, this would be one of the breaking changes we could consider.

@jedieaston
Copy link
Contributor

jedieaston commented Apr 19, 2022

As this was detected by someone who was automating something against the progress bar

I get that this has created a breaking change, but I wonder if we should document that people automating against the progress bar should be using the VT progress reporting characters (which is how Windows Terminal figures it out) since those are a documented(ish) standard vs how winget chooses to show progress to a user on stdout.

@JohnMcPMS
Copy link
Member Author

I've learned that conhost will be getting font fallback as discussed here: microsoft/terminal#10472

I've also been informed that attempting to detect support is not a good idea here.

As this was detected with someone who was automating something against the progress bar, we should treat this as a breaking change 😔.

This was detected by @KevinLaMS as far as I know, with his eyes, not scripting. But maybe you have more details. Regardless of character support, any output change could be considered breaking for someone processing output.

I would lean towards "acceptable impact" currently, but if it is deemed unacceptable, we should add a setting and default to the current progress.

@sba923
Copy link

sba923 commented Apr 19, 2022

I like the improvement to the progress bar, so it would be nice to be able to use it. If we can correctly determine if the appropriate glyphs are supported, we could gracefully fail back. I think we should have this as an option as an argument on the command-line as well as a setting. As this was detected with someone who was automating something against the progress bar, we should treat this as a breaking change 😔.

The default should be the current released stable progress bar, and when we release 2.0 and can take breaking change, this would be one of the breaking changes we could consider.

My questioning was not about the new progress bar being a breaking change, which I think it isn't since building something on top of something else's UI is inherently fragile.

I was asking @denelon whether he shares my thought that having code within winget that would make choices based on which terminal its output is actually rendered by and how that terminal's configured is not something a CLI app should do.

And indeed I didn't detect that the progress bar would be rendered incorrectly if the font doesn't support the required glyphs. I merely was informed (by @denelon himself) that my winget-output-parsing code would break with the new progress bar, which led me here.

@denelon
Copy link
Contributor

denelon commented Apr 19, 2022

Ahh, I misunderstood. Yes, I think we should be careful about trying to detect what a font supports or what font is available in some given terminal. I also forgot about what @jedieaston had referred to with the "--no-vt" argument. I need to make sure that gets documented correctly.

@denelon denelon added this to the v1.3-Client milestone Apr 20, 2022
@denelon denelon modified the milestones: v1.3-Client, v1.4-Client May 31, 2022
@denelon denelon modified the milestones: v1.4-Client, v1.5-Client Dec 28, 2022
@denelon denelon modified the milestones: v1.5-Client, v.Next-Client Apr 18, 2023
@Trenly
Copy link
Contributor

Trenly commented Jun 16, 2023

[Policy] Area-Output

@microsoft-github-policy-service microsoft-github-policy-service bot added the Area-Output Issue related to CLI output label Jun 16, 2023
@denelon denelon removed this from the v.Next-Client milestone Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Output Issue related to CLI output Issue-Bug It either shouldn't be doing this or needs an investigation.
Projects
None yet
Development

No branches or pull requests

5 participants