Add tests for render_gravity and crop area calculations

app/controllers/alchemy/admin/essence_pictures_controller.rb

@@ -111,22 +111,25 @@ def infer_width_or_height_from_ratio
       # Combines separated gravity params (size, x, y) into single gravity hash
       #
       def combine_gravities(essence_picture_params)
-        gravity = @essence_picture.render_gravity || {}
+        gravity = @essence_picture.render_gravity.presence || {}
         gravity = gravity.symbolize_keys.merge({
           size: essence_picture_params.delete(:render_gravity_size),
           x: essence_picture_params.delete(:render_gravity_x),
-          y: essence_picture_params.delete(:render_gravity_y)
-        })
+          y: essence_picture_params.delete(:render_gravity_y),
+        }.compact)
 
-        essence_picture_params[:render_gravity] = gravity.reject{ |_k,v| v.blank? }.presence || nil
+        # Remove any blanks after settings values, render_gravity=nil if all empty
+        gravity = gravity.delete_if { |_k, v| v.blank? }.presence
+
+        essence_picture_params[:render_gravity] = gravity
         essence_picture_params
       end
 
       def essence_picture_params
         essence_picture_params = params.require(:essence_picture).permit(:alt_tag, :caption, :css_class, :render_size, :title, :crop_from, :crop_size, :render_gravity, :render_gravity_size, :render_gravity_x, :render_gravity_y)
 
         if @essence_picture.present? && [:render_gravity_size, :render_gravity_x, :render_gravity_y].any? { |k| essence_picture_params.key?(k) }
-          combine_gravities(essence_picture_params)
+          essence_picture_params = combine_gravities(essence_picture_params)
         end
 
         essence_picture_params

app/models/alchemy/essence_picture.rb

@@ -140,17 +140,25 @@ def cropping_mask
     # But can also have a general use in aligning background-images with dynamic aspect ratio etc.
     #
     def gravity(gravity_override = nil)
-      # Check for gravity in db or settings
-      g = render_gravity || content.settings[:gravity]
-      g = g.merge(gravity_override) if gravity_override.present?
+      g = default_gravity
 
-      return default_gravity if g.blank? || g == true # If settings.gravity = true => use defaults
-      raise "Gravity not hash" unless g.is_a?(Hash)
+      # settings_gravity = true would leave default gravity but allow users to edit it in essence_picture/edit
+      settings_gravity = content.settings[:gravity] == true ? nil : content.settings[:gravity]
 
-      g = default_gravity.merge(g.symbolize_keys)
+      [settings_gravity, render_gravity, gravity_override].each do |gravity|
+        next if gravity.blank?
+
+        if gravity.is_a?(Hash)
+          g = g.merge(gravity.symbolize_keys)
+        else
+          raise ArgumentError, "Gravity not a hash"
+        end
+      end
 
       g.each do |key, val|
-        raise "Invalid gravity option" unless available_gravities[key] && available_gravities[key].include?(val)
+        unless available_gravities[key] &.include?(val)
+          raise ArgumentError, "Invalid gravity option: #{key}: #{val}"
+        end
       end
 
       g
@@ -161,21 +169,23 @@ def gravity(gravity_override = nil)
     # 1. It usually makes sense to show as much as possible of an image.
     # Lets say the user uploads a perfectly fine 4:3 image and is then forced to crop it to 16:9
     # as per defined settings. Then in some place you want to render it in 4:3 again => use original.
-    # If the user specifically cropped an image down to hide something => use gravity = shrink.
+    # If the user specifically cropped an image down to hide something => use gravity = shrink instead.
     #
     # 2. More backwards compatible to old crop system.
-    # If the user never actually applied a cropping mask the original image was simply center cropped,
-    # ignoring the boundaries of the rendered cropping mask => this equates to "grow".
+    # If the user never actually applied a cropping mask the original image was center cropped to fit
+    # size aspect ratio, ignoring the boundaries of the rendered cropping mask => this equates to "grow".
     #
     def default_gravity
       { size: 'grow', x: 'center', y: 'center'}
     end
 
+    # Available gravities are used to validate gravity method input
+    #
     def available_gravities
       {
         size: ['grow', 'shrink', 'closest_fit'],
         x: ['left', 'center', 'right'],
-        y: ['top', 'center', 'bottom']
+        y: ['top', 'center', 'bottom'],
       }
     end
 
@@ -212,7 +222,12 @@ def fix_crop_values
     def validate_render_gravity
       return if render_gravity.blank?
 
-      gravity rescue errors.add(:render_gravity, :invalid)
+      # Use helper to validate gravity
+      begin
+        gravity
+      rescue ArgumentError
+        errors.add(:render_gravity, :invalid)
+      end
     end
 
     def normalize_crop_value(crop_value)

app/models/alchemy/essence_picture_view.rb

@@ -13,7 +13,9 @@ class EssencePictureView
       show_caption: true,
       disable_link: false,
       srcset: [],
-      sizes: []
+      sizes: [],
+      render_size: nil,
+      gravity: nil,
     }.with_indifferent_access
 
     def initialize(content, options = {}, html_options = {})
