Added support for the "crop" method.

clickworkorange commented 5 months ago

I had a need for this functionality, and while I know I could use the vips! method in my uploader it felt like an omission - so I put together a PR with a (passing) spec. The test is a little basic, but I wasn't sure if it would be ok to add new image fixtures, which I would need to do in order to test that the cropping produces the correct output (my test only checks the resulting dimensions). I'd be happy to look at implementing further vips methods if desired!

clickworkorange commented 5 months ago

Two tests are failing, but I'm pretty sure neither has anything to do with me!

1. Test / RSpec and Cucumber (ruby-head, gemfiles/rails-7-1.gemfile, false) (pull_request)

LoadError:
     cannot load such file -- RMagick (You may need to install the rmagick gem)
     ...
     # ./vendor/bundle/ruby/3.4.0+0/gems/webmock-3.19.1/lib/webmock/rspec.rb:39:in `block (2 levels) in <top (required)>'
     # ------------------
     # --- Caused by: ---
     # LoadError:
     #   cannot load such file -- observer
     #   ./vendor/bundle/ruby/3.4.0+0/gems/rmagick-5.3.0/lib/rmagick_internal.rb:24:in `<top (required)>'

2. Test / RSpec and Cucumber (jruby-head, gemfiles/rails-7-0.gemfile, true) (pull_request)

CarrierWave::ActiveRecord#mount_uploader #image should return valid XML when to_xml is called when image is nil
     Failure/Error: hash = Hash.from_xml(@event.to_xml)["event"]

     REXML::ParseException:
       #<RuntimeError: Illegal character "\u0000" in raw string "true\"/>\u0000\u0000\u0000">
       /home/runner/work/carrierwave/carrierwave/vendor/bundle/jruby/3.1.0/gems/rexml-3.2.6/lib/rexml/text.rb:140:in `block in check'
       org/jruby/RubyArray.java:1989:in `each'

clickworkorange commented 5 months ago

Only one test failing now, on JRuby-Head. Been looking at it for a while and it seems to have to do with the serialisation of ActiveRecord object; it's failing on @event.to_xml every time, with @event being an ActiveRecord object:

# activerecord_spec.rb

describe CarrierWave::ActiveRecord do
  def create_table(name)
    ActiveRecord::Base.connection.create_table(name, force: true) do |t|
      t.column :image, :string
      t.column :images, :json
      t.column :textfile, :string
      t.column :textfiles, :json
      t.column :foo, :string
    end
  end

 ...

  def reset_class(class_name)
    Object.send(:remove_const, class_name) rescue nil
    Object.const_set(class_name, Class.new(ActiveRecord::Base))
  end

  before(:all) { create_table("events") }
  after(:all) { drop_table("events") }

  before do
    @uploader = Class.new(CarrierWave::Uploader::Base)
    reset_class("Event")
    @event = Event.new
  end

  ...

      it "should return valid XML when to_xml is called when image is nil" do
        #### THIS TEST FAILS ON @event.to_xml ####
        hash = Hash.from_xml(@event.to_xml)["event"]

        expect(@event[:image]).to be_nil
        expect(hash.keys).to include("image")
        expect(hash["image"].keys).to include("url")
        expect(hash["image"]["url"]).to be_nil
      end

      it "should return valid XML when to_xml is called when image is present" do
        #### THIS TEST ALSO FAILS ON @event.to_xml ####
        @event[:image] = 'test.jpeg'
        @event.save!
        @event.reload

        expect(Hash.from_xml(@event.to_xml)["event"]["image"]).to eq({"url" => "/uploads/test.jpeg"})
      end

      ...

It might be that a change in rexml and/or activemodel-serializers-xml which pulls it in is where the "illegal" null characters are coming from (\u0000) - but whatever that change might be it only appears to affect JRuby.

clickworkorange commented 5 months ago

Some obeservations:

activemodel-serializers-xml hasn't seen any changes for the last four years, so that's not it.
Rexml had a new release on July 23 - but since it's rexml complaining about those null characters my feeling is that the problem lies elsewhere, namely:
JRuby had a major release in November last year, which added Ruby 3.1 compatibility. I'm really not a Java guy >shudder<, but this looks like it may be relevant: https://github.com/jruby/jruby/commit/830c0dc0f0c916bbbcad0ad001d0a288f93350d0 Specifically, they changed from java.util.HashMap to java.util.LinkedHashMap in the VariableAccessor getVariableAccessorWithBuilder.

clickworkorange commented 5 months ago

I've added a question to the JRuby commit mentioned in my previous comment:

https://github.com/jruby/jruby/commit/830c0dc0f0c916bbbcad0ad001d0a288f93350d0

mshibuya commented 5 months ago

