Add Image Diff for SVG files

[KN4CK3R]

Addition to #14450
Close #14329
Close #15920

Added support for diffing svg images. Since svg images don't need to have a size the bounds are guessed and the display is hidden in this case.

No size info available:

Size info available:

Changes:

• Moved content type sniffing from base in own module which caches the result.
• Moved image size info logic from server to client.

[KN4CK3R]

Currently there is no source toggle for SVG images because I want to reuse the logic from #14661 after it gets merged. This will be added in a new PR.

[zeripath]

CI fails

[lunny]

Please resolve the conflicts.

[kdumontnu]

Love this PR! Tested it against a number of files locally and it looks great.

One potential issue is that the image diff is suppressed if the git diff settings are exceeded. This essentially means that most svgs which have been minified won't get the image diff because character length will likely be greater than the default (5000).

The calculated width ends up with a lot of margin in the max case. I wonder if the js logic to calculate the width is working properly. Can we borrow styling from file view page?

I think either of these are potential candidates for separate PR, so I'm happy to change this to an approval if you'd rather merge this as-is!

[silverwind]

I was briefly looking at the existing image diff code before and one thing I don't like about it is are all these size calculations done in JS. I think most if not all these calculations could be replaced by CSS-sized images (e.g. using sensible max-width and max-height) values.

Or do you think some stuff is not possible without those calculations?

[KN4CK3R]

Honestly I don't know if/what you can do with pure CSS here.

[silverwind]

I can check this out later. Not sure whether I'm motivated to refactor to CSS sizes but to me it only seems natural to let images size without JS, but I guess some use cases like swipe diff do require a bit of JS logic for the absolute positioning, the slider and such.

I guess for side-by side alone, JS sizing seems certainly unnecessary and the only justification doing in JS would be so one can re-use the code from swipe diff.

[silverwind]

Hmm, doesn't seem to do anything for me, neither for PNG nor SVG diff. It puts up a spinner but I don't observe any network request and nothing is logged in the console either.

[silverwind]

@KN4CK3R my latest push seems to have introduced some commits from main on this branch, could yo do a rebase please? There are some conflicts so I prefer if you can it.

[silverwind]

The bug with the spinner is with this IntersectionObserver thingy, entry.intersectionRatio comes out as 0 on Firefox for me on page load so onContainerVisibleHandler is never triggered. I'd appreciate if you would not use such arcane APIs and instead just load in first click of View Rendered, I think that should be the default view mode.

[silverwind]

You can probably just use <img loading="lazy"> which in my eyes is vastly superior alternative to IntersectionObserver because it does not even require JavaScript to work. In my experience, it works very well for not loading hidden images.

[KN4CK3R]

Did not work with lazy for me. Could you try this patch? I'm not happy with that because the image diff is now coupled with the toggle. If the rendered view would be the default view all of this should not be needed. I set the rendered view as default because it was the old default too.

diff --git a/web_src/js/features/imagediff.js b/web_src/js/features/imagediff.js
index d41655259..645aa89d0 100644
--- a/web_src/js/features/imagediff.js
+++ b/web_src/js/features/imagediff.js
@@ -61,25 +61,21 @@ export default async function initImageDiff() {
 
   $('.image-diff').each(function() {
     const $container = $(this);
+    const $fileBodyContainer = $container.closest('.file-body');
+    $fileBodyContainer.data('activation-handlers', [onContainerActiveHandler])
+
+    let initialized = false;
 
     if ($container.is(':visible')) {
-      onContainerVisibleHandler();
-    } else {
-      const observer = new IntersectionObserver((entries) => {
-        for (const entry of entries) {
-          if (entry.intersectionRatio > 0) {
-            observer.disconnect();
-            onContainerVisibleHandler();
-            break;
-          }
-        }
-      }, {
-        root: document.documentElement
-      });
-      observer.observe(this);
+      onContainerActiveHandler();
     }
 
-    function onContainerVisibleHandler() {
+    function onContainerActiveHandler() {
+      if (initialized) {
+        return;
+      }
+      initialized = true;
+
       const diffContainerWidth = $container.width() - 300;
       const pathAfter = $container.data('path-after');
       const pathBefore = $container.data('path-before');
diff --git a/web_src/js/index.js b/web_src/js/index.js
index 6cb47cf10..32c6c8a37 100644
--- a/web_src/js/index.js
+++ b/web_src/js/index.js
@@ -2483,15 +2483,22 @@ function initIssueReferenceRepositorySearch() {
 }
 
 function initFileViewToggle() {
-  $('.file-view-toggle').on('click', function() {
-    const $this = $(this);
-    $this.parent().children().removeClass('active');
-    $this.addClass('active');
-
-    const $target = $($this.data('toggle-selector'));
-    $target.parent().children().addClass('hide');
-    $target.removeClass('hide');
-  });
+  for (const el of document.querySelectorAll('.file-view-toggle')) {
+    const target = document.querySelector(el.dataset.toggleSelector);
+    if (target) {
+      el.addEventListener('click', function() {
+        el.parentNode.children.forEach((child) => child.classList.remove('active'));
+        el.classList.add('active');
+
+        target.parentNode.children.forEach((child) => child.classList.add('hide'));
+        target.classList.remove('hide');
+
+        for (const handler of $(target).data('activation-handlers') || []) {
+          handler();
+        }
+      });
+    }
+  }
 }
 
 function initLinkAccountView() {

[codecov-commenter]

Codecov Report

Merging #14867 (221e7e8) into main (7979c36) will increase coverage by 0.02%.
The diff coverage is 63.91%.

@@            Coverage Diff             @@
##             main   #14867      +/-   ##
==========================================
+ Coverage   44.06%   44.08%   +0.02%     
==========================================
  Files         682      683       +1     
  Lines       82460    82442      -18     
==========================================
+ Hits        36338    36348      +10     
+ Misses      40205    40179      -26     
+ Partials     5917     5915       -2     

Impacted Files	Coverage Δ
modules/avatar/avatar.go	47.72% <ø> (ø)
modules/base/tool.go	93.47% <ø> (-0.79%)	⬇️
modules/git/commit.go	60.16% <ø> (+3.69%)	⬆️
modules/indexer/code/bleve.go	68.80% <0.00%> (ø)
modules/indexer/code/elastic_search.go	1.65% <0.00%> (ø)
routers/repo/editor.go	23.83% <0.00%> (ø)
routers/repo/lfs.go	0.00% <0.00%> (ø)
routers/repo/setting.go	13.51% <0.00%> (-0.02%)	⬇️
routers/repo/view.go	41.31% <42.85%> (+0.06%)	⬆️
routers/user/setting/profile.go	45.69% <50.00%> (+0.29%)	⬆️
... and 8 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7979c36...221e7e8. Read the comment docs.

[kdumontnu]

I really think image diff needs to ignore MAX_GIT_DIFF_LINE_CHARACTERS and MAX_GIT_DIFF_LINES before this gets merged. Otherwise, pretty much all svgs will not be diffed in the default case (other than small icon files).

[KN4CK3R]

I merged the incomplete with the normal diff view. Now there is less code with more functionality.

[kdumontnu]

🎉 @silverwind is there anything else you wanted to check out here?