Merge pull request #15 from benissimo/master

Quality and extension for convert image + fixes and specs

README.rdoc

@@ -42,11 +42,25 @@ proportional, and width proportional.
 my_image_model.attachment.s_640_480 will generate a 640x480 pixel image.
 
 my_image_model.attachment.s_640_width will generate an image that is 640 pixels
+
 wide and proportionally high based on the original image dimensions.
 
 my_image_model.attachment.s_480_height will generate an image that is 480 pixels
 high and proportionally wide based on the original image dimensions.
 
+== Parameters options 
+Attachement-on-the-fly uses 'Convert' from image-magick to resize image and
+you can pass any convert options in hash notation.
+my_image_model.attachement.s_800_600 :morph => 15
+
+The only exception is :extension parameter
+my_image_model.attachement.s_400_300 :quality => 75,:extension => 'jpg' will generate
+an image that is 400x300 with quality 75% and convert jpg if necessary.
+
+link:http://www.imagemagick.org/script/command-line-options.php
+
+
+
 When an image's instance is first accessed through the s_ method, a new image
 will be generated according to the image's path interpolation.  Then, every time
 the image is accessed with the same dimensions, the previously generated image

lib/attachment_on_the_fly.rb

@@ -9,8 +9,9 @@
 
   # we respond to s_ and cls_
   def respond_to?(method,*args, &block)
-    if method.to_s.match(/^s_[0-9]+_[0-9]+/) ||  method.to_s.match(/^s_[0-9]+_[a-z]+/) || method.to_s.match(/^s[0-9]+/)  ||
-      method.to_s.match(/^cls_[0-9]+_[0-9]+/) ||  method.to_s.match(/^cls_[0-9]+_[a-z]+/) || method.to_s.match(/^cls[0-9]+/)
+    if method.to_s.match(/^(cls|s)_[0-9]+_[0-9]+$/) ||
+      method.to_s.match(/^(cls|s)_[0-9]+_(width|height|both)$/) ||
+      method.to_s.match(/^(cls|s)[0-9]+$/)
       return true
     end
     super
@@ -19,34 +20,43 @@ def respond_to?(method,*args, &block)
   def method_missing(symbol , *args, &block )
     # We are looking for methods with S_[number]_[number]_(height | width | proportion)
     # Height and width
-    # Check to see if file exist if so return string
-    # if not generate image and return string to file  Fiel is in format S_Height_x_Width_FILE_NAME
+    # Check to see if file exists, if so return string
+    # if not generate image and return string to file
     image_name = nil
-    if symbol.to_s.match(/^s_[0-9]+_[0-9]+/) || symbol.to_s.match(/^cls_[0-9]+_[0-9]+/)
+    parameters = args.shift
+    parameters ||= {}
+
+    if symbol.to_s.match(/^(cls|s)_[0-9]+_[0-9]+$/)
       values = symbol.to_s.split("_")
-      height = values[1]
-      width = values[2]
-      image_name = generate_image("both", height.to_i, width.to_i)
-    elsif symbol.to_s.match(/^s_[0-9]+_[a-z]+/)   || symbol.to_s.match(/^cls_[0-9]+_[a-z]+/)
+      height = values[1].to_i
+      width = values[2].to_i
+      image_name = generate_image("both", height, width, parameters)
+    elsif symbol.to_s.match(/^(cls|s)_[0-9]+_(width|height|both)$/)
       values = symbol.to_s.split("_")
-      size = values[1]
-      who = values[2]
-      image_name = generate_image(who, size.to_i)
-    elsif symbol.to_s.match(/^s[0-9]+/)  || symbol.to_s.match(/^cls[0-9]+/)
+      size = values[1].to_i
+      kind = values[2]
+      image_name = generate_image(kind, size, size, parameters)
+    elsif symbol.to_s.match(/^(cls|s)[0-9]+$/)
       values = symbol.to_s.split("s")
-      size = values[1]
-      who = "width"
-      image_name = generate_image(who, size.to_i)
+      size = values[1].to_i
+      kind = "width"
+      image_name = generate_image(kind, size, size, parameters)
     else
       # if our method string does not match, we kick things back up to super ... this keeps ActiveRecord chugging along happily
       super
     end
     return image_name
   end
 
-  def generate_image(kind, height = 0, width = 0)
+  def generate_image(kind, height = 0, width = 0, parameters = {})
     convert_command_path = (Paperclip.options[:command_path] ? Paperclip.options[:command_path] + "/" : "")
-
+    parameters.symbolize_keys!
+    [:extension, :quality].each do |opt|
+      parameters.reverse_merge!({opt => Paperclip.options[opt]}) if Paperclip.options[opt]
+    end
+    quality = parameters[:quality] ||= 100
+    parameters.delete :quality
+
     prefix = ""
 
     if kind == "height"
