Improve inline-diff highlighting - #2619
Conversation
|
I don't think this will be better 
This is the diff in GitHub: 
You can see that GitHub also ignore inline-diff when the count of deleted is mismatch the count of added. |
|
@love-linger It's of course a bit hit-or-miss, but there are many VERY TRIVIAL cases where a "mismatched" inline-diff would make a lot of sense, and would otherwise miss the inline-diff entirely. For example:
I can easily see past some "false positives" if I get to see these very useful (trivial) ones! NOTE: The "matched number of lines" inline-diffs are sometimes just as "false" as the "mismatched" ones, so there's actually not much of a difference in these two cases. (The only way to make both cases better would be to implement a deeper and more "expensive" search to find the most similar lines in the diff-hunk. But for now, this change at least makes sure we don't miss the most trivial cases of mismatched lines.) (Also, just because GitHub doesn't do this, it doesn't mean their implementation is the better one...) |
|
@love-linger Also, to motivate my other commit in this PR (where I increased the limit on the number of inline-diffs per line), here are two trivial examples where the earlier existing implementation skips showing the (already calculated) inline-diffs:
But with my proposed change, both of these cases are handled more correctly:
|
|
The original design intention of inline‑diff is to highlight modified parts when the changed content is minor compared with the original content (which may be hard to spot); when the changes are extensive, amounting to practically a full rewrite, no highlighting will be applied (the entire line will be directly treated as modified). The current solution does have certain issues, since code cannot identify differences at a glance the way humans can. For example: 
You can see the 3 lines have been totally rewrited. We should not use inline-diff highlights here. |
I beg to differ! I'd gladly take these "false" highlights, as long as the trivial highlights from my examples are covered! This is as good we can make it without a more extensive and expensive algorithm. There's a pretty large risk of not spotting small/minor changes, when the inline-diff highlights SOME of them but not ALL of them, as all my example screenshots above show. |
|
You may have misunderstood this feature: No hightlights does not mean no changes. Instead, it means the change is large!!! |
I completely understand that. 🙄 BUT if you look closely at my examples, the minor changes could easily be non-spotted JUST BECAUSE the overall changes are larger. Also, some of the "large" changes in my examples are NOT large, they are in fact very small but still cause mismatched lines (as in the example of an added empty line). And the limit on the number of inline-chunks causes VERY SIMILAR changed lines to HAVE or NOT HAVE highlights - which even seems random at first! A coworker asked me about this and was very confused, which led me to implement this PR... Additional suggestion: add a toggle "Highlight inline diffs" so we can quickly turn the feature on/off. (Similar to the existing "Show hidden symbols" toggle.) That way, we can allow the inline-diff to cover these cases, while also allowing it to be easily toggled off if/when it "gets in the way". |
|
Why do you think the examples you gave are common scenarios? Even if we increase the number of change blocks displayed per line, is it necessary to jump directly from 4 to 32? Can changes with more than 10 blocks in a single line really be regarded as minor changes? |
In fact, the example above is precisely a special case. For us humans, it is certainly easy to understand that what remains unchanged is the overall structure of the XML node, while what has changed are the values of the attributes. However, it can also be interpreted that the new position/rotation is completely different from the old one, since they have lots of differences (not minor modifications). |
They are all real-world scenarios, from our codebase (.xml files) and from SourceGit (.md file). The only "contrived" example is the one where I inserted a new XML attribute "a" and changed the indentation, to quickly create a case where both lines would lack highlights due to the number of inline-chunks.
Yes, the increase needs to be substantial. These individual blocks can be trivial, they sometimes include whitespace and indentation etc. XML / XAML examples are telling, there could be many small changes to separate attributes, and highlighting the changed values help a lot in reading the diff. It's rather frustrating if two similar changed lines make a different decision of highlighted vs not. (And the work of calculating these changes is already done, so there's very little performance impact in actually showing them.)
So what? Of course I understand that the whole pos/rot is changed, but the highlights still help me with readability (similar to syntax coloring, but for spotting the actual changed values). Also, it still makes it much quicker to determine if (for example) 1 out of 4 values is UNCHANGED - but for that to be reliable, we must also be able to trust that the highlighting is not "arbitrarily" disabled/skipped on some lines... What about my suggestion of adding a toggle for "Highlight inline diffs" (or similar wording)? If that's something to consider, I could look into it (as a separate PR)... |
|
I do think this is the most common scenarios of git users in real-world: 
You can see that with your PR it highlights blocks anywhere. I know that there's no performance issue with your PR. I just think that hightlighting does not help me to read code any longer. |
03b6fd1 to
29c6d39
Compare
OK. I force-pushed my PR to now include only the commit with increased limit on inline-chunks, to remove some of the perceived "randomness". How about adding a toggle for "Highlight inline diffs" (or similar wording)? If that's something to consider, I could look into it (as a separate PR)? |
|
…se named constants. * The current limit was way too restrictive. Two similar (and perfectly ordinary) changed lines could easily fall into one being inline-highlighted and the other not. * A lot of work would already have been performed to calculate these inline-chunks, only to throw them away above this very restrictive limit.
29c6d39 to
91b391c
Compare
Done. |
2 participants
This PR does two simple changes to make the highlighting of inline diffs more consistent and useful: