Closed Bug 1224733 Opened 7 years ago Closed 6 years ago

diff highlighting is counterintuitive when there are whole-line changes mixed with lines where a few characters were changed

Categories

(MozReview Graveyard :: Review Board: DiffViewer, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: botond, Assigned: glob)

References

Details

Attachments

(7 files)

When MozReview identifies that two lines are the same except for an added or removed bit, the added/removed bit is highlighted in dark yellow while the rest of the line is highlighted in light yellow. Thus, on such lines, "light yellow" means "the part that stayed the same".

For a line that was rewritten in its entirety, the entire line is highlighted in light yellow. Thus, on such lines, "light yellow" means "the part that changed".

When two such lines are adjacent to each other, it's really easy to get confused and miss the change made to the line that was rewritten.

I think lines of the second kind (rewritten entirely) should be highlighted in dark yellow in their entirety.
Product: Developer Services → MozReview
Assignee: nobody → botond
Assignee: botond → nobody
^ The above MozReview request demonstrates this issue. It's really easy to miss the line that completely changed!
Thanks for the demonstration.  I couldn't even figure out what you were talking about, precisely because the entirely changed line was so hard to see!

I'd like to see what upstream thinks, but I agree that it is a problem.
Component: General → Review Board: Upstream
(In reply to Mark Côté [:mcote] from comment #3)
> Thanks for the demonstration.  I couldn't even figure out what you were
> talking about, precisely because the entirely changed line was so hard to
> see!
> 
> I'd like to see what upstream thinks, but I agree that it is a problem.

I'm still befuddled... is this a colour thing? can someone really spell it out for me
(In reply to Steven MacLeod [:smacleod] from comment #4)
> (In reply to Mark Côté [:mcote] from comment #3)
> > Thanks for the demonstration.  I couldn't even figure out what you were
> > talking about, precisely because the entirely changed line was so hard to
> > see!
> > 
> > I'd like to see what upstream thinks, but I agree that it is a problem.
> 
> I'm still befuddled... is this a colour thing? can someone really spell it
> out for me

spoke to soon. I like botond's suggestion of highlighting completely different lines with the same dark yellow
I think this is entirely on the front end.  Could you take a look?  See also bug 1257396 and bug 1287247.
Assignee: nobody → dwalsh
Attached image Missed Line
I can see how this would be super frustrating :dbaron, thank you for filing and issue about it.

At first I thought this was a front-end issue but it turns out it's a chunking issue. The entire yellow box is output by Reviewboard as a single chunk despite an entire line changing.  I found that out by turning all highlight elements orange for more contrast, then noticing that line wasn't orange at all; the HTML source shows as one chunk.  I'll investigate upstream to see if their parser has gotten better.
https://reviewboard.mozilla.org/r/55160/#review62020

Just pointing out the line that's hard to miss.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:3308
(Diff revision 1)
>        mFrameMetrics.SetScrollableRect(aLayerMetrics.GetScrollableRect());
>        needContentRepaint = true;
>      }
> -    mFrameMetrics.SetCompositionBounds(aLayerMetrics.GetCompositionBounds());
> -    mFrameMetrics.SetRootCompositionSize(aLayerMetrics.GetRootCompositionSize());
> -    mFrameMetrics.SetPresShellResolution(aLayerMetrics.GetPresShellResolution());
> +    mFrameMetrics.SetCompositionBounds(aLayerMetrics.mCompositionBounds);
> +    mFrameMetrics.SetRootCompositionSize(aLayerMetrics.mRootCompositionSize);
> +    iAm.CompletelyChangingThisLine(toDoSomething.CompletelyDifferent());

This is the easily-missable line.
Attached image Upstream Diff.png
Sadly no change upstream.  I've filed an upstream bug about this issue:

https://hellosplat.com/s/beanbag/tickets/4444/
Upstream considers this an edge case and is choosing to call "NotABug". We'll need ot do something internally if we want to take action.
https://hellosplat.com/s/beanbag/tickets/4444/
It's an edge case, but I think it's an important one.  davidwalsh, any ideas on how to fix this for MozReview?
Totally agree it's important.  We'll need to dive into ReviewBoard's diff'ing app/extension code and separate that completely changed line into its own "chunk".
Summary: MozReview diff highlighting is counterintuitive → diff highlighting is counterintuitive when there are whole-line changes mixed with lines where a few characters were changed
Attached image github screenshot
here's what github does in this case.  much clearer.
I'll note that it's not entirely about "the whole line changed". The examples in bug 1262737 are cases where a significant portion of the line, but not the entire line, changed. In fact, as per github screenshot, that's also the case with dbaron's testcase.
I'll also note that I very much prefer the github coloring with only two colors.
Assignee: dwalsh → glob
Component: Review Board: Upstream → Review Board: DiffViewer
Attached image patched screenshot.png
Comment on attachment 8812682 [details]
diffviewer: highlight all same-line changes (bug 1224733);

https://reviewboard.mozilla.org/r/94346/#review94602

::: reviewboard/reviewboard/diffviewer/diffutils.py:728
(Diff revision 1)
> +    # if differ.ratio() < 0.6:
> +    #     return None, None

I don't think you need to leave this code here commented out -> checking the blame on when this comment is added should do the trick.
Attachment #8812682 - Flags: review?(smacleod) → review+
Pushed by bjones@mozilla.com:
https://hg.mozilla.org/webtools/reviewboard/rev/b80eef4c9511
diffviewer: highlight all same-line changes ; r=smacleod
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.