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

Use background colors to improve diff readability #286

Closed
wants to merge 5 commits into from

Conversation

egrieco
Copy link

@egrieco egrieco commented May 15, 2022

This pull request changes the styling of difftastic output to be closer to delta and git-split-diffs. Using background colors to show differences is a lot easier to understand when there is syntax highlighting involved.

I have probably missed a few things that need to be addressed before a merge. Please let me know if you see omissions or issues.

Part of this pull request rendered in the new style.

This addresses issues #167, #265, and #275.

egrieco added 2 commits May 14, 2022 16:56
In theory, basic math like this should be faster than string allocations and operations.
@egrieco
Copy link
Author

egrieco commented May 15, 2022

Just realized, I'm not sure I handle dark mode correctly for the general line backgrounds. It should work reasonably well for the novel item highlights though.

The colors could be chosen more carefully/intentionally in general.

The GitHub CI pipeline didn't like abs_diff() for some reason.
@egrieco
Copy link
Author

egrieco commented May 15, 2022

Let's see if that makes the CI pipeline happy.

@egrieco
Copy link
Author

egrieco commented May 15, 2022

Found an omission. Newly added files do not have the lighter background color applied properly.

@egrieco
Copy link
Author

egrieco commented May 18, 2022

I've actually started using this more frequently than delta. With the better highlighting difftastic is excellent.

I'll try to get the conflicts and omissions resolved soon, but there is a separate piece of software that I must finish first.

@Wilfred
Copy link
Owner

Wilfred commented May 20, 2022

This is really really exciting! I agree this is the right direction for the display, bringing difftastic much closer to how changes are displayed in e.g. GitHub.

Display logic has been surprisingly hard to write for difftastic. It's often been just as challenging as the core diffing. Do you have any questions / concerns / blockers?

@egrieco
Copy link
Author

egrieco commented May 20, 2022

Just merged in your recent changes @Wilfred. Hopefully this makes the CI pipeline happy.

I don't have any questions or concerns at the moment, though I have been collecting a few test cases where the diffing seems to be a bit less than accurate.

Also, I have not addressed my omissions above and I'm sure there a few more that I have missed. Please let me know if you notice any obvious omissions in my pull request.

@Wilfred
Copy link
Owner

Wilfred commented May 24, 2022

Wow, I'd assumed you were using 24-bit colour, but it's really impressive you've done this with the default terminal colours!

Here's before and after on one of the sample files:

Screenshot from 2022-05-23 23-03-18

Screenshot from 2022-05-23 23-01-26

Things that jump out at me:

  • We probably don't need to make line numbers red/green with background highlighting
  • It looks like subword bold highlighting (the 6 in Version 0.6) and delimiters (the ( before reverse) is missing
  • I don't think you need cansi, you should be able to calculate lengths before applying colours

@egrieco
Copy link
Author

egrieco commented Jun 7, 2022

I recently became the main organizer of a security meetup. I've been rather busy with the hand-off, hence the radio silence here.

Things that jump out at me:

* We probably don't need to make line numbers red/green with background highlighting
* It looks like subword bold highlighting (the `6` in `Version 0.6`) and delimiters (the `(` before `reverse`) is missing
* I don't think you need cansi, you should be able to calculate lengths before applying colours

Thanks for the guidance. I'll try to correct these issues soon. 😃

@FranklinYu
Copy link

Would I be able to turn this off? My problem with 256-color is that they don’t work well when I SSH into another machine, because that machine won’t know the color scheme of frontend (SSH client). If I have multiple layers of SSH (not so rare in corp environment) it is much easier to handle the color scheme by the terminal itself, instead of telling every single CLI app to switch color. I ran from Delta simply because of this.

@egrieco
Copy link
Author

egrieco commented Aug 29, 2022

I have not forgotten about this issue, but enough time and changes have past that I will need to go back and ensure that everything is still working.

Also, I need to address the issues that @Wilfred raised:

  • We probably don't need to make line numbers red/green with background highlighting
  • It looks like subword bold highlighting (the 6 in Version 0.6) and delimiters (the ( before reverse) is missing
  • I don't think you need cansi, you should be able to calculate lengths before applying colours

@egrieco
Copy link
Author

egrieco commented Aug 29, 2022

Also, I totally agree that I shouldn't need to use cansi but I couldn't find where to grab the pre-styled text. Will take another look and bug you for help if I truly can't find the correct spot @Wilfred.

@blag
Copy link

blag commented Oct 4, 2022

This looks amazing, I'm excited for it! Thank you!

@tgrosinger
Copy link

It would be really wonderful to get this merged. I'm slightly colorblind and find that with just the thin text being colored it's very hard for me to tell if it's green or red. Adding a highlighted background would make a big difference.

Thank you!

@vasylnakvasiuk
Copy link

delta also allows to emulate diff-highlight and diff-so-fancy. For me diff-so-fancy highlighting style is more readable.

@mattneub
Copy link

Ditto, please merge as there are entire lines of code that I cannot even see with the default colors.

Flakebi added a commit to Flakebi/difftastic that referenced this pull request Dec 7, 2022
@muescha
Copy link

muescha commented Dec 14, 2022

instead of fixed colors: should it make sense to add custom defined color themes so that styling can be adjusted by the user?

@mattneub
Copy link

Or even without entire themes, wouldn't it be great if we could change individual colors as desired? Each color clearly corresponds to a particular "category" in difftastic's mind: "the name of a file", "code that was deleted", that sort of thing. Maybe it could be possible to show difftastic a config file that would set particular colors to something other then the default?

@muescha
Copy link

muescha commented Dec 14, 2022

difftastic a config file that would set particular colors

thx to clarify - exactly this was my intention

@mattneub
Copy link

Thank you for thinking about this feature!

@shelper
Copy link

shelper commented Jan 24, 2023

is this already in the latest build? this is very useful....

Wilfred pushed a commit that referenced this pull request Mar 16, 2023
Remove impossible base types from nullables, pointers and object creation
@egrieco
Copy link
Author

egrieco commented Mar 25, 2024

It's been quite a while since I created this pull request. Lots of updates have occurred since then, and as @Wilfred had pointed out previously, there are better ways to implement the coloring than I had cobbled together back then.

I'll need to clean it up or redo it entirely before it can be merged. I might get some time to do so in the next few weeks, depending on how much prep I need to do for a pending job offer.

@egrieco
Copy link
Author

egrieco commented Mar 25, 2024

Did a little playing around and got some basic background color and theme support working. There are still bugs so I'm not updating or replacing this pull request for now.

difftastic-screenshot-2024-03-25

If anyone wants to take a look, the current changes are here: https://github.com/egrieco/difftastic/tree/background_colors

@muescha
Copy link

muescha commented Mar 25, 2024

I like more the color scheme from the first post: changed lines in light green, changed words in dark green.

@egrieco
Copy link
Author

egrieco commented Mar 26, 2024

I like more the color scheme from the first post: changed lines in light green, changed words in dark green.

Yeah, me too. This is just my attempt to redo the initial work on the new version, and to do so in a cleaner more maintainable way.

@egrieco
Copy link
Author

egrieco commented Mar 29, 2024

Just ran into a compilation error #689 while trying to rebase the new changes onto the latest HEAD.

I'll rebase onto the previous commit where things still compile...

@egrieco
Copy link
Author

egrieco commented Mar 29, 2024

Created a new pull request as a complete redo of this concept. Please continue the discussion over on #690.

@egrieco egrieco closed this Mar 29, 2024
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.

10 participants