-
Notifications
You must be signed in to change notification settings - Fork 348
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
Conversation
In theory, basic math like this should be faster than string allocations and operations.
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.
Let's see if that makes the CI pipeline happy. |
Found an omission. Newly added files do not have the lighter background color applied properly. |
I've actually started using this more frequently than I'll try to get the conflicts and omissions resolved soon, but there is a separate piece of software that I must finish first. |
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? |
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. |
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: Things that jump out at me:
|
I recently became the main organizer of a security meetup. I've been rather busy with the hand-off, hence the radio silence here.
Thanks for the guidance. I'll try to correct these issues soon. 😃 |
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. |
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:
|
Also, I totally agree that I shouldn't need to use |
This looks amazing, I'm excited for it! Thank you! |
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! |
delta also allows to emulate diff-highlight and diff-so-fancy. For me |
Ditto, please merge as there are entire lines of code that I cannot even see with the default colors. |
instead of fixed colors: should it make sense to add custom defined color themes so that styling can be adjusted by the user? |
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? |
thx to clarify - exactly this was my intention |
Thank you for thinking about this feature! |
is this already in the latest build? this is very useful.... |
Remove impossible base types from nullables, pointers and object creation
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. |
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. If anyone wants to take a look, the current changes are here: https://github.com/egrieco/difftastic/tree/background_colors |
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. |
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... |
Created a new pull request as a complete redo of this concept. Please continue the discussion over on #690. |
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.
This addresses issues #167, #265, and #275.