Skip to content
This repository has been archived by the owner on Apr 19, 2024. It is now read-only.

Remove ANSI formatting before measuring string width #462

Merged
merged 3 commits into from
Nov 7, 2022

Conversation

zimeg
Copy link
Contributor

@zimeg zimeg commented Oct 30, 2022

Summary

Hello! This PR hopes to address an issue in the select prompt where formatted lines may delete the past output on terminals of certain sizes.

The problem

When ANSI escape codes are used in the options of a prompt, these lines are measured wider than what is visibly printed to the terminal. This is caused by the StringWidth counting these escaped characters as a part of the actual string, and results in previous output being deleted (if the measured line count is greater than the actual line count of an option).

The following can demonstrate this on terminals that are only slightly wider than the option's visible length:

prompt := &survey.Select{
  Message: "Select an option:",
  Options: []string{
    "\033[35m..........................................................",
    "\033[36m..........................................................",
  },
}

survey.AskOne(prompt, "")

See this in action:

before.mov

Proposed solution

The github.com/acarl005/stripansi library offers a nice regex pattern to remove ANSI codes, and when used in the StringWidth method can remove these codes before the string's width is measured.

Additionally, a check for non-printable runes was added to the runeWidth method so that only printable characters are measured. unicode.IsPrint was chosen over unicode.IsGraphic to conform with the definitions of Go, but I could see a reason for preferring IsGraphic!

With the changes of this PR applied, the following result is achieved:

after.mov

Copy link
Collaborator

@mislav mislav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for proposing to fix this!

func StringWidth(str string) int {
w := 0
rs := []rune(str)
cleanedStr := stripansi.Strip(str)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's an overkill to pull in a dependency just to get access to a single regular expression. However, since we're looping to measure the printable string width here, I would be more in favor of an approach like this one: https://github.com/muesli/reflow/blob/v0.3.0/ansi/buffer.go#L20-L40

I'm also not suggesting to import github.com/muesli/reflow/ansi here, but we could inline the approach of ignoring ANSI escape codes while iterating over a string. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I similarly agree that a whole import for this one line might be too much.. I also really like the iterative approach as it's slightly more readable than the regex too! I'll make these changes in the next day or two!

go.sum Show resolved Hide resolved
@zimeg zimeg requested a review from mislav November 6, 2022 20:01

// isAnsiTerminator returns if a rune denotes the end of an ANSI sequence
func isAnsiTerminator(r rune) bool {
return (r >= 0x40 && r <= 0x5a) || (r == 0x5e) || (r >= 0x60 && r <= 0x7e)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These terminators were collected from various lists and other examples to match the XTerm Control Sequences.

@mislav
Copy link
Collaborator

mislav commented Nov 7, 2022

Thank you!

@mislav mislav merged commit b54ce38 into AlecAivazis:master Nov 7, 2022
@zimeg zimeg deleted the strip-ansi branch November 7, 2022 23:09
@mwbrooks
Copy link

mwbrooks commented Feb 3, 2023

@mislav Some wonderful improvements and bug fixes have landed since September's v2.3.6 release. When you have a moment in the coming months, we'd greatly appreciate a fresh release 🙏🏻 ☺️

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

Successfully merging this pull request may close these issues.

None yet

3 participants