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

Redo of background colors to improve diff readability #690

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

egrieco
Copy link

@egrieco egrieco commented Mar 29, 2024

This is a complete rewrite of #286 on top of the current version of difftastic (with the exception of the last cargo update which is causing a compilation error for me #689).

Screenshot of default background colors for difftastic.

Previous Issues

Hopefully this addresses the issues that @Wilfred mentioned in the last pull request:

  • We probably don't need to make line numbers red/green with background highlighting: no background highlighting behind line numbers
  • It looks like subword bold highlighting (the 6 in Version 0.6) and delimiters (the ( before reverse) is missing: not sure about this, please let me know if you still see this issue
  • I don't think you need cansi, you should be able to calculate lengths before applying colours: got rid of cansi, mostly able to calculate proper lengths without the ANSI stripping step (except for inline view)

Themes

This pull request lays the groundwork for theming of difftastic in the future. The missing piece here is the ability to parse theme information from a config file. Internally, the various styles are stored in a HashMap and a lookup with fallback is performed when retrieving various values.

Notes and Caveats

While this is a decent proof of concept there are probably a few issues in need of attention:

  • Switched to yansi for rendering colors in the terminal since it has this incredibly useful wrapping functionality. This makes "nesting" of ANSI colors feel possible.
  • Inline view does not color lines all the way to the right side of the screen. I'm having a rather difficult time tracking down where I need to add padding to make this happen.
  • Completely blank lines do not have any color. They only indication that they have changed is the color of their line number.
  • Dark mode is not supported in themes. The idea is to use separate themes for dark and light mode.
  • Header styles are not fully supported in the themes.
  • NO_COLOR is not currently supported even though yansi supports conditional styling quite well.

Reasoning

I'm creating this pull request now, in spite of it not being finished, since I'd like feedback on the above.

NOTE: There appears to be a bug in testing. Lines in the left hand side are being colored green in some cases.
NOTE: This version does not compile due to a conflict between some remaining owo_colors types and some yansi types.
NOTE: Colors need adjusting. Sub-line differences seem to have the same background color as the rest of the line.
Use color theme for default background colors.
@gerhardol
Copy link

This looks better.
Do you have a plan to create custom themes? To use git colors is one way I prefer.

@egrieco
Copy link
Author

egrieco commented Apr 1, 2024

Do you have a plan to create custom themes? To use git colors is one way I prefer.

No. If you have more detail about what you'd like to see in custom theme support please share.

Right now, I have implemented this in a way that could support custom themes, specifically yansi::Styles are pulled from a lookup table implemented via a HashMap. The biggest missing pieces are

  1. A format for storing themes on disk: Do we go with TOML, YAML, JSON, etc.
  2. Parsing code to convert user specified theme info into actual yansi::Style types.

@egrieco
Copy link
Author

egrieco commented Apr 1, 2024

@gerhardol
Copy link

More or less those links.
Git colors are defined in the doc: https://git-scm.com/docs/git-config#Documentation/git-config.txt-colordiffltslotgt
But you do not get the hardcoded defaults from Git what I know.
Also, getting colors dynamically will be slow (some caching will have to be used).

But using the same names for the slot (with some additions and deletions) would simplify.
Maybe not this PR to define the available slots, but maybe the name of them.

Difftastic is more than Git, but as Git is dominant, at least configuration and documentation is simpler if the concepts ar the same.

In my case to use Difftastic as a difftool from within another tool, I would need some way to control tis at runtime, prefeably with environment variables. So a comma separated list with the Git names of the slots is my preference. A user may want other colors from the command line and use other theme colors when embedded in the app.

A sidetrack...

Will try to build and try this branch myself

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