clean up comment handling, still a bug in assignment

include/xlnt/cell/cell.hpp

@@ -43,17 +43,21 @@ struct timedelta;
 
 namespace detail {    
 struct cell_impl;
+struct comment_impl;
 } // namespace detail
     
 class comment
 {
 public:
-    comment(const std::string &type, const std::string &value) : type_(type), value_(value) {}
+    comment(const std::string &text, const std::string &author);
-    std::string get_type() const { return type_; }
+    ~comment();
-    std::string get_value() const { return value_; }
+    std::string get_text() const;
+    std::string get_author() const;
+    
 private:
-    std::string type_;
+    friend class cell;
-    std::string value_;
+    comment(detail::comment_impl *d);
+    detail::comment_impl *d_;
 };
 
 /// <summary>
@@ -123,10 +127,9 @@ public:
     
     bool is_date() const;
     
-    //std::pair<int, int> get_anchor() const;
-    
     comment get_comment() const;
-    void set_comment(const comment &comment);
+    void set_comment(comment &comment);
+    void set_comment(comment &&comment);
     void clear_comment();
 
     std::string get_formula() const;

include/xlnt/worksheet/worksheet.hpp

@@ -296,8 +296,8 @@ public:
     bool has_auto_filter() const;
     
     // comments
-    void add_comment(const comment &c);
+    void increment_comments();
-    void remove_comment(const comment &c);
+    void decrement_comments();
     std::size_t get_comment_count() const;
 
     void reserve(std::size_t n);

source/cell.cpp

@@ -17,6 +17,44 @@ namespace xlnt {
 const xlnt::color xlnt::color::black(0);
 const xlnt::color xlnt::color::white(1);
 
+detail::comment_impl::comment_impl() : parent_worksheet_(nullptr)
+{
+}
+    
+detail::comment_impl::comment_impl(detail::worksheet_impl *ws, const cell_reference &ref, const std::string &text, const std::string &author) : parent_worksheet_(ws), parent_cell_(ref), text_(text), author_(author)
+{
+    
+}
+    
+comment::comment(const std::string &text, const std::string &author) : d_(new detail::comment_impl())
+{
+    d_->text_ = text;
+    d_->author_ = author;
+}
+
+comment::comment(detail::comment_impl *d) : d_(d)
+{
+}
+
+comment::~comment()
+{
+    if(d_->parent_worksheet_ == nullptr)
+    {
+        delete d_;
+        d_ = nullptr;
+    }
+}
+
+std::string comment::get_author() const
+{
+    return d_->author_;
+}
+
+std::string comment::get_text() const
+{
+    return d_->text_;
+}
+    
 const std::unordered_map<std::string, int> cell::ErrorCodes =
 {
     {"#NULL!", 0},
@@ -603,24 +641,62 @@ void cell::set_formula(const std::string &formula)
     d_->string_value = formula;
 }
 
-void cell::set_comment(const xlnt::comment &comment)
+void cell::set_comment(xlnt::comment &c)
 {
-    if(d_->comment_.get_value() == "")
+    if(c.d_->parent_worksheet_ != nullptr)
     {
-        get_parent().add_comment(comment);
+        if(c.d_->parent_cell_ != get_reference())
+        {
+            throw std::runtime_error("");
+        }
+    }
+    else
+    {
+        delete c.d_;
     }
     
-    d_->comment_ = comment;
+    if(d_->comment_.parent_worksheet_ == nullptr)
+    {
+        get_parent().increment_comments();
+    }
+    
+    d_->comment_.text_ = c.get_text();
+    d_->comment_.author_ = c.get_author();
+    d_->comment_.parent_worksheet_ = d_->parent_;
+    d_->comment_.parent_cell_ = get_reference();
+    
+    c.d_ = &d_->comment_;
+}
+
+void cell::set_comment(xlnt::comment &&c)
+{
+    if(c.d_->parent_worksheet_ != nullptr && c.d_->parent_cell_ != get_reference())
+    {
+        throw std::runtime_error("");
+    }
+    
+    if(d_->comment_.parent_worksheet_ == nullptr)
+    {
+        get_parent().increment_comments();
+    }
+    
+    d_->comment_.text_ = c.get_text();
+    d_->comment_.author_ = c.get_author();
+    d_->comment_.parent_worksheet_ = d_->parent_;
+    d_->comment_.parent_cell_ = get_reference();
 }
 
 void cell::clear_comment()
 {
-    if(d_->comment_.get_value() != "")
+    if(d_->comment_.parent_worksheet_ != nullptr)
     {
-        get_parent().remove_comment(d_->comment_);
+        get_parent().decrement_comments();
     }
     
-    d_->comment_ = comment("", "");
+    d_->comment_.parent_worksheet_ = nullptr;
+    d_->comment_.parent_cell_ = "A1";
+    d_->comment_.text_.clear();
+    d_->comment_.author_.clear();
 }
 
 void cell::set_error(const std::string &error)
@@ -646,7 +722,7 @@ worksheet cell::get_parent()
 
 comment cell::get_comment() const
 {
-    return d_->comment_;
+    return comment(&d_->comment_);
 }
 
 } // namespace xlnt

source/detail/cell_impl.cpp

@@ -4,15 +4,15 @@
 namespace xlnt {
 namespace detail {
 
-  cell_impl::cell_impl() : parent_(nullptr), type_(cell::type::null), column(0), row(0), style_(nullptr), merged(false), has_hyperlink_(false), comment_("", "")
+cell_impl::cell_impl() : parent_(nullptr), type_(cell::type::null), column(0), row(0), style_(nullptr), merged(false), has_hyperlink_(false), comment_(nullptr, "A1", "", "")
 {
 }
     
-cell_impl::cell_impl(worksheet_impl *parent, int column_index, int row_index) : parent_(parent), type_(cell::type::null), column(column_index), row(row_index), style_(nullptr), merged(false), has_hyperlink_(false), comment_("", "")
+cell_impl::cell_impl(worksheet_impl *parent, int column_index, int row_index) : parent_(parent), type_(cell::type::null), column(column_index), row(row_index), style_(nullptr), merged(false), has_hyperlink_(false), comment_(nullptr, "A1", "", "")
 {
 }
     
-cell_impl::cell_impl(const cell_impl &rhs) : comment_("", "")
+cell_impl::cell_impl(const cell_impl &rhs) : comment_(nullptr, "A1", "", "")
 {
     *this = rhs;
 }

source/detail/cell_impl.hpp

@@ -3,6 +3,7 @@
 #include "cell/cell.hpp"
 #include "common/types.hpp"
 #include "common/relationship.hpp"
+#include "comment_impl.hpp"
 
 namespace xlnt {
 
@@ -30,7 +31,7 @@ struct cell_impl
     bool merged;
     bool is_date_;
     bool has_hyperlink_;
-    comment comment_;
+    comment_impl comment_;
 };
     
 } // namespace detail

source/detail/comment_impl.hpp

@@ -0,0 +1,24 @@
+#pragma once
+
+#include "cell/cell_reference.hpp"
+
+namespace xlnt {
+namespace detail {
+
+struct worksheet_impl;
+
+struct comment_impl
+{
+    comment_impl();
+    comment_impl(worksheet_impl *parent, const cell_reference &ref, const std::string &text, const std::string &author);
+    comment_impl(const comment_impl &rhs);
+    comment_impl &operator=(const comment_impl &rhs);
+    
+    worksheet_impl *parent_worksheet_;
+    cell_reference parent_cell_;
+    std::string text_;
+    std::string author_;
+};
+
+} // namespace detail
+} // namespace xlnt

source/detail/worksheet_impl.hpp

@@ -3,6 +3,7 @@
 #include <vector>
 
 #include "cell_impl.hpp"
+#include "comment_impl.hpp"
 
 namespace xlnt {
 
@@ -13,7 +14,7 @@ namespace detail {
 struct worksheet_impl
 {
     worksheet_impl(workbook *parent_workbook, const std::string &title)
-    : parent_(parent_workbook), title_(title), freeze_panes_("A1")
+    : parent_(parent_workbook), title_(title), freeze_panes_("A1"), comment_count_(0)
     {
         page_margins_.set_left(0.75);
         page_margins_.set_right(0.75);
@@ -47,20 +48,22 @@ struct worksheet_impl
         page_margins_ = other.page_margins_;
         merged_cells_ = other.merged_cells_;
         named_ranges_ = other.named_ranges_;
+        comment_count_ = other.comment_count_;
+        header_footer_ = other.header_footer_;
     }
     
     workbook *parent_;
     std::unordered_map<row_t, row_properties> row_properties_;
     std::string title_;
     cell_reference freeze_panes_;
-    std::unordered_map<int, std::unordered_map<int, cell_impl>> cell_map_;
+    std::unordered_map<row_t, std::unordered_map<column_t, cell_impl>> cell_map_;
     std::vector<relationship> relationships_;
     page_setup page_setup_;
     range_reference auto_filter_;
     margins page_margins_;
     std::vector<range_reference> merged_cells_;
     std::unordered_map<std::string, range_reference> named_ranges_;
-    std::vector<comment> comments_;
+    std::size_t comment_count_;
     header_footer header_footer_;
 };
 

source/worksheet.cpp

@@ -171,7 +171,7 @@ cell worksheet::get_cell(const cell_reference &reference)
 {
     if(d_->cell_map_.find(reference.get_row_index()) == d_->cell_map_.end())
     {
-        d_->cell_map_[reference.get_row_index()] = std::unordered_map<int, detail::cell_impl>();
+        d_->cell_map_[reference.get_row_index()] = std::unordered_map<column_t, detail::cell_impl>();
     }
     
     auto &row = d_->cell_map_[reference.get_row_index()];
@@ -501,19 +501,19 @@ void worksheet::reserve(std::size_t n)
     d_->cell_map_.reserve(n);
 }
     
-void worksheet::add_comment(const xlnt::comment &c)
+void worksheet::increment_comments()
 {
-    d_->comments_.push_back(c);
+    d_->comment_count_++;
 }
 
-void worksheet::remove_comment(const xlnt::comment &c)
+void worksheet::decrement_comments()
 {
-    d_->comments_.pop_back();
+    d_->comment_count_--;
 }
 
 std::size_t worksheet::get_comment_count() const
 {
-    return d_->comments_.size();
+    return d_->comment_count_;
 }
 
 header_footer &worksheet::get_header_footer()