Call strip on thumbnails to reduce their size #2261
Conversation
| end | ||
| thumbnail.strip |
There was a problem hiding this comment.
In terms of factoring the code have you considered the following:
thumbnail = image
thumbnail = thumbnail.thumb(geometry) if geometry.present? && !geometry.is_a?(Symbol)
thumbnail.stripI'm not advocating for it being better but it feels like it might be.
There was a problem hiding this comment.
One thing I thought is make the the strip a configuration option passed to the thumbnail method.. and then image_fu can be configurable too. I'd consider changing the API to:
def thumbnail(options = {})
options = {:geometry => options} if options.is_a?(String) # AND deprecation warning here.
options = {:geometry => nil, :strip => true}.merge(options)
# ...
endThere was a problem hiding this comment.
I actually tried that exact code but decided the middle line was too long. I'm not sure if I like that style in general or not, but in this case I'm happy with whichever people want.
Probably a better option would be to move some of it out into different methods and have the body of this one be something like
resize_image(parse_geometry(geometry)).stripThere was a problem hiding this comment.
That refers to the first comment only.
|
Are you still happy with this solution? |
|
I think I managed to improve it quite a bit. Thanks for the feedback :) |
| def thumbnail(options = {}) | ||
| if options.is_a?(String) || options.is_a?(Symbol) | ||
| Refinery.deprecate 'Refinery::Image#thumbnail(geometry)', | ||
| :replacement => 'Refinery::Image#url(:geometry => value)' |
There was a problem hiding this comment.
Could you add :when here and set it to 2.2? Thanks!
There was a problem hiding this comment.
Also shouldn't Refinery::Image#url be Refinery::Image#thumbnail instead?
|
Looks good and with a changelog entry it would be even more awesome :) |
|
Merged. Thanks! |
GRR >:| |
|
I take it you don't like it. Want to make it configurable or something? |
|
@parndt I spent a couple of hours debugging my way to this issue, trying to figure out why the fridging Exif data was being stripped from all the JPEGs being resized with Fortunately it's already configurable (see 16c013b), so adding |
|
@parndt I do wonder whether |
|
Probably we shouldn't have unexpected behaviour by default. What do you think, @gwagener & @ugisozols ? |
|
I think when you call ImageMagick thumbnail on the command line it strips the metadata, but in dragonfly it doesn't so these changes make Refinery behave like the command line. Someone will have to check that, but I think that's what I figured out when I was investigating why I didn't get the same results for thumbnails in Refinery as I did on the command line. |
|
Looking at previous comments it's possible I was using an older version if ImageMagick when I did this stuff. |
|
@gwagener The newer versions of ImageMagick don't strip metadata unless you specify
|
If that's the case then we need to set strip option to @benjaminbrent keen to send a PR? |
@ugisozols I'll do that. :) |
This can cut the size of 300x200 thumbnails in half. I'm curious to get feedback on whether people think this is a good idea as
thumbnailgets used byimage_fuand people might use that with much larger geometries. I'm not sure if there would be cases where people would notice the lack of color profiles and whatever else strip cuts out.