Page MenuHomeSoftware Heritage

assets/diff-utils: Add diff lines highlighting feature
ClosedPublic

Authored by anlambert on Aug 13 2020, 6:44 PM.

Details

Summary

That new feature enables to highlight code fragments in a diff generated
from the revision view by selecting a lines range with the mouse (click
and Shift+click on line numbers).

Both unified and split diff can be highlighted. For the split case,
only lines from the left or right part can be highlighted but also lines
spanning both.

When a diff gets highlighted, an URL fragment will be added describing the
highlighting state. When the revision view gets reloaded with it, the
diff will be automatically computed, highlighted and displayed.

Below are some screenshots of highlighted diff examples:

Closes T2491

Diff Detail

Repository
rDWAPPS Web applications
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

anlambert created this revision.Aug 13 2020, 6:44 PM

Build is green

Patch application report for D3788 (id=13306)

Rebasing onto 0cb8a77860...

Current branch diff-target is up to date.
Changes applied before test
commit 3174b773c3dcb9537d4527d2da0d8b8ae3d4f815
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Wed Aug 5 16:05:39 2020 +0200

    assets/diff-utils: Add diff lines highlighting feature
    
    That new feature enables to highlight code fragments in a diff generated
    from the revision view by selecting a lines range with the mouse (click
    and Shift+click on line numbers).
    
    Both unified and split diff can be highlighted. For the split case,
    only lines from the left or right part can be highlighted but also lines
    spanning both.
    
    When a diff gets highlighted, an URL fragment will be added describing the
    highlighting state. When the revision view gets reloaded with it, the
    diff will be automatically computed, highlighted and displayed.
    
    Closes T2491

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/323/ for more details.

I didn't read the code yet, but I have some overall remarks.


First, the new functions in diff-utils.js are not tested at all. It could easily be unit-tested as they are pure functions, and should be.

Additionally, the new tests in revision-diff.spec.js select random lines and rely on these functions to know which line should be highlighted. Why not make these tests deterministic and hard-code what lines should be highlighted? It would make stronger and reproducible tests. (eg. when trying out the diff with color changes, every run of the tests selected a different file/range, which made comparison difficult)


Second, it is my understanding that browse URLs aren't guaranteed to be stable, right? If so, then are we planning on adding this range feature to SWHIDs?


Finally, the contrast between green, red, and this new color in-between isn't great IMO, even if you have good eyesight. And it get worse for color-blind users.

With red-insensitivity:

and green-insensitivity:

I played around with colors a little, and const lineHighlightColor = 'rgba(128, 128, 255, 0.1)'; gives okay-ish results. It's better for me (not color-blind) and color-blind users.

Non-color-blind:

red-insensitivity:

green-insensitivity:

The contrast between red and green is still pretty bad for users with green insensitivity, but it wasn't great to begin with.

So alternatively, I propose that instead of highlighting code area, we only highlight line numbers. It could be either a solid color like this diff currently does (like phabricator), or some kind of vertical bar (like Github does) which would stand out better but is also harder to implement.

First, the new functions in diff-utils.js are not tested at all. It could easily be unit-tested as they are pure functions, and should be.

They are not unit tested but at least their use are covered by the cypress tests (see https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/Istanbul_20Code_20Coverage/bundles/revision/diff-utils.js.html). Most of them manipulate the DOM so I prefer the cypress approach to validate a new frontend feature.

Additionally, the new tests in revision-diff.spec.js select random lines and rely on these functions to know which line should be highlighted. Why not make these tests deterministic and hard-code what > lines should be highlighted? It would make stronger and reproducible tests. (eg. when trying out the diff with color changes, every run of the tests selected a different file/range, which made comparison > difficult)

Because I wanted to test every possible highlighting scenarios without hard-coding any test data apart a revision number. Anyway, hard-coding tests data will speedup overall tests execution as large lines range can be selected using the random approach. I will see what I can do to improve that.

Second, it is my understanding that browse URLs aren't guaranteed to be stable, right? If so, then are we planning on adding this range feature to SWHIDs?

