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

Add task to generate images from SVG and change to new logo #2194

Merged
merged 2 commits into from
Jul 28, 2017

Conversation

lafriks
Copy link
Member

@lafriks lafriks commented Jul 22, 2017

Fixes #1516

Screenshot:
attels

Image building requires Inkscape, ImageMagick, zopflipng

@lafriks lafriks added the topic/ui Change the appearance of the Gitea UI label Jul 22, 2017
@lafriks lafriks added this to the 1.2.0 milestone Jul 22, 2017
@lunny
Copy link
Member

lunny commented Jul 22, 2017

I think the 32px icon need an improvement. @lafriks maybe a white branch routes but not a white square.

@silverwind
Copy link
Member

Suggestion: Minify the SVG via svgo.

Maybe build dependencies like this and your three tools should be documented somewhere?

@lafriks
Copy link
Member Author

lafriks commented Jul 22, 2017

Not much point in minimizing svg as it wont be served to client and is used only to generate rasterized images. As for dependencies in makefile I can add some checks but this command will be used only when changing logo so probably not very often :)

@silverwind
Copy link
Member

Not much point in minimizing svg as it wont be served to client and is used only to generate rasterized images

Okay, makes sense to keep the full file around in that case. We should serve SVG to client at some point :)

As for dependencies in makefile I can add some checks but this command will be used only when changing logo so probably not very often :)

No need to add checks imho, should just be documented. I filed #2198 for it.

@lafriks
Copy link
Member Author

lafriks commented Jul 22, 2017

@silverwind when svg file will be needed to serve to client it will need to be copied to public folder and there it will need to be optimized

@lunny - options for small logo #1516 (comment)

@lafriks lafriks added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 22, 2017
@lafriks
Copy link
Member Author

lafriks commented Jul 24, 2017

@lunny i think small logo that is in this PR has won the vote

@lunny
Copy link
Member

lunny commented Jul 25, 2017

OK, maybe a new Logo PR could be sent

@lafriks
Copy link
Member Author

lafriks commented Jul 25, 2017

@lunny why new PR? In this one everything is as needed

@lunny
Copy link
Member

lunny commented Jul 25, 2017

@lafriks oh, mistake.

@sapk
Copy link
Member

sapk commented Jul 25, 2017

I think that @whilei logo is better even if it come after selection. But we could go with that first and this PR lay down some struct to generate different sizes.
I don't really like depending on inkscape and maybe use convert in place.

Even if I would make some littles changes, LGTM for going forward.

@lafriks
Copy link
Member Author

lafriks commented Jul 25, 2017

@sapk inkscape has best rendering quality&feature wise. Also this is anyway needed only when changing svg files so no need to regenerate images often.
As for @whilei logo it is no problem that it did come after vote started but it did received less votes

Maybe we can start new vote again than?

@sapk
Copy link
Member

sapk commented Jul 25, 2017

I am ok for merging this PR with this logo. Migrating to the other will only need updating logo.svg (in another PR).

@lafriks lafriks added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 25, 2017
@lunny
Copy link
Member

lunny commented Jul 26, 2017

LGTM

@lafriks
Copy link
Member Author

lafriks commented Jul 26, 2017

Make LG-TM work

@bkcsoft bkcsoft 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, 2017
@lunny
Copy link
Member

lunny commented Jul 26, 2017

➜  gitea git:(lafriks-new_icon) make generate-images
mkdir -p /var/folders/00/n3p889nx3sl82rjbh1kftjd80000gn/T/tmp.sEocOQ7j/images
inkscape -f assets/logo.svg -w 400 -h 400 -e public/img/gitea-lg.png

** (inkscape-bin:6435): WARNING **: Can't open file: assets/logo.svg (doesn't exist)

** (inkscape-bin:6435): WARNING **: Can't open file: assets/logo.svg (doesn't exist)

** (inkscape-bin:6435): WARNING **: Specified document assets/logo.svg cannot be opened (does not exist or not a valid SVG file)
make: *** [generate-images] Error 1

@lafriks
Copy link
Member Author

lafriks commented Jul 26, 2017

@lunny can you check if you have file assets/logo.png?

@lunny
Copy link
Member

lunny commented Jul 26, 2017

➜  gitea git:(lafriks-new_icon) cd assets
➜  assets git:(lafriks-new_icon) ls
logo.svg
➜  assets git:(lafriks-new_icon)

There is not.

@lafriks
Copy link
Member Author

lafriks commented Jul 26, 2017

@lunny I meant .svg not .png... If file is there than I don't understand why it fails to open it :(

@lunny lunny mentioned this pull request Jul 27, 2017
@philfry
Copy link
Contributor

philfry commented Jul 27, 2017

