styles encapsulated, to delay calling of procs until context is useful and to…

styles encapsulated, to delay calling of procs until context is useful and to allow per-style processor lists

lib/paperclip.rb

@@ -34,6 +34,7 @@ require 'paperclip/processor'
 require 'paperclip/thumbnail'
 require 'paperclip/storage'
 require 'paperclip/interpolations'
+require 'paperclip/style'
 require 'paperclip/attachment'
 if defined? RAILS_ROOT
   Dir.glob(File.join(File.expand_path(RAILS_ROOT), "lib", "paperclip_processors", "*.rb")).each do |processor|

lib/paperclip/attachment.rb

@@ -10,6 +10,8 @@ module Paperclip
         :url               => "/system/:attachment/:id/:style/:filename",
         :path              => ":rails_root/public:url",
         :styles            => {},
+        :processors        => [:thumbnail],
+        :convert_options   => {},
         :default_url       => "/:attachment/:style/missing.png",
         :default_style     => :original,
         :validations       => [],
@@ -18,7 +20,7 @@ module Paperclip
       }
     end
 
-    attr_reader :name, :instance, :styles, :default_style, :convert_options, :queued_for_write, :options
+    attr_reader :name, :instance, :default_style, :convert_options, :queued_for_write, :whiny, :options
 
     # Creates an Attachment object. +name+ is the name of the attachment,
     # +instance+ is the ActiveRecord object instance it's attached to, and
@@ -34,14 +36,14 @@ module Paperclip
       @path              = options[:path]
       @path              = @path.call(self) if @path.is_a?(Proc)
       @styles            = options[:styles]
-      @styles            = @styles.call(self) if @styles.is_a?(Proc)
+      @normalized_styles = nil
       @default_url       = options[:default_url]
       @validations       = options[:validations]
       @default_style     = options[:default_style]
       @storage           = options[:storage]
       @whiny             = options[:whiny_thumbnails] || options[:whiny]
-      @convert_options   = options[:convert_options] || {}
-      @processors        = options[:processors] || [:thumbnail]
+      @convert_options   = options[:convert_options]
+      @processors        = options[:processors]
       @options           = options
       @queued_for_delete = []
       @queued_for_write  = {}
@@ -49,10 +51,23 @@ module Paperclip
       @validation_errors = nil
       @dirty             = false
 
-      normalize_style_definition
       initialize_storage
     end
     
+    def styles
+      unless @normalized_styles
+        @normalized_styles = {}
+        (@styles.respond_to?(:call) ? @styles.call(self) : @styles).each do |name, args|
+          @normalized_styles[name] = Paperclip::Style.new(name, args, self)
+        end
+      end
+      @normalized_styles
+    end
+    
+    def processors
+      @processors.respond_to?(:call) ? @processors.call(instance) : @processors
+    end
+
     # What gets called when you call instance.attachment = File. It clears
     # errors, assigns attributes, processes the file, and runs validations. It
     # also queues up the previous file for deletion, to be flushed away on
@@ -100,8 +115,8 @@ module Paperclip
     # security, however, for performance reasons.  set
     # include_updated_timestamp to false if you want to stop the attachment
     # update time appended to the url
-    def url style = default_style, include_updated_timestamp = true
-      url = original_filename.nil? ? interpolate(@default_url, style) : interpolate(@url, style)
+    def url style_name = default_style, include_updated_timestamp = true
+      url = original_filename.nil? ? interpolate(@default_url, style_name) : interpolate(@url, style_name)
       include_updated_timestamp && updated_at ? [url, updated_at].compact.join(url.include?("?") ? "&" : "?") : url
     end
 
@@ -109,13 +124,13 @@ module Paperclip
     # file is stored in the filesystem the path refers to the path of the file
     # on disk. If the file is stored in S3, the path is the "key" part of the
     # URL, and the :bucket option refers to the S3 bucket.
-    def path style = default_style
-      original_filename.nil? ? nil : interpolate(@path, style)
+    def path style_name = default_style
+      original_filename.nil? ? nil : interpolate(@path, style_name)
     end
 
     # Alias to +url+
-    def to_s style = nil
-      url(style)
+    def to_s style_name = nil
+      url(style_name)
     end
 
     # Returns true if there are no errors on this attachment.
