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

Ignore ANSI sequences when measuring string width #147

Merged
merged 2 commits into from
Jan 29, 2023

Conversation

zimeg
Copy link
Contributor

@zimeg zimeg commented Jan 29, 2023

Summary

Hello! This PR updates the computeNumberOfLinesNeededToPrintStringInternal function to ignore ANSI sequences from the string width measurement since these are not visibly printed to the terminal.

Currently, ANSI sequences are included in this measurement which can cause the computed number of lines to be greater than the actual number of lines needed to print a string. When this happens, previous lines are unexpectedly erased from the screen.

Demo

Before changes

before.mov

After changes

after.mov

Reviewers

The following snippet can be used to demonstrate the erasure behavior on a terminal of width 80:

package main

import (
	"time"

	"github.com/briandowns/spinner"
)

func main() {
	s := spinner.New(spinner.CharSets[9], 100*time.Millisecond)
	s.Suffix = " \x1b[36mHere we are.\x1b[0m Waiting ever so \x1b[1;33mpatiently\x1b[0m as we watch the spinner spin."

	s.Start()
	time.Sleep(2 * time.Second)
	s.Stop()
}

To test these changes, checkout this branch and update your go.mod to include:

replace github.com/briandowns/spinner v1.20.0 => ../path-to-branch/spinner

Notes

  • The computeNumberOfLinesNeededToPrintStringInternal function was updated to measure the line count in an iterative manner.
  • Tests for the computeNumberOfLinesNeededToPrintStringInternal function were updated to use a table-driven testing approach. New tests were also added for checking expected behavior of strings containing ANSI sequences.

@briandowns
Copy link
Owner

Thank you for this!

@briandowns briandowns merged commit 9ab734a into briandowns:master Jan 29, 2023
@shinji62
Copy link

shinji62 commented Feb 2, 2023

@briandowns Can we get a new release ? I am impacted by this issues as well.

@briandowns
Copy link
Owner

All set @shinji62 ! https://github.com/briandowns/spinner/releases/tag/v1.21.0

@shinji62
Copy link

shinji62 commented Feb 2, 2023

Wow that's was fast !

@mwbrooks
Copy link

mwbrooks commented Feb 3, 2023

Fantastic, thank you for the release @briandowns! 🙇🏻 🚀

@briandowns
Copy link
Owner

No prob. :D

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 this pull request may close these issues.

4 participants