@lunny just curious, what distribution and which inkscape version do you use? 'cause make generate-images works fine on Fedora 25/inkscape 0.92.1-4.20170510bzr15686.
You may want to check whether or not inkscape really cannot see that file:

strace -qq -ff -e trace=file -P assets/logo.svg -P $(which inkscape) make generate-images 2>&1 > /dev/null | grep logo

@lunny
Copy link
Member

lunny commented Jul 27, 2017

@philfry My OS is macOS not linux. I installed inkscape via brew cask.

@lafriks
Copy link
Member Author

lafriks commented Jul 27, 2017

@lunny sorry I have no idea how to fix that for macos as I don't have it :)

@lunny
Copy link
Member

lunny commented Jul 27, 2017

@lafriks I will try to fix that.

@lunny
Copy link
Member

lunny commented Jul 28, 2017

@lafriks I pushed 1 commit to use absolute path then I can run successfully in macOS. Of course you have to install inkscape, imagemagick and zopfli before that.

@lafriks
Copy link
Member Author

lafriks commented Jul 28, 2017

Strange that on macos relative paths does not work :) Thanks

@lunny lunny merged commit 60d7e56 into go-gitea:master Jul 28, 2017
@philfry
Copy link
Contributor

philfry commented Jul 28, 2017

uh... please do not overwrite $PWD. It's an internal variable and overwriting it will break things.
Either remove PWD := $(shell pwd) or use another variable name (which would be redundant, because ${PWD} already contains what you want).

[phil· ~/go/src/code.gitea.io/gitea] [master✓]$ TAGS="bindata sqlite" make generate build
go generate code.gitea.io/gitea code.gitea.io/gitea/cmd code.gitea.io/gitea/models code.gitea.io/gitea/models/migrations code.gitea.io/gitea/modules/auth code.gitea.io/gitea/modules/auth/ldap code.gitea.io/gitea/modules/auth/oauth2 code.gitea.io/gitea/modules/auth/openid code.gitea.io/gitea/modules/auth/pam code.gitea.io/gitea/modules/avatar code.gitea.io/gitea/modules/base code.gitea.io/gitea/modules/context code.gitea.io/gitea/modules/cron code.gitea.io/gitea/modules/highlight code.gitea.io/gitea/modules/httplib code.gitea.io/gitea/modules/indexer code.gitea.io/gitea/modules/lfs code.gitea.io/gitea/modules/log code.gitea.io/gitea/modules/mailer code.gitea.io/gitea/modules/markdown code.gitea.io/gitea/modules/markup code.gitea.io/gitea/modules/minwinsvc code.gitea.io/gitea/modules/notification code.gitea.io/gitea/modules/options code.gitea.io/gitea/modules/private code.gitea.io/gitea/modules/process code.gitea.io/gitea/modules/public code.gitea.io/gitea/modules/setting code.gitea.io/gitea/modules/ssh code.gitea.io/gitea/modules/sync code.gitea.io/gitea/modules/templates code.gitea.io/gitea/modules/user code.gitea.io/gitea/modules/util code.gitea.io/gitea/modules/validation code.gitea.io/gitea/routers code.gitea.io/gitea/routers/admin code.gitea.io/gitea/routers/api/v1 code.gitea.io/gitea/routers/api/v1/admin code.gitea.io/gitea/routers/api/v1/convert code.gitea.io/gitea/routers/api/v1/misc code.gitea.io/gitea/routers/api/v1/org code.gitea.io/gitea/routers/api/v1/repo code.gitea.io/gitea/routers/api/v1/user code.gitea.io/gitea/routers/api/v1/utils code.gitea.io/gitea/routers/dev code.gitea.io/gitea/routers/org code.gitea.io/gitea/routers/private code.gitea.io/gitea/routers/repo code.gitea.io/gitea/routers/routes code.gitea.io/gitea/routers/user
bindata.go
bindata.go
bindata.go
go build -i -v  -tags 'bindata sqlite' -ldflags '-s -w -X "main.Version=1.1.0+451-g60d7e56c6" -X "main.Tags=bindata sqlite"' -o gitea
main.go:16:2: cannot find package "github.com/urfave/cli" in any of:
        /usr/local/go/src/github.com/urfave/cli (from $GOROOT)
        /home/phil/go/src/github.com/urfave/cli (from $GOPATH)
Makefile:205: recipe for target 'gitea' failed
make: *** [gitea] Error 1
Exit 2

@lafriks lafriks deleted the new_icon branch July 28, 2017 10:29
@lunny lunny added the type/enhancement An improvement of existing functionality label Aug 25, 2017
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 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. topic/ui Change the appearance of the Gitea UI type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Voting for new logo
6 participants