cleaning up comments for pre-release

12 files changed, 255 insertions, 46 deletions

diff --git a/lib/axlsx/drawing/vml_drawing.rb b/lib/axlsx/drawing/vml_drawing.rb
index 2ae1dc68..c8ea68a6 100644
--- a/lib/axlsx/drawing/vml_drawing.rb
+++ b/lib/axlsx/drawing/vml_drawing.rb
@@ -33,7 +33,7 @@ module Axlsx
   <v:path gradientshapeok="t" o:connecttype="rect"/>
  </v:shapetype>
 BAD_PROGRAMMER
-      @comments.comment_list.each { |comment| comment.vml_shape.to_xml_string str }
+      @comments.each { |comment| comment.vml_shape.to_xml_string str }
       str << "</xml>"
 
     end
diff --git a/lib/axlsx/drawing/vml_shape.rb b/lib/axlsx/drawing/vml_shape.rb
index eff39ae3..64bf4398 100644
--- a/lib/axlsx/drawing/vml_shape.rb
+++ b/lib/axlsx/drawing/vml_shape.rb
@@ -43,12 +43,7 @@ module Axlsx
     # @return [Integer]
     attr_reader :bottom_offset
 
-    # The id of this shape derrived from teh comment's worksheet index and the index of this comment
-    # @return [String]
-    attr_reader   :id
-
-    # Creates a new VmlShape object for the comment provided.
-    # @param [Comment] comment
+    # Creates a new VmlShape
     # @option options [Integer|String] left_column
     # @option options [Integer|String] left_offset
     # @option options [Integer|String] top_row
@@ -57,8 +52,7 @@ module Axlsx
     # @option options [Integer|String] right_offset
     # @option options [Integer|String] bottom_row
     # @option options [Integer|String] bottom_offset
-    def initialize(comment, options={})
-      @id = "_x0000_s#{comment.comments.worksheet.index+1}08#{comment.index+1}"
+    def initialize(options={})
       @row = @column = @left_column = @top_row = @right_column = @bottom_row = 0
       @left_offset = 15
       @top_offset = 2
@@ -70,6 +64,11 @@ module Axlsx
       yield self if block_given?
     end
 
+    # @see column
+    def column=(v); Axlsx::validate_integerish(v); @column = v.to_i end
+ 
+    # @see row
+    def row=(v); Axlsx::validate_integerish(v); @row = v.to_i end
     # @see left_column
     def left_column=(v); Axlsx::validate_integerish(v); @left_column = v.to_i end
 
@@ -100,7 +99,7 @@ module Axlsx
     def to_xml_string(str ='')
 str << <<SHAME_ON_YOU
 
-<v:shape id="#{id}" type="#_x0000_t202"
+<v:shape id="" type="#_x0000_t202"
 style='position:absolute;margin-left:104pt;margin-top:2pt;width:800px;height:27pt;z-index:1;mso-wrap-style:tight'
  fillcolor="#ffffa1 [80]" o:insetmode="auto">
 
diff --git a/lib/axlsx/package.rb b/lib/axlsx/package.rb
index 75f574d4..34249ce4 100644
--- a/lib/axlsx/package.rb
+++ b/lib/axlsx/package.rb
@@ -192,7 +192,7 @@ module Axlsx
       end
 
       workbook.comments.each do|comment|
-        if comment.comment_list.size > 0
+        if comment.size > 0
           @parts << { :entry => "xl/#{comment.pn}", :doc => comment.to_xml_string, :schema => SML_XSD }
           @parts << { :entry => "xl/#{comment.vml_drawing.pn}", :doc => comment.vml_drawing.to_xml_string, :schema => nil }
         end
@@ -255,7 +255,7 @@ module Axlsx
       end
 
       workbook.comments.each do |comment|
-        if comment.comment_list.size > 0
+        if comment.size > 0
         c_types << Axlsx::Override.new(:PartName => "/xl/#{comment.pn}",
                                        :ContentType => COMMENT_CT)
         end
diff --git a/lib/axlsx/util/constants.rb b/lib/axlsx/util/constants.rb
index 9ff0a119..7c8c000e 100644
--- a/lib/axlsx/util/constants.rb
+++ b/lib/axlsx/util/constants.rb
@@ -261,4 +261,7 @@ module Axlsx
 
   # error message for invalid angles
   ERR_ANGLE = "Angles must be a value between -90 and 90. You provided: %s"
+
+  # error message for non 'integerish' value
+  ERR_INTEGERISH = "You value must be, or be castable via to_i, an Integer. You provided %s"
 end
diff --git a/lib/axlsx/util/validators.rb b/lib/axlsx/util/validators.rb
index e5d2c79a..2f43a121 100644
--- a/lib/axlsx/util/validators.rb
+++ b/lib/axlsx/util/validators.rb
@@ -50,7 +50,7 @@ module Axlsx
   # @para, [Any] v the value to validate
   # @raise [ArgumentError] raised if the value cannot be converted to an integer
   def self.validate_integerish(v)
-    raise ArugumentError, (ERR_INTEGERISH % v.inspect) unless (v.to_i.is_a?(Integer))
+    raise ArgumentError, (ERR_INTEGERISH % v.inspect) unless (v.respond_to?(:to_i) && v.to_i.is_a?(Integer))
   end
 
   # Requires that the value is between -54000000 and 54000000
diff --git a/lib/axlsx/workbook/worksheet/comment.rb b/lib/axlsx/workbook/worksheet/comment.rb
index 708b6ac4..f3d19e13 100644
--- a/lib/axlsx/workbook/worksheet/comment.rb
+++ b/lib/axlsx/workbook/worksheet/comment.rb
@@ -7,10 +7,10 @@ module Axlsx
     # @return [String]
     attr_reader :text
 
-    # The index of the the author for this comment in the owning Comments object
+    # The author of this comment
     # @see Comments
-    # @return [Integer]
-    attr_reader :author_index
+    # @return [String]
+    attr_reader :author
 
     # The owning Comments object
     # @return [Comments]
@@ -44,7 +44,15 @@ module Axlsx
     # The index of this comment
     # @return [Integer]
     def index
-      @comments.comment_list.index(self)
+      @comments.index(self)
+    end
+
+    #
+    # The index of this author in a unique sorted list of all authors in
+    # the comment.
+    # @return [Integer]
+    def author_index
+      @comments.authors.index(author)
     end
 
     # @see ref
@@ -60,10 +68,9 @@ module Axlsx
       @text = v
     end
 
-    # @see author_index
-    def author_index=(v)
-      Axlsx::validate_unsigned_int(v)
-      @author_index = v
+    # @see author
+    def author=(v)
+      @author = v
     end
 
     # serialize the object
@@ -87,9 +94,8 @@ module Axlsx
     # initialize the vml shape based on this comment's ref/position in the worksheet.
     # by default, all columns are 5 columns wide and 5 rows high
     def initialize_vml_shape
-      ws = self.comments.worksheet
       pos = Axlsx::name_to_indices(ref)
-      @vml_shape = VmlShape.new(self, :row => pos[1], :column => pos[0]) do |vml|
+      @vml_shape = VmlShape.new(:row => pos[1], :column => pos[0]) do |vml|
         vml.left_column = vml.row + 1
         vml.right_column = vml.column + 4
         vml.top_row = vml.row
diff --git a/lib/axlsx/workbook/worksheet/comments.rb b/lib/axlsx/workbook/worksheet/comments.rb
index 9334af7b..f140f409 100644
--- a/lib/axlsx/workbook/worksheet/comments.rb
+++ b/lib/axlsx/workbook/worksheet/comments.rb
@@ -1,15 +1,7 @@
 # -*- coding: utf-8 -*-
 module Axlsx
 
-  class Comments
-
-    # a collection of the comment authors
-    # @return [SimpleTypedList]
-    attr_reader :authors
-
-    # a collection of comment objects
-    # @return [SimpleTypedList]
-    attr_reader :comment_list
+  class Comments < SimpleTypedList
 
     # the vml_drawing that holds the shapes for comments
     # @return [VmlDrawing]
@@ -35,9 +27,8 @@ module Axlsx
     # @param [Worksheet] worksheet The sheet that these comments belong to.
     def initialize(worksheet)
       raise ArgumentError, "you must provide a worksheet" unless worksheet.is_a?(Worksheet)
+      super(Comment)
       @worksheet = worksheet
-      @authors = SimpleTypedList.new String
-      @comment_list = SimpleTypedList.new Comment
       @vml_drawing = VmlDrawing.new(self)
     end
 
@@ -50,10 +41,13 @@ module Axlsx
       raise ArgumentError, "Comment require an author" unless options[:author]
       raise ArgumentError, "Comment requires text" unless options[:text]
       raise ArgumentError, "Comment requires ref" unless options[:ref]
-      options[:author_index] = @authors.index(options[:author]) || @authors << options[:author]
-      @comment_list << Comment.new(self, options)
-      yield @comment_list.last if block_given?
-      @comment_list.last
+      @list << Comment.new(self, options)
+      yield @list.last if block_given?
+      @list.last
+    end
+
+    def authors
+      @list.map { |comment| comment.author.to_s }.uniq.sort
     end
 
     # serialize the object
@@ -68,7 +62,7 @@ module Axlsx
       end
       str << '</authors>'
       str << '<commentList>'
-      comment_list.each do |comment|
+      @list.each do |comment|
         comment.to_xml_string str
       end
       str << '</commentList></comments>'
diff --git a/lib/axlsx/workbook/worksheet/worksheet.rb b/lib/axlsx/workbook/worksheet/worksheet.rb
index 8b868765..dfa7fbdc 100644
--- a/lib/axlsx/workbook/worksheet/worksheet.rb
+++ b/lib/axlsx/workbook/worksheet/worksheet.rb
@@ -488,7 +488,7 @@ module Axlsx
       @conditional_formattings.each do |cf|
         str.concat cf.to_xml_string
       end
-      str << '<legacyDrawing r:id="rId1"/>' if @comments.comment_list.size > 0
+      str << '<legacyDrawing r:id="rId1"/>' if @comments.size > 0
       str + '</worksheet>'
     end
 
@@ -500,9 +500,9 @@ module Axlsx
         r << Relationship.new(TABLE_R, "../#{table.pn}")
       end
 
-      r << Relationship.new(VML_DRAWING_R, "../#{@comments.vml_drawing.pn}") if @comments.comment_list.size > 0
-      r << Relationship.new(COMMENT_R, "../#{@comments.pn}") if @comments.comment_list.size > 0
-      r << Relationship.new(COMMENT_R_NULL, "NULL") if @comments.comment_list.size > 0
+      r << Relationship.new(VML_DRAWING_R, "../#{@comments.vml_drawing.pn}") if @comments.size > 0
+      r << Relationship.new(COMMENT_R, "../#{@comments.pn}") if @comments.size > 0
+      r << Relationship.new(COMMENT_R_NULL, "NULL") if @comments.size > 0
 
       r << Relationship.new(DRAWING_R, "../#{@drawing.pn}") if @drawing
       r
diff --git a/test/drawing/tc_vml_drawing.rb b/test/drawing/tc_vml_drawing.rb
index 741fc9e3..ac95a2f3 100644
--- a/test/drawing/tc_vml_drawing.rb
+++ b/test/drawing/tc_vml_drawing.rb
@@ -6,6 +6,7 @@ class TestVmlDrawing < Test::Unit::TestCase
     p = Axlsx::Package.new
     wb = p.workbook
     @ws = wb.add_worksheet
+    @ws.add_comment :ref => 'A1', :text => 'penut machine', :author => 'crank'
     @ws.add_comment :ref => 'C3', :text => 'rust bucket', :author => 'PO'
     @vml_drawing = @ws.comments.vml_drawing
   end
@@ -15,10 +16,10 @@ class TestVmlDrawing < Test::Unit::TestCase
   end
 
   def test_to_xml_string
-    str = '<?xml version="1.0" encoding="UTF-8"?>'
-    str << '<c:chartSpace xmlns:c="' << Axlsx::XML_NS_C << '">'
-    str << @vml_drawing.to_xml_string(0)
+    str = @vml_drawing.to_xml_string()
     doc = Nokogiri::XML(str)
+    assert_equal(doc.xpath("//v:shape").size, 2)
+    assert(doc.xpath("//o:idmap[@o:data='#{@ws.index+1}']"))
   end
 
 end
diff --git a/test/drawing/tc_vml_shape.rb b/test/drawing/tc_vml_shape.rb
new file mode 100644
index 00000000..621c6bdd
--- /dev/null
+++ b/test/drawing/tc_vml_shape.rb
@@ -0,0 +1,100 @@
+require 'tc_helper.rb'
+
+class TestVmlDrawing < Test::Unit::TestCase
+
+  def setup
+    p = Axlsx::Package.new
+    wb = p.workbook
+    @ws = wb.add_worksheet
+    @ws.add_comment :ref => 'A1', :text => 'penut machine', :author => 'crank'
+    @ws.add_comment :ref => 'C3', :text => 'rust bucket', :author => 'PO'
+    @comments = @ws.comments
+  end
+
+  def test_initialize
+    assert_raise(ArgumentError) { Axlsx::VmlDrawing.new }
+  end
+
+  def test_row
+    shape = @comments.first.vml_shape
+    assert_equal(shape.row, 0)
+    shape = @comments.last.vml_shape
+    assert_equal(shape.row, 2)
+  end
+
+  def test_column
+    shape = @comments.first.vml_shape
+    assert_equal(shape.column, 0)
+    shape = @comments.last.vml_shape
+    assert_equal(shape.column, 2)
+  end
+
+  def test_left_column
+    shape = @comments.first.vml_shape
+    shape.left_column = 3
+    assert(shape.left_column == 3)
+    assert_raise(ArgumentError) { shape.left_column = [] }
+  end
+
+  def test_left_offset
+    shape = @comments.first.vml_shape
+    shape.left_offset = 3
+    assert(shape.left_offset == 3)
+    assert_raise(ArgumentError) { shape.left_offset = [] }
+  end
+
+  def test_right_column
+    shape = @comments.first.vml_shape
+    shape.right_column = 3
+    assert(shape.right_column == 3)
+    assert_raise(ArgumentError) { shape.right_column = [] }
+  end
+
+  def test_right_offset
+    shape = @comments.first.vml_shape
+    shape.right_offset = 3
+    assert(shape.right_offset == 3)
+    assert_raise(ArgumentError) { shape.right_offset = [] }
+  end
+
+  def test_top_offset
+    shape = @comments.first.vml_shape
+    shape.top_offset = 3
+    assert(shape.top_offset == 3)
+    assert_raise(ArgumentError) { shape.top_offset = [] }
+  end
+
+  def test_bottom_offset
+    shape = @comments.first.vml_shape
+    shape.bottom_offset = 3
+    assert(shape.bottom_offset == 3)
+    assert_raise(ArgumentError) { shape.bottom_offset = [] }
+  end
+
+  def test_bottom_row
+    shape = @comments.first.vml_shape
+    shape.bottom_row = 3
+    assert(shape.bottom_row == 3)
+    assert_raise(ArgumentError) { shape.bottom_row = [] }
+  end
+
+  def test_top_row
+    shape = @comments.first.vml_shape
+    shape.top_row = 3
+    assert(shape.top_row == 3)
+    assert_raise(ArgumentError) { shape.top_row = [] }
+  end
+
+  def test_to_xml_string
+    str = @comments.vml_drawing.to_xml_string()
+    doc = Nokogiri::XML(str)
+    assert_equal(doc.xpath("//v:shape").size, 2)
+    @comments.each do |comment|
+      shape = comment.vml_shape
+      assert(doc.xpath("//v:shape/x:ClientData/x:Row[text()='#{shape.row}']").size == 1)
+      assert(doc.xpath("//v:shape/x:ClientData/x:Column[text()='#{shape.column}']").size == 1)
+      assert(doc.xpath("//v:shape/x:ClientData/x:Anchor[text()='#{shape.left_column}, #{shape.left_offset}, #{shape.top_row}, #{shape.top_offset}, #{shape.right_column}, #{shape.right_offset}, #{shape.bottom_row}, #{shape.bottom_offset}']").size == 1)
+    end
+  end
+
+end
diff --git a/test/workbook/worksheet/tc_comment.rb b/test/workbook/worksheet/tc_comment.rb
new file mode 100644
index 00000000..352f30a0
--- /dev/null
+++ b/test/workbook/worksheet/tc_comment.rb
@@ -0,0 +1,56 @@
+require 'tc_helper.rb'
+
+class TestComment < Test::Unit::TestCase
+  def setup
+    p = Axlsx::Package.new
+    wb = p.workbook
+    @ws = wb.add_worksheet
+    @c1 = @ws.add_comment :ref => 'A1', :text => 'penut machine', :author => 'crank'
+    @c2 = @ws.add_comment :ref => 'C3', :text => 'rust bucket', :author => 'PO'
+  end
+
+  def test_initailize
+    assert_raise(ArgumentError) { Axlsx::Comment.new }
+  end
+
+  def test_author
+    assert(@c1.author == 'crank')
+    assert(@c2.author == 'PO')
+  end
+
+  def test_text
+    assert(@c1.text == 'penut machine')
+    assert(@c2.text == 'rust bucket')
+  end
+
+  def test_author_index
+    assert_equal(@c1.author_index, 1)
+    assert_equal(@c2.author_index, 0)
+  end
+
+  def test_ref
+    assert(@c1.ref == 'A1')
+    assert(@c2.ref == 'C3')
+  end
+
+  def test_vml_shape
+    assert(@c1.vml_shape.is_a?(Axlsx::VmlShape))
+    assert(@c1.vml_shape.column == Axlsx::name_to_indices(@c1.ref)[0])
+    assert(@c1.vml_shape.row == Axlsx::name_to_indices(@c1.ref)[1])
+    assert(@c1.vml_shape.row == Axlsx::name_to_indices(@c1.ref)[1])
+    assert(@c1.vml_shape.left_column == Axlsx::name_to_indices(@c1.ref)[1]+1)
+    assert(@c1.vml_shape.top_row == Axlsx::name_to_indices(@c1.ref)[1])
+    assert(@c1.vml_shape.right_column == Axlsx::name_to_indices(@c1.ref)[0]+4)
+    assert(@c1.vml_shape.bottom_row == Axlsx::name_to_indices(@c1.ref)[1]+4)
+  end
+
+  def to_xml_string
+    doc = Nokogiri::XML(@c1.to_xml_string)
+    assert_equal(doc.xpath("//comment[@ref='#{@c1.ref}']").size, 1)
+    assert_equal(doc.xpath("//comment[@authorId='#{@c1.author_index.to}']").size, 1)
+    assert_equal(doc.xpath("//t[text()='#{@c1.author}']").size, 1)
+    assert_equal(doc.xpath("//t[text()='#{@c1.text}']").size, 1)
+  end
+
+end
+
diff --git a/test/workbook/worksheet/tc_comments.rb b/test/workbook/worksheet/tc_comments.rb
new file mode 100644
index 00000000..cbc267ad
--- /dev/null
+++ b/test/workbook/worksheet/tc_comments.rb
@@ -0,0 +1,50 @@
+require 'tc_helper.rb'
+
+class TestComments < Test::Unit::TestCase
+  def setup
+    p = Axlsx::Package.new
+    wb = p.workbook
+    @ws = wb.add_worksheet
+    @c1 = @ws.add_comment :ref => 'A1', :text => 'penut machine', :author => 'crank'
+    @c2 = @ws.add_comment :ref => 'C3', :text => 'rust bucket', :author => 'PO'
+  end
+
+  def test_initialize
+    assert_raise(ArgumentError) { Axlsx::Comments.new }
+    assert(@ws.comments.vml_drawing.is_a?(Axlsx::VmlDrawing))
+  end
+
+  def test_add_comment
+    assert_equal(@ws.comments.size, 2)
+    assert_raise(ArgumentError) { @ws.comments.add_comment() }
+    assert_raise(ArgumentError) { @ws.comments.add_comment(:text => 'Yes We Can', :ref => 'A1') }
+    assert_raise(ArgumentError) { @ws.comments.add_comment(:author => 'bob', :ref => 'A1') }
+    assert_raise(ArgumentError) { @ws.comments.add_comment(:author => 'bob', :text => 'Yes We Can')}
+    assert_nothing_raised { @ws.comments.add_comment(:author => 'bob', :text => 'Yes We Can', :ref => 'A1') }
+    assert_equal(@ws.comments.size, 3)
+  end
+  def test_authors
+    assert_equal(@ws.comments.authors.size, @ws.comments.size)
+    @ws.add_comment(:text => 'Yes We Can!', :author => :bob, :ref => 'F1')
+    assert_equal(@ws.comments.authors.size, 3)
+    @ws.add_comment(:text => 'Yes We Can!', :author => :bob, :ref => 'F1')
+    assert_equal(@ws.comments.authors.size, 3, 'only unique authors are returned') 
+  end
+  def test_pn
+    assert_equal(@ws.comments.pn, Axlsx::COMMENT_PN % (@ws.index+1).to_s)
+  end
+
+  def test_index
+    assert_equal(@ws.index, @ws.comments.index)
+  end
+  def test_to_xml_string
+    doc = Nokogiri::XML(@ws.comments.to_xml_string)
+    # puts doc.xpath("comments").to_xml
+    # TODO figure out why these xpath expressions dont work!
+    # assert(doc.xpath("//comments"))
+    # assert_equal(doc.xpath("//xmlns:author").size, @ws.comments.authors.size)
+    # assert_equal(doc.xpath("//comment").size, @ws.comments.size)
+  end
+end
+
+