Fix rounding inconsistencies when cropping

dotdiff.gemspec

@@ -22,7 +22,7 @@ Gem::Specification.new do |spec|
   spec.executables   = spec.files.grep(%r{^exe/}) { |f| File.basename(f) }
   spec.require_paths = ['lib']
 
-  spec.add_runtime_dependency 'mini_magick', '>= 4.11.0'
+  spec.add_runtime_dependency 'mini_magick', '>= 4.12.0'
 
   spec.add_development_dependency 'bundler', '>= 2'
   spec.add_development_dependency 'capybara', '>= 2.6'

lib/dotdiff/image/cropper.rb

@@ -6,44 +6,34 @@ module DotDiff
   module Image
     module Cropper
       def crop_and_resave(element)
-        image = load_image(fullscreen_file)
+        crop_left = element.rectangle.x.floor
+        crop_right = (element.rectangle.x + element.rectangle.width).ceil
+        crop_top = element.rectangle.y.floor
+        crop_bottom = (element.rectangle.y + element.rectangle.height).ceil
+
+        crop_width = crop_right - crop_left
+        crop_height = crop_bottom - crop_top
 
-        # @see http://www.imagemagick.org/script/command-line-options.php?#crop
-        crop_area =
-          '' + width(element, image).to_s +
-          'x' + height(element, image).to_s +
-          '+' + element.rectangle.x.floor.to_s +
-          '+' + element.rectangle.y.floor.to_s
+        if crop_width - element.rectangle.width > 1
+          crop_left += 1
+          crop_width -= 1
+        end
+        if crop_height - element.rectangle.height > 1
+          crop_top += 1
+          crop_height -= 1
+        end
 
+        # @see https://www.imagemagick.org/script/command-line-options.php?#crop
+        crop_area = "#{crop_width}x#{crop_height}+#{crop_left}+#{crop_top}"
+
+        image = load_image(fullscreen_file)
         image.crop crop_area
         image.write(cropped_file)
       end
 
       def load_image(file)
         MiniMagick::Image.open(file)
       end
-
-      def height(element, image)
-        element_height = element.rectangle.height + element.rectangle.y
-        image_height = image.height
-
-        if element_height > image_height
-          image_height - element.rectangle.y
-        else
-          element.rectangle.height
-        end
-      end
-
-      def width(element, image)
-        element_width = element.rectangle.width + element.rectangle.x
-        image_width = image.width
-
-        if element_width > image_width
-          image_width - element.rectangle.x
-        else
-          element.rectangle.width
-        end
-      end
     end
   end
 end

spec/unit/image/cropper_spec.rb

@@ -44,10 +44,7 @@ def height; end
       allow(element).to receive(:rectangle).and_return(rectangle)
 
       expect(subject).to receive(:load_image).with('/home/se/full.png').and_return(mock_png).once
-      expect(subject).to receive(:width).with(element, mock_png).and_return(13).once
-      expect(subject).to receive(:height).with(element, mock_png).and_return(14).once
 
-      expect(mock_png).to receive(:crop).with("13x14+1+2").once
       expect(mock_png).to receive(:write).with('/tmp/T/cropped.png').once
     end
 
@@ -56,59 +53,29 @@ def height; end
         { 'top' => 2, 'left' => 1, 'height' => 4, 'width' => 3 }
       )
 
+      expect(mock_png).to receive(:crop).with("3x4+1+2").once
+
       subject.crop_and_resave(element)
     end
 
-    it 'rounds down the x and y values received from the browser' do
+    it 'correctly crops when the browser returns floating point numbers' do
       allow(rectangle).to receive(:rect).and_return(
-        { 'top' => 2.3, 'left' => 1.7, 'height' => 4.5, 'width' => 3.6 }
+        { 'top' => 2.3, 'left' => 1.3, 'height' => 4.2, 'width' => 3.2 }
       )
 
-      subject.crop_and_resave(element)
-    end
-  end
-
-  describe '#height' do
-    before { allow(element.rectangle).to receive(:rect).and_return(rect) }
-
-    context 'when element height is larger than the image height' do
-      let(:rect) { { 'top' => -180, 'left' => 0, 'width' => 800, 'height' => 1400 } }
-
-      it 'returns the image height minus the top point' do
-        allow(mock_png).to receive(:height).and_return(1200)
-        expect(subject.height(element, mock_png)).to eq 1380
-      end
-    end
-
-    context 'when element height is smaller than the image height' do
-      let(:rect) { { 'top' => -180, 'left' => 0, 'width' => 500, 'height' => 800 } }
+      expect(mock_png).to receive(:crop).with("4x5+1+2").once
 
-      it 'returns the element height' do
-        allow(mock_png).to receive(:height).and_return(1200)
-        expect(subject.height(element, mock_png)).to eq 800
-      end
+      subject.crop_and_resave(element)
     end
-  end
-
-  describe '#width' do
-    before { allow(element.rectangle).to receive(:rect).and_return(rect) }
-
-    context 'when element width is larger than the image width' do
-      let(:rect) { { 'top' => -180, 'left' => -30, 'width' => 731, 'height' => 1200 } }
 
-      it 'returns the image width minus the left point' do
-        allow(mock_png).to receive(:width).and_return(700)
-        expect(subject.width(element, mock_png)).to eq 730
-      end
-    end
+    it 'correctly shifts the cropped position when rounding leads to a bigger image' do
+      allow(rectangle).to receive(:rect).and_return(
+        { 'top' => 2.9, 'left' => 1.2, 'height' => 3.6, 'width' => 2.9 }
+      )
 
-    context 'when element width is smaller than the image width' do
-      let(:rect) { { 'top' => -180, 'left' => -20, 'width' => 800, 'height' => 800 } }
+      expect(mock_png).to receive(:crop).with("3x4+2+3").once
 
-      it 'returns the element width' do
-        allow(mock_png).to receive(:width).and_return(850)
-        expect(subject.width(element, mock_png)).to eq 800
-      end
+      subject.crop_and_resave(element)
     end
   end
 end