implement reading of cell comments

include/xlnt/cell/cell.hpp

@@ -435,6 +435,28 @@ public:
     /// </summary>
     std::string check_string(const std::string &to_check);
 
+    // comment
+
+    /// <summary>
+    /// Return true if this cell has a comment applied.
+    /// </summary>
+    bool has_comment();
+
+    /// <summary>
+    /// Delete the comment applied to this cell if it exists.
+    /// </summary>
+    void clear_comment();
+
+    /// <summary>
+    /// Get the comment applied to this cell.
+    /// </summary>
+    comment comment();
+
+    /// <summary>
+    /// Apply the comment provided as the ony argument to the cell.
+    /// </summary>
+    void comment(const class comment &new_comment);
+
     // operators
 
     /// <summary>

include/xlnt/cell/comment.hpp

@@ -1,5 +1,4 @@
 // Copyright (c) 2014-2016 Thomas Fussell
-// Copyright (c) 2010-2015 openpyxl
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal
@@ -26,64 +25,54 @@
 #include <string>
 
 #include <xlnt/xlnt_config.hpp>
+#include <xlnt/cell/formatted_text.hpp>
 
 namespace xlnt {
 
-class cell;
-namespace detail {
-struct comment_impl;
-}
-
 /// <summary>
-/// A comment can be applied to a cell to provide extra information.
+/// A comment can be applied to a cell to provide extra information about its contents.
 /// </summary>
 class XLNT_API comment
 {
 public:
     /// <summary>
-    /// The default constructor makes an invalid comment without a parent cell.
+    /// Constructs a new blank comment.
     /// </summary>
     comment();
 
     /// <summary>
-    /// Constructs a comment applied to the given cell, parent, and with the comment
+    /// Constructs a new comment with the given text and author.
-    /// text and author set to the provided respective values.
+    /// </summary>
-    comment(cell parent, const std::string &text, const std::string &auth);
+    comment(const formatted_text &text, const std::string &author);
 
-    ~comment();
+    /// <summary>
+    /// Constructs a new comment with the given unformatted text and author.
+    /// </summary>
+    comment(const std::string &text, const std::string &author);
 
     /// <summary>
     /// Return the text that will be displayed for this comment.
     /// </summary>
-    std::string get_text() const;
+    formatted_text text() const;
+    
+    /// <summary>
+    /// Return the plain text that will be displayed for this comment without formatting information.
+    /// </summary>
+    std::string plain_text() const;
 
     /// <summary>
     /// Return the author of this comment.
     /// </summary>
-    std::string get_author() const;
+    std::string author() const;
 
     /// <summary>
-    /// True if the comments point to the same sell (false if
+    /// Return true if both comments are equivalent.
-    /// they are different cells but identical comments). Note
-    /// that a cell can only have one comment and a comment
-    /// can only be applied to one cell.
     /// </summary>
-    bool operator==(const comment &other) const;
+    friend bool operator==(const comment &left, const comment &right);
 
 private:
-    friend class cell; // cell needs access to private constructor
+    formatted_text text_;
-
+    std::string author_;
-    /// <summary>
-    /// Construct a comment from an implementation of a comment.
-    /// </summary>
-    comment(detail::comment_impl *d);
-
-    /// <summary>
-    /// Pointer to the implementation of this comment.
-    /// This allows comments to be passed by value while
-    /// retaining the ability to modify the parent cell.
-    /// </summary>
-    detail::comment_impl *d_;
 };
 
 } // namespace xlnt

include/xlnt/cell/formatted_text.hpp

@@ -25,27 +25,27 @@
 #include <string>
 #include <vector>
 
-#include <xlnt/xlnt_config.hpp> // for XLNT_API, XLNT_API
+#include <xlnt/xlnt_config.hpp>
 #include <xlnt/cell/text_run.hpp>
 
 namespace xlnt {
 
-class text_run;
+class XLNT_API formatted_text
-
-class XLNT_API text
 {
 public:
-	void clear();
+    void clear();
-	void set_plain_string(const std::string &s);
-	std::string get_plain_string() const;
-	std::vector<text_run> get_runs() const;
-	void add_run(const text_run &t);
-	void set_run(const std::vector<text_run> &parts);
 
-    bool operator==(const text &rhs) const;
+    void plain_text(const std::string &s);
+    std::string plain_text() const;
+
+    std::vector<text_run> runs() const;
+    void runs(const std::vector<text_run> &new_runs);
+    void add_run(const text_run &t);
+    
+    bool operator==(const formatted_text &rhs) const;
 
 private:
-	std::vector<text_run> runs_;
+    std::vector<text_run> runs_;
 };
 
 } // namespace xlnt

include/xlnt/cell/text_run.hpp

@@ -24,7 +24,8 @@
 
 #include <string>
 
-#include <xlnt/xlnt_config.hpp> // for XLNT_API, XLNT_API
+#include <xlnt/xlnt_config.hpp>
+#include <xlnt/styles/color.hpp>
 #include <xlnt/utils/optional.hpp>
 
 namespace xlnt {
@@ -45,8 +46,8 @@ public:
     void set_size(std::size_t size);
     
     bool has_color() const;
-    std::string get_color() const;
+    color get_color() const;
-    void set_color(const std::string &color);
+    void set_color(const color &new_color);
     
     bool has_font() const;
     std::string get_font() const;
@@ -60,14 +61,19 @@ public:
     std::string get_scheme() const;
     void set_scheme(const std::string &scheme);
 
+    bool bold_set() const;
+    bool is_bold() const;
+    void set_bold(bool bold);
+
 private:
 	std::string string_;
     
     optional<std::size_t> size_;
-    optional<std::string> color_;
+    optional<color> color_;
     optional<std::string> font_;
     optional<std::size_t> family_;
     optional<std::string> scheme_;
+    optional<bool> bold_;
 };
 
 } // namespace xlnt

include/xlnt/packaging/relationship.hpp

@@ -85,6 +85,7 @@ enum class XLNT_API relationship_type
 	single_cell_table_definitions,
 	styles,
 	table_definition,
+    vml_drawing,
 	volatile_dependencies,
 	worksheet,
 

include/xlnt/styles/color.hpp

@@ -133,6 +133,9 @@ public:
 
 	void set_tint(double tint);
     
+    bool operator==(const color &other) const;
+    bool operator!=(const color &other) const { return !(*this == other); }
+
 protected:
     std::string to_hash_string() const override;
 

include/xlnt/workbook/workbook.hpp

@@ -45,6 +45,7 @@ class drawing;
 class fill;
 class font;
 class format;
+class formatted_text;
 class manifest;
 class named_range;
 class number_format;
@@ -56,7 +57,6 @@ class range_reference;
 class relationship;
 class style;
 class style_serializer;
-class text;
 class theme;
 class workbook_view;
 class worksheet;
@@ -429,9 +429,9 @@ public:
 
     // shared strings
 
-    void add_shared_string(const text &shared, bool allow_duplicates=false);
+    void add_shared_string(const formatted_text &shared, bool allow_duplicates=false);
-    std::vector<text> &get_shared_strings();
+    std::vector<formatted_text> &get_shared_strings();
-    const std::vector<text> &get_shared_strings() const;
+    const std::vector<formatted_text> &get_shared_strings() const;
 
     // thumbnail
 
@@ -470,30 +470,30 @@ public:
     bool operator!=(const workbook &rhs) const;
 
 private:
-	friend class worksheet;
+    friend class worksheet;
-	friend class detail::xlsx_consumer;
+    friend class detail::xlsx_consumer;
-	friend class detail::xlsx_producer;
+    friend class detail::xlsx_producer;
 
-	workbook(detail::workbook_impl *impl);
+    workbook(detail::workbook_impl *impl);
 
-	detail::workbook_impl &impl();
+    detail::workbook_impl &impl();
 
-	const detail::workbook_impl &impl() const;
+    const detail::workbook_impl &impl() const;
 
     /// <summary>
     /// Apply the function "f" to every cell in every worksheet in this workbook.
     /// </summary>
     void apply_to_cells(std::function<void(cell)> f);
 
-	void register_app_properties_in_manifest();
+    void register_app_properties_in_manifest();
 
-	void register_core_properties_in_manifest();
+    void register_core_properties_in_manifest();
 
-	void register_shared_string_table_in_manifest();
+    void register_shared_string_table_in_manifest();
 
-	void register_stylesheet_in_manifest();
+    void register_stylesheet_in_manifest();
 
-	void register_theme_in_manifest();
+    void register_theme_in_manifest();
     
     /// <summary>
     /// An opaque pointer to a structure that holds all of the data relating to this workbook.

include/xlnt/xlnt.hpp

@@ -30,8 +30,8 @@
 #include <xlnt/cell/cell_reference.hpp>
 #include <xlnt/cell/cell_type.hpp>
 #include <xlnt/cell/comment.hpp>
+#include <xlnt/cell/formatted_text.hpp>
 #include <xlnt/cell/index_types.hpp>
-#include <xlnt/cell/text.hpp>
 #include <xlnt/cell/text_run.hpp>
 
 // packaging

source/cell/cell.cpp

@@ -28,7 +28,7 @@
 #include <xlnt/cell/cell.hpp>
 #include <xlnt/cell/cell_reference.hpp>
 #include <xlnt/cell/comment.hpp>
-#include <xlnt/cell/text.hpp>
+#include <xlnt/cell/formatted_text.hpp>
 #include <xlnt/packaging/relationship.hpp>
 #include <xlnt/styles/color.hpp>
 #include <xlnt/styles/format.hpp>
@@ -44,8 +44,6 @@
 #include <xlnt/worksheet/worksheet.hpp>
 
 #include <detail/cell_impl.hpp>
-#include <detail/comment_impl.hpp>
-
 
 namespace {
 
@@ -316,13 +314,13 @@ XLNT_API void cell::set_value(std::string s)
 	}
 	else
 	{
-		d_->type_ = type::string;
+	    d_->type_ = type::string;
-        d_->value_text_.set_plain_string(s);
+	    d_->value_text_.plain_text(s);
         
-        if (s.size() > 0)
+	    if (s.size() > 0)
-        {
+	    {
-            get_workbook().add_shared_string(d_->value_text_);
+		get_workbook().add_shared_string(d_->value_text_);
-        }
+	    }
 	}
 
 	if (get_workbook().get_guess_types())
@@ -332,17 +330,17 @@ XLNT_API void cell::set_value(std::string s)
 }
 
 template <>
-XLNT_API void cell::set_value(text t)
+XLNT_API void cell::set_value(formatted_text text)
 {
-    if (t.get_runs().size() == 1 && !t.get_runs().front().has_formatting())
+    if (text.runs().size() == 1 && !text.runs().front().has_formatting())
     {
-        set_value(t.get_plain_string());
+        set_value(text.plain_text());
     }
     else
     {
         d_->type_ = type::string;
-        d_->value_text_ = t;
+        d_->value_text_ = text;
-        get_workbook().add_shared_string(t);
+        get_workbook().add_shared_string(text);
     }
 }
 
@@ -518,7 +516,7 @@ void cell::set_error(const std::string &error)
         throw invalid_data_type();
     }
 
-    d_->value_text_.set_plain_string(error);
+    d_->value_text_.plain_text(error);
     d_->type_ = type::error;
 }
 
@@ -804,11 +802,11 @@ void cell::set_protection(const xlnt::protection &protection_)
 template <>
 XLNT_API std::string cell::get_value() const
 {
-    return d_->value_text_.get_plain_string();
+    return d_->value_text_.plain_text();
 }
 
 template <>
-XLNT_API text cell::get_value() const
+XLNT_API formatted_text cell::get_value() const
 {
     return d_->value_text_;
 }
@@ -1026,4 +1024,31 @@ bool cell::has_hyperlink() const
 	return d_->hyperlink_;
 }
 
+// comment
+
+bool cell::has_comment()
+{
+    return (bool)d_->comment_;
+}
+
+void cell::clear_comment()
+{
+    d_->comment_.clear();
+}
+
+comment cell::comment()
+{
+    if (!has_comment())
+    {
+	throw xlnt::exception("cell has no comment");
+    }
+
+    return d_->comment_.get();
+}
+
+void cell::comment(const class comment &new_comment)
+{
+    d_->comment_.set(new_comment);
+}
+
 } // namespace xlnt

source/cell/comment.cpp

@@ -21,44 +21,45 @@
 //
 // @license: http://www.opensource.org/licenses/mit-license.php
 // @author: see AUTHORS file
-#include <xlnt/cell/cell.hpp>
 #include <xlnt/cell/comment.hpp>
 
-#include "detail/comment_impl.hpp"
-
 namespace xlnt {
 
-comment::comment(detail::comment_impl *d) : d_(d)
+comment::comment() : comment("", "")
 {
 }
 
-comment::comment(cell parent, const std::string &text, const std::string &author) : d_(nullptr)
+comment::comment(const formatted_text &text, const std::string &author)
-{
+    : text_(text),
-    /*d_->text_ = text;
+      author_(author)
-    d_->author_ = author;*/
-}
-
-comment::comment() : d_(nullptr)
 {
 }
 
-comment::~comment()
+comment::comment(const std::string &text, const std::string &author)
+    : text_(),
+      author_(author)
 {
+    text_.plain_text(text);
 }
 
-std::string comment::get_author() const
+formatted_text comment::text() const
 {
-    return d_->author_;
+    return text_;
 }
 
-std::string comment::get_text() const
+std::string comment::plain_text() const
 {
-    return d_->text_;
+    return text_.plain_text();
 }
 
-bool comment::operator==(const xlnt::comment &other) const
+std::string comment::author() const
 {
-    return d_ == other.d_;
+    return author_;
+}
+
+bool operator==(const comment &left, const comment &right)
+{
+    return left.text_ == right.text_ && left.author_ == right.author_;
 }
 
 } // namespace xlnt

source/cell/formatted_text.cpp

@@ -21,46 +21,41 @@
 //
 // @license: http://www.opensource.org/licenses/mit-license.php
 // @author: see AUTHORS file
+#include <numeric>
 
