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

Underline highlighted lines in ANSI theme #1985

Merged
merged 2 commits into from
Feb 14, 2022

Conversation

mdibaiee
Copy link
Contributor

@mdibaiee mdibaiee commented Dec 23, 2021

Hey 👋
I have attempted to fix this in two ways: the first commit is a simple, ANSI-specific condition.
The second commit is using a custom scope to allow each theme to customise how the highlighted line's font style is set.

Let me know what you think and I can squash to a single final commit.

Fixes #1730

@mdibaiee mdibaiee force-pushed the 1730/ansi-highlight branch 2 times, most recently from 847d07b to 31404f6 Compare December 24, 2021 10:09
Copy link
Collaborator

@keith-hall keith-hall 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 your contribution! I like the first commit - despite it being specific for the ANSI theme and hard-coded, it is very simple. I have a few concerns about the second commit's approach, namely it seems weird to use a "custom" scope selector as a global setting, so I'd be interested to hear what the other maintainers think :)

assets/themes/ansi.tmTheme Outdated Show resolved Hide resolved
@mdibaiee mdibaiee force-pushed the 1730/ansi-highlight branch 3 times, most recently from c38883c to 13f35bc Compare January 24, 2022 08:35
@sharkdp
Copy link
Owner

sharkdp commented Jan 28, 2022

This PR still has a change in syntaxes.bin. Should that be removed?

@mdibaiee
Copy link
Contributor Author

@sharkdp I had overlooked that! removed that change 👍

@Enselic
Copy link
Collaborator

Enselic commented Feb 7, 2022

Would you mind adding a simple regression test for this change please? It is useful not only preventing regressions, but also to make it clearer what the use case is and how it works.

It would be great if you could set Approved on this when you think it looks good @keith-hall (when you have time, of course).

Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Approving. But I agree with @Enselic that a regression test would be great.

@mdibaiee
Copy link
Contributor Author

mdibaiee commented Feb 7, 2022

Yes definitely, good call. I just added a test, and while doing so realised it actually had a bug with --style=plain (it worked with the grid style) where it wouldn't clean up the underline. 🐛 All good now.

Also moved changelog entry to Unreleased section.

@mdibaiee
Copy link
Contributor Author

@sharkdp I think this is ready to be merged

@Enselic Enselic merged commit d21f1e8 into sharkdp:master Feb 14, 2022
@mdibaiee mdibaiee deleted the 1730/ansi-highlight branch February 14, 2022 20:23
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.

Add lineHighlight for ansi theme
4 participants