@@ -55,27 +65,37 @@ def generate_image(kind, height = 0, width = 0)
       width = height
       prefix = "S_" + height.to_s + "_WIDTH_"
     elsif kind == "both"
-      prefix = "S_" + height.to_s + "_" + height.to_s + "_"
+      prefix = "S_" + width.to_s + "_" + height.to_s + "_"
     end
-
+    presuffix = parameters.map{|k,v| "#{k}_#{v}" }.join('___') + "_q_#{quality}_"
+    prefix = "#{prefix}#{presuffix}_"
+    prefix = "#{prefix}#{Paperclip.options[:version_prefix]}_" if Paperclip.options[:version_prefix]
+
     path = self.path
     url = self.url
 
     path_arr = path.split("/")
     file_name = path_arr.pop
     path = path_arr.join("/")
 
+    base_arr = file_name.split('.');
+    original_extension = extension = base_arr.pop
+    base_name = base_arr.join('.')
+    extension = parameters[:extension] || extension
+    parameters.delete :extension
+
     url_arr = url.split("/")
     url_file_name = url_arr.pop
     url_path = url_arr.join("/")
 
-    original = path + "/" + self.original_filename
-    newfilename = path + "/" + prefix + file_name
-    new_path = url_path + "/" + prefix + file_name
-
-    return new_path  if  File.exist?(newfilename)
+    original = path + "/" + self.original_filename    
+    newfilename = path + "/" + prefix + base_name +  '.' + extension
+    fallback_newfilename = path + "/" + prefix + base_name +  '.' + original_extension
+    new_path = url_path + "/" + prefix + base_name + '.' + extension
+    fallback_new_path = url_path + "/" + prefix + base_name + '.' + original_extension
+    return new_path if File.exist?(newfilename)
 
-    if  !File.exist?(original)
+    if !File.exist?(original)
       if Paperclip.options[:whiny]
         raise AttachmentOnTheFlyError.new("Original asset could not be read from disk at #{original}")
       else
@@ -91,26 +111,50 @@ def generate_image(kind, height = 0, width = 0)
 
     command = ""
 
+    options_for_extension = ""
+    if original_extension != extension && has_alpha?(original)
+      # Converting extension with alpha channel is problematic.
+      # Fall back to original extension.
+      Paperclip.log("Ignoring extension parameter because original file is transparent")
+      newfilename = fallback_newfilename
+      new_path = fallback_new_path
+    end
+
+    base_command = "#{convert_command_path}convert -strip -geometry"
+
     if kind == "height"
       # resize_image infilename, outfilename , 0, height
-      command = "#{convert_command_path}convert -colorspace RGB -geometry x#{height} -quality 100 -sharpen 1 #{original} #{newfilename} 2>&1 > /dev/null"
+      command = "#{base_command} x#{height} -quality #{quality} -sharpen 1 '#{original}' '#{newfilename}' 2>&1 > /dev/null"
     elsif kind == "width"
       # resize_image infilename, outfilename, width
-      command = "#{convert_command_path}convert -colorspace RGB -geometry #{width} -quality 100 -sharpen 1 #{original} #{newfilename} 2>&1 > /dev/null"
+      command = "#{base_command} #{width} -quality #{quality} -sharpen 1 '#{original}' '#{newfilename}' 2>&1 > /dev/null"
     elsif kind == "both"
       # resize_image infilename, outfilename, height, width
-      command = "#{convert_command_path}convert -colorspace RGB -geometry #{width}x#{height} -quality 100 -sharpen 1 #{original} #{newfilename} 2>&1 > /dev/null"
+      command = "#{base_command} #{width}x#{height} -quality #{quality} -sharpen 1 '#{original}' '#{newfilename}' 2>&1 > /dev/null"
     end
 
-    `#{command}`
+    convert_command command
+
+    return new_path
+  end
 
-    if $? != 0
-      raise AttachmentOnTheFlyError.new("Execution of convert failed. Please set path in Paperclip.options[:command_path] or ensure that file permissions are correct.")
+  def convert_command command
+    `#{command}`
+    if ($? != 0)
+      raise AttachmentOnTheFlyError.new("Execution of convert failed. Please set path in Paperclip.options[:command_path] or ensure that file permissions are correct. Failed trying to do: #{command}")
     end
+  end
 
-    return new_path
+  def has_alpha? image
+    identify_command_path = (Paperclip.options[:identify_command_path] ? Paperclip.options[:identify_command_path] + "/" : "")
+    # http://stackoverflow.com/questions/2581469/detect-alpha-channel-with-imagemagick
+    command = "#{identify_command_path}identify -format '%[channels]' '#{image}'"
+    result = `#{command}`
+    if ($? != 0)
+      raise AttachmentOnTheFlyError.new("Execution of identify failed. Please set path in Paperclip.options[:identify_command_path] or ensure that file permissions are correct. Failed trying to do: #{command}")
+    end
+    result && result.chomp == "rgba"
   end
 end
 
 class AttachmentOnTheFlyError < StandardError; end
-

spec/attachment_on_the_fly_spec.rb

@@ -0,0 +1,99 @@
+require_relative './spec_helper'
+
+describe "Attachment on the fly mixin" do
+
+  subject { Paperclip::Attachment.new }
+
+  context "#respond_to?" do
+    method_names = %w{s125 cls125 s_125_250 cls_125_250
+      s_125_width cls_125_width s_125_height cls_125_height
+      s_125_both cls_125_both
+    }
+
+    method_names.each do |method_name|
+      it { should respond_to(method_name.to_sym) }
+    end
+
+    it { should_not respond_to(:x_125_250) }
+    it { should_not respond_to(:s_125_250foo) }
+    it { should_not respond_to(:S_125_250) }
+  end
+
+  context "#method_missing" do
+
+    context "translates method into a generate image call" do
+      method_name_to_generate_image_call = {
+        :s_125_225 => ["both", 125, 225, {}],
+        :s125 => ["width", 125, 125, {}],
+        :s_125_height => ["height", 125, 125, {}],
+        :s_125_width => ["width", 125, 125, {}],
+        :s_125_both => ["both", 125, 125, {}]
+      }
+
+      method_name_to_generate_image_call.each do |method_name, generate_image_args|
+        it "#{method_name}" do
+          subject.should_receive(:generate_image).with(*generate_image_args)
+          subject.send(method_name)
+        end
+      end
+
+      it "passes parameters through as well" do
+        subject.should_receive(:generate_image).with("width", 125, 125, {:quality => 90, :extension => "jpeg"})
+        subject.s_125_width :quality => 90, :extension => "jpeg"
+      end
+    end
+  end
+
+  context "#generate_image" do
+
+    context "it should generate a new image" do
+      method_name_to_expectations = {
+        :s_125_width => {
+          :new => "/S_125_WIDTH__q_100__path.png",
+          :regex => /-geometry 125 /
+        },
+        :s_125_height => {
+          :new => "/S_125_HEIGHT__q_100__path.png",
+          :regex => /-geometry x125 /
+        },
+        :s_125_both => {
+          :new => "/S_125_125__q_100__path.png",
+          :regex => /-geometry 125x125 /
+        }
+      }
+      method_name_to_expectations.each do |method_name, expected|
+        it "for #{method_name}" do
+          File.should_receive(:exist?).with(expected[:new]).and_return(false)
+          File.should_receive(:exist?).with("//file.png").and_return(true)
+          subject.should_receive(:convert_command).with(expected[:regex])
+          subject.send(method_name)
+        end
+      end
+
+      it "passes in parameters for quality" do
+        File.should_receive(:exist?).with("/S_125_WIDTH__q_75__path.png").and_return(false)
+        File.should_receive(:exist?).with("//file.png").and_return(true)
+        subject.should_receive(:convert_command).with(/-quality 75/)
+        subject.s_125_width :quality => 75
+      end
+
+      it "passes in parameters for extension" do
+        File.should_receive(:exist?).with("/S_125_WIDTH_extension_jpeg_q_75__path.jpeg").and_return(false)
+        File.should_receive(:exist?).with("//file.png").and_return(true)
+        subject.should_receive(:convert_command).with(/-quality 75/)
+        subject.should_receive(:has_alpha?).with("//file.png").and_return(false)
+        subject.s_125_width(:quality => 75, :extension => "jpeg").should == "/S_125_WIDTH_extension_jpeg_q_75__path.jpeg"
+      end
+
+      it "preserves original extension if file has alpha channel" do
+        File.should_receive(:exist?).with("/S_125_WIDTH_extension_jpeg_q_75__path.jpeg").and_return(false)
+        File.should_receive(:exist?).with("//file.png").and_return(true)
+        subject.should_receive(:convert_command).with(/-quality 75/)
+        subject.should_receive(:has_alpha?).with("//file.png").and_return(true)
+        subject.s_125_width(:quality => 75, :extension => "jpeg").should == "/S_125_WIDTH_extension_jpeg_q_75__path.png"
+      end
+
+    end
+  end
+
+end

spec/spec_helper.rb

@@ -0,0 +1,34 @@
+require 'rubygems'
+require 'rspec'
+
+class Hash
+  def symbolize_keys!
+    keys.each do |key|
+      self[(key.to_sym rescue key) || key] = delete(key)
+    end
+    self
+  end
+end
+
+class Paperclip
+  def self.options
+    {}
+  end
+  def self.log msg
+  end
+  class Attachment
+    def initialize
+    end
+    def path
+      "/path.png"
+    end
+    def url
+      "/url"
+    end
+    def original_filename
+      "/file.png"
+    end
+  end
+end
+
+require_relative '../lib/attachment_on_the_fly'