4444: Diff Logic Misses Completely Changed Line

davidwalsh

What version are you running?

3.0 alpha 0 (dev)

What's the URL of the page containing the problem?

No public URL but local URL path is (/r/1/diff/1#index_header); i.e. a diff page

What steps will reproduce the problem?

  1. Create a commit with the code from the left side of the attached image
  2. Create a second commit with code changes from the right side of the attached image
  3. Create review request, view diff page

What is the expected output? What do you see instead?

The line that's been completely changed mixes in with the minor changes -- it's very difficult to see an entire line has been changed.

What operating system are you using? What browser?

Mac / Chrome

Please provide any additional information below.

Line which have been completely changed, which display in the middle of minor changes, should be chunked separately and have a more apparent color scheme. The minor changes have span.hl around them to show minor differences, but unforunately an entire line change has no discernible call out.

david
#1 david

The point of the small character highlights is to call out cases where only a few characters have changed, which are otherwise very difficult to spot. In the case where the entire line has changed, we don't want to overload the screen with the darker background because of the reduced contrast.

In your screenshot, you've managed to find the edge case in this behavior, which is where most of the changes are very small but one is not. In this case it's probably less than optimal, but it's definitely not "missing" the line (it's highlighted in yellow, indicating that the line has changed).

Unfortunately, we can't do much about this without making the overall experience worse for the general case.

  • -New
    +NotABug
#2 theres-waldo

I have a very simple suggestion that doesn't seem to make the overall experience worse: when the entire line has changed, choose the darker yellow as the highlight color for the entire line.

#3 glob

another option would be to not highlight an entire line unless the entire line has changed.
ie. "few character" changes would have a while background, with just the modified characters highlighted with a yellow background.

chipx86
#4 chipx86

We'll probably want to keep the entire line yellow. It's how other diff viewers work, and how Review Board has worked its entire life, so changing that would be very disruptive. Making the whole line darker when we can't find a few characters that change would also make the entire line's text a bit harder to read, and nearby replace lines with only segments changing would blend in too much. The current worst-case scenario behavior (keeping the whole line in yellow) is at least still consistent with traditional diff viewers.

I think we'll want to keep this as-is for now, but we'll keep the suggestions in mind.

#5 glob

github's side-by-side view makes these sorts of changes crystal clear, imho without making the overall experience worse.

#6 davidwalsh

@glob : Yes, that's incredibly useful!