-#include <xlnt/cell/text.hpp>
+#include <xlnt/cell/formatted_text.hpp>
 #include <xlnt/cell/text_run.hpp>
 
 namespace xlnt {
 
-void text::clear()
+void formatted_text::clear()
 {
-	runs_.clear();
+    runs_.clear();
 }
 
-void text::set_plain_string(const std::string &s)
+void formatted_text::plain_text(const std::string &s)
 {
-	clear();
+    clear();
-	add_run(text_run(s));
+    add_run(text_run(s));
 }
 
-std::string text::get_plain_string() const
+std::string formatted_text::plain_text() const
 {
-	std::string plain_string;
+    return std::accumulate(runs_.begin(), runs_.end(), std::string(),
-
+        [](const std::string &a, const text_run &run) { return a + run.get_string(); });
-	for (const auto &run : runs_)
-	{
-		plain_string.append(run.get_string());
-	}
-
-	return plain_string;
 }
 
-std::vector<text_run> text::get_runs() const
+std::vector<text_run> formatted_text::runs() const
 {
-	return runs_;
+    return runs_;
 }
 
-void text::add_run(const text_run &t)
+void formatted_text::add_run(const text_run &t)
 {
-	runs_.push_back(t);
+    runs_.push_back(t);
 }
 
-bool text::operator==(const text &rhs) const
+bool formatted_text::operator==(const formatted_text &rhs) const
 {
     if (runs_.size() != rhs.runs_.size()) return false;
     

source/cell/tests/test_cell.hpp

@@ -612,4 +612,19 @@ public:
         TS_ASSERT(cell.has_hyperlink());
         TS_ASSERT_EQUALS(cell.get_hyperlink(), "http://example.com");
     }
+
+    void test_comment()
+    {
+	xlnt::workbook wb;
+	auto ws = wb.get_active_sheet();
+	auto cell = ws.get_cell("A1");
+	TS_ASSERT(!cell.has_comment());
+	TS_ASSERT_THROWS(cell.comment(), xlnt::exception);
+	cell.comment(xlnt::comment("comment", "author"));
+	TS_ASSERT(cell.has_comment());
+	TS_ASSERT_EQUALS(cell.comment(), xlnt::comment("comment", "author"));
+	cell.clear_comment();
+	TS_ASSERT(!cell.has_comment());
+	TS_ASSERT_THROWS(cell.comment(), xlnt::exception);
+    }
 };

source/cell/tests/test_text.hpp

@@ -7,13 +7,13 @@
 
 #include <xlnt/xlnt.hpp>
 
-class test_text : public CxxTest::TestSuite
+class test_formatted_text : public CxxTest::TestSuite
 {
 public:
     void test_operators()
     {
-        xlnt::text text1;
+        xlnt::formatted_text text1;
-        xlnt::text text2;
+        xlnt::formatted_text text2;
         TS_ASSERT_EQUALS(text1, text2);
         xlnt::text_run run_default;
         text1.add_run(run_default);
@@ -22,42 +22,42 @@ public:
         TS_ASSERT_EQUALS(text1, text2);
 
         xlnt::text_run run_formatted;
-        run_formatted.set_color("maroon");
+        run_formatted.set_color(xlnt::color::green());
         run_formatted.set_font("Cambria");
         run_formatted.set_scheme("ascheme");
         run_formatted.set_size(40);
         run_formatted.set_family(17);
 
-        xlnt::text text_formatted;
+        xlnt::formatted_text text_formatted;
         text_formatted.add_run(run_formatted);
 
         xlnt::text_run run_color_differs = run_formatted;
-        run_color_differs.set_color("mauve");
+        run_color_differs.set_color(xlnt::color::red());
-        xlnt::text text_color_differs;
+        xlnt::formatted_text text_color_differs;
         text_color_differs.add_run(run_color_differs);
         TS_ASSERT_DIFFERS(text_formatted, text_color_differs);
 
         xlnt::text_run run_font_differs = run_formatted;
         run_font_differs.set_font("Calibri");
-        xlnt::text text_font_differs;
+        xlnt::formatted_text text_font_differs;
         text_font_differs.add_run(run_font_differs);
         TS_ASSERT_DIFFERS(text_formatted, text_font_differs);
 
         xlnt::text_run run_scheme_differs = run_formatted;
         run_scheme_differs.set_scheme("bscheme");
-        xlnt::text text_scheme_differs;
+        xlnt::formatted_text text_scheme_differs;
         text_scheme_differs.add_run(run_scheme_differs);
         TS_ASSERT_DIFFERS(text_formatted, text_scheme_differs);
 
         xlnt::text_run run_size_differs = run_formatted;
         run_size_differs.set_size(41);
-        xlnt::text text_size_differs;
+        xlnt::formatted_text text_size_differs;
         text_size_differs.add_run(run_size_differs);
         TS_ASSERT_DIFFERS(text_formatted, text_size_differs);
 
         xlnt::text_run run_family_differs = run_formatted;
         run_family_differs.set_family(18);
-        xlnt::text text_family_differs;
+        xlnt::formatted_text text_family_differs;
         text_family_differs.add_run(run_family_differs);
         TS_ASSERT_DIFFERS(text_formatted, text_family_differs);
     }

source/cell/text_run.cpp

@@ -22,6 +22,7 @@
 // @author: see AUTHORS file
 
 #include <xlnt/cell/text_run.hpp>
+#include <xlnt/styles/color.hpp>
 
 namespace xlnt {
 
@@ -68,14 +69,14 @@ bool text_run::has_color() const
     return (bool)color_;
 }
 
-std::string text_run::get_color() const
+color text_run::get_color() const
 {
     return *color_;
 }
 
-void text_run::set_color(const std::string &color)
+void text_run::set_color(const color &new_color)
 {
-    color_ = color;
+    color_ = new_color;
 }
 
 bool text_run::has_font() const
@@ -123,4 +124,19 @@ void text_run::set_scheme(const std::string &scheme)
     scheme_ = scheme;
 }
 
