Add Ability to Use Color Class

[slabounty]

Add the ability to set fill_color, stroke_color, etc. with something
other than a string. This class should have a method color_str that
returns a hex string representing the color.

Accept classes that are "closely related" to String and Array (via the checks for respond_to?(:to_str) and respond_to?(:to_ary).

Add tests for the above. I only saw integration type tests, so that is what I added. If I missed some tests that should be there, please let me know.

I should note that it seems like there should be a Color class in here somewhere and that the fill_color and stroke_color should create these rather than keeping the String/Array directly. However, given how little I know about the code base and design, it's entirely possible, that I'm completely mistaken.

[slabounty]

Closing for now due to 1.9.3 issues.

[slabounty]

OK ... fixed issues with stabby lambda and spaces ... which are apparently different between 1.9.3 and 2.0.0.

[practicingruby]

Something like this will work, but I don't want to rely on generic APIs like to_str, because a meaningful string representation has little to do with a meaningful color string representation.

I think I'd prefer something like to_rgb and to_cmyk, which would return either some sort of color struct, or an array (can't decide what's better).

For handling our base case, which is string representation for RGB, and array for CMYK, we'd just check for something like case color; when String; Prawn::Color::RGB.new(color); ...

I know this is a rough sketch, but feel free to implement it if it makes sense. If it doesn't, I will work on an alternative patch when I find time, and we can discuss further from there.

[practicingruby]

/cc @packetmonkey since this kind of stuff usually captures your interest.

[slabounty]

I'll take a look. My thought for is_str and is_ary was that these would (probably) be classes derived from String and Array respectively and should probably work as if you'd passed one of them in, but, I'm glad to remove that.

It looks like you're recommending we go ahead an implement a Color class here (I assume that's what the Prawn::Color::RGB.new(color), so I'll go ahead and take a look at adding one.

[practicingruby]

Subclassing Ruby core classes is both bad design and can lead to unexpected internal errors because of optimizations at the language level, so I don't want to encourage it. There's nothing about colors that make them natural to represent as a string or array, it's just an implementation detail.

With that in mind, a color class is indeed what we'll want here.

[packetmonkey]

(accidentally submitted incomplete)

If I where to swing at this problem, I would probably come up with something similar to below

class Prawn::RGBColor
  def initialize(value)
    @value = value
  end

  def to_pdf
    # return PDF serialization
  end
end

class Prawn::CMYKColor
  def initialize(value)
    @value = value
  end

  def to_pdf
        # return PDF serialization
  end
end

module Prawn
  def color(val)
    case val
    when Prawn::RGBColor
    when Prawn::CMYKColor
      return val
    when String return Prawn::RGBColor.new(val)
    when Array return Prawn::CMYKColor.new(val)
    else
      raise ArgumentError, "cannot handle color type #{val.type}"
    end
  end  
end

That way you can set up your own PDF Color externally however you like and just pass it in. We could also loosen it up to accept any object that responds to to_pdf so we can accept any pdf serializable object. This solution may take more refactoring than we would like and I think we could solve similar problems to the pull request #724 trying to add additional ways to set colors.

[practicingruby]

@packetmonkey This solves a different problem, though. A custom color class would most likely be used for dynamic determination of colors, not color types!

The change you suggested might be welcome at the PDF::Core level, though.

[slabounty]

Fair call on not encouraging bad behavior.

On question though ... right now (and one thing I kept in my implementation) was that if you did a
pdf.fill_color '00ff00

followed by a ...

 pdf.fill_color

you get back a string. Same with my example MyColor or an array.
Do we need to keep this or would returning something of a different type be acceptable? In other words, are breaking changes OK?

[practicingruby]

@slabounty: Returning a meaningful value wasn't ever documented anywhere, I don't think. So it's currently undefined behavior, and we can change it.

[slabounty]

@sandal - Simplifies things a bit. Seems like this is pretty doable. I'll do some work on it and update the pull request. Thanks for the direction.

[slabounty]

@sandal - OK so the renderer PDF::Core::Renderer in pdf-core initializes the fill and stroke colors with "000000" (it doesn't look like it actually uses these anywhere, just stores them). Did we want to try to push the new classes down to the pdf-core or handle these initializers a different way. Should we always store the fill/stroke colors in the graphics_state as strings or arrays? I'm not coming up with a clean way to handle the core.

[practicingruby]

@slabounty For right now it'd probably be OK to convert into whatever PDF::Core wants. A patch to PDF::Core would also be alright, but there are many undefined or poorly defined APIs lurking down there.

[slabounty]

@sandal - OK check this out. It's not perfect (quite possibly not even good), but I think it's the direction you'd wanted to head in. Right now, it's kind of at the same place as when I started (you can't pass in your own classes and have them work), but that'll be easy to address if we get the other pieces right.

[practicingruby]

@slabounty This hasn't been forgotten, it's just blocking on me. I think your patch proves the concept, but I'll want to change the interfaces a little bit, and won't know exactly how I want it until I drop down to the code level. I will play around with getting this integrated some time after the next release.