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

Spinners in the examples don't render properly in the Windows Command Prompt #574

Open
LikeLakers2 opened this issue Aug 8, 2023 · 16 comments

Comments

@LikeLakers2
Copy link

LikeLakers2 commented Aug 8, 2023

Hi! This is fairly minor, but I noticed that some examples don't appear to show their spinners properly when run on the Windows Command Prompt. For example, with the multi example:
image

As best as I can tell, this is due to using unsupported characters as the spinner. If I switch the spinner characters to "-\\|/ ":
image

then it appears to show up properly.

I'm unsure of how this might be fixed - but it would be appreciated if spinners could default to supported characters on shells that may not support them.

@djc
Copy link
Collaborator

djc commented Aug 8, 2023

Hmm, okay. I don't think I'd be able to test this, but would be happy to review a PR if you are able to submit one. I think the main change here is that you'd want to change the default tick_strings in https://github.com/console-rs/indicatif/blob/main/src/style.rs#L94.

@LikeLakers2
Copy link
Author

LikeLakers2 commented Aug 8, 2023

@djc I'd be happy to submit a PR soon. :) Thank you for linking me that file, by the way, I was looking for that before submitting this issue and couldn't find it.

That said, I also think it may be useful if we could figure out a way to automatically detect unsupported characters - i.e. if the terminal supports it, we might default to the current braille characters, and otherwise default to a simple spinner using "-\\|/ ".

However, I have no idea how we might accurately figure that out - the only idea that comes to mind is to check for emoji support, and use the braille characters if that's true.

@djc
Copy link
Collaborator

djc commented Aug 8, 2023

Yeah, I don't know how you might figure it out, though emoji support is probably a decent proxy (for just general Unicode support, which I think is what missing for these spinner code points).

And I definitely was not suggesting we should always use the non-Unicode characters by default -- only in contexts where Unicode is not properly supported. I personally don't have much context on how we'd figure that out, so I suggest you spend some time researching that aspect, for example by looking at the API of the console crate (and maybe other projects in the same space, like https://github.com/rust-cli/anstyle).

@LikeLakers2
Copy link
Author

@djc Gotcha. I'll consider researching the matter at some point, when I have some free time.

For now, would replacing the default tick_strings with "-\\|/ " be an acceptable solution?

@djc
Copy link
Collaborator

djc commented Aug 8, 2023

No, I don't want to regress this for all of today's users who have Unicode-capable terminals -- especially since this is the first time (as far as I recall) that anyone has complained about (which suggests this kind of usage is rare).

@LikeLakers2
Copy link
Author

It was less a complaint, and more a note that Command Prompt doesn't properly render the default tick_strings currently being used.

However, I will note that this doesn't impact functionality at all - it only impacts how a shell program's output looks - which may be part of why nobody's ever brought it up.

In any case, I'm unsure what I should do in my PR then. Would I check for emoji support, and decide between the current default (if emoji is supported) and a simple "-\\|/ " spinner (if it's not supported)?

@chris-laplante
Copy link
Collaborator

In any case, I'm unsure what I should do in my PR then. Would I check for emoji support, and decide between the current default (if emoji is supported) and a simple "-\\|/ " spinner (if it's not supported)?

You might look into what console (https://github.com/console-rs/console) supports, which is what indicatif leverages for feature detection.

@djc
Copy link
Collaborator

djc commented Aug 9, 2023

In any case, I'm unsure what I should do in my PR then. Would I check for emoji support, and decide between the current default (if emoji is supported) and a simple "-\\|/ " spinner (if it's not supported)?

Yup, that seems like a decent step forward.

@LikeLakers2
Copy link
Author

In any case, I'm unsure what I should do in my PR then. Would I check for emoji support, and decide between the current default (if emoji is supported) and a simple "-\\|/ " spinner (if it's not supported)?

Yup, that seems like a decent step forward.

Right, so I looked further into this - just to make sure I know what I'm doing - and I think I know what I want to do now.

The plan I have is thus: Use a console::Emoji, with the emoji field filled with the fancy braille unicode characters, and the fallback filled with the non-unicode ("-\\|/ ") variant. When constructing the default spinner, we would call .to_string() on this Emoji, which would in turn check for us if we support emoji. We can then send the output of that to other functions to turn it into the format we want.

