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

fix(utils/utils): use color.Output to handle escape sequence on Windows #826

Merged
merged 1 commit into from
Nov 8, 2020

Conversation

LussacZheng
Copy link
Contributor

@LussacZheng LussacZheng commented Nov 8, 2020

This PR focuses on the colorized outputs in Windows CMD and PowerShell.

But I'm not sure if it will break on non-Windows system. I have only tested on my windows and centos.

Issue

annie -v doesn't print the colorized version info correctly in Windows CMD and PowerShell, but other commands such as annie -i works well.

  • CMD
    1
  • PowerShell
    2

Reason

In PrintVersion() , CMD can't handle escape sequence strings provided by cyan/blue.Sprintf() . However, in printStream() , directly using cyan/blue.Printf() works well. That is because the package color invokes fmt.Fprintf(color.Output, format string, a ...interface{}) to print colorized strings:

  • https://github.com/fatih/color/blob/v1.7.0/color.go#L224-L229

    // Printf formats according to a format specifier and writes to standard output.
    // It returns the number of bytes written and any write error encountered.
    // This is the standard fmt.Printf() method wrapped with the given color.
    func (c *Color) Printf(format string, a ...interface{}) (n int, err error) {
    	c.Set()
    	defer c.unset()
    
    	return fmt.Fprintf(Output, format, a...)
    }
  • https://github.com/fatih/color/blob/v1.7.0/color.go#L25

    // Output defines the standard output of the print functions. By default
    // os.Stdout is used.
    Output = colorable.NewColorableStdout()

So just use Fprintf with color.Output instead of Printf in PrintVersion():

fmt.Fprintf(
    color.Output,
    "\n%s: version %s, A fast, simple and clean video downloader.\n\n",
    cyan.Sprintf("annie"),
    blue.Sprintf(config.VERSION),
) `

And similar issue to printError():
https://github.com/iawia002/annie/blob/d0d473ae01e75b83a14066a37ab85da6a1a0f3c0/main.go#L195-L200

Intuitive test

Here are some test codes and screenshots (from CMD):

PrintVersion()

func PrintVersion() {
	blue := color.New(color.FgBlue)
	cyan := color.New(color.FgCyan)

	// New method 1
	fmt.Fprintf(
		color.Output,
		"\n%s: version %s, A fast, simple and clean video downloader.\n\n",
		cyan.Sprintf("annie"),
		blue.Sprintf(config.VERSION),
	)

	// New method 2
	// just like https://github.com/iawia002/annie/blob/d0d473ae01e75b83a14066a37ab85da6a1a0f3c0/request/request.go#L110-L118
	fmt.Println()
	cyan.Printf("annie")
	fmt.Print(": version ")
	blue.Printf(config.VERSION)
	fmt.Println(", A fast, simple and clean video downloader.\n")

	// Original method
	fmt.Printf(
		"\n%s: version %s, A fast, simple and clean video downloader.\n\n",
		cyan.Sprintf("annie"),
		blue.Sprintf(config.VERSION),
	)
}

3

PrintError()

func printError(url string, err error) {
	// New method 1
	fmt.Fprintf(
		color.Output,
		"Downloading %s error:\n%s\n",
		color.CyanString("%s", url), color.RedString("%v", err),
	)
	fmt.Println()

	// Original method
	fmt.Printf(
		"Downloading %s error:\n%s\n",
		color.CyanString("%s", url), color.RedString("%v", err),
	)
	fmt.Println()
}

Try to download a YouTube video without proxy:

4

References

  1. fatih/color/README.md at v1.7.0
  2. colorString() not working on Windows cmd.exe fatih/color#91
  3. Windows support golangci/golangci-lint#14

Copy link
Owner

@iawia002 iawia002 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@iawia002 iawia002 merged commit 68e47f1 into iawia002:master Nov 8, 2020
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.

2 participants