3949: Diffs: Distinguish between light yellow "nothing changed/whitespace changes" and light yellow "completely different lines"
- New
- Review Board
| AlexC | |
What version are you running? 2.0.18.1 Describe the enhancement and the motivation for it. The colour coding of lines in diffs is very helpful. Green is clearly lines that are added; red is clearly lines that are removed; white is clearly unchanged (or only indentation changes). Yellow, however, is an ambiguous colour with several possible meanings: * If a line has merely had some trailing whitespace removed, or some internal whitespace removed, the line is highlighted in light yellow on both sides with no dark yellow. We generally don't actually care about these "changes". * If a line has had a tiny change, the whole line is highlighted in light yellow and the tiny changed bit is highlighted in dark yellow on both sides. * If a line has been partly changed, the whole line is highlighted in light yellow and the medium-sized changed bit is highlighted in dark yellow on both sides. * If a line has had some text added, the whole line is highlighted in light yellow and the new content is highlighted in dark yellow on the right-hand-side only. * If a line has been wholesale replaced with a completely different line, the line is highlighted in LIGHT yellow and NOTHING is highlighted in dark yellow. Reading down just the right-hand-side (new code), therefore, it's ambiguous what some code in light yellow means. Is this 1) a line that preexisted as and has just had some neglegibly small change made to it? 2) A line that's had some amount of text removed? Or 3) a line that's completely unrelated to whatever was there in the previous version? Manually eyeballing the lines on the left and right looking for any dark yellow blocks helps distinguish case 2) from the others, but cases 1) and 3) can only be resolved by looking at the text on the left and comparing it. Fundamentally, it's not clear for any given line whether light yellow means "unchanged text" or "very changed text". See the example attachment rb_yellows.png. Looking at the new code, there are three lines on the right all of which are highlighted light yellow but with no dark yellow highlights. On line 5, this is a line where some code is deleted, and all the text there pre-existed. On line 13, this is a completely different line to what was there before. On line 11, ReviewBoard has decided this is a completely different line, though perhaps it should notice that half of the line pre-existed and the second half should be highlighted in dark yellow. This is the frustration and the motivation for my proposed change. My actual proposal would be: In the case where a yellow line is completely different, don't leave it only highlighted in light yellow; highlight all non-whitespace characters on both sides in dark yellow. My example mockup of what this might look like is attached in rb_yellows_mine.png. What operating system are you using? What browser? Chrome / Windows.
Another example of the current confusing behaviour to demonstrate my point further. If I'm trying to follow the code flow on the right, reading the new code, I can see that although some of the lines are in light yellow, the only changes are adding the string 'NotSupported' to the contents of a string and a variable name. I therefore know that the basic functional flow of the code is unchanged. However, then I get to lines 245-246, which are also in light yellow, but this does *NOT* indicate the basic functional flow is unchanged; in fact it indicates the code is *completely different*. Once again, it's not clear for any given line whether light yellow means "unchanged text" or "very changed text".
-
+ 
Hello, I've been very frustrated by the same thing, and I actually have something you can do for the problem you describe here. (But it doesn't solve everything.) The following edit to the reviewboard source files will remove the filter that says "if the two lines are very different, don't highlight any part of them in dark yellow".
Edit reviewboard/diffviewer/diffutils.py
Find these two lines:
if differ.ratio() < 0.6:
return None, None
Delete them or comment them out.
Restart your reviewboard server.I documented the major remaining problem I have with the diff viewer after making this change in bug 3988.