Fixes defining thumbnail sizes through ActiveStorage adapter #4318

waiting-for-dev · 2022-03-28T04:47:28Z

Description

The adapter intends to make it possible to work with ActiveStorage as it was Paperclip. We had been using Paperclip until Rails 6.1 introduced ActiveStorage's public links. We moved to ActiveStorage as the default file attachment backend from that point on, but we tried to make its API compatible with Paperclip's one. See #3501 for details.

When trying to translate Paperclip's style definitions (based on ImageMagick) to what ActiveStorage expects (which is image_processing syntax, common for ImageMagick and libvips), we were transforming everything to a resize_to_limit
option. However, that corresponds to the > symbol in ImageMagick, but not to other options that can be given.

Whit this commit, we add support for defining other kinds of transformations in ActiveStorage. However, we're still missing more
advanced options that can be given when using standalone Paperclip (example { geometry: '100x100', format: :jpg }) or standalone ActiveStorage (example rotate: 90). In the long run, we should aim to completely
differentiate how definitions are given for each library.

Needs to be backported.

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
I have updated Guides and README accordingly to this change (if needed)
I have added tests to cover this change (if needed)
I have attached screenshots to this PR for visual changes (if needed)

tvdeyen

makes sense

core/app/models/concerns/spree/active_storage_adapter/attachment.rb

spaghetticode · 2022-03-30T15:22:37Z

core/app/models/concerns/spree/active_storage_adapter/attachment.rb

+      attr_reader :decorated_attachment
+
+      def initialize(decorated_attachment, styles: {})
+        @decorated_attachment = decorated_attachment


Not sure how likely it is that this file gets decorated by stores, but should we deprecate the old instance variable name? Or maybe simply keep the old name?

Fair enough. I'm keeping the old name now.

spaghetticode

Thank you Marc 🎉

tvdeyen

Currently working on porting Alchemy from Dragonfly to active storage and I think this needs to be changed.

tvdeyen · 2022-04-04T20:05:46Z

core/app/models/concerns/spree/active_storage_adapter/attachment.rb

+        case definition[-1].to_sym
+        when :>
+          { resize_to_limit: width_height }
+        when :^


This actually has to be

Suggested change

when :^

when :#

for resize_to_fill, because it crops the image to fill the space. See https://github.com/janko/image_processing/wiki/Resize-To-Fill

Actually, image_processing transforms resize_to_fill into ^:

https://github.com/janko/image_processing/blob/12e7cf50b512d0f4c0735138013c5ea6d87fdee5/lib/image_processing/mini_magick.rb#L74

See also docs in imagemagick.

Do I get it right?

Yes. I see, this seems to be a Dragonfly thing. Really confusing actually, because Dragonfly just forwards the string to convert -thumbnail https://markevans.github.io/dragonfly/imagemagick#thumb

tvdeyen · 2022-04-04T20:09:55Z

core/app/models/concerns/spree/active_storage_adapter/attachment.rb

+        when :^
+          { resize_to_fill: width_height }
+        else
+          { resize_to_fit: width_height }


This is the case for :^, BUT this upscales the image, if the original dimensions are smaller than the requested dimensions. Not sure if we want that as default? We probably want resize_to_limit as default.

We probably want resize_to_limit as default.

Thanks, I think you're right, plus it won't change the previous behavior. Fixed.

The adapter intends to make it possible to work with ActiveStorage as it was Paperclip. We had been using Paperclip until Rails 6.1 introduced ActiveStorage's public links. We moved to ActiveStorage as the default file attachment backend from that point on, but we tried to make its API compatible with Paperclip's one. See #3501 for details. When trying to translate [Paperclip's style definitions](https://github.com/solidusio/solidus/blob/564b4fe54648537b6b1420eed5a05cba6854e915/core/lib/spree/app_configuration.rb#L494) (based on [ImageMagick](https://legacy.imagemagick.org/Usage/thumbnails/)) to what ActiveStorage expects (which is [image_processing syntax](https://github.com/janko/image_processing), common for ImageMagick and libvips), we were transforming everything to a [`resize_to_limit`](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#resize_to_limit) option. However, that corresponds to the `>` symbol in ImageMagick, but not to other options that can be given. Whit this commit, we add support for defining other kinds of transformations in ActiveStorage. However, we're still missing more advanced options that can be given when using [standalone Paperclip](https://www.rubydoc.info/gems/kt-paperclip/7.1.1) (example `{ geometry: '100x100', format: :jpg }`) or [standalone ActiveStorage](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#methods) (example `rotate: 90`). In the long run, we should aim to completely differentiate how definitions are given for each library.

waiting-for-dev mentioned this pull request Mar 28, 2022

Add Rails 7 support #4220

Merged

14 tasks

waiting-for-dev force-pushed the waiting-for-dev/active_storage_size branch from 09b0107 to 12e80cf Compare March 28, 2022 05:02

waiting-for-dev added the Needs Backport label Mar 28, 2022

waiting-for-dev force-pushed the waiting-for-dev/libvips branch from f13ddba to a502886 Compare March 28, 2022 09:03

waiting-for-dev force-pushed the waiting-for-dev/active_storage_size branch from 12e80cf to 11a12f6 Compare March 28, 2022 09:04

tvdeyen approved these changes Mar 29, 2022

View reviewed changes

core/app/models/concerns/spree/active_storage_adapter/attachment.rb Outdated Show resolved Hide resolved

spaghetticode reviewed Mar 30, 2022

View reviewed changes

Base automatically changed from waiting-for-dev/libvips to master March 31, 2022 11:36

waiting-for-dev force-pushed the waiting-for-dev/active_storage_size branch from 11a12f6 to 1284caf Compare April 1, 2022 03:47

spaghetticode approved these changes Apr 1, 2022

View reviewed changes

tvdeyen requested changes Apr 4, 2022

View reviewed changes

waiting-for-dev force-pushed the waiting-for-dev/active_storage_size branch from 1284caf to b231ddd Compare April 5, 2022 11:25

tvdeyen approved these changes Apr 6, 2022

View reviewed changes

waiting-for-dev merged commit 6c79f34 into master Apr 11, 2022

waiting-for-dev deleted the waiting-for-dev/active_storage_size branch April 11, 2022 04:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes defining thumbnail sizes through ActiveStorage adapter #4318

Fixes defining thumbnail sizes through ActiveStorage adapter #4318

waiting-for-dev commented Mar 28, 2022 •

edited

Loading

tvdeyen left a comment

spaghetticode Mar 30, 2022

waiting-for-dev Apr 1, 2022

spaghetticode left a comment

tvdeyen left a comment

tvdeyen Apr 4, 2022

waiting-for-dev Apr 5, 2022

tvdeyen Apr 6, 2022

tvdeyen Apr 4, 2022

waiting-for-dev Apr 5, 2022

Fixes defining thumbnail sizes through ActiveStorage adapter #4318

Fixes defining thumbnail sizes through ActiveStorage adapter #4318

Conversation

waiting-for-dev commented Mar 28, 2022 • edited Loading

tvdeyen left a comment

Choose a reason for hiding this comment

spaghetticode Mar 30, 2022

Choose a reason for hiding this comment

waiting-for-dev Apr 1, 2022

Choose a reason for hiding this comment

spaghetticode left a comment

Choose a reason for hiding this comment

tvdeyen left a comment

Choose a reason for hiding this comment

tvdeyen Apr 4, 2022

Choose a reason for hiding this comment

waiting-for-dev Apr 5, 2022

Choose a reason for hiding this comment

tvdeyen Apr 6, 2022

Choose a reason for hiding this comment

tvdeyen Apr 4, 2022

Choose a reason for hiding this comment

waiting-for-dev Apr 5, 2022

Choose a reason for hiding this comment

waiting-for-dev commented Mar 28, 2022 •

edited

Loading