[cli] Disable spinner animation on non-live TTYs#4706
[cli] Disable spinner animation on non-live TTYs#4706riken127 wants to merge 2 commits intocanonical:mainfrom
Conversation
ricab
left a comment
There was a problem hiding this comment.
Thanks for the PR! I have a couple of requests and questions for clarification.
I would also like to see manual tests results exercising this in the multiple commands that use the spinner, showing correct handling of tricky cases like errors from the daemon, update prompts, password in auhenticate...
| } | ||
| else | ||
| { | ||
| cout << start_message << std::flush; |
There was a problem hiding this comment.
I wonder if we should print any messages at all. Did you do it like this because of the way start is used in the spinner callbacks?
Let's see what @amylily1011's says too.
There was a problem hiding this comment.
Actually, I kept the messages because the original concern seemed to be about the clutter generated by the spinner itself.
From what I gather in the discussion, it seems safe to remove the else branch, which I’ve gone ahead and done.
Please correct me if I’ve misunderstood anything. Thanks again for your patience!
There was a problem hiding this comment.
Indeed, the spinner clutter was the main issue. However (as per discussion in the issue) it also makes sense to skip status messages when running in the background.
So yeah, I guess skipping the start message makes sense. I don't know if that will break things though, I believe start is used for things beyond mere status updates (see callbacks above).
b344e47 to
485b969
Compare
Gating the spinner animation logic on the term->cout_is_live() flag. This prevents messy output (backspaces and animation frames) when Multipass is run in non-interactive environments.
485b969 to
58e0502
Compare
|
Hey @riken127, is this ready for another pass? BTW, I know approaches vary a lot on different projects, but we favor separate atomic commits, without squashing, including review requests. See GIT5 and others in CONTRIBUTING.md. It makes it easier to see new changes. No worries about the one you already pushed, just something to keep in mind going forward. |
0987866 to
2f411e5
Compare
|
Hi @ricab, Yes, i believe so! I've modified the logic and performed additional manual tests. I had to push force on my last commit to fix a minor whitespace oversight and keep the history clean; I'm sorry about that! I've verified the following cases in non-interactive mode using
Regarding the callbacks and interaction angles you mentioned, I've made start() and stop() no-ops in non-live TTYs. I'm not sure if this covers all the edge cases you were thinking of, so if I've missed a specific interaction angle, please let me know. I'd be happy to take a closer look if you could provide more detail on what else might be affected. Thank you for your patience, it took me a while to answer as I had some issues with my linux machine. Would love to hear some feedback! |
|
Cool, thanks again for your involvement @riken127! I am queuing this to review soon. |
Gating the spinner animation logic on the term->cout_is_live() flag. This prevents messy output (backspaces and animation frames) when Multipass is run in non-interactive environments.
Description
This PR gates the CLI spinner's animation logic on the
term->cout_is_live()flag.Related Issue(s)
Closes #1541
Testing
./build/bin/multipass wait-ready(Verified animation is visible)./build/bin/multipass wait-ready | catGot:
Screenshots (if applicable)
N/A (Behavioural change for terminal output)
Checklist
Additional Notes