@@ -314,35 +329,6 @@ module Paperclip
       end
     end
 
-    def normalize_style_definition #:nodoc:
-      @styles.each do |name, args|
-        unless args.is_a? Hash
-          dimensions, format = [args, nil].flatten[0..1]
-          format             = nil if format.blank?
-          @styles[name]      = {
-            :processors      => @processors,
-            :geometry        => dimensions,
-            :format          => format,
-            :whiny           => @whiny,
-            :convert_options => extra_options_for(name)
-          }
-        else
-          @styles[name] = {
-            :processors => @processors,
-            :whiny => @whiny,
-            :convert_options => extra_options_for(name)
-          }.merge(@styles[name])
-        end
-      end
-    end
-
-    def solidify_style_definitions #:nodoc:
-      @styles.each do |name, args|
-        @styles[name][:geometry] = @styles[name][:geometry].call(instance) if @styles[name][:geometry].respond_to?(:call)
-        @styles[name][:processors] = @styles[name][:processors].call(instance) if @styles[name][:processors].respond_to?(:call)
-      end
-    end
-
     def initialize_storage #:nodoc:
       @storage_module = Paperclip::Storage.const_get(@storage.to_s.capitalize)
       self.extend(@storage_module)
@@ -359,7 +345,6 @@ module Paperclip
 
     def post_process #:nodoc:
       return if @queued_for_write[:original].nil?
-      solidify_style_definitions
       return if fire_events(:before)
       post_process_styles
       return if fire_events(:after)
@@ -375,11 +360,11 @@ module Paperclip
     end
 
     def post_process_styles #:nodoc:
-      @styles.each do |name, args|
+      styles.each do |name, style|
         begin
-          raise RuntimeError.new("Style #{name} has no processors defined.") if args[:processors].blank?
-          @queued_for_write[name] = args[:processors].inject(@queued_for_write[:original]) do |file, processor|
-            Paperclip.processor(processor).make(file, args, self)
+          raise RuntimeError.new("Style #{name} has no processors defined.") if style.processors.blank?
+          @queued_for_write[name] = style.processors.inject(@queued_for_write[:original]) do |file, processor|
+            Paperclip.processor(processor).make(file, style.processor_options, self)
           end
         rescue PaperclipError => e
           log("An error was received while processing: #{e.inspect}")
@@ -388,13 +373,13 @@ module Paperclip
       end
     end
 
-    def interpolate pattern, style = default_style #:nodoc:
-      Paperclip::Interpolations.interpolate(pattern, self, style)
+    def interpolate pattern, style_name = default_style #:nodoc:
+      Paperclip::Interpolations.interpolate(pattern, self, style_name)
     end
 
     def queue_existing_for_delete #:nodoc:
       return unless file?
-      @queued_for_delete += [:original, *@styles.keys].uniq.map do |style|
+      @queued_for_delete += [:original, *styles.keys].uniq.map do |style|
         path(style) if exists?(style)
       end.compact
       instance_write(:file_name, nil)

lib/paperclip/interpolations.rb

@@ -34,30 +34,30 @@ module Paperclip
     end
 
     # Returns the filename, the same way as ":basename.:extension" would.
-    def filename attachment, style
-      "#{basename(attachment, style)}.#{extension(attachment, style)}"
+    def filename attachment, style_name
+      "#{basename(attachment, style_name)}.#{extension(attachment, style_name)}"
     end
 
     # Returns the interpolated URL. Will raise an error if the url itself
     # contains ":url" to prevent infinite recursion. This interpolation
     # is used in the default :path to ease default specifications.
-    def url attachment, style
+    def url attachment, style_name
       raise InfiniteInterpolationError if attachment.options[:url].include?(":url")
-      attachment.url(style, false)
+      attachment.url(style_name, false)
     end
 
     # Returns the timestamp as defined by the <attachment>_updated_at field
-    def timestamp attachment, style
+    def timestamp attachment, style_name
       attachment.instance_read(:updated_at).to_s
     end
 
     # Returns the RAILS_ROOT constant.
-    def rails_root attachment, style
+    def rails_root attachment, style_name
       RAILS_ROOT
     end
 
     # Returns the RAILS_ENV constant.
-    def rails_env attachment, style
+    def rails_env attachment, style_name
       RAILS_ENV
     end
 
@@ -65,44 +65,44 @@ module Paperclip
     # e.g. "users" for the User class.
     # NOTE: The arguments need to be optional, because some tools fetch
     # all class names. Calling #class will return the expected class.
-    def class attachment = nil, style = nil
-      return super() if attachment.nil? && style.nil?
+    def class attachment = nil, style_name = nil
+      return super() if attachment.nil? && style_name.nil?
       attachment.instance.class.to_s.underscore.pluralize
     end
 
     # Returns the basename of the file. e.g. "file" for "file.jpg"
-    def basename attachment, style
+    def basename attachment, style_name
       attachment.original_filename.gsub(/#{File.extname(attachment.original_filename)}$/, "")
     end
 
     # Returns the extension of the file. e.g. "jpg" for "file.jpg"
     # If the style has a format defined, it will return the format instead
     # of the actual extension.
-    def extension attachment, style 
-      ((style = attachment.styles[style]) && style[:format]) ||
+    def extension attachment, style_name 
+      ((style = attachment.styles[style_name]) && style[:format]) ||
         File.extname(attachment.original_filename).gsub(/^\.+/, "")
     end
 
     # Returns the id of the instance.
-    def id attachment, style
+    def id attachment, style_name
       attachment.instance.id
     end
 
     # Returns the id of the instance in a split path form. e.g. returns
     # 000/001/234 for an id of 1234.
-    def id_partition attachment, style
+    def id_partition attachment, style_name
       ("%09d" % attachment.instance.id).scan(/\d{3}/).join("/")
     end
 
     # Returns the pluralized form of the attachment name. e.g.
     # "avatars" for an attachment of :avatar
-    def attachment attachment, style
+    def attachment attachment, style_name
       attachment.name.to_s.downcase.pluralize
     end
 
     # Returns the style, or the default style if nil is supplied.
-    def style attachment, style
-      style || attachment.default_style
+    def style attachment, style_name
+      style_name || attachment.default_style
     end
   end
 end

lib/paperclip/storage.rb

@@ -20,9 +20,9 @@ module Paperclip
       def self.extended base
       end
       
-      def exists?(style = default_style)
+      def exists?(style_name = default_style)
         if original_filename
-          File.exist?(path(style))
+          File.exist?(path(style_name))
         else
           false
         end
@@ -30,17 +30,17 @@ module Paperclip
 
       # Returns representation of the data of the file assigned to the given
       # style, in the format most representative of the current storage.
-      def to_file style = default_style
-        @queued_for_write[style] || (File.new(path(style), 'rb') if exists?(style))
+      def to_file style_name = default_style
+        @queued_for_write[style_name] || (File.new(path(style_name), 'rb') if exists?(style_name))
       end
 
       def flush_writes #:nodoc:
-        @queued_for_write.each do |style, file|
+        @queued_for_write.each do |style_name, file|
           file.close
-          FileUtils.mkdir_p(File.dirname(path(style)))
-          log("saving #{path(style)}")
-          FileUtils.mv(file.path, path(style))
-          FileUtils.chmod(0644, path(style))
+          FileUtils.mkdir_p(File.dirname(path(style_name)))
+          log("saving #{path(style_name)}")
+          FileUtils.mv(file.path, path(style_name))
+          FileUtils.chmod(0644, path(style_name))
         end
         @queued_for_write = {}
       end

lib/paperclip/style.rb

+# encoding: utf-8
+module Paperclip
+  # The Style class holds the definition of a thumbnail style,  applying
+  # whatever processing is required to normalize the definition and delaying
+  # the evaluation of block parameters until useful context is available.
+  
+  class Style
+
+    attr_reader :name, :attachment, :format
+
+    # Creates a Style object. +name+ is the name of the attachment,
+    # +definition+ is the style definition from has_attached_file, which
+    # can be string, array or hash
+    def initialize name, definition, attachment
+      @name = name
+      @attachment = attachment
+      if definition.is_a? Hash
+        @geometry = definition.delete(:geometry)
+        @format = definition.delete(:format)
+        @processors = definition.delete(:processors)
+        @other_args = definition
+      else
+        @geometry, @format = [definition, nil].flatten[0..1]
+        @other_args = {}
+      end
+      @format  = nil if @format.blank?
+    end
+    
+    # retrieves from the attachment the processors defined in the has_attached_file call
+    # (which method (in the attachment) will call any supplied procs)
+    # There is an important change of interface here: a style rule can set its own processors
+    # by default we behave as before, though.
+    def processors
+      @processors || attachment.processors
+    end
+    
+    # retrieves from the attachment the whiny setting
+    def whiny
+      attachment.whiny
+    end
+    
+    # returns true if we're inclined to grumble
+    def whiny?
+      !!whiny
+    end
+    
+    def convert_options
+      attachment.send(:extra_options_for, name)
+    end
+
+    # returns the geometry string for this style
+    # if a proc has been supplied, we call it here
+    def geometry
+      @geometry.respond_to?(:call) ? @geometry.call(attachment.instance) : @geometry
+    end
+
+    # Supplies the hash of options that processors expect to receive as their second argument
+    # Arguments other than the standard geometry, format etc are just passed through from 
+    # initialization and any procs are called here, just before post-processing.
+    def processor_options
+      args = {}
+      @other_args.each do |k,v|
+        args[k] = v.respond_to?(:call) ? v.call(attachment) : v
+      end
+      [:processors, :geometry, :format, :whiny, :convert_options].each do |k|
+        (arg = send(k)) && args[k] = arg 
+      end
+      args
+    end
+
+    # Supports getting and setting style properties with hash notation to ensure backwards-compatibility
+    # eg. @attachment.styles[:large][:geometry]@ will still work
+    def [](key)
+      if [:name, :convert_options, :whiny, :processors, :geometry, :format].include?(key) 
+        send(key)
+      elsif defined? @other_args[key]
+        @other_args[key]
+      end
+    end
+    
+    def []=(key, value)
+      if [:name, :convert_options, :whiny, :processors, :geometry, :format].include?(key)
+        send("#{key}=".intern, value)
+      else
+        @other_args[key] = value
+      end
+    end
+    
+  end
+end
\ No newline at end of file

lib/paperclip/thumbnail.rb

@@ -12,6 +12,7 @@ module Paperclip
     # set, the options will be appended to the convert command upon image conversion 
     def initialize file, options = {}, attachment = nil
       super
+      
       geometry             = options[:geometry]
       @file                = file
       @crop                = geometry[-1,1] == '#'
@@ -24,6 +25,7 @@ module Paperclip
 
       @current_format      = File.extname(@file.path)
       @basename            = File.basename(@file.path, @current_format)
+      
     end
 
     # Returns true if the +target_geometry+ is meant to crop.

test/attachment_test.rb

@@ -162,11 +162,6 @@ class AttachmentTest < Test::Unit::TestCase
     should "report the correct options when sent #extra_options_for(:large)" do
       assert_equal "-do_stuff", @dummy.avatar.send(:extra_options_for, :large)
     end
-
-    before_should "call extra_options_for(:thumb/:large)" do
-      Paperclip::Attachment.any_instance.expects(:extra_options_for).with(:thumb)
-      Paperclip::Attachment.any_instance.expects(:extra_options_for).with(:large)
-    end
   end
 
   context "An attachment with :convert_options that is a proc" do
@@ -194,11 +189,6 @@ class AttachmentTest < Test::Unit::TestCase
     should "report the correct options when sent #extra_options_for(:large)" do
       assert_equal "-all", @dummy.avatar.send(:extra_options_for, :large)
     end
-
-    before_should "call extra_options_for(:thumb/:large)" do
-      Paperclip::Attachment.any_instance.expects(:extra_options_for).with(:thumb)
-      Paperclip::Attachment.any_instance.expects(:extra_options_for).with(:large)
-    end
   end
   
   context "An attachment with :path that is a proc" do
@@ -267,10 +257,6 @@ class AttachmentTest < Test::Unit::TestCase
         @attachment = Dummy.new.avatar
       end
 
-      should "not run the procs immediately" do
-        assert_kind_of Proc, @attachment.styles[:normal][:geometry]
-      end
-
       context "when assigned" do
         setup do
           @file = StringIO.new(".")
@@ -307,10 +293,6 @@ class AttachmentTest < Test::Unit::TestCase
       @attachment = Dummy.new.avatar
     end
 
-    should "not run the proc immediately" do
-      assert_kind_of Proc, @attachment.styles[:normal][:processors]
-    end
-
     context "when assigned" do
       setup do
         @attachment.assign(StringIO.new("."))
@@ -354,19 +336,22 @@ class AttachmentTest < Test::Unit::TestCase
       setup { @dummy.avatar = @file }
 
       before_should "call #make on all specified processors" do
+        Paperclip::Thumbnail.expects(:make).with(any_parameters).returns(@file)
+        Paperclip::Test.expects(:make).with(any_parameters).returns(@file)
+      end
+      
+      before_should "call #make with the right parameters passed as second argument" do
         expected_params = @style_params[:once].merge({:processors => [:thumbnail, :test], :whiny => true, :convert_options => ""})
-        Paperclip::Thumbnail.expects(:make).with(@file, expected_params, @dummy.avatar).returns(@file)
-        Paperclip::Test.expects(:make).with(@file, expected_params, @dummy.avatar).returns(@file)
+        Paperclip::Thumbnail.expects(:make).with(anything, expected_params, anything).returns(@file)
       end
       
       before_should "call #make with attachment passed as third argument" do
-        expected_params = @style_params[:once].merge({:processors => [:thumbnail, :test], :whiny => true, :convert_options => ""})
-        Paperclip::Test.expects(:make).with(@file, expected_params, @dummy.avatar).returns(@file)
+        Paperclip::Test.expects(:make).with(anything, anything, @dummy.avatar).returns(@file)
       end
     end
   end
 
-  context "An attachment with no processors defined" do
+  context "An attachment with styles but no processors defined" do
     setup do
       rebuild_model :processors => [], :styles => {:something => 1}
       @dummy = Dummy.new
@@ -377,6 +362,17 @@ class AttachmentTest < Test::Unit::TestCase
     end
   end
 
+  context "An attachment without styles and with no processors defined" do
+    setup do
+      rebuild_model :processors => [], :styles => {}
+      @dummy = Dummy.new
+      @file = StringIO.new("...")
+    end
+    should "not raise when assigned to" do
+      @dummy.avatar = @file
+    end
+  end
+
   context "Assigning an attachment with post_process hooks" do
     setup do
       rebuild_model :styles => { :something => "100x100#" }
@@ -498,6 +494,7 @@ class AttachmentTest < Test::Unit::TestCase
       FileUtils.rm_rf("tmp")
       rebuild_model
       @instance = Dummy.new
+      @instance.stubs(:id).returns 123
       @attachment = Paperclip::Attachment.new(:avatar, @instance)
       @file = File.new(File.join(File.dirname(__FILE__), "fixtures", "5k.png"), 'rb')
     end
@@ -606,6 +603,7 @@ class AttachmentTest < Test::Unit::TestCase
             should "commit the files to disk" do
               [:large, :medium, :small].each do |style|
                 io = @attachment.to_file(style)
+                # p "in commit to disk test, io is #{io.inspect} and @instance.id is #{@instance.id}"
                 assert File.exists?(io)
                 assert ! io.is_a?(::Tempfile)
                 io.close

test/style_test.rb

+# encoding: utf-8
+require 'test/helper'
+
+class StyleTest < Test::Unit::TestCase
+
+  context "A style rule" do
+    setup do
+      @attachment = attachment :path => ":basename.:extension",
+                               :styles => { :foo => {:geometry => "100x100#", :format => :png} }
+      @style = @attachment.styles[:foo]
+    end
+    
+    should "be held as a Style object" do
+      assert_kind_of Paperclip::Style, @style
+    end
+
+    should "get processors from the attachment definition" do
+      assert_equal [:thumbnail], @style.processors
+    end
+
+    should "have the right geometry" do
+      assert_equal "100x100#", @style.geometry
+    end
+    
+    should "be whiny if the attachment is" do
+      @attachment.expects(:whiny).returns(true)
+      assert @style.whiny?
+    end
+    
+    should "respond to hash notation" do
+      assert_equal [:thumbnail], @style[:processors]
+      assert_equal "100x100#", @style[:geometry]
+    end
+  end
+  
+  context "A style rule with properties supplied as procs" do
+    setup do
+      @attachment = attachment :path => ":basename.:extension",
+                               :whiny_thumbnails => true,
+                               :processors => lambda {|a| [:test]},
+                               :styles => {
+                                 :foo => lambda{|a| "300x300#"},
+                                 :bar => {
+                                   :geometry => lambda{|a| "300x300#"}
+                                 }
+                               }
+    end
+  
+    should "defer processing of procs until they are needed" do
+      assert_kind_of Proc, @attachment.styles[:foo].instance_variable_get("@geometry")
+      assert_kind_of Proc, @attachment.styles[:bar].instance_variable_get("@geometry")
+      assert_kind_of Proc, @attachment.instance_variable_get("@processors")
+    end
+  
+    should "call procs when they are needed" do
+      assert_equal "300x300#", @attachment.styles[:foo].geometry
+      assert_equal "300x300#", @attachment.styles[:bar].geometry
+      assert_equal [:test], @attachment.styles[:foo].processors
+      assert_equal [:test], @attachment.styles[:bar].processors
+    end
+  end
+
+  context "An attachment with style rules in various forms" do
+    setup do
+      @attachment = attachment :path => ":basename.:extension",
+                               :styles => { 
+                                 :aslist => ["100x100", :png],
+                                 :ashash => {:geometry => "100x100", :format => :png},
+                                 :asstring => "100x100"
+                                }
+    end
+    should "have the right number of styles" do
+      assert_kind_of Hash, @attachment.styles
+      assert_equal 3, @attachment.styles.size
+    end
+    
+    should "have styles as Style objects" do
+      [:aslist, :ashash, :aslist].each do |s|
+        assert_kind_of Paperclip::Style, @attachment.styles[s]
+      end
+    end
+    
+    should "have the right geometries" do
+      [:aslist, :ashash, :aslist].each do |s|
+        assert_equal @attachment.styles[s].geometry, "100x100"
+      end
+    end
+
+    should "have the right formats" do
+      assert_equal @attachment.styles[:aslist].format, :png
+      assert_equal @attachment.styles[:ashash].format, :png
+      assert_nil @attachment.styles[:asstring].format
+    end
+
+  end
+
+  context "An attachment with :convert_options" do
+    setup do
+      @attachment = attachment :path => ":basename.:extension",
+                               :styles => {:thumb => "100x100", :large => "400x400"},
+                               :convert_options => {:all => "-do_stuff", :thumb => "-thumbnailize"}
+      @style = @attachment.styles[:thumb]
+      @file = StringIO.new("...")
+      @file.stubs(:original_filename).returns("file.jpg")
+    end
+
+    before_should "not have called extra_options_for(:thumb/:large) on initialization" do
+      @attachment.expects(:extra_options_for).never
+    end
+    
+    should "call extra_options_for(:thumb/:large) when convert options are requested" do
+      @attachment.expects(:extra_options_for).with(:thumb)
+      @attachment.styles[:thumb].convert_options
+    end
+  end
+  
+  context "A style rule with its own :processors" do
+    setup do
+      @attachment = attachment :path => ":basename.:extension",
+                               :styles => { 
+                                 :foo => {
+                                   :geometry => "100x100#", 
+                                   :format => :png,
+                                   :processors => [:test]
+                                  } 
+                                },
+                               :processors => [:thumbnail]
+      @style = @attachment.styles[:foo]
+    end
+    
+    should "not get processors from the attachment" do
+      @attachment.expects(:processors).never
+      assert_not_equal [:thumbnail], @style.processors
+    end
+  
+    should "report its own processors" do
+      assert_equal [:test], @style.processors
+    end
+
+  end
+end

test/thumbnail_test.rb

@@ -201,6 +201,7 @@ class ThumbnailTest < Test::Unit::TestCase
 
     should "start with two pages with dimensions 612x792" do
       cmd = %Q[identify -format "%wx%h" "#{@file.path}"]
+      p "pdf page size test: cmd is #{cmd}"
       assert_equal "612x792"*2, `#{cmd}`.chomp
     end