Keep (add if not existing) xmlns attribute for generated SVG images

[wxiaoguang]

Fix #23409

Developers could browse & preview the local SVG images files directly.

It still has clear output.

[silverwind]

How about adding xmlns only when RUN_MODE=dev, at runtime only (no modification to SVG files)? That way, we avoid the perf impact in prod mode.

Generally I see this change as borderline useless, we never render SVG in standalone documents.

[wxiaoguang]

How about adding xmlns only when RUN_MODE=dev, at runtime only (no modification to SVG files)? That way, we avoid the perf impact in prod mode.

Generally I see this change borderline useless thought, we never render SVG in standalone documents.

The purpose is to make developers could browse local SVG images from the assets directory.

[wxiaoguang]

How about adding xmlns only when RUN_MODE=dev, at runtime only (no modification to SVG files)? That way, we avoid the perf impact in prod mode.

@silverwind @KN4CK3R I don't understand what does it mean, and I do not think it's related to the problem.

The purpose of this PR is to make the SVG images in $HOME/work/gitea/public/img/svg could be viewed locally & directly.

At the moment, when I am working with SVG images in Gitea, I even can't know what a image looks like, it's difficult to manage/confirm these SVG files, and it's difficult to pick/choose a SVG image.

You can open your local $HOME/work/gitea/public/img/svg directory to check.

[wxiaoguang]

@silverwind @KN4CK3R how do you think? This PR:

• makes the SVG images in $HOME/work/gitea/public/img/svg could be viewed locally & directly.
• doesn't affect HTML output, the generated HTML code is still clear (the same as before).

[KN4CK3R]

My image viewer is able to display the files without that change. But as this work is done only once in an Init method I think it is not harmful. First I thought you want to remove the string on the fly when serving the file but that's not the case.

[silverwind]

I guess it does not hurt if it only affects the files when viewed directly in the browser, which BTW is just a side-effect, we do not use these file URLs anywhere. Their existance at that location it currently only so that the go template code can access them via bindata embed.

Thought I guess if we ever decide to make use of these files via URL, it is the right change to do because standalone documents indeed need the xmlns.

[delvh]

LGTM.
However, I'm not sure if we should merge #23528 or this PR first, there will always be merge conflicts.
I'd rather say the other one as the chance that we forget to add the namespace for the new icons is lower then.

[silverwind]

Would merge the other one first and then run make svg here.

[silverwind]

This change does not actually add xmlns for sources where it is missing, it just does not remove it any more. I would try using addAttributesToSVGElement to add it, just in case it is missing.

[silverwind]

Blocking as per above. The code does not do what the PR title says.

[wxiaoguang]

Blocking as per above. The code does not do what the PR title says.

Done in 9412e17

[wxiaoguang]

However, I'm not sure if we should merge #23528 or this PR first, there will always be merge conflicts.
I'd rather say the other one as the chance that we forget to add the namespace for the new icons is lower then.

No worry, I am glad and happy to resolve necessary conflicts.

[delvh]

Please run make svg

[codecov-commenter]

Codecov Report

Merging #23410 (eca010b) into main (f521e88) will decrease coverage by 0.01%.
The diff coverage is 32.37%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main   #23410      +/-   ##
==========================================
- Coverage   47.14%   47.13%   -0.01%     
==========================================
  Files        1149     1154       +5     
  Lines      151446   152270     +824     
==========================================
+ Hits        71397    71780     +383     
- Misses      71611    72016     +405     
- Partials     8438     8474      +36     

Impacted Files	Coverage Δ
cmd/dump.go	0.67% <0.00%> (ø)
cmd/web.go	0.00% <0.00%> (ø)
models/actions/run.go	1.64% <0.00%> (-0.08%)	⬇️
models/packages/package.go	45.45% <0.00%> (-1.13%)	⬇️
models/user/search.go	77.50% <0.00%> (-6.29%)	⬇️
modules/actions/github.go	0.00% <0.00%> (ø)
modules/actions/workflows.go	0.00% <0.00%> (ø)
modules/doctor/storage.go	31.93% <0.00%> (ø)
modules/setting/git.go	45.45% <ø> (ø)
modules/storage/minio.go	1.51% <0.00%> (-0.06%)	⬇️
... and 24 more

... and 31 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.