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

The "clear current line and reprint" doesn't work with long suffix #145

Closed
gbouv opened this issue Dec 2, 2022 · 5 comments · Fixed by #146
Closed

The "clear current line and reprint" doesn't work with long suffix #145

gbouv opened this issue Dec 2, 2022 · 5 comments · Fixed by #146

Comments

@gbouv
Copy link
Contributor

gbouv commented Dec 2, 2022

Easy repro:

func main() {
	suffix := "    super long string super long string super long string super long string super long string super long string super long string super long string super long string super long string super long string super long string super long string super long string super long string "
	spin := spinner.New(spinner.CharSets[11], 100*time.Millisecond, spinner.WithSuffix(suffix))
	spin.Start()
	time.Sleep(2 * time.Second)
	spin.Stop()
}

Screenshot 2022-12-02 at 15 38 42

Other side effect: we can't have multi line suffix (string with \n)

I think rather than clearing current line only, it should mark the location of the spinner and then erase everything between the marked location and current location.

@gbouv
Copy link
Contributor Author

gbouv commented Dec 2, 2022

This can be "easily" fixed with the following spinner:

spin := spinner.New(spinner.CharSets[11], 100*time.Millisecond, spinner.WithSuffix(suffix))
spin.PreUpdate = func(s *spinner.Spinner) {
    fmt.Print("\0337\033[J") // save cursor position and erase everything until end of screen
}
spin.PostUpdate = func(s *spinner.Spinner) {
    fmt.Print("\0338")  // restore cursor position
}

But I would have expected the spinner to handle it by itself

Also, the downside of doing \033[J in the PreUpdate is that we're clearing current line twice, which is unnecessary

@gbouv
Copy link
Contributor Author

gbouv commented Dec 5, 2022

In fact the above "hack" doesn't even work when we start printing at the bottom of the terminal because scrolling messes up with the saved position.
So, I had to implement the actual fix of computing in advance how many lines will be needed to print the entire "spinner + suffix" string, and clearing this exact number of lines on every update in the PreUpdate function. It seems to be the only working solution

@briandowns
Copy link
Owner

Care to share your solution or submit a PR?

@gbouv
Copy link
Contributor Author

gbouv commented Dec 13, 2022

Yup @briandowns I'll submit a PR and tag you on it in the next few days when I have a bit more bandwidth 👍

@gbouv
Copy link
Contributor Author

gbouv commented Dec 22, 2022

@briandowns sorry it took so long, but the PR is up and ready for review: #146
Let me know what you think

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

Successfully merging a pull request may close this issue.

2 participants