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

RESOLVED FIXED

Status

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: botond, Assigned: glob)

Tracking

Details

Attachments

(7 attachments)

Reporter

Description

4 years ago
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

Updated

3 years ago
Assignee: nobody → botond
Assignee

Updated

3 years ago
Assignee: botond → nobody
Reporter

Comment 2

3 years ago
^ 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
Duplicate of this bug: 1257396
Duplicate of this bug: 1287247
I think this is entirely on the front end.  Could you take a look?  See also bug 1257396 and bug 1287247.
Assignee: nobody → dwalsh
Posted 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.
Posted 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".
Assignee

Updated

3 years ago
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
Assignee

Updated

3 years ago
Duplicate of this bug: 1262737
Assignee

Comment 17

3 years ago
Posted 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

Updated

3 years ago
Duplicate of this bug: 1210204
Assignee

Updated

3 years ago
Assignee: dwalsh → glob
Component: Review Board: Upstream → Review Board: DiffViewer
Comment hidden (mozreview-request)

Comment 23

3 years ago
mozreview-review
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+
Comment hidden (mozreview-request)

Comment 25

3 years ago
Pushed by bjones@mozilla.com:
https://hg.mozilla.org/webtools/reviewboard/rev/b80eef4c9511
diffviewer: highlight all same-line changes ; r=smacleod
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.