danvk / codediff.js Goto Github PK

Need some way to stick arbitrary markup under a line to show inline comments.

This will make balancing left/right sides more difficult. (Would require either table layout or JS-based layout).

It's silly to show "2 lines omitted". There should be a minimum, say 20 by default, below which we'll just show the code.

From webdiff:

github shows the function that the diff you're looking at is part of, even if its declaration is many lines outside of the diffhunk:

I'm not sure how this is implemented, or if it requires language support. git diff does this, too:

$ git diff d5ce61bccb2
diff --git a/extras/synchronizer.js b/extras/synchronizer.js
index cbe26d2..f39209b 100644
--- a/extras/synchronizer.js
+++ b/extras/synchronizer.js
@@ -200,7 +200,7 @@ function dygraphsBinarySearch(g, xVal) {
   var low = 0,
       high = g.numRows() - 1;

-  while (low <= high) {
+  while (low < high) {
     var idx = (high + low) >> 1;
     var x = g.getValue(idx, 0);
     if (x < xVal) {

Critique, github and others do this. codediff.js should as well.

Current behavior is to scroll both panels horizontally.

This will require some reworking of the markup to keep the left/right side balanced.

See screenshot, I'm getting a scrollbar on both the container page and the iframe'd page.

codediff.js often makes poor choices about how to present character diffs. For example, in this three line diff:

Before:

<ImageDiffModeSelector filePair={filePair}
                       mode={this.state.imageDiffMode}
                       changeHandler={this.changeImageDiffModeHandler}/>

After:

<DiffView filePair={filePair}
          imageDiffMode={this.state.imageDiffMode}
          changeImageDiffModeHandler={this.changeImageDiffModeHandler} />

it shows these character diffs:

All three of these have problems, and all three would be improved by making the intra-line diffs operate at the word level, rather than the character level. In other words, rather than applying the diffing algorithm to line.split('') (which splits the line into individual characters), it should be applied to a list containing the words in the line. These are broken by whitespace, symbols and capital/lowercase transitions.

e.g. when I run webdiff https://github.com/danvk/dygraphs/pull/534/files, I see this:

it would be nice if I could see, say, 50 of those lines of context rather than all 2011 of them.

The mocha test runner uses this to get character-by-character wrapping w/o adding soft breaks everywhere.

coveralls.io is pretty nice!

It's not terrible, but the added comment plus character diffs produce a sub-optimal rendering:

It would be better if the comment were a pure add and each individual line were a three character change (adding 'chr').

Would be helpful for jumping to a comment (e.g. #7)

Blank lines aren't visibly selected:

They are copy/pasted, but you wouldn't know if from the display.

You should never see "Skip 1 lines" when minJumpSize=10.

This is codediff.ts d22a5f5..805e8c1 in this repo

In hammerlab/cycledash, diffing ef8764395079dfc2ab5b9c70db9ad9b735d5e0cd..6bbc1747de05b4cadfb4e900928047d8c8bad15c produces a really bad diff in cycledash/views.py. One small function (start_workers_for_run) moves, while the rest of the file is modified in small ways. codediff.js aligns on start_workers_for_run, whereas GitHub shows it moving while the rest of the file stays relatively motionless.

It's not clear what distinction is being made between the two shades of green on the right. It's certainly not a helpful one.

I think one is a change and one is an add, but this distinction just doesn't matter.

The screenshot in README.md is getting increasingly out of step with the current UI.