Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework 'make generate-images' #12316

Merged
merged 7 commits into from
Jul 26, 2020

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Jul 24, 2020

  • Remove external dependencies and replace it with a node script that does does the same.
  • Move detail removal from gitea-sm.png to favicon.png
  • Remove favicon.ico and its generation, it is unused and we already serve favicon.png in its place.

Fixes: #12314

@silverwind silverwind force-pushed the simplify-generate-images branch 2 times, most recently from 071ccef to bd89078 Compare July 24, 2020 14:52
@techknowlogick techknowlogick added this to the 1.13.0 milestone Jul 24, 2020
@techknowlogick techknowlogick added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Jul 24, 2020
@stertingen
Copy link

Thanks for this PR as this will fix my initial problem. (#12314)

But isn't it a bit overkill to download chromium just to build a bunch of png images? Why not use the original SVG image instead of the PNG images?

logo.svg: 5.6K (base image size)
gitea-lg.png: 33K
gitea-512.png: 17K
gitea-192.png: 4.2K
gitea-sm.png: 3.2K
favicon.png: 4.1K
apple-touch-icon.png: 4.1K

In some cases, the resulting PNG files are much bigger than the original SVG file, even after compression. I see that favicon and apple-touch-icon have to be a rasterized, but using the SVG for the larger variants might reduce the amount and size of transmitted images.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 24, 2020
@silverwind
Copy link
Member Author

silverwind commented Jul 24, 2020

But isn't it a bit overkill to download chromium just to build a bunch of png images

Probably is, but svgexport was just the first cross-platform tool I've found that was up to the job that can be easily installed with only our existing dependencies on Node/Go and which does not require messing with PATH (like Inkscape would).

Probably possible to use https://github.com/lovell/sharp but it'll be more involved.

Why not use the original SVG image instead of the PNG images?

I agree we should ideally only use the SVG image in the UI but it'd be quite a bit of work and I think some parts of the code can only work with bitmap images (avatar) and then there's the issue that not every image can be represented as an SVG (think photos).

@silverwind silverwind force-pushed the simplify-generate-images branch 2 times, most recently from 4be6fe9 to aef8d48 Compare July 24, 2020 21:57
@silverwind
Copy link
Member Author

Updated to use a node script and two npm modules in place of previous Chromium-based solution. It uses JSDOM's canvas renderer under the hood whose SVG implementation is probably not as complete as Chrome's but seems to work fine for our logo.

Those modules are now only installed on demand, otherwise we would gain 215 new transitive devDependencies which I think are better avoided for such a rarely used task.

Reviewers should check the diff on public/img/gitea-sm.png which gained some detail. I think the change is fine to keep.

@stertingen
Copy link

I like this better, thanks!

@lafriks
Copy link
Member

lafriks commented Jul 25, 2020

You could just use domxml to remove layer in svg before generating gitea-sm

@silverwind
Copy link
Member Author

silverwind commented Jul 25, 2020

xmldom you mean? Yeah, certainly possible and probably better than the string modification.

I'm still contemplating whether to add the dependencies to devDependencies. Probably better in the long run.

@silverwind
Copy link
Member Author

I think I will actually do the layer removal but only for the favicon because that is rendered the smallest (16px):

image

gitea-sm is used on the page header at 30px and I think it would benefit from the detail, especially on hidpi screens:

image

Regarding devDependencies: I will keep it the current way because zopflipng requires a native dependency and that's a no-go for our bundled node_modules in the src tarballs.

@lafriks
Copy link
Member

lafriks commented Jul 25, 2020

Yes, xmldom :) It's already dependency for svg2img so that would not add much to node_modules size

@silverwind
Copy link
Member Author

silverwind commented Jul 25, 2020

Done. Detail is now removed from favicon.png instead of gitea-sm.png previously. Also squashed and update description.

@silverwind silverwind changed the title Simplify 'make generate-images' Rework 'make generate-images' Jul 25, 2020
- Remove external dependencies and replace it with a node script that
  does does the same.
- Move detail removal from gitea-sm.png to favicon.png
- Remove favicon.ico and its generation, it is unused and we already serve
  favicon.png in its place.

Fixes: go-gitea#12314
@silverwind silverwind force-pushed the simplify-generate-images branch from 0448d59 to 9e1edbf Compare July 25, 2020 14:27
@silverwind
Copy link
Member Author

silverwind commented Jul 25, 2020

Updated to use fabric to render into the canvas which allows us to skip a few wonky dependencies.

@GiteaBot GiteaBot removed the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 25, 2020
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jul 25, 2020
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 26, 2020
@lunny lunny merged commit 7cf2339 into go-gitea:master Jul 26, 2020
@silverwind silverwind deleted the simplify-generate-images branch July 26, 2020 10:00
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make generate-images fails with newer inkscape versions
6 participants