+bool text_run::bold_set() const
+{
+    return (bool)bold_;
+}
+
+bool text_run::is_bold() const
+{
+    return *bold_;
+}
+
+void text_run::set_bold(bool bold)
+{
+    bold_ = bold;
+}
+
 } // namespace xlnt

source/detail/cell_impl.cpp

@@ -24,10 +24,19 @@
 #include <xlnt/worksheet/worksheet.hpp>
 
 #include "cell_impl.hpp"
-#include "comment_impl.hpp"
 
 namespace xlnt {
 namespace detail {
 
+cell_impl::cell_impl()
+    : type_(cell_type::null),
+      parent_(nullptr),
+      column_(1),
+      row_(1),
+      is_merged_(false),
+      value_numeric_(0)
+{
+}
+
 } // namespace detail
 } // namespace xlnt

source/detail/cell_impl.hpp

@@ -26,19 +26,20 @@
 #include <string>
 
 #include <xlnt/cell/cell_type.hpp>
+#include <xlnt/cell/comment.hpp>
+#include <xlnt/cell/formatted_text.hpp>
 #include <xlnt/cell/index_types.hpp>
-#include <xlnt/cell/text.hpp>
 #include <xlnt/utils/optional.hpp>
 
 namespace xlnt {
-
 namespace detail {
 
-struct comment_impl;
 struct worksheet_impl;
 
 struct cell_impl
 {
+    cell_impl();
+
     cell_type type_;
 
     worksheet_impl *parent_;
@@ -46,18 +47,16 @@ struct cell_impl
     column_t column_;
     row_t row_;
 
-	bool is_merged_;
+    bool is_merged_;
 
-    text value_text_;
+    formatted_text value_text_;
     long double value_numeric_;
 
     optional<std::string> formula_;
-
     optional<std::string> hyperlink_;
-
     optional<std::size_t> format_id_;
-
     optional<std::string> style_name_;
+    optional<comment> comment_;
 };
 
 } // namespace detail

source/detail/comment_impl.cpp

@@ -1,46 +0,0 @@
-// Copyright (c) 2014-2016 Thomas Fussell
-//
-// Permission is hereby granted, free of charge, to any person obtaining a copy
-// of this software and associated documentation files (the "Software"), to deal
-// in the Software without restriction, including without limitation the rights
-// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
-// copies of the Software, and to permit persons to whom the Software is
-// furnished to do so, subject to the following conditions:
-//
-// The above copyright notice and this permission notice shall be included in
-// all copies or substantial portions of the Software.
-//
-// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
-// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
-// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
-// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
-// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, WRISING FROM,
-// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
-// THE SOFTWARE
-//
-// @license: http://www.opensource.org/licenses/mit-license.php
-// @author: see AUTHORS file
-#include "comment_impl.hpp"
-
-namespace xlnt {
-namespace detail {
-
-comment_impl::comment_impl()
-{
-}
-
-comment_impl::comment_impl(const comment_impl &rhs)
-{
-    *this = rhs;
-}
-
-comment_impl &comment_impl::operator=(const xlnt::detail::comment_impl &rhs)
-{
-    text_ = rhs.text_;
-    author_ = rhs.author_;
-
-    return *this;
-}
-
-} // namespace detail
-} // namespace xlnt

source/detail/comment_impl.hpp

@@ -1,44 +0,0 @@
-// Copyright (c) 2014-2016 Thomas Fussell
-//
-// Permission is hereby granted, free of charge, to any person obtaining a copy
-// of this software and associated documentation files (the "Software"), to deal
-// in the Software without restriction, including without limitation the rights
-// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
-// copies of the Software, and to permit persons to whom the Software is
-// furnished to do so, subject to the following conditions:
-//
-// The above copyright notice and this permission notice shall be included in
-// all copies or substantial portions of the Software.
-//
-// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
-// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
-// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
-// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
-// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, WRISING FROM,
-// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
-// THE SOFTWARE
-//
-// @license: http://www.opensource.org/licenses/mit-license.php
-// @author: see AUTHORS file
-#pragma once
-
-#include <xlnt/cell/comment.hpp>
-
-namespace xlnt {
-namespace detail {
-
-struct cell_impl;
-
-struct comment_impl
-{
-    comment_impl();
-    comment_impl(cell_impl *parent, const std::string &text, const std::string &author);
-    comment_impl(const comment_impl &rhs);
-    comment_impl &operator=(const comment_impl &rhs);
-
-    std::string text_;
-    std::string author_;
-};
-
-} // namespace detail
-} // namespace xlnt

source/detail/custom_value_traits.cpp

@@ -49,6 +49,10 @@ std::string to_string(relationship::type t)
 		return "http://schemas.openxmlformats.org/officeDocument/2006/relationships/hyperlink";
 	case relationship::type::chartsheet:
 		return "http://schemas.openxmlformats.org/officeDocument/2006/relationships/chartsheet";
+	case relationship::type::comments:
+		return "http://schemas.openxmlformats.org/officeDocument/2006/relationships/comments";
+	case relationship::type::vml_drawing:
+		return "http://schemas.openxmlformats.org/officeDocument/2006/relationships/vmlDrawing";
 	default:
 		return default_case("?");
 	}

