search

scottgonzalez / pretty-diff

colorized HTML diffs
MIT License
218 stars 27 forks source link

Support split view #13

Open jzaefferer opened 10 years ago

jzaefferer commented 10 years ago

Like the new one GitHub has.

jzaefferer commented 9 years ago

Is that reasonable at all?

scottgonzalez commented 9 years ago

Yeah, it shouldn't be too hard to implement. Just need some time to work on it.

scottgonzalez commented 9 years ago

Another early Christmas present for you...

I just pushed a new branch (split), which has an initial implementation for this. The code is ugly as hell, but it seems to work. The branch version just changes all diffs to be split diffs, but the real version will have an option. Play with it and see if it works for various diffs. If everything looks good, I'll get this into master with the option.

jzaefferer commented 9 years ago

Nice. Looks promising!

Looking at https://github.com/jquery/jquery-ui/commit/b6bec797d6a8ef0b377a866c38c67e66a626b45f through pretty-diff, the first difference doesn't line up, maybe due to the lines being too long? The first line on both sides wraps on my screen. This is a real problem.

Looking at https://github.com/jquery/qunit/commit/d86d6c38991a82e2c534eacc03dd10eac1ca1e69 there's a blank/pale line between each of the lines on the right for the new setUrl function. On Github, the background is just solid green, no lines in between. This is okay, though it should be just a CSS issue.

Otherwise I noticed the "file name diff", like this:

--- a/reporter/html.js
+++ b/reporter/html.js

Except now they're side-by-side. They were there before, so not really part of this issue. Could be optimized to show renames when they occur and hide this otherwise.

scottgonzalez commented 9 years ago

Just pushed some updates. I've been thinking about changing the handling for the file names for a while. I'll tackle that separately though. Let me know if you find any problems with it now.

jzaefferer commented 9 years ago

I ran npm install -g scottgonzalez/pretty-diff#split to install this again.

For this commit, I think the output is worse then before. The last change doesn't line up. That also happens for the other commits I mentioned before:

...

scottgonzalez commented 9 years ago

Yeah, I broke the detection of the first line in the lookahead, so everything was off. I also just realized why GitHub is using tables for split view; it's needed to handle one side of the diff having a long line which wraps when the other side doesn't. I'll get that updated soon too.

jzaefferer commented 9 years ago

Looks better now. Let me know when you got this tabled, and I'll test again.

Regarding configuration: I like GitHub's model of remember the choice and using that for all diffs until I pick the other view. Maybe you can do the same on the CLI? Once I pick split view using an option, make that the default until I switch again. Like --split and --unified.

tauren commented 9 years ago

This is great stuff, nice work!

I have a use for both your split branch and your word-diff branch. Will they eventually be usable together and published to NPM? I can make due for now, just interested in your thoughts on the project's roadmap.

scottgonzalez commented 9 years ago

Will they eventually be usable together and published to NPM?

I'm not sure that word-diff will ever be usable. If it is, then they'll work together.

jzaefferer commented 9 years ago

Btw. I've been using the split view since January or so, haven't noticed any other issues.

jzaefferer commented 9 years ago

I don't know where my previous comment came from, I haven't actually used the split view locally...