Thank you so much for taking a deep look on the JRuby failure! It's not a blocker for this PR anyway, as it's unrelated.

But is it possible to add #crop to CarrierWave::MiniMagick and CarrierWave::RMagick as well? They should be considered as a sort of adapter, so they'd better to have the same interface unless doing so is infeasible.

clickworkorange commented 5 months ago

I'll take a look!

clickworkorange commented 5 months ago

I have added support for image cropping to the RMagic and MiniMagic processors as well. In doing so I found a difference in behaviour between Vips and the other two: while both RMagic and MiniMagic silently accept cropping beyond the image boundaries (retaining the original image bottom/right edge), Vips would throw an Vips::Error: extract_area: bad extract area exception when asked to do so. I thought it made most sense to make Vips match the behaviour of the other two processors, and did so by recalculating the cropping area if either edge of it falls outside the image:

def crop(left, top, width, height, combine_options: {})
  width, height = resolve_dimensions(width, height)
  width = vips_image.width - left if width + left > vips_image.width
  height = vips_image.height - top if height + top > vips_image.height

  vips! do |builder|
    builder.crop(left, top, width, height)
      .apply(combine_options)
  end
end

I added specs to all three processors to test for this case.

clickworkorange commented 5 months ago

I'm working on updating the README to include information about Vips support and the crop method - would you prefer that as a separate PR, or should I include those changes here?

mshibuya commented 5 months ago

You can include it into this PR 👍

clickworkorange commented 5 months ago

Ok, here's what I had in mind:

Manipulating images

[no changes]

Using MiniMagick

[no changes]

Using RMagick

[no changes]

Using Vips

CarrierWave version 2.2.0 added support for the libvips image processing library, through ImageProcessing::Vips. Its functionality matches that of the RMagic and MiniMagic processors, but it uses less memory and offers faster processing. To use the Vips processing module you must first install libvips, for example:

$ sudo apt install libvips

You also need to tell your uploader to use Vips:

class ImageFileUploader < CarrierWave::Uploader::Base
  include CarrierWave::Vips

  ...

end

List of available processing methods:

[!NOTE] While the intention is to provide uniform interfaces to all three processing libraries the availability and implementation of processing methods can vary slightly between them.

convert - Changes the image encoding format to the given format (eg. jpg). This operation is treated specially to trigger the change of the file extension, so it matches with the format of the resulting file.
resize_to_limit - Resize the image to fit within the specified dimensions while retaining the original aspect ratio. Will only resize the image if it is larger than the specified dimensions. The resulting image may be shorter or narrower than specified in the smaller dimension but will not be larger than the specified values.
resize_to_fit - Resize the image to fit within the specified dimensions while retaining the original aspect ratio. The image may be shorter or narrower than specified in the smaller dimension but will not be larger than the specified values.
resize_to_fill - Resize the image to fit within the specified dimensions while retaining the aspect ratio of the original image. If necessary, crop the image in the larger dimension. Optionally, a "gravity" may be specified, for example "Center", or "NorthEast".
resize_and_pad - Resize the image to fit within the specified dimensions while retaining the original aspect ratio. If necessary, will pad the remaining area with the given color, which defaults to transparent (for gif and png, white for jpeg). Optionally, a "gravity" may be specified, as above.
crop - Crop the image to the contents of a box with the specified height and width, positioned a given number of pixels from the top and left. The original image edge will be retained should the bottom and/or right edge of the box fall outside the image bounds.

Supported processing methods

The following table shows which processing methods are supported by each processing library, and which parameters they accept:

Method	RMagick	MiniMagick	Vips
`convert`	`format`	`format`, `page`¹	`format`, `page`¹
`resize_to_limit`	`width`, `height`	`width`, `height`	`width`, `height`
`resize_to_fit`	`width`, `height`	`width`, `height`	`width`, `height`
`resize_to_fill`	`width`, `height`, `gravity`²	`width`, `height`, `gravity`²	`width`, `height`
`resize_and_pad`	`width`, `height`, `background`, `gravity`²	`width`, `height`, `background`, `gravity`²	`width`, `height`, `background`, `gravity`²
`resize_to_geometry_string`	`geometry_string`³	not implemented	not implemented
`crop`	`left`, `top`, `width`, `height`	`left`, `top`, `width`, `height`	`left`, `top`, `width`, `height`

¹page refers to the page number when converting from PDF, frame number when converting from GIF, and layer number when converting from PSD.

²gravity refers to an image position given as one of Center, North, NorthWest, West, SouthWest, South, SouthEast, East, or NorthEast.

³geometry_string is an ImageMagick geometry string.

mshibuya commented 4 months ago

Great work, thank you so much!

clickworkorange commented 4 months ago

My pleasure!

carrierwaveuploader / carrierwave