1311571 - (bz-diff2html) Replace PatchReader with a modern diff viewer

Status: ASSIGNED

(Bugzilla :: Attachments & Requests, enhancement, P1)

Description

[Jeff Fearn 🐞]

Attachment: Patch to add diff2html

PatchReader is old, unattractive, and isn't having new features added, it needs to be replaced with a more modern diff viewer.

Attached is a patch to replace the use of PatchReader for side-by-side and line-by-line modes with https://diff2html.xyz, it includes highlight.js and the highlight.js github theme. It does not add any new Perl dependencies.

I left PatchReader in place as I've not used the interdiff functionality and didn't know if it was appropriate to remove or change that.

Comment 1

[Dylan Hardison [:dylan] (he/him)]

we already include highlight js (for markdown code blocks) but merging that should be easy. I'll do some testing later on.

Comment 2

[Jeff Fearn 🐞]

Attachment: diff2html.patch

Modified to use existing highlight.js files

Comment 3

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8807043 [details] [diff] [review]
diff2html.patch

Review of attachment 8807043 [details] [diff] [review]:
-----------------------------------------------------------------

I like this patch. It needs a little work:

1) background colors don't look good with sandstone. Possible adding #diff { background-color: white } might work.
2) We don't need to shop the non-minified css and js.

Comment 4

[Jeff Fearn 🐞]

Attachment: updated patch, sans min files, tweaked css

Comment 5

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8817577 [details] [diff] [review]
updated patch, sans min files, tweaked css

Review of attachment 8817577 [details] [diff] [review]:
-----------------------------------------------------------------

this is a soft r- -- it would be an r+ but I think I miscommunicated :)

I want only the *minified* js files for diff2html. Basically external code should be minified.

Otherwise this works and looks great! On very large patches it can be a little slow.

Comment 6

[Jeff Fearn 🐞]

Are you sure only shipping minified is a good policy? i.e. Debian requires unminified sources for JS files and the like.

Comment 7

[Dylan Hardison [:dylan] (he/him)]

(In reply to Jeff Fearn from comment #6)
> Are you sure only shipping minified is a good policy? i.e. Debian requires
> unminified sources for JS files and the like.

It is what we currently do, but we can be better. Let me file a bug.