@@ -22,14 +24,12 @@ def initialize(content, options = {}, html_options = {})
       @picture = essence.picture
       @html_options = html_options
 
-      added_opts = {
-        # Extract the size specified for render so we can calculate cropping masks etc
-        render_size: @essence.render_size || content.settings[:size],
-        # Set gravity with correct fallbacks and validation
+      @options = DEFAULT_OPTIONS.merge(content.settings).merge(options).merge(
+        # Get potential user selected size so we can calculate crop area
+        render_size: @essence.render_size,
+        # Set gravity with correct fallbacks and validation through helper
         gravity: @essence.gravity(options.delete(:gravity))
-      }
-
-      @options = DEFAULT_OPTIONS.merge(content.settings).merge(options).merge(added_opts)
+      )
     end
 
     def render

app/models/alchemy/picture/transformations.rb

@@ -68,21 +68,22 @@ def crop(size, render_size, crop_from = nil, crop_size = nil, render_crop = fals
     #
     def get_crop_area(size, render_size, crop_from, crop_size, render_crop, gravity)
       size = sizes_from_string(size)
-      render_size = sizes_from_string(render_size)
+      render_size = sizes_from_string(render_size) if render_size.present?
 
       if crop_from.present? && crop_size.present? # User has cropped image
         crop_from = point_from_string(crop_from)
         crop_size = sizes_from_string(crop_size)
-      elsif render_size.present? # Use default_mask
+      elsif render_size.present? # User has selected a size in picture properties, essence_pictures/edit.html.erb
         crop_from, crop_size = default_mask(render_size, false)
-      else # Crop area covers the entire image
-        crop_from = { x: 0, y: 0 }
-        crop_size = { width: image_file_width, height: image_file_height }
+      else # Size specified in content settings/render options
+        crop_from, crop_size = default_mask(size, false)
       end
 
       size_ar, crop_ar = size_aspect_ratio(size), size_aspect_ratio(crop_size)
 
       if render_crop && size_ar != crop_ar # Render cropping allowed and aspect ratio changed => adjust cropping
+        raise ArgumentError, "Cannot render_crop without gravity" if !gravity.present?
+
         crop_from, crop_size = adjust_crop_area_to_aspect_ratio(crop_from, crop_size, size_ar, crop_ar, gravity)
       end
 
@@ -112,12 +113,11 @@ def adjust_crop_area_to_aspect_ratio(crop_from, crop_size, size_ar, crop_ar, gra
     def shrink_crop_area(crop_size, size_ar, crop_ar)
       if crop_ar < size_ar # Crop is taller than size => shrink y
         crop_size[:height] = crop_size[:height] * (crop_ar / size_ar)
-
       else # Crop is wider than size => shrink x
         crop_size[:width] = (crop_size[:width] * (size_ar / crop_ar))
       end
 
-      crop_size
+      round_dimensions(crop_size)
     end
 
     # Attempt to grow crop area to fit requested size aspect ratio.
@@ -137,7 +137,7 @@ def grow_crop_area(crop_from, crop_size, size_ar, crop_ar, gravity, closest_fit
         crop_size[:width] = crop_size[:height] * size_ar
       end
 
-      crop_size
+      round_dimensions(crop_size)
     end
 
     # Returns the new crop_from position based on crop gravity after the cropped area has been resized
@@ -174,7 +174,7 @@ def max_crop_area_growth(crop_from, crop_size)
         top: crop_from[:y],
         right: image_size[:width] - crop_from[:x] - crop_size[:width],
         bottom: image_size[:height] - crop_from[:y] - crop_size[:height],
-        left: crop_from[:x]
+        left: crop_from[:x],
       }
     end
 
@@ -229,10 +229,12 @@ def wanted_crop_area_growth(crop_size, size_ar, crop_ar, gravity, closest_fit =
     #
     def actual_crop_area_growth(wanted_growth, max_growth)
       possible_growth = {} # Will only contain non zero wanted growth directions
-      wanted_growth.reject{ |d, v| v.zero? }.each do |direction, value|
-        possible_growth[direction] = wanted_growth[direction] <= max_growth[direction] ?
-          wanted_growth[direction] :
+      wanted_growth.reject{ |_d, v| v.zero? }.each do |direction, _value|
+        possible_growth[direction] = if wanted_growth[direction] <= max_growth[direction]
+          wanted_growth[direction]
+        else
           max_growth[direction]
+        end
       end
 
       max_growth_any_direction = possible_growth.values.min || 0
@@ -245,10 +247,16 @@ def actual_crop_area_growth(wanted_growth, max_growth)
       actual_growth
     end
 
+    # Round to whole pixels
+    #
+    def round_dimensions(dim)
+      dim.each{ |key, val| dim[key] = val.round }
+    end
+
     # Returns the aspect ratio width / height from size hash
     #
     def size_aspect_ratio(size)
-      (size[:width].to_f / size[:height]).round(3)
+      (size[:width].to_f / size[:height]).round(5)
     end
 
     # Returns the rendered resized image using imagemagick directly.
@@ -423,12 +431,17 @@ def is_smaller_than(dimensions)
     # Use imagemagick to custom crop an image. Uses -thumbnail for better performance when resizing.
     #
     def xy_crop_resize(dimensions, top_left, crop_dimensions, upsample)
-      crop_argument = "-crop #{dimensions_to_string(crop_dimensions)}"
-      crop_argument += "+#{top_left[:x]}+#{top_left[:y]}"
+      crop_argument = ''
+
+      # Only crop if dimensions changed
+      if crop_dimensions[:width] != image_file_width || crop_dimensions[:height] != image_file_height
+        crop_argument += "-crop #{dimensions_to_string(crop_dimensions)}"
+        crop_argument += "+#{top_left[:x]}+#{top_left[:y]} "
+      end
 
       resize_argument = "-resize #{dimensions_to_string(dimensions)}"
       resize_argument += ">" unless upsample
-      image_file.convert "#{crop_argument} #{resize_argument}"
+      image_file.convert "#{crop_argument}#{resize_argument}"
     end
 
     # Used when centercropping.

db/migrate/20200701100718_add_render_gravity_to_essence_picture.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddRenderGravityToEssencePicture < ActiveRecord::Migration[6.0]
+  def change
+    add_column :alchemy_essence_pictures, :render_gravity, :string
+  end
+end

spec/controllers/alchemy/admin/essence_pictures_controller_spec.rb

@@ -202,8 +202,8 @@ module Alchemy
     end
 
     describe '#update' do
-      before do
-        expect(EssencePicture).to receive(:find).and_return(essence)
+      before(:each) do |test|
+        expect(EssencePicture).to receive(:find).and_return(essence) unless test.metadata[:stub_essence_picture]
         expect(Content).to receive(:find).and_return(content)
       end
 
@@ -233,6 +233,82 @@ module Alchemy
           }
         }, xhr: true
       end
+
+      context 'with render_gravity params' do
+        it "combines separated render_gravitiy params into serialized json hash" do
+          put :update, params: {
+            id: 1,
+            essence_picture: {
+              render_gravity_size: 'shrink',
+              render_gravity_x: 'left',
+              render_gravity_y: 'top',
+            },
+          }, xhr: true
+
+          expect(response.status).to eq(200)
+          expect(essence.render_gravity).to eq({
+            'size' => 'shrink',
+            'x' => 'left',
+            'y' => 'top',
+          })
+        end
+
+        it "can set/unset individual gravity values", :stub_essence_picture do
+          essence = EssencePicture.new(id: 1, content: content, picture: picture, render_gravity: {size: 'shrink', x: 'left'})
+          allow(EssencePicture).to receive(:find).and_return(essence)
+
+          # load_essencse_picture did not find our essence in before_action so i set @essence_picture manually
+          expect(controller).to receive(:load_essence_picture).and_return(nil)
+          controller.instance_variable_set(:@essence_picture, essence)
+
+          put :update, params: {
+            id: 1,
+            essence_picture: {
+              render_gravity_y: 'top',
+              render_gravity_x: '',
+            },
+          }, xhr: true
+
+          expect(response.status).to eq(200)
+          expect(essence.render_gravity).to eq({
+            'size' => 'shrink',
+            'y' => 'top',
+          })
+        end
+
+        it "renders 400 with errors if gravity invalid" do
+          put :update, params: {
+            id: 1,
+            essence_picture: {
+              render_gravity_x: 'unknown',
+            },
+          }, xhr: true
+
+          expect(response.status).to eq(400)
+          expect(essence.errors.details[:render_gravity]).to eq([{error: :invalid}])
+        end
+
+        it "Sets render_gravity = nil if all render_gravities are blank", :stub_essence_picture do
+          essence = EssencePicture.new(id: 1, content: content, picture: picture, render_gravity: {size: 'shrink'})
+          allow(EssencePicture).to receive(:find).and_return(essence)
+
+          # load_essence_picture did not find our essence in before_action so i set @essence_picture manually
+          expect(controller).to receive(:load_essence_picture).and_return(nil)
+          controller.instance_variable_set(:@essence_picture, essence)
+
+          put :update, params: {
+            id: 1,
+            essence_picture: {
+              render_gravity_size: '',
+              render_gravity_x: '',
+              render_gravity_y: '',
+            },
+          }, xhr: true
+
+          expect(response.status).to eq(200)
+          expect(essence.render_gravity).to equal(nil)
+        end
+      end
     end
 
     describe '#assign' do

spec/dummy/db/migrate/20200701100718_add_render_gravity_to_essence_picture.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddRenderGravityToEssencePicture < ActiveRecord::Migration[6.0]
+  def change
+    add_column :alchemy_essence_pictures, :render_gravity, :string
+  end
+end

spec/dummy/db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2020_02_26_213334) do
+ActiveRecord::Schema.define(version: 2020_07_01_100718) do
 
   create_table "alchemy_attachments", force: :cascade do |t|
     t.string "name"
@@ -134,6 +134,7 @@
     t.string "crop_from"
     t.string "crop_size"
     t.string "render_size"
+    t.string "render_gravity"
     t.index ["picture_id"], name: "index_alchemy_essence_pictures_on_picture_id"
   end