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

6 participants
@lafriks
Copy link
Member

lafriks commented Jul 22, 2017

Fixes #1516

Screenshot:
attels

Image building requires Inkscape, ImageMagick, zopflipng

@lafriks lafriks force-pushed the lafriks:new_icon branch from 0cfea21 to aa30a96 Jul 22, 2017

@lafriks lafriks added the kind/ui label Jul 22, 2017

@lafriks lafriks added this to the 1.2.0 milestone Jul 22, 2017

@lunny

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

silverwind commented Jul 22, 2017

Suggestion: Minify the SVG via svgo.

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

@lafriks

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

silverwind 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

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

This comment has been minimized.

Copy link
Member

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 label Jul 22, 2017

@lafriks

This comment has been minimized.

Copy link
Member

lafriks commented Jul 24, 2017

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

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Jul 25, 2017

OK, maybe a new Logo PR could be sent

@lafriks

This comment has been minimized.

Copy link
Member

lafriks commented Jul 25, 2017

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

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Jul 25, 2017

@lafriks oh, mistake.

@sapk

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

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 and removed lgtm/need 2 labels Jul 25, 2017

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Jul 26, 2017

LGTM

@lafriks

This comment has been minimized.

Copy link
Member

lafriks commented Jul 26, 2017

Make LG-TM work

@bkcsoft bkcsoft added lgtm/done and removed lgtm/need 1 labels Jul 26, 2017

@lunny

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

lafriks commented Jul 26, 2017

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

@lunny

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

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 referenced this pull request Jul 27, 2017

Closed

Voting for new logo #1516

@philfry

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

lunny commented Jul 27, 2017

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

@lafriks

This comment has been minimized.

Copy link
Member

lafriks commented Jul 27, 2017

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

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Jul 27, 2017

@lafriks I will try to fix that.

@lunny

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

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

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details
@philfry

This comment has been minimized.

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 lafriks:new_icon branch Jul 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment