Merge pull request #2 from xing/opsidao/new_jpeg_defaults

Allow to pass quality and resolution when including

.rubocop.yml

@@ -18,8 +18,8 @@ AllCops:
 
   # By default, the rails cops are not run. Override in project or home
   # directory .rubocop.yml files, or by giving the -R/--rails option.
-Rails:
-  Enabled: true
+  Rails:
+    Enabled: true
 
 # Indent private/protected/public as deep as method definitions
 AccessModifierIndentation:

README.md

@@ -6,6 +6,9 @@ images generated for their first page.
 Support is provided both for [Paperclip](https://github.com/thoughtbot/paperclip)
 and [CarrierWave](https://github.com/carrierwaveuploader/carrierwave).
 
+In both cases the JPEG quality and resolution are optional and will be set to 85%
+and 300dpi respectively when not provided.
+
 ## Paperclip Support
 
 To add a PDF cover style to your attachments you can do something like this:
@@ -15,7 +18,7 @@ class WithPaperclip < ActiveRecord::Base
   include PdfCover
 
   pdf_cover_attachment :pdf, styles: { pdf_cover: ['', :jpeg]},
-    convert_options: { all: '-quality 95' },
+    convert_options: { all: '-quality 95 -density 300' },
 
   validates_attachment_content_type :pdf, content_type: %w(application/pdf)
 end
@@ -38,7 +41,7 @@ class WithCarrierwaveUploader < CarrierWave::Uploader::Base
   storage :file
 
   version :image do
-    pdf_cover_attachment
+    pdf_cover_attachment quality: 95, resolution: 300
   end
 end
 ```

lib/paperclip/pdf_cover.rb

@@ -11,20 +11,33 @@ module Paperclip
     #   @attachment the Paperclip::Attachment itself
     class PdfCover < Processor
       QUALITY_CONVERT_OPTION_REGEX = /-quality\s+(?<quality>\d+)/
+      RESOLUTION_CONVERT_OPTION_REGEX = /-density\s+(?<resolution>\d+)/
 
       def make
-        ::PdfCover::Converter.new(@file, format: format, quality: jpeg_quality).converted_file
+        ::PdfCover::Converter.new(@file, converter_options).converted_file
+      end
+
+      def converter_options
+        format.merge(jpeg_quality).merge(jpeg_resolution)
       end
 
       def format
-        @options[:format].to_s
+        { format: @options[:format].to_s }
       end
 
       def jpeg_quality
-        return nil unless %w(jpeg jpg).include?(format)
+        extract_convert_option(:quality, QUALITY_CONVERT_OPTION_REGEX)
+      end
+
+      def jpeg_resolution
+        extract_convert_option(:resolution, RESOLUTION_CONVERT_OPTION_REGEX)
+      end
+
+      def extract_convert_option(key, regex)
+        match_data = regex.match(@options[:convert_options])
+        match = match_data && match_data[key]
 
-        match_data = QUALITY_CONVERT_OPTION_REGEX.match(@options[:convert_options])
-        match_data && match_data[:quality]
+        match ? { key => match } : {}
       end
     end
   end

lib/pdf_cover.rb

@@ -29,8 +29,8 @@ module CarrierWave
       #       pdf_cover_attachment
       #     end
       #   end
-      def pdf_cover_attachment
-        process :pdf_cover
+      def pdf_cover_attachment(options = {})
+        process pdf_cover: [options[:quality], options[:resolution]]
 
         define_method :full_filename do |for_file = model.logo.file|
           for_file.gsub(/pdf$/, "jpeg")
@@ -54,7 +54,7 @@ module Paperclip
       #     include PdfCover
       #
       #     pdf_cover_attachment :pdf, styles: { pdf_cover: ['', :jpeg]},
-      #       convert_options: { all: '-quality 95' }
+      #       convert_options: { all: '-quality 95 -density 300' }
       #
       #     # Note that you must set content type validation as required by Paperclip
       #     validates_attachment_content_type :pdf, content_type: %w(application/pdf)
@@ -89,9 +89,10 @@ def paperclip_defined?
     end
   end
 
-  # This is the method used by the PaperClip processor mechanism
-  def pdf_cover
-    converted_file = PdfCover::Converter.new(file).converted_file
+  # This is the method used by the CarrierWave processor mechanism
+  def pdf_cover(quality, resolution)
+    options = { quality: quality, resolution: resolution }
+    converted_file = PdfCover::Converter.new(file, options).converted_file
     FileUtils.cp(converted_file, current_path)
   end
 end

lib/pdf_cover/converter.rb

@@ -1,15 +1,25 @@
 module PdfCover
   class Converter
+    # NOTE: Update the generate_jpegs.sh script when changing these values
+    DEFAULT_FORMAT = "jpeg"
+    DEFAULT_QUALITY = 85
+    DEFAULT_RESOLUTION = 300
+
     class CommandNotFoundError < StandardError
       def initialize
         super("Could not run the `gs` command. Make sure that GhostScript is in your PATH.")
       end
     end
 
+    class InvalidOption < StandardError
+      def initialize(option_name, option_value)
+        super("Invalid option '#{option_name}' with value: '#{option_value}'")
+      end
+    end
+
     def initialize(file, options = {})
       @file = file
-      @format = options[:format] || "jpeg"
-      @quality = options[:quality] || 95
+      extract_options(options)
     end
 
     # @raises PdfCover::Converter::CommandNotFoundError if GhostScript is not found
@@ -18,7 +28,7 @@ def converted_file
         when :ok
           destination_file
         when :command_failed
-          @file # TODO: Why this? Shouldn't we just crash?
+          @file
         when :command_not_found
           fail CommandNotFoundError
       end
@@ -34,7 +44,7 @@ def build_parameters(source, device, destination)
       %W(-sOutputFile='#{destination}' -dNOPAUSE
          -sDEVICE='#{device}' -dJPEGQ=#{@quality}
          -dFirstPage=1 -dLastPage=1
-         -r300 -q '#{source}'
+         -r#{@resolution} -q '#{source}'
          -c quit
       ).join(" ")
     end
@@ -64,6 +74,16 @@ def execute_command(command)
       Kernel.system(command)
     end
 
+    def extract_options(options)
+      @format = options.fetch(:format, DEFAULT_FORMAT)
+      @quality = options.fetch(:quality, DEFAULT_QUALITY).to_i
+      @resolution = options.fetch(:resolution, DEFAULT_RESOLUTION).to_i
+
+      fail InvalidOption.new(:format, @format) unless %w(jpg jpeg png).include?(@format)
+      fail InvalidOption.new(:quality, @quality) unless @quality.between?(1, 100)
+      fail InvalidOption.new(:resolution, @resolution) unless @resolution > 1
+    end
+
     def failed_result(result)
       destination_file.close
 

lib/pdf_cover/version.rb

@@ -1,3 +1,3 @@
 module PdfCover
-  VERSION = "0.1.0".freeze
+  VERSION = "0.1.1".freeze
 end

spec/dummy/app/models/with_paperclip.rb

@@ -4,7 +4,9 @@ class WithPaperclip < ActiveRecord::Base
   include PdfCover
 
   pdf_cover_attachment :pdf, styles: { pdf_cover: ['', :jpg]},
-    convert_options: { all: '-quality 95' }
+    convert_options: {
+      all: "-quality #{PdfCover::Converter::DEFAULT_QUALITY} -density #{PdfCover::Converter::DEFAULT_RESOLUTION}"
+    }
 
   validates_attachment_content_type :pdf, content_type: %w(application/pdf)
 end

spec/dummy/app/uploaders/with_carrierwave_uploader.rb

@@ -4,6 +4,7 @@ class WithCarrierwaveUploader < CarrierWave::Uploader::Base
   storage :file
 
   version :image do
-    pdf_cover_attachment
+    pdf_cover_attachment quality: PdfCover::Converter::DEFAULT_QUALITY,
+                         resolution: PdfCover::Converter::DEFAULT_RESOLUTION
   end
 end

spec/generate_jpegs.sh

@@ -1,5 +1,10 @@
 #!/bin/bash
 
+# NOTE: Update the converter.rb file when changing these values
+DEFAULT_FORMAT="jpeg"
+DEFAULT_QUALITY=85
+DEFAULT_RESOLUTION=300
+
 DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )/test_pdfs"
 
 echo "Working directory $DIR"
@@ -14,5 +19,5 @@ for INPUT_FILE in $(cd $DIR && ls *.pdf)
 do
   OUTPUT_FILE="$DIR/${INPUT_FILE/.pdf/.jpg}"
   echo "Generating $OUTPUT_FILE from $INPUT_FILE"
-  gs -sOutputFile="$OUTPUT_FILE" -dNOPAUSE -sDEVICE="jpeg" -dJPEGQ=95 -dFirstPage=1 -dLastPage=1 -r300 -q "$DIR/$INPUT_FILE" -c quit
+  gs -sOutputFile="$OUTPUT_FILE" -dNOPAUSE -sDEVICE="$DEFAULT_FORMAT" -dJPEGQ=$DEFAULT_QUALITY -dFirstPage=1 -dLastPage=1 -r$DEFAULT_RESOLUTION -q "$DIR/$INPUT_FILE" -c quit
 done

spec/paperclip/with_paperclip_spec.rb

@@ -1,5 +1,4 @@
 require "spec_helper"
-require "RMagick"
 require File.expand_path("../../dummy/config/environment.rb", __FILE__)
 
 describe WithPaperclip do

spec/spec_helper.rb

@@ -11,6 +11,7 @@
 require "paperclip"
 require "paperclip/matchers"
 require "carrierwave"
+require "RMagick"
 
 # The gem itself
 require "pdf_cover"

spec/xing/pdf_cover/converter_spec.rb

@@ -1,5 +1,3 @@
-require "RMagick"
-
 require "spec_helper"
 require "pdf_cover/converter"
 
@@ -9,6 +7,68 @@
   let(:source_file) { double(File, path: "original_path") }
   let(:destination_file) { double(File, path: "destination_path") }
 
+  describe "#initialize" do
+    subject { described_class.new(source_file, options) }
+
+    context "parameter provided" do
+      let(:format) { "jpg" }
+      let(:quality) { "89" }
+      let(:options) do
+        {
+          format: format,
+          quality: quality,
+          resolution: resolution
+        }
+      end
+      let(:resolution) { "273" }
+
+      context "all parameters are ok" do
+        it "saves the parameters" do
+          expect(subject.instance_variable_get(:@format)).to eq(format)
+          expect(subject.instance_variable_get(:@quality)).to eq(quality.to_i)
+          expect(subject.instance_variable_get(:@resolution)).to eq(resolution.to_i)
+        end
+      end
+
+      context "format is invalid" do
+        let(:format) { "gif" }
+
+        it "raises an error" do
+          expect { subject }.to raise_error(PdfCover::Converter::InvalidOption)
+        end
+      end
+
+      context "quality is invalid" do
+        let(:quality) { -10 }
+
+        it "raises an error" do
+          expect { subject }.to raise_error(PdfCover::Converter::InvalidOption)
+        end
+      end
+
+      context "resolution is invalid" do
+        let(:resolution) { -10 }
+
+        it "raises an error" do
+          expect { subject }.to raise_error(PdfCover::Converter::InvalidOption)
+        end
+      end
+    end
+
+    context "parameters not provided" do
+      let(:options) { {} }
+
+      it "uses the default values" do
+        expect(subject.instance_variable_get(:@format))
+          .to eq(described_class::DEFAULT_FORMAT)
+        expect(subject.instance_variable_get(:@quality))
+          .to eq(described_class::DEFAULT_QUALITY)
+        expect(subject.instance_variable_get(:@resolution))
+          .to eq(described_class::DEFAULT_RESOLUTION)
+      end
+    end
+  end
+
   describe "#converted_file" do
     context "general logic" do
       before do