I do not think so. If the browse URL scheme evolves, we will have to maintain backward compatibility.

Finally, the contrast between green, red, and this new color in-between isn't great IMO, even if you have good eyesight. And it get worse for color-blind users.

I inspired from Phabricator, Github and Gitlab, for the highlighting color, both use a yellowish color to highlight lines in their diff views.
I should use a lighter one though.

I played around with colors a little, and const lineHighlightColor = 'rgba(128, 128, 255, 0.1)'; gives okay-ish results. It's better for me (not color-blind) and color-blind users.

For me it's not better, the highlighted lines do not stand out from the diff and it takes more time to identify them.

So alternatively, I propose that instead of highlighting code area, we only highlight line numbers. It could be either a solid color like this diff currently does (like phabricator), or some kind of vertical bar (like Github does) which would stand out better but is also harder to implement.

I prefer to keep the highlighted code areas as they stand out visually for non color-blind users. I agree that highlighting lines numbers should also be added. The ultimate visual clue will be to add borders to the highlighted area like Github does but it will be a pain to implement. I will add line numbers highlighting at the moment.

Regarding the accessibility issues, I think we should identify them with concerned users and create a dedicated meta-task on the subject.

First, the new functions in diff-utils.js are not tested at all. It could easily be unit-tested as they are pure functions, and should be.

They are not unit tested but at least their use are covered by the cypress tests (see https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/Istanbul_20Code_20Coverage/bundles/revision/diff-utils.js.html). Most of them manipulate the DOM so I prefer the cypress approach to validate a new frontend feature.

Hmm indeed they are tested, I missed that checkDiffHighlighted does its own computation instead of relying on them.

But I still think unit-testing would be nice to have in addition to round-tripping through the DOM.

I do not think so. If the browse URL scheme evolves, we will have to maintain backward compatibility.

ack

Finally, the contrast between green, red, and this new color in-between isn't great IMO, even if you have good eyesight. And it get worse for color-blind users.

I inspired from Phabricator, Github and Gitlab, for the highlighting color, both use a yellowish color to highlight lines in their diff views.
I should use a lighter one though.

I played around with colors a little, and const lineHighlightColor = 'rgba(128, 128, 255, 0.1)'; gives okay-ish results. It's better for me (not color-blind) and color-blind users.

For me it's not better, the highlighted lines do not stand out from the diff and it takes more time to identify them.

You could fiddle with the opacity, but I agree it's far from perfect (hence "okay-ish").

Regarding the accessibility issues, I think we should identify them with concerned users and create a dedicated meta-task on the subject.

ack

anlambert planned changes to this revision.Aug 14 2020, 3:45 PM

Thanks @vlorentz for the review. It seems there is still room for improvements before landing this.

anlambert updated this revision to Diff 13438.Aug 20 2020, 1:26 PM

Update:

  • rebase
  • set lighter highlighting color
  • emphasize selected diff line numbers by setting their font weight to bold
  • simplify cypress tests: harcode diffs to highlight instead of using a random approach to get reproducible tests

Below is a screenshot of updated diff highlighting:

Build is green

Patch application report for D3788 (id=13438)

Rebasing onto 4332763d1c...

Current branch diff-target is up to date.
Changes applied before test
commit da90abe1da5b832d97f86d96d4927e2e6dac0772
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Wed Aug 5 16:05:39 2020 +0200

    assets/diff-utils: Add diff lines highlighting feature
    
    That new feature enables to highlight code fragments in a diff generated
    from the revision view by selecting a lines range with the mouse (click
    and Shift+click on line numbers).
    
    Both unified and split diff can be highlighted. For the split case,
    only lines from the left or right part can be highlighted but also lines
    spanning both.
    
    When a diff gets highlighted, an URL fragment will be added describing the
    highlighting state. When the revision view gets reloaded with it, the
    diff will be automatically computed, highlighted and displayed.
    
    Closes T2491

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/335/ for more details.

vlorentz accepted this revision.Aug 20 2020, 1:33 PM

great!

This revision is now accepted and ready to land.Aug 20 2020, 1:33 PM