search

ymattw / ydiff

View colored, incremental diff in workspace or from stdin with side by side and auto pager support
Other
873 stars 62 forks source link

'Del' and 'Add' colors don't display correctly when converted to HTML. #47

Closed NHDaly closed 9 years ago

NHDaly commented 9 years ago

cdiff outputs block diffed text with red foreground AND red background, or green foreground AND green background, but I think the terminal is kind enough to display it with just the background color set. I noticed this when I was converting output from cdiff to html using aha. For example (I've trimmed a bit of the unnecessary bits):

$ diff <(echo 'a') <(echo 'b') --unified | cdiff -s --color=always | aha
<head>
<title>stdin</title>
</head>
<body>
<pre>
</span><span style="color:olive;">1</span> <span style="color:red;"></span><span style="color:gray;background-color:red;"></span><span style="color:red;background-color:red;">a</span><span style="color:red;"></span>                                                                                <span style="color:olive;">1</span> <span style="color:green;"></span><span style="color:gray;background-color:green;"></span><span style="color:green;background-color:green;">b</span><span style="color:green;"></span>
</pre>
</body>
</html>

I think it's because aha is converting from

ESC[0mESC[33m1ESC[0m ESC[31mESC[7mESC[31maESC[0mESC[31mESC[0m                                                                                ESC[0mESC[33m1ESC[0m ESC[32mESC[7mESC[32mbESC[0mESC[32mESC[0m

which you'll notice has ESC[31mESC[7mESC[31ma, which I think should just be ESC[31mESC[7ma. As it is, it sets the foreground, reverses, and then sets the foreground again.

NHDaly commented 9 years ago

Oh, I've broken the builds because I changed the markdown output. Let me know if I need to mark this pull request with something after you've looked at it! :)

ymattw commented 9 years ago

Sorry I haven't looked into the details, but I think cdiff does the right thing. The base_color is required as the design is to use different base color for del and add lines.

On Sunday, May 10, 2015, Nathan Daly notifications@github.com wrote:

Oh, I've broken the builds because I changed the markdown output. Let me know if I need to mark this pull request with something after you've looked at it! :)

— Reply to this email directly or view it on GitHub https://github.com/ymattw/cdiff/issues/47#issuecomment-100772365.

NHDaly commented 9 years ago

No problem. Yeah, I think it looks like the right thing, but it's actually setting the base_color twice, that's the issue. Once at line 455 before the reverse, and once where i've deleted it after the reverse, so the outcome is the base_color in the fg and bg.

NHDaly commented 9 years ago

But actually, I didn't notice before that this is a while-loop... so maybe my change isn't correct in all the cases either...

Maybe we should just remove the statement on line 455?

But does the problem I'm describing make sense? Thanks for taking a look at this! :)

ymattw commented 9 years ago

It should be an issue of aha. Cdiff is doing the right thing, the duplicate base_color comes from upstream python difflib. It's not worth to optimize there.

ymattw commented 9 years ago

Closing as this should be either enhanced in difflib or fixed in tool aha.