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

Status: RESOLVED FIXED

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

Description

[Botond Ballo [:botond]]

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.

Comment 1

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1224733 - Test commit to demonstrate the issue

Review commit: https://reviewboard.mozilla.org/r/55160/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55160/

Comment 2

[Botond Ballo [:botond]]

^ The above MozReview request demonstrates this issue. It's really easy to miss the line that completely changed!

Comment 3

[Mark Côté [:mcote]]

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.

Comment 4

[Steven MacLeod [:smacleod]]

(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

Comment 5

[Steven MacLeod [:smacleod]]

(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

Comment 8

[Mark Côté [:mcote]]

I think this is entirely on the front end.  Could you take a look?  See also bug 1257396 and bug 1287247.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: screenshot of test commit showing the problem

Comment 10

[David Walsh :davidwalsh]

Attachment: 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.

Comment 11

[David Walsh :davidwalsh]

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.

Comment 12

[David Walsh :davidwalsh]

Attachment: Upstream Diff.png

Sadly no change upstream.  I've filed an upstream bug about this issue:

https://hellosplat.com/s/beanbag/tickets/4444/

Comment 13

[David Walsh :davidwalsh]

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/

Comment 14

[Mark Côté [:mcote]]

It's an edge case, but I think it's an important one.  davidwalsh, any ideas on how to fix this for MozReview?

Comment 15

[David Walsh :davidwalsh]

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".

Comment 17

[:glob ✱]

Attachment: github screenshot

here's what github does in this case.  much clearer.

Comment 18

[Mike Hommey [:glandium]]

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.

Comment 19

[Mike Hommey [:glandium]]

I'll also note that I very much prefer the github coloring with only two colors.

Comment 21

[:glob ✱]

Attachment: diffviewer: highlight all same-line changes (bug 1224733);

Always highlight same-line changes, instead of representing the line as
unchanged.

Review commit: https://reviewboard.mozilla.org/r/94346/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94346/

Comment 22

[:glob ✱]

Attachment: patched screenshot.png

Comment 23

[Steven MacLeod [:smacleod]]

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.

Comment 24

[:glob ✱]

Comment on attachment 8812682 [details]
diffviewer: highlight all same-line changes (bug 1224733);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94346/diff/1-2/

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/webtools/reviewboard/rev/b80eef4c9511
diffviewer: highlight all same-line changes ; r=smacleod