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(cli): paging issues with colored diff #210

Merged
merged 1 commit into from
Feb 12, 2020

Conversation

rfratto
Copy link
Member

@rfratto rfratto commented Feb 12, 2020

NOTE: Do not merge until #202 is merged; for this to work correctly it depends on color sequences being applied per line rather than per diff chunk, which is introduced in #202.

The less command has two flags: --raw-control-chars and --RAW-CONTROL-CHARS, each of which keeps ASCII control characters present in the paginated output. The former flag was being used previously, and the man page warns about various display problems occurring when using that flag.

On the other hand, --RAW-CONTROL-CHARS only keeps color escape sequences, and the man page notes that appearance will be maintained correctly in most cases. In my local testing, this appears to be true and colorized output in less will now work as expected.

Fixes #82.

@rfratto rfratto requested a review from sh0rez February 12, 2020 13:29
The less command has two flags: --raw-control-chars and
--RAW-CONTROL-CHARS, each of which keeps ASCII control characters
present in the paginated output. The former flag was being used
previously, and the man page warns about various display problems
occurring when using that flag.

On the other hand, --RAW-CONTROL-CHARS only keeps color escape
sequences, and the man page notes that appearance will be maintained
correctly in most cases. In my local testing, this appears to be true
and colorized output in less will now work as expected.

Fixes grafana#82.
Copy link

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

This PR introduces another regression to me.

I've done a test change and the master version shows:
Screen Shot 2020-02-12 at 14 59 13

This PR version shows:
Screen Shot 2020-02-12 at 14 59 23

@rfratto
Copy link
Member Author

rfratto commented Feb 12, 2020

@pracucci ah, nice catch! I did a little bit more testing and found out that this fix only works on top of #202, since it is applying colors per line rather than per diff chunk.

I'll add a warning to the PR description to note this.

Copy link

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Thanks @rfratto for checking it! LGTM to me then.

Copy link
Member

@sh0rez sh0rez left a comment

Choose a reason for hiding this comment

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

It is silly that this works. 🚀

@sh0rez sh0rez changed the title cmd/tk: fix paging issues with colored diff fix(cli): paging issues with colored diff Feb 12, 2020
@sh0rez sh0rez merged commit 5dc82ec into grafana:master Feb 12, 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.

Color of diff changes when scrolling up and down
3 participants