It's worth noting that <console::Emoji as Display>::fmt() and console::TermFeatures::wants_emoji() have slightly different implementations. In particular, the TermFeatures variant also checks if the console is current attended (which, as best as I can tell, means if the text would get output to a file or not).

However, there's a problem with using the TermFeatures variant: It requires selecting a output location (stdout or stderr, or a buffered variant) when constructing the Term. While most spinners will appear on stderr, the output location can be changed after we create the default spinner. This isn't an unsolvable issue - but solving it would most likely require tweaking the internals of indicatif to indicate whether we're still using the default spinner or not. That's not something I'm particularly comfortable doing, at least not currently.

Thus, I'm inclined to go ahead with my current plan of using a console::Emoji. Are there any objections or thoughts on this matter? Or should I go ahead with this?

@djc
Copy link
Collaborator

djc commented Aug 9, 2023

I... think you're overthinking it. Just use Term::stderr().features() and it should be fine. This is just a heuristic, and it's unlikely the stdout emoji support is different from the stderr emoji support.

@LikeLakers2
Copy link
Author

I... think you're overthinking it. Just use Term::stderr().features() and it should be fine. This is just a heuristic, and it's unlikely the stdout emoji support is different from the stderr emoji support.

I do tend to overthink things a bit, but I'm not sure this is one of those times. Let me explain.

It is as you said: Emoji support is pretty much guaranteed to be the same between stdout and stderr.

What isn't guaranteed to be the same, is whether both are considered to be "attended" - something which TermFeatures::wants_emoji() checks for but <Emoji as ToString>::to_string() does not. In particular, stdout is considered unattended if it's being piped into a file - and same with stderr. And since one can be piped to a file but not the other, this means that stdout can be "unattended" while stderr is not.

That's why I'm hesitant to use Term::stderr().features() - if the output for a spinner gets switched to stdout afterwards, then the spinner would still be assuming stderr's attended status despite stdout potentially being different.

If you still think I should go ahead with Term::stderr().features().wants_emoji(), then I will - but I want you to be aware of the differences between that and <Emoji as ToString>::to_string() first.

@djc
Copy link
Collaborator

djc commented Aug 9, 2023

Why do you think the support for attended is related to the support for emoji? If we're writing to a Unicode-capable non-attended stream, why shouldn't we use the Unicode tick strings?

(Alternatively, TermFeatures supports is_attended() and has_emoji() so you could check both.)

@LikeLakers2
Copy link
Author

LikeLakers2 commented Aug 9, 2023

Why do you think the support for attended is related to the support for emoji? If we're writing to a Unicode-capable non-attended stream, why shouldn't we use the Unicode tick strings?

Because we may not be writing to a Unicode-capable non-attended stream? I don't know if that's true, though - but I do know that console itself checks if the output stream is attended when you call TermFeatures.wants_emoji(), so there must be a good reason for that, such as a non-Unicode-capable non-attended stream.

(Alternatively, TermFeatures supports is_attended() and has_emoji() so you could check both.)

...I've mentioned several times that TermFeatures::wants_emoji() already checks the attended status, and even linked the code. This actually makes me unsure that you've understood what I've said properly. So please, reread my messages?

@djc
Copy link
Collaborator

djc commented Aug 9, 2023

Sorry for not reading your message carefully. I also feel like I've told you several times what I think will be an adequate (if admittedly not perfect) solution and you keep pushing back, so I get a bit impatient (too?). I suggested this solution like 5 messages ago and as a volunteer maintainer you've taken a lot of round trips while, in my view, not adding any new (for me) insights to the conversation.

If you want to go through and make sure that the tick strings match the current terminal's emoji support (but only if the tick strings haven't been changed from the default), I'll review that code, but I might shoot it down for adding too much complexity. I think there's a four-line fix here that is likely to be good enough (TM) for a large majority of cases.

@LikeLakers2
Copy link
Author

Frankly, I don't even want to interact here much more, after what felt like having my concerns be thrown to the wayside in favor of "why does that matter, just do this".

I don't know when I'll get to it, but I'll try to get to this at some point in the future.

@LikeLakers2
Copy link
Author

I suppose I should mention, since it's been two months, that I have absolutely zero interest in pursuing this matter.

If anyone wants to pursue this in my stead, I hope I've left enough information in this issue for you to figure out what to do. If I haven't, or if discussions need be had on something I've missed, please leave me out of the talking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants