Paperclip functionality inconsistent /w ImageMagick options
Reported by Clemens Kofler | July 10th, 2008 @ 08:03 AM
Hi folks,
I've just tried to use Paperclip to resize an image to width = 120px - no matter if it's smaller or larger than that. According to the ImageMagick docs on geometry definition (http://www.imagemagick.net/scrip...) it should be possible to do this using the following specifications:
- "120" (width only, up and down scaling)
- "x120" (height only)
However, Paperclip sets the height to the value of width if no width is given and vice versa - which is kinda wicked, considering it's shown differently in the IM docs.
I'm not sure if I've spotted every place where little code changes are necessary but I think it's mostly in the geometry.rb:
- initialize needs to be changed so that it keeps blank values blank.
- parse needs a slightly modified Regexp. /\b(\d*)x(\d*)\b([\>\<\#\@\%^!])?/ should read /\b(\d*)x?(\d*)\b([\>\<\#\@\%^!])?/.
- to_s needs modification so that it handles blank values correctly - they should be left blank instead of 0 and the modifier can only be applied if both parameters are present (as far as I understand the IM docs).
- transformation_to should be refactored as I posted in this ticket: http://thoughtbot.lighthouseapp....
Since I'm note entirely sure whether you actually want this functionality to change, I haven't created a patch yet. If you want me to supply a patch, just tell me and I'm happy to do it.
- Clemens
Comments and changes to this ticket
-
Jon Yurek August 13th, 2008 @ 05:52 PM
- → State changed from “new” to “open”
Actually, yeah, if you could supply a patch that would be good. I thought I would have gotten to this ticket by now.
-
Clemens Kofler August 18th, 2008 @ 12:27 PM
Can I break existing tests? I'm asking because most of the geometry tests assume that if I leave either width or height blank, it should equal the other, which is, of course, bound to fail if I want to add the new options mentioned in the ticket.
-
Jon Yurek August 21st, 2008 @ 10:42 AM
I don't want to accept a patch that will break tests. If you're changing things around, I would appreciate it if the tests cover the changes and everything passes correctly.
-
Clemens Kofler August 21st, 2008 @ 10:50 AM
I'm afraid this can't be done. As I said, most of the current geometry tests (incorrectly) parse ImageMagick formatting strings that I'd need to change in order to make them valid formatting strings. So I guess, this means that either it has to stay that way or break backwards compatibility.
-
Jon Yurek August 21st, 2008 @ 11:18 AM
All I mean is that if you're going to refactor the code, I want the tests to be changed as well. I don't really care if your changes break backwards compatibility since I consider this a bug in the Geo class, I just want to make sure any code you change or add is well tested. Sorry if that wasn't clear.
-
Clemens Kofler August 21st, 2008 @ 11:25 AM
Okay, that's what I had in mind anyway. I wanted to go over the existing geometry tests and modify those that aren't valid (in IM terms) plus add new ones if needed.
-
-
Clemens Kofler August 28th, 2008 @ 01:57 PM
- no changes were found...
-
Clemens Kofler August 28th, 2008 @ 01:57 PM
- → Tag changed from “bug feature geometry imagemagick inconsistence” to “bug feature geometry imagemagick inconsistence patch”
Here's the patch. I've just tested it in a real project and it seems to work without flaws.
Feedback appreciated!
-
-
-
Jon Yurek October 14th, 2008 @ 12:02 PM
- → State changed from “open” to “resolved”
- → Tag changed from “bug feature geometry imagemagick inconsistence patch” to “bug bug feature feature geometry imagemagick inconsistence patch”
I added your changes. Thanks, and sorry about taking so long getting them added.
Please Login or create a free account to add a new comment.
You can update this ticket by sending an email to from your email client. (help)
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
