From 73b63d93117db36feda11e53099baa8995a38df0 Mon Sep 17 00:00:00 2001 From: silverwind Date: Wed, 11 Oct 2023 14:34:21 +0200 Subject: [PATCH] Replace ajax with fetch, improve image diff (#27267) 1. Dropzone attachment removal, pretty simple replacement 2. Image diff: The previous code fetched every image twice, once via `img[src]` and once via `$.ajax`. Now it's only fetched once and a second time only when necessary. The image diff code was partially rewritten. --------- Co-authored-by: Giteabot --- routers/web/repo/compare.go | 27 +++++--- templates/repo/diff/box.tmpl | 8 ++- templates/repo/diff/image_diff.tmpl | 7 ++- web_src/js/features/common-global.js | 7 +-- web_src/js/features/imagediff.js | 93 ++++++++++++---------------- web_src/js/modules/fetch.js | 4 +- web_src/js/svg.js | 10 ++- web_src/js/utils.js | 11 ++++ web_src/js/utils/dom.js | 11 ++++ 9 files changed, 96 insertions(+), 82 deletions(-) diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index d66dd582a1..b69af3c61c 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -32,6 +32,7 @@ import ( "code.gitea.io/gitea/modules/markup" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/typesniffer" "code.gitea.io/gitea/modules/upload" "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/services/gitdiff" @@ -60,6 +61,21 @@ func setCompareContext(ctx *context.Context, before, head *git.Commit, headOwner return blob } + ctx.Data["GetSniffedTypeForBlob"] = func(blob *git.Blob) typesniffer.SniffedType { + st := typesniffer.SniffedType{} + + if blob == nil { + return st + } + + st, err := blob.GuessContentType() + if err != nil { + log.Error("GuessContentType failed: %v", err) + return st + } + return st + } + setPathsCompareContext(ctx, before, head, headOwner, headName) setImageCompareContext(ctx) setCsvCompareContext(ctx) @@ -87,16 +103,7 @@ func setPathsCompareContext(ctx *context.Context, base, head *git.Commit, headOw // setImageCompareContext sets context data that is required by image compare template func setImageCompareContext(ctx *context.Context) { - ctx.Data["IsBlobAnImage"] = func(blob *git.Blob) bool { - if blob == nil { - return false - } - - st, err := blob.GuessContentType() - if err != nil { - log.Error("GuessContentType failed: %v", err) - return false - } + ctx.Data["IsSniffedTypeAnImage"] = func(st typesniffer.SniffedType) bool { return st.IsImage() && (setting.UI.SVG.Enabled || !st.IsSvgImage()) } } diff --git a/templates/repo/diff/box.tmpl b/templates/repo/diff/box.tmpl index 94a5a9a295..289ed90d3f 100644 --- a/templates/repo/diff/box.tmpl +++ b/templates/repo/diff/box.tmpl @@ -97,7 +97,9 @@ {{/*notice: the index of Diff.Files should not be used for element ID, because the index will be restarted from 0 when doing load-more for PRs with a lot of files*/}} {{$blobBase := call $.GetBlobByPathForCommit $.BeforeCommit $file.OldName}} {{$blobHead := call $.GetBlobByPathForCommit $.HeadCommit $file.Name}} - {{$isImage := or (call $.IsBlobAnImage $blobBase) (call $.IsBlobAnImage $blobHead)}} + {{$sniffedTypeBase := call $.GetSniffedTypeForBlob $blobBase}} + {{$sniffedTypeHead := call $.GetSniffedTypeForBlob $blobHead}} + {{$isImage:= or (call $.IsSniffedTypeAnImage $sniffedTypeBase) (call $.IsSniffedTypeAnImage $sniffedTypeHead)}} {{$isCsv := (call $.IsCsvFile $file)}} {{$showFileViewToggle := or $isImage (and (not $file.IsIncomplete) $isCsv)}} {{$isExpandable := or (gt $file.Addition 0) (gt $file.Deletion 0) $file.IsBin}} @@ -198,9 +200,9 @@
{{if $isImage}} - {{template "repo/diff/image_diff" dict "file" . "root" $ "blobBase" $blobBase "blobHead" $blobHead}} + {{template "repo/diff/image_diff" dict "file" . "root" $ "blobBase" $blobBase "blobHead" $blobHead "sniffedTypeBase" $sniffedTypeBase "sniffedTypeHead" $sniffedTypeHead}} {{else}} - {{template "repo/diff/csv_diff" dict "file" . "root" $ "blobBase" $blobBase "blobHead" $blobHead}} + {{template "repo/diff/csv_diff" dict "file" . "root" $ "blobBase" $blobBase "blobHead" $blobHead "sniffedTypeBase" $sniffedTypeBase "sniffedTypeHead" $sniffedTypeHead}} {{end}}
diff --git a/templates/repo/diff/image_diff.tmpl b/templates/repo/diff/image_diff.tmpl index 8abce9479e..02cca784f6 100644 --- a/templates/repo/diff/image_diff.tmpl +++ b/templates/repo/diff/image_diff.tmpl @@ -1,7 +1,12 @@ {{if or .blobBase .blobHead}} -
+
{{ctx.Locale.Tr "repo.diff.image.side_by_side"}} diff --git a/web_src/js/features/common-global.js b/web_src/js/features/common-global.js index bc775ae545..2d8289d4e3 100644 --- a/web_src/js/features/common-global.js +++ b/web_src/js/features/common-global.js @@ -11,7 +11,7 @@ import {htmlEscape} from 'escape-goat'; import {showTemporaryTooltip} from '../modules/tippy.js'; import {confirmModal} from './comp/ConfirmModal.js'; import {showErrorToast} from '../modules/toast.js'; -import {request} from '../modules/fetch.js'; +import {request, POST} from '../modules/fetch.js'; const {appUrl, appSubUrl, csrfToken, i18n} = window.config; @@ -243,9 +243,8 @@ export function initGlobalDropzone() { this.on('removedfile', (file) => { $(`#${file.uuid}`).remove(); if ($dropzone.data('remove-url')) { - $.post($dropzone.data('remove-url'), { - file: file.uuid, - _csrf: csrfToken, + POST($dropzone.data('remove-url'), { + data: new URLSearchParams({file: file.uuid}), }); } }); diff --git a/web_src/js/features/imagediff.js b/web_src/js/features/imagediff.js index 23b048f295..80b7e83385 100644 --- a/web_src/js/features/imagediff.js +++ b/web_src/js/features/imagediff.js @@ -1,11 +1,14 @@ import $ from 'jquery'; -import {hideElem} from '../utils/dom.js'; +import {GET} from '../modules/fetch.js'; +import {hideElem, loadElem} from '../utils/dom.js'; +import {parseDom} from '../utils.js'; -function getDefaultSvgBoundsIfUndefined(svgXml, src) { +function getDefaultSvgBoundsIfUndefined(text, src) { const DefaultSize = 300; const MaxSize = 99999; - const svg = svgXml.documentElement; + const svgDoc = parseDom(text, 'image/svg+xml'); + const svg = svgDoc.documentElement; const width = svg?.width?.baseVal; const height = svg?.height?.baseVal; if (width === undefined || height === undefined) { @@ -65,73 +68,53 @@ export function initImageDiff() { }; } - $('.image-diff:not([data-image-diff-loaded])').each(function() { + $('.image-diff:not([data-image-diff-loaded])').each(async function() { const $container = $(this); $container.attr('data-image-diff-loaded', 'true'); // the container may be hidden by "viewed" checkbox, so use the parent's width for reference const diffContainerWidth = Math.max($container.closest('.diff-file-box').width() - 300, 100); - const pathAfter = $container.data('path-after'); - const pathBefore = $container.data('path-before'); const imageInfos = [{ - loaded: false, - path: pathAfter, - $image: $container.find('img.image-after'), + path: this.getAttribute('data-path-after'), + mime: this.getAttribute('data-mime-after'), + $images: $container.find('img.image-after'), // matches 3 $boundsInfo: $container.find('.bounds-info-after') }, { - loaded: false, - path: pathBefore, - $image: $container.find('img.image-before'), + path: this.getAttribute('data-path-before'), + mime: this.getAttribute('data-mime-before'), + $images: $container.find('img.image-before'), // matches 3 $boundsInfo: $container.find('.bounds-info-before') }]; - for (const info of imageInfos) { - if (info.$image.length > 0) { - $.ajax({ - url: info.path, - success: (data, _, jqXHR) => { - info.$image.on('load', () => { - info.loaded = true; - setReadyIfLoaded(); - }).on('error', () => { - info.loaded = true; - setReadyIfLoaded(); - info.$boundsInfo.text('(image error)'); - }); - info.$image.attr('src', info.path); - - if (jqXHR.getResponseHeader('Content-Type') === 'image/svg+xml') { - const bounds = getDefaultSvgBoundsIfUndefined(data, info.path); - if (bounds) { - info.$image.attr('width', bounds.width); - info.$image.attr('height', bounds.height); - hideElem(info.$boundsInfo); - } - } - } - }); - } else { - info.loaded = true; - setReadyIfLoaded(); + await Promise.all(imageInfos.map(async (info) => { + const [success] = await Promise.all(Array.from(info.$images, (img) => { + return loadElem(img, info.path); + })); + // only the first images is associated with $boundsInfo + if (!success) info.$boundsInfo.text('(image error)'); + if (info.mime === 'image/svg+xml') { + const resp = await GET(info.path); + const text = await resp.text(); + const bounds = getDefaultSvgBoundsIfUndefined(text, info.path); + if (bounds) { + info.$images.attr('width', bounds.width); + info.$images.attr('height', bounds.height); + hideElem(info.$boundsInfo); + } } + })); + + const $imagesAfter = imageInfos[0].$images; + const $imagesBefore = imageInfos[1].$images; + + initSideBySide(createContext($imagesAfter[0], $imagesBefore[0])); + if ($imagesAfter.length > 0 && $imagesBefore.length > 0) { + initSwipe(createContext($imagesAfter[1], $imagesBefore[1])); + initOverlay(createContext($imagesAfter[2], $imagesBefore[2])); } - function setReadyIfLoaded() { - if (imageInfos[0].loaded && imageInfos[1].loaded) { - initViews(imageInfos[0].$image, imageInfos[1].$image); - } - } - - function initViews($imageAfter, $imageBefore) { - initSideBySide(createContext($imageAfter[0], $imageBefore[0])); - if ($imageAfter.length > 0 && $imageBefore.length > 0) { - initSwipe(createContext($imageAfter[1], $imageBefore[1])); - initOverlay(createContext($imageAfter[2], $imageBefore[2])); - } - - $container.find('> .image-diff-tabs').removeClass('is-loading'); - } + $container.find('> .image-diff-tabs').removeClass('is-loading'); function initSideBySide(sizes) { let factor = 1; diff --git a/web_src/js/modules/fetch.js b/web_src/js/modules/fetch.js index 9fccf4a81e..b3529d27fc 100644 --- a/web_src/js/modules/fetch.js +++ b/web_src/js/modules/fetch.js @@ -11,9 +11,7 @@ const safeMethods = new Set(['GET', 'HEAD', 'OPTIONS', 'TRACE']); export function request(url, {method = 'GET', headers = {}, data, body, ...other} = {}) { let contentType; if (!body) { - if (data instanceof FormData) { - body = data; - } else if (data instanceof URLSearchParams) { + if (data instanceof FormData || data instanceof URLSearchParams) { body = data; } else if (isObject(data) || Array.isArray(data)) { contentType = 'application/json'; diff --git a/web_src/js/svg.js b/web_src/js/svg.js index 46372e7d62..c2a96fba3f 100644 --- a/web_src/js/svg.js +++ b/web_src/js/svg.js @@ -1,4 +1,5 @@ import {h} from 'vue'; +import {parseDom, serializeXml} from './utils.js'; import giteaDoubleChevronLeft from '../../public/assets/img/svg/gitea-double-chevron-left.svg'; import giteaDoubleChevronRight from '../../public/assets/img/svg/gitea-double-chevron-right.svg'; import giteaEmptyCheckbox from '../../public/assets/img/svg/gitea-empty-checkbox.svg'; @@ -145,22 +146,19 @@ const svgs = { // At the moment, developers must check, pick and fill the names manually, // most of the SVG icons in assets couldn't be used directly. -const parser = new DOMParser(); -const serializer = new XMLSerializer(); - // retrieve an HTML string for given SVG icon name, size and additional classes export function svg(name, size = 16, className = '') { if (!(name in svgs)) throw new Error(`Unknown SVG icon: ${name}`); if (size === 16 && !className) return svgs[name]; - const document = parser.parseFromString(svgs[name], 'image/svg+xml'); + const document = parseDom(svgs[name], 'image/svg+xml'); const svgNode = document.firstChild; if (size !== 16) { svgNode.setAttribute('width', String(size)); svgNode.setAttribute('height', String(size)); } if (className) svgNode.classList.add(...className.split(/\s+/).filter(Boolean)); - return serializer.serializeToString(svgNode); + return serializeXml(svgNode); } export function svgParseOuterInner(name) { @@ -176,7 +174,7 @@ export function svgParseOuterInner(name) { if (p1 === -1 || p2 === -1) throw new Error(`Invalid SVG icon: ${name}`); const svgInnerHtml = svgStr.slice(p1 + 1, p2); const svgOuterHtml = svgStr.slice(0, p1 + 1) + svgStr.slice(p2); - const svgDoc = parser.parseFromString(svgOuterHtml, 'image/svg+xml'); + const svgDoc = parseDom(svgOuterHtml, 'image/svg+xml'); const svgOuter = svgDoc.firstChild; return {svgOuter, svgInnerHtml}; } diff --git a/web_src/js/utils.js b/web_src/js/utils.js index 1b701e1c6a..c82e42d349 100644 --- a/web_src/js/utils.js +++ b/web_src/js/utils.js @@ -128,3 +128,14 @@ export function decodeURLEncodedBase64(base64url) { .replace(/_/g, '/') .replace(/-/g, '+')); } + +const domParser = new DOMParser(); +const xmlSerializer = new XMLSerializer(); + +export function parseDom(text, contentType) { + return domParser.parseFromString(text, contentType); +} + +export function serializeXml(node) { + return xmlSerializer.serializeToString(node); +} diff --git a/web_src/js/utils/dom.js b/web_src/js/utils/dom.js index cdc52b1a74..403933883a 100644 --- a/web_src/js/utils/dom.js +++ b/web_src/js/utils/dom.js @@ -183,3 +183,14 @@ export function autosize(textarea, {viewportMarginBottom = 0} = {}) { export function onInputDebounce(fn) { return debounce(300, fn); } + +// Set the `src` attribute on an element and returns a promise that resolves once the element +// has loaded or errored. Suitable for all elements mention in: +// https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/load_event +export function loadElem(el, src) { + return new Promise((resolve) => { + el.addEventListener('load', () => resolve(true), {once: true}); + el.addEventListener('error', () => resolve(false), {once: true}); + el.src = src; + }); +}