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

Character diffs should typically be on word boundaries #4

Closed
danvk opened this issue Sep 16, 2014 · 4 comments
Closed

Character diffs should typically be on word boundaries #4

danvk opened this issue Sep 16, 2014 · 4 comments
Labels

Comments

@danvk
Copy link
Owner

danvk commented Sep 16, 2014

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:

screen shot 2014-09-16 at 11 40 27 am

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.

@danvk danvk added the bug label Sep 16, 2014
@danvk
Copy link
Owner Author

danvk commented Sep 16, 2014

As a counterpoint, here's an example where the current character diffs do really well, but word diffs might not:

(before)

    var lis = this.props.filePairs.map((file_pair, idx) => {
      var content;
      if (idx != props.selectedIndex) {
        content = <a data-idx={idx} onClick={this.clickHandler} href='#'>
          {file_pair.path}</a>;
      } else {
        content = <b>{file_pair.path}</b>
      }
      return <li key={idx}>{content}</li>
    });
    return <ul className="file-list">{lis}</ul>

(after)

    var lis = this.props.filePairs.map((filePair, idx) => {
      var content;
      if (idx != props.selectedIndex) {
        content = <a data-idx={idx} onClick={this.clickHandler} href='#'>
          {filePair.path}</a>;
      } else {
        content = <b>{filePair.path}</b>;
      }
      return <li key={idx}>{content}</li>;
    });
    return <ul className="file-list">{lis}</ul>;

screen shot 2014-09-16 at 5 38 20 pm

@danvk
Copy link
Owner Author

danvk commented Sep 22, 2014

Should be fixed by e8d7b91.

@danvk danvk closed this as completed Sep 22, 2014
@danvk
Copy link
Owner Author

danvk commented Sep 22, 2014

The '_p' → 'P' diff isn't ideal (shows up as '_pair' → 'Pair'), but otherwise this is a big win.

@ryan-williams
Copy link

cool! fwiw, I don't mind file_pair -> FilePair showing up as a coarser / word-level diff.

also just saw this in the wild:
word-diff would win here.

happy to see this fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants