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

Adds a level option to gifsicle #51

Merged
merged 16 commits into from
Oct 28, 2014
Merged

Adds a level option to gifsicle #51

merged 16 commits into from
Oct 28, 2014

Conversation

kaspergrubbe
Copy link
Contributor

Oi!

I had an issue with corruption on some gifs with gifsicle. The corruption (not relevant for image_optim) happened after a resize like:

convert funny_image.gif -resize 100x100 funny_image_resized.gif

I've also added the --careful flag from gifsicle so we produce more compatible gifs, but that might have been a mistake, and please notify if you want me to remove it.

Gifsicle actually obeys the flag -O0 so I considered using that instead of removing the flag, but since it isn't documented by Gifsicle I opted for the solution without -O for when level is zero.

@kaspergrubbe
Copy link
Contributor Author

I've decided if level == 0 it should send the --unoptimize flag.

@toy
Copy link
Owner

toy commented Aug 2, 2014

Great! But few notes: as image_optim is for optimizing images, I don't think that level 0 or --unoptimize make sence unless you can think of a good use case. Also --careful flag is for "Some Java and Internet Explorer versions" so can be added as another worker option but should be off by default.

@toy
Copy link
Owner

toy commented Aug 2, 2014

Also I would prefer if you can create a feature branch (unless it will require another pull request, I will add notes about contribution to readme) and make travis happy ;)

@kaspergrubbe
Copy link
Contributor Author

I will make a featurebranch and work on the specs. I was away for the weekend, but I am planning on looking at this during the week, thanks for your feedback, I will get back to you.

@kaspergrubbe
Copy link
Contributor Author

Hey @toy

I have cleaned the code up, and merged master in.

I don't think I can make it a featurebranch without making a new PR. Do you know anything about these spec failures?

  1) ImageOptim optimize_image optimizes images
     Failure/Error: expect(nrmse(compare_to, optimized)).to eq(0)

       expected: 0
            got: 0.155859

       (compared using ==)
     # ./spec/image_optim_spec.rb:125:in `block (4 levels) in <top (required)>'
     # ./lib/image_optim.rb:205:in `call'
     # ./lib/image_optim.rb:205:in `block in run_method_for'

args.unshift('--interlace') if interlace
args.unshift('--careful') if careful
args.unshift('--optimize=#{level}') if level
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No interpolation happens.
gifsicle shows only warning about getting "#{level}", so I've created an issue (kohler/gifsicle#29) as this should abort execution.

@toy
Copy link
Owner

toy commented Oct 27, 2014

@kaspergrubbe Sorry for delay with reply.

That failure means at least that I need to make failure for that spec more descriptive :)
I can't reproduce failure on local - not good, I'll try further.

I've added few notes, also please run script/update_worker_options_in_readme.

@kaspergrubbe
Copy link
Contributor Author

Hey @toy, no worries for the delay, we are all busy :-)

Specs passing, and your notes are fixed. I've played around with the descriptions and decided than my initial ones were too long, let me know what you think.

@toy toy merged commit 49e1a85 into toy:master Oct 28, 2014
@toy
Copy link
Owner

toy commented Oct 28, 2014

Nice!
So it seems that spec failure was caused by uninterpolated --optimized argument and different behaviour of older gifsicle (1.64 vs 1.84).

@kaspergrubbe
Copy link
Contributor Author

So the specs worked, good! :P

Glad to have this merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants