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.

Sign up for GitHub

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

Jump to bottom

Fix an issue not working OptiPNG interlace option #136

Merged
merged 1 commit into from
Aug 7, 2016
Merged

Fix an issue not working OptiPNG interlace option #136

merged 1 commit into from
Aug 7, 2016

Conversation

mrk21
Copy link
Contributor

@mrk21 mrk21 commented Jul 20, 2016 •
edited
Loading

I have fixed an issue not working OptiPNG worker when the interlace option enabled.

The reason is that the file size of an interlaced PNG is greater than the file size of a non interlaced PNG generally, so I have changed so that optimized? conditions for OptiPNG worker is always true if the interlace option is enabled.

toy
toy reviewed Jul 24, 2016
View reviewed changes
lib/image_optim/worker/optipng.rb
@@ -42,6 +42,10 @@ def optimize(src, dst)
end
execute(:optipng, *args) && optimized?(src, dst)
end

def optimized?(src, dst)
(interlace && dst.size?) || super
Copy link
Owner

Choose a reason for hiding this comment

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

If interlace is true, running super is not needed, so interlace ? dst.size? : super would be clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All right. And I've fixed it.

@toy
Copy link
Owner

toy commented Jul 24, 2016

Thanks for contribution.
Please add an entry to CHANGELOG.markdown.

mrk21 added a commit to mrk21/image_optim that referenced this pull request Jul 26, 2016
@mrk21
changelog entry: fix an issue not working OptiPNG interlace option, r…
2ae9ef5 
…esolves toy#136
@mrk21
Copy link
Contributor Author

mrk21 commented Jul 26, 2016

I've added the content of this PR to CHANGELOG. Then, so I run the following steps?

$ git rebase master
$ git rebase -i ####
$ git push -f

@toy
Copy link
Owner

toy commented Jul 26, 2016

What do you mean by ####?
You can run git rebase -i master, combine commits and run git push -f.

@mrk21 mrk21 force-pushed the fix-issue-not-working-optipng-interlace-option branch from 2ae9ef5 to 73d3f6d Compare July 27, 2016 02:58
mrk21 added a commit to mrk21/image_optim that referenced this pull request Jul 27, 2016
@mrk21
fix issue not working optipng interlace option, resolves toy#136
73d3f6d
@mrk21
Copy link
Contributor Author

mrk21 commented Jul 27, 2016 •
edited
Loading

#### is the latest commit hash before this PR.

I didn't know a method to combine commits of a PR by git rebase -i master command, so I was instead using git rebase master and git rebase -i <latest commit hash before the PR> commands until now.

@mrk21
Copy link
Contributor Author

mrk21 commented Jul 27, 2016

Oh, CI failed... Is there anything else that I need to work?

@toy
Copy link
Owner

toy commented Jul 27, 2016

I'm investigating why appveyor fails and travis is missing from the checks

@toy
Copy link
Owner

toy commented Jul 27, 2016

I've fixed appveyor and travis should be working

@mrk21 mrk21 force-pushed the fix-issue-not-working-optipng-interlace-option branch from 73d3f6d to d37e43a Compare July 28, 2016 02:55
mrk21 added a commit to mrk21/image_optim that referenced this pull request Jul 28, 2016
@mrk21
fix issue not working optipng interlace option, resolves toy#136
d37e43a
@mrk21 mrk21 force-pushed the fix-issue-not-working-optipng-interlace-option branch from d37e43a to 1d3aa63 Compare August 1, 2016 04:50
@mrk21
Copy link
Contributor Author

mrk21 commented Aug 1, 2016

@toy Thanks! I rebased this branch by master, and then CI was succeeded.

@toy toy merged commit 1d3aa63 into toy:master Aug 7, 2016
@mrk21
Copy link
Contributor Author

mrk21 commented Aug 7, 2016

Thanks😇

@toy
Copy link
Owner

toy commented Aug 7, 2016

Thanks you! I'll release after few more changes unless you want it as soon as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mrk21 @toy