source/detail/custom_value_traits.hpp

@@ -124,6 +124,7 @@ struct value_traits<xlnt::relationship_type>
             relationship_type::theme,
             relationship_type::thumbnail,
             relationship_type::unknown,
+            relationship_type::vml_drawing,
             relationship_type::volatile_dependencies,
             relationship_type::worksheet
         };

source/detail/workbook_impl.hpp

@@ -179,7 +179,7 @@ struct workbook_impl
 
     std::size_t active_sheet_index_;
     std::list<worksheet_impl> worksheets_;
-    std::vector<text> shared_strings_;
+    std::vector<formatted_text> shared_strings_;
 
     bool guess_types_;
     bool data_only_;

source/detail/xlsx_consumer.cpp

@@ -21,6 +21,7 @@
 // @license: http://www.opensource.org/licenses/mit-license.php
 // @author: see AUTHORS file
 #include <cctype>
+#include <numeric> // for std::accumulate
 
 #include <detail/xlsx_consumer.hpp>
 #include <detail/constants.hpp>
@@ -532,7 +533,7 @@ void xlsx_consumer::read_workbook()
     static const auto xmlns_mx = constants::get_namespace("mx");
     static const auto xmlns_r = constants::get_namespace("r");
     static const auto xmlns_s = constants::get_namespace("worksheet");
-	static const auto xmlns_x15 = constants::get_namespace("x15");
+    static const auto xmlns_x15 = constants::get_namespace("x15");
     static const auto xmlns_x15ac = constants::get_namespace("x15ac");
 
     parser().next_expect(xml::parser::event_type::start_element, xmlns, "workbook");
@@ -844,14 +845,14 @@ void xlsx_consumer::read_shared_string_table()
 		parser().content(xml::content::complex);
         parser().next_expect(xml::parser::event_type::start_element);
         
-        text t;
+        formatted_text t;
         
         parser().attribute_map();
         
 		if (parser().name() == "t")
 		{
 			parser().next_expect(xml::parser::event_type::characters);
-			t.set_plain_string(parser().value());
+			t.plain_text(parser().value());
 		}
 		else if (parser().name() == "r") // possible multiple text entities.
 		{
@@ -884,7 +885,7 @@ void xlsx_consumer::read_shared_string_table()
                         }
                         else if (parser().qname() == xml::qname(xmlns, "color"))
                         {
-                            run.set_color(parser().attribute("rgb"));
+                            run.set_color(read_color(parser()));
                         }
                         else if (parser().qname() == xml::qname(xmlns, "family"))
                         {
@@ -1589,6 +1590,7 @@ void xlsx_consumer::read_worksheet(const std::string &rel_id)
 {
     static const auto xmlns = constants::get_namespace("worksheet");
     static const auto xmlns_mc = constants::get_namespace("mc");
+    static const auto xmlns_r = constants::get_namespace("r");
     static const auto xmlns_x14ac = constants::get_namespace("x14ac");
 
 	auto title = std::find_if(target_.d_->sheet_title_rel_id_map_.begin(),
@@ -1984,15 +1986,164 @@ void xlsx_consumer::read_worksheet(const std::string &rel_id)
             
             parser().next();
         }
+        else if (parser().qname() == xml::qname(xmlns, "legacyDrawing"))
+        {
+	    parser().attribute(xml::qname(xmlns_r, "id"));
+            parser().next_expect(xml::parser::event_type::end_element, xmlns, "legacyDrawing");
+	}
     }
     
     parser().next_expect(xml::parser::event_type::end_element, xmlns, "worksheet");
+
+    auto &manifest = target_.get_manifest();
+    const auto workbook_rel = manifest.get_relationship(path("/"), relationship::type::office_document);
+    const auto sheet_rel = manifest.get_relationship(workbook_rel.get_target().get_path(), rel_id);
+    path sheet_path(sheet_rel.get_source().get_path().parent().append(sheet_rel.get_target().get_path()));
+
+    for (const auto &rel : manifest.get_relationships(sheet_path))
+    {
+        path part_path(sheet_path.parent().append(rel.get_target().get_path()));
+        auto split_part_path = part_path.split();
+        auto part_path_iter = split_part_path.begin();
+        while (part_path_iter != split_part_path.end())
+        {
+            if (*part_path_iter == "..")
+            {
+                part_path_iter = split_part_path.erase(part_path_iter - 1, part_path_iter + 1);
+                continue;
+            }
+            
+            ++part_path_iter;
+        }
+        part_path = std::accumulate(split_part_path.begin(), split_part_path.end(), path(""),
+            [](const path &a, const std::string &b) { return a.append(b); });
+        std::istringstream parser_stream(source_.read(part_path));
+        auto receive = xml::parser::receive_default;
+        xml::parser parser(parser_stream, rel.get_target().get_path().string(), receive);
+        parser_ = &parser;
+
+        switch (rel.get_type())
+        {
+        case relationship::type::comments:
+            read_comments(ws);
+            break;
+
+        default:
+            break;
+	    }
+
+        parser_ = nullptr;
+    }
 }
 
 // Sheet Relationship Target Parts
 
-void xlsx_consumer::read_comments()
+void xlsx_consumer::read_comments(worksheet ws)
 {
+    static const auto xmlns = xlnt::constants::get_namespace("worksheet");
+    
+    std::vector<std::string> authors;
+    
+    parser().next_expect(xml::parser::event_type::start_element, xmlns, "comments");
+    parser().next_expect(xml::parser::event_type::start_element, xmlns, "authors");
+    
+    for (;;)
+    {
+        if (parser().peek() == xml::parser::event_type::end_element) break;
+
+        parser().next_expect(xml::parser::event_type::start_element, xmlns, "author");
+        parser().next_expect(xml::parser::event_type::characters);
+        authors.push_back(parser().value());
+        parser().next_expect(xml::parser::event_type::end_element, xmlns, "author");
+    }
+    
+    parser().next_expect(xml::parser::event_type::end_element, xmlns, "authors");
+    parser().next_expect(xml::parser::event_type::start_element, xmlns, "commentList");
+    
+    for (;;)
+    {
+        if (parser().peek() == xml::parser::event_type::end_element) break;
+
+        parser().next_expect(xml::parser::event_type::start_element, xmlns, "comment");
+
+        auto cell_ref = parser().attribute("ref");
+        auto author_id = parser().attribute<std::size_t>("authorId");
+        
+        parser().next_expect(xml::parser::event_type::start_element, xmlns, "text");
+
+        // todo: this is duplicated from shared strings
+        formatted_text text;
+
+        for (;;)
+        {
+            if (parser().peek() == xml::parser::event_type::end_element) break;
+            
+            parser().next_expect(xml::parser::event_type::start_element, xmlns, "r");
+            text_run run;
+            
+            for (;;)
+            {
+                if (parser().peek() == xml::parser::event_type::end_element) break;
+                
+                parser().next_expect(xml::parser::event_type::start_element);
+                
+                if (parser().name() == "t")
+                {
+                    parser().next_expect(xml::parser::event_type::characters);
+                    run.set_string(parser().value());
+                    parser().next_expect(xml::parser::event_type::end_element, xmlns, "t");
+                }
+                else if (parser().name() == "rPr")
+                {
+                    for (;;)
+                    {
+                        if (parser().peek() == xml::parser::event_type::end_element) break;
+                        
+                        parser().next_expect(xml::parser::event_type::start_element);
+
+                        if (parser().qname() == xml::qname(xmlns, "sz"))
+                        {
+                            run.set_size(string_to_size_t(parser().attribute("val")));
+                        }
+                        else if (parser().qname() == xml::qname(xmlns, "rFont"))
+                        {
+                            run.set_font(parser().attribute("val"));
+                        }
+                        else if (parser().qname() == xml::qname(xmlns, "color"))
+                        {
+                            run.set_color(read_color(parser()));
+                        }
+                        else if (parser().qname() == xml::qname(xmlns, "family"))
+                        {
+                            run.set_family(string_to_size_t(parser().attribute("val")));
+                        }
+                        else if (parser().qname() == xml::qname(xmlns, "scheme"))
+                        {
+                            run.set_scheme(parser().attribute("val"));
+                        }
+                        else if (parser().qname() == xml::qname(xmlns, "b"))
+                        {
+                            run.set_bold(true);
+                        }
+                        
+                        parser().next_expect(xml::parser::event_type::end_element);
+                    }
+                    
+                    parser().next_expect(xml::parser::event_type::end_element, xmlns, "rPr");
+                }
+            }
+            
+            text.add_run(run);
+            parser().next_expect(xml::parser::event_type::end_element, xmlns, "r");
+        }
+    
+        ws.get_cell(cell_ref).comment(comment(text, authors.at(author_id)));
+        parser().next_expect(xml::parser::event_type::end_element, xmlns, "text");
+        parser().next_expect(xml::parser::event_type::end_element, xmlns, "comment");
+    }
+    
+    parser().next_expect(xml::parser::event_type::end_element, xmlns, "commentList");
+    parser().next_expect(xml::parser::event_type::end_element, xmlns, "comments");
 }
 
 void xlsx_consumer::read_drawings()

source/detail/xlsx_consumer.hpp

@@ -37,6 +37,7 @@ namespace xlnt {
 class path;
 class relationship;
 class workbook;
+class worksheet;
 
 namespace detail {
 
@@ -200,7 +201,7 @@ private:
 	/// <summary>
 	/// 
 	/// </summary>
-	void read_comments();
+	void read_comments(worksheet ws);
 
 	/// <summary>
 	/// 

source/detail/xlsx_crypto.cpp

@@ -154,8 +154,8 @@ std::vector<std::uint8_t> decrypt_xlsx_standard(const std::vector<std::uint8_t>
 
 	auto header_length = read_int<std::uint32_t>(offset, encryption_info);
 	auto index_at_start = offset;
-	auto skip_flags = read_int<std::uint32_t>(offset, encryption_info);
+	/*auto skip_flags = */read_int<std::uint32_t>(offset, encryption_info);
-	auto size_extra = read_int<std::uint32_t>(offset, encryption_info);
+	/*auto size_extra = */read_int<std::uint32_t>(offset, encryption_info);
 	auto alg_id = read_int<std::uint32_t>(offset, encryption_info);
 
 	if (alg_id == 0 || alg_id == 0x0000660E || alg_id == 0x0000660F || alg_id == 0x00006610)

source/detail/xlsx_producer.cpp

@@ -655,10 +655,10 @@ void xlsx_producer::write_shared_string_table(const relationship &rel)
 
 	for (const auto &string : source_.get_shared_strings())
 	{
-		if (string.get_runs().size() == 1 && !string.get_runs().at(0).has_formatting())
+		if (string.runs().size() == 1 && !string.runs().at(0).has_formatting())
 		{
             serializer().start_element(xmlns, "si");
-            serializer().element(xmlns, "t", string.get_plain_string());
+            serializer().element(xmlns, "t", string.plain_text());
             serializer().end_element(xmlns, "si");
             
             continue;
@@ -666,7 +666,7 @@ void xlsx_producer::write_shared_string_table(const relationship &rel)
 
         serializer().start_element(xmlns, "si");
 
-        for (const auto &run : string.get_runs())
+        for (const auto &run : string.runs())
         {
             serializer().start_element(xmlns, "r");
 
@@ -684,7 +684,7 @@ void xlsx_producer::write_shared_string_table(const relationship &rel)
                 if (run.has_color())
                 {
                     serializer().start_element(xmlns, "color");
-                    serializer().attribute("val", run.get_color());
+                    write_color(run.get_color());
                     serializer().end_element(xmlns, "color");
                 }
 
@@ -1974,7 +1974,7 @@ void xlsx_producer::write_worksheet(const relationship &rel)
 
 					for (std::size_t i = 0; i < shared_strings.size(); i++)
 					{
-						if (shared_strings[i] == cell.get_value<text>())
+						if (shared_strings[i] == cell.get_value<formatted_text>())
 						{
 							match_index = static_cast<int>(i);
 							break;

source/styles/color.cpp

@@ -240,4 +240,20 @@ void color::assert_type(type t) const
 	}
 }
 
+bool color::operator==(const xlnt::color &other) const
+{
+    if (type_ != other.type_ || tint_ != other.tint_) return false;
+    switch(type_)
+    {
+        case type::auto_:
+        case type::indexed :
+            return indexed_.get_index() == other.indexed_.get_index();
+        case type::theme:
+            return theme_.get_index() == other.theme_.get_index();
+        case type::rgb:
+            return rgb_.get_hex_string() == other.rgb_.get_hex_string();
+    }
+    return false;
+}
+
 } // namespace xlnt

source/workbook/tests/test_consume_xlsx.hpp

@@ -34,4 +34,20 @@ public:
 		xlnt::workbook wb;
 		wb.load(path_helper::get_data_directory("17_encrypted_numbers.xlsx"), "secret");
 	}
+
+    void test_comments()
+    {
+	xlnt::workbook wb;
+	wb.load("data/18_basic_comments.xlsx");
+
+	auto sheet1 = wb[0];
+	TS_ASSERT_EQUALS(sheet1.get_cell("A1").get_value<std::string>(), "Sheet1!A1");
+	TS_ASSERT_EQUALS(sheet1.get_cell("A1").comment().plain_text(), "Sheet1 comment");
+	TS_ASSERT_EQUALS(sheet1.get_cell("A1").comment().author(), "Microsoft Office User");
+
+	auto sheet2 = wb[1];
+	TS_ASSERT_EQUALS(sheet2.get_cell("A1").get_value<std::string>(), "Sheet2!A1");
+	TS_ASSERT_EQUALS(sheet2.get_cell("A1").comment().plain_text(), "Sheet2 comment");
+	TS_ASSERT_EQUALS(sheet2.get_cell("A1").comment().author(), "Microsoft Office User");
+    }
 };

source/workbook/workbook.cpp

@@ -954,17 +954,17 @@ const manifest &workbook::get_manifest() const
     return d_->manifest_;
 }
 
-std::vector<text> &workbook::get_shared_strings()
+std::vector<formatted_text> &workbook::get_shared_strings()
 {
     return d_->shared_strings_;
 }
 
-const std::vector<text> &workbook::get_shared_strings() const
+const std::vector<formatted_text> &workbook::get_shared_strings() const
 {
     return d_->shared_strings_;
 }
 
-void workbook::add_shared_string(const text &shared, bool allow_duplicates)
+void workbook::add_shared_string(const formatted_text &shared, bool allow_duplicates)
 {
 	register_shared_string_table_in_manifest();