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 #width and #height methods to the RMagick processor #1805

Merged

Conversation

mehlah
Copy link
Member

@mehlah mehlah commented Dec 29, 2015

This keeps the symmetry with MiniMagick processor where #width and
#height methods has been introduced by #1405.

@@ -351,5 +373,9 @@ def destroy_image(image)
image.destroy! if image.respond_to?(:destroy!)
end

def rmagick_image
::Magick::Image.ping(current_path).first
Copy link
Member Author

Choose a reason for hiding this comment

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

I used ping instead of read as we are interested in the image metadata, but don't care about the pixel data.
Ref http://www.rubydoc.info/gems/rmagick/2.15.4/Magick/Image#ping-class_method

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little concerned about this. Calling ping will return an image with invalid pixel data, and naming this rmagick_image seems to be asking for trouble.

Either we need to read the entire image, or we should rename the method.

@thomasfedb
Copy link
Contributor

I'm happy with this change in concept, but I've noted one concern above.

Mehdi Lahmam added 2 commits December 30, 2015 16:16
This keeps the symmetry with MiniMagick processor where `#width` and
`#height` methods has been introduced by carrierwaveuploader#1405.
This was introduced by a bad merge of carrierwaveuploader#1804 and carrierwaveuploader#1806 which both were
based on master and  added the `private` keyword for two different
methods.
@mehlah
Copy link
Member Author

mehlah commented Dec 30, 2015

@thomasfedb I agree 👍. I fixed it by using read instead of ping to keep it consistent with MiniMagick code.
I also added an additional commit to remove an unnecessary private keyword introduced after recent merges (see the commit message for details).
Thanks!

thomasfedb added a commit that referenced this pull request Dec 30, 2015
Add `#width` and `#height` methods to the RMagick processor
@thomasfedb thomasfedb merged commit 4f6b5a8 into carrierwaveuploader:master Dec 30, 2015
@thomasfedb
Copy link
Contributor

Thanks @mehlah!

@mehlah mehlah deleted the rmagick_width_and_height branch December 31, 2015 11:26
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.

None yet

2 participants