Skip to content

fix(iostreams): make coloured output work on Windows#625

Merged
craicoverflow merged 4 commits intoredhat-developer:mainfrom
craicoverflow:windows-colors
Apr 23, 2021
Merged

fix(iostreams): make coloured output work on Windows#625
craicoverflow merged 4 commits intoredhat-developer:mainfrom
craicoverflow:windows-colors

Conversation

@craicoverflow
Copy link
Contributor

@craicoverflow craicoverflow commented Apr 22, 2021 •
edited
Loading

Fixes #621

Verification

Set up this project on a Windows machine, check out and run this PR from Command Prompt

OR

Check these screenshots.

Powershell
image

Cmd
image

craicoverflow
craicoverflow commented Apr 23, 2021
View reviewed changes
pkg/cmd/kafka/consumergroup/describe/describe.go
activeMembersCount := cgutil.GetActiveConsumersCount(consumers)
partitionsWithLagCount := cgutil.GetPartitionsWithLag(consumers)

fmt.Fprintln(w, color.Bold(localizer.MustLocalizeFromID("kafka.consumerGroup.describe.output.activeMembers")), activeMembersCount, "\t", color.Bold(localizer.MustLocalizeFromID("kafka.consumerGroup.describe.output.partitionsWithLag")), partitionsWithLagCount)
Copy link
Contributor Author

@craicoverflow craicoverflow Apr 23, 2021

Choose a reason for hiding this comment

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

Removed the bolding of text here because Command Prompt does not support ANSI colors. Powershell does and it worked fine though. Do we need to support both??

Copy link
Contributor Author

@craicoverflow craicoverflow Apr 23, 2021

Choose a reason for hiding this comment

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

image

Here's how it looks without bold titles. Looks just fine. I originally added bold to differentiate it from the values, but capitalising the title does an equally good job.

Copy link
Collaborator

@pmuir pmuir Apr 23, 2021

Choose a reason for hiding this comment

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

Can you detect somehow?

Copy link
Contributor Author

@craicoverflow craicoverflow Apr 23, 2021

Choose a reason for hiding this comment

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

Yes - if runtime.GOOS != "windows".

Copy link
Collaborator

@pmuir pmuir Apr 23, 2021

Choose a reason for hiding this comment

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

I meant between powershell and cmd...

Copy link
Contributor Author

@craicoverflow craicoverflow Apr 23, 2021

Choose a reason for hiding this comment

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

Oh sorry, no I did not see a simple way..not using Go libraries anyway.

Copy link
Collaborator

@pmuir pmuir Apr 23, 2021

Choose a reason for hiding this comment

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

That sucks. I guess we need to support both... I'm not sure how well adopted powershell actually is.

Copy link
Contributor Author

@craicoverflow craicoverflow Apr 23, 2021

Choose a reason for hiding this comment

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

Not sure either, it seems much better and has the same commands as Unix. Best to support both until we know for sure.

@craicoverflow craicoverflow marked this pull request as ready for review April 23, 2021 10:01
@craicoverflow craicoverflow requested review from pmuir and wtrocki April 23, 2021 10:01
@craicoverflow
Copy link
Contributor Author

craicoverflow commented Apr 23, 2021

It's a lot of hassle to verify this, so I've added screenshots to help.

@pmuir
Copy link
Collaborator

pmuir commented Apr 23, 2021

Do we need this one before Summit?

@craicoverflow
Copy link
Contributor Author

craicoverflow commented Apr 23, 2021

Do we need this one before Summit?

It depends - the output looks poor and does not give a good impression to Windows users.
It does not block functionality but you get something like this:

You are now logged in as "kafka-german-pilsner"

�[96mA new version of rhoas is available:�[0m �[95m0.23.1�[0m
�[96mhttps://github.com/redhat-developer/app-services-cli/releases/tag/0.23.1�[0m

@pmuir
Copy link
Collaborator

pmuir commented Apr 23, 2021

Aha -thanks for providing the for - I wasn't sure if it was just to enable colours on Windows!

Yes, then we should do this.

@craicoverflow craicoverflow changed the title fix(iostreams): use colorable stdout to print colors on Windows fix(iostreams): make coloroued output work on Windows Apr 23, 2021
@craicoverflow craicoverflow changed the title fix(iostreams): make coloroued output work on Windows fix(iostreams): make coloured output work on Windows Apr 23, 2021
@craicoverflow craicoverflow merged commit 6ca2650 into redhat-developer:main Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pmuir pmuir pmuir approved these changes

@wtrocki wtrocki Awaiting requested review from wtrocki

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Printing xterm colors in windows cmd

2 participants

@craicoverflow @pmuir