Closed
Bug 1224733
Opened 9 years ago
Closed 8 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)
MozReview Graveyard
Review Board: DiffViewer
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.
Updated•8 years ago
|
Product: Developer Services → MozReview
Reporter | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/55160/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/55160/
Reporter | ||
Comment 2•8 years ago
|
||
^ The above MozReview request demonstrates this issue. It's really easy to miss the line that completely changed!
Comment 3•8 years ago
|
||
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
Comment 4•8 years ago
|
||
(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•8 years ago
|
||
(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•8 years ago
|
||
I think this is entirely on the front end. Could you take a look? See also bug 1257396 and bug 1287247.
Assignee: nobody → dwalsh
Comment 10•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
Sadly no change upstream. I've filed an upstream bug about this issue: https://hellosplat.com/s/beanbag/tickets/4444/
Comment 13•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
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
Assignee | ||
Comment 17•8 years ago
|
||
here's what github does in this case. much clearer.
Comment 18•8 years ago
|
||
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•8 years ago
|
||
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
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•8 years ago
|
||
Comment 23•8 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•8 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
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•