improve workbook interface

include/xlnt/workbook/workbook.hpp

@@ -34,8 +34,6 @@
 #include <xlnt/xlnt_config.hpp>
 #include <xlnt/packaging/relationship.hpp>
 
-namespace CxxTest { class TestSuite; }
-
 namespace xlnt {
 
 class alignment;
@@ -66,8 +64,6 @@ class worksheet;
 class worksheet_iterator;
 class zip_file;
 
-enum class encoding;
-
 namespace detail {
 struct workbook_impl;
 } // namespace detail
@@ -83,20 +79,34 @@ public:
     using reverse_iterator = std::reverse_iterator<iterator>;
     using const_reverse_iterator = std::reverse_iterator<const_iterator>;
 
-    static std::size_t index_from_ws_filename(const std::string &filename);
+    /// <summary>
-
+    /// Swap the data held in workbooks "left" and "right".
-    // constructors
+    /// </summary>
-    workbook();
-
-    workbook &operator=(workbook other);
-    workbook(workbook &&other);
-    workbook(const workbook &other);
-    
-    ~workbook();
-
     friend void swap(workbook &left, workbook &right);
 
-    worksheet get_active_sheet();
+    // constructors
+
+    /// <summary>
+    /// Create a workbook containing a single empty worksheet.
+    /// </summary>
+    workbook();
+
+    /// <summary>
+    /// Move construct this workbook from existing workbook "other".
+    /// </summary>
+    workbook(workbook &&other);
+
+    /// <summary>
+    /// Copy construct this workbook from existing workbook "other".
+    /// </summary>
+    workbook(const workbook &other);
+
+    /// <summary>
+    /// Destroy this workbook.
+    /// </summary>
+    ~workbook();
+
+    // general properties
 
     bool get_guess_types() const;
     void set_guess_types(bool guess);
@@ -107,31 +117,67 @@ public:
     bool get_read_only() const;
     void set_read_only(bool read_only);
 
-    // create
+    // add worksheets
+
     worksheet create_sheet();
     worksheet create_sheet(std::size_t index);
     worksheet create_sheet(const std::string &title);
     worksheet create_sheet(std::size_t index, const std::string &title);
     worksheet create_sheet(const std::string &title, const relationship &rel);
 
-    // add
+    void copy_sheet(worksheet worksheet);
-    void add_sheet(worksheet worksheet);
+    void copy_sheet(worksheet worksheet, std::size_t index);
-    void add_sheet(worksheet worksheet, std::size_t index);
+
+    // get worksheets
+
+    /// <summary>
+    /// Returns the worksheet that was most recently accessed.
+    /// This is also the sheet that will be shown when the workbook is opened
+    /// in the spreadsheet editor program.
+    /// </summary>
+    worksheet get_active_sheet();
+
+    /// <summary>
+    /// Return the worksheet with the given name.
+    /// This may throw an exception if the sheet isn't found.
+    /// Use workbook::contains(const std::string &) to make sure the sheet exists.
+    /// </summary>
+    worksheet get_sheet_by_name(const std::string &sheet_name);
+    
+    /// <summary>
+    /// Return the const worksheet with the given name.
+    /// This may throw an exception if the sheet isn't found.
+    /// Use workbook::contains(const std::string &) to make sure the sheet exists.
+    /// </summary>
+    const worksheet get_sheet_by_name(const std::string &sheet_name) const;
+
+    /// <summary>
+    /// Return the worksheet at the given index.
+    /// </summary>
+    worksheet get_sheet_by_index(std::size_t index);
+
+    /// <summary>
+    /// Return the const worksheet at the given index.
+    /// </summary>
+    const worksheet get_sheet_by_index(std::size_t index) const;
+
+    /// <summary>
+    /// Return true if this workbook contains a sheet with the given name.
+    /// </summary>
+    bool contains(const std::string &key) const;
+
+    /// <summary>
+    /// Return the index of the given worksheet.
+    /// The worksheet must be owned by this workbook.
+    /// </summary>
+    std::size_t get_index(worksheet worksheet);
+
+    // remove worksheets
 
-    // remove
     void remove_sheet(worksheet worksheet);
     void clear();
 
-    // container operations
+    // iterators
-    worksheet get_sheet_by_name(const std::string &sheet_name);
-    const worksheet get_sheet_by_name(const std::string &sheet_name) const;
-    worksheet get_sheet_by_index(std::size_t index);
-    const worksheet get_sheet_by_index(std::size_t index) const;
-    bool contains(const std::string &key) const;
-    int get_index(worksheet worksheet);
-
-    worksheet operator[](const std::string &name);
-    worksheet operator[](std::size_t index);
 
     iterator begin();
     iterator end();
@@ -160,6 +206,7 @@ public:
     const app_properties &get_app_properties() const;
 
     // named ranges
+
     std::vector<named_range> get_named_ranges() const;
     void create_named_range(const std::string &name, worksheet worksheet, const range_reference &reference);
     void create_named_range(const std::string &name, worksheet worksheet, const std::string &reference_string);
@@ -168,6 +215,7 @@ public:
     void remove_named_range(const std::string &name);
 
     // serialization
+
     bool save(std::vector<unsigned char> &data);
     bool save(const std::string &filename);
     bool load(const std::vector<unsigned char> &data);
@@ -175,29 +223,15 @@ public:
     bool load(std::istream &stream);
     bool load(zip_file &archive);
 
-    bool operator==(const workbook &rhs) const;
-
-    bool operator!=(const workbook &rhs) const
-    {
-        return !(*this == rhs);
-    }
-
-    bool operator==(std::nullptr_t) const;
-
-    bool operator!=(std::nullptr_t) const
-    {
-        return !(*this == std::nullptr_t{});
-    }
-
-    void create_relationship(const std::string &id, const std::string &target, relationship::type type);
-    relationship get_relationship(const std::string &id) const;
-    const std::vector<relationship> &get_relationships() const;
-
     void set_code_name(const std::string &code_name);
 
+    // theme
+
     bool has_loaded_theme() const;
     const theme &get_loaded_theme() const;
 
+    // formats
+    
     format &get_format(std::size_t format_index);
     const format &get_format(std::size_t format_index) const;
     std::size_t add_format(const format &new_format);
@@ -205,7 +239,8 @@ public:
     std::vector<format> &get_formats();
     const std::vector<format> &get_formats() const;
 
-    // Named Styles
+    // named styles
+
     bool has_style(const std::string &name) const;
     style &get_style(const std::string &name);
     const style &get_style(const std::string &name) const;
@@ -217,26 +252,83 @@ public:
     void clear_styles();
     const std::vector<style> &get_styles() const;
 
+    // manifest
+
     manifest &get_manifest();
     const manifest &get_manifest() const;
 
+    // relationships
+
+    void create_relationship(const std::string &id, const std::string &target, relationship::type type);
+    relationship get_relationship(const std::string &id) const;
+    const std::vector<relationship> &get_relationships() const;
+
     void create_root_relationship(const std::string &id, const std::string &target, relationship::type type);
     const std::vector<relationship> &get_root_relationships() const;
 
+    // shared strings
+
     void add_shared_string(const text &shared, bool allow_duplicates=false);
     std::vector<text> &get_shared_strings();
     const std::vector<text> &get_shared_strings() const;
     
+    // thumbnail
+
     void set_thumbnail(const std::vector<std::uint8_t> &thumbnail);
     const std::vector<std::uint8_t> &get_thumbnail() const;
 
+    // operators
+
+    /// <summary>
+    /// Set the contents of this workbook to be equal to those of "other".
+    /// Other is passed as value to allow for copy-swap idiom.
+    /// </summary>
+    workbook &operator=(workbook other);
+
+    /// <summary>
+    /// Return the worksheet with a title of "name".
+    /// </summary>
+    worksheet operator[](const std::string &name);
+
+    /// <summary>
+    /// Return the worksheet at "index".
+    /// </summary>
+    worksheet operator[](std::size_t index);
+
+    /// <summary>
+    /// Return true if this workbook internal implementation points to the same
+    /// memory as rhs's.
+    /// </summary>
+    bool operator==(const workbook &rhs) const;
+
+    /// <summary>
+    /// Return true if this workbook internal implementation doesn't point to the same
+    /// memory as rhs's.
+    /// </summary>
+    bool operator!=(const workbook &rhs) const;
+
 private:
     friend class excel_serializer;
     friend class worksheet;
 
+    /// <summary>
+    /// Helper function to calculate an index from a worksheet filename.
+    /// </summary>
+    static std::size_t index_from_ws_filename(const std::string &filename);
+
+    /// <summary>
+    /// Get the name of the next unused relationship.
+    /// </summary>
     std::string next_relationship_id() 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);
     
+    /// <summary>
+    /// An opaque pointer to a structure that holds all of the data relating to this workbook.
+    /// </summary>
     std::unique_ptr<detail::workbook_impl> d_;
 };
 

source/workbook/tests/test_read.hpp

@@ -29,7 +29,7 @@ public:
 
     void test_read_standard_workbook()
     {
-        TS_ASSERT_DIFFERS(standard_workbook(), nullptr);
+        TS_ASSERT_THROWS_NOTHING(standard_workbook());
     }
 
     void test_read_standard_workbook_from_fileobj()
@@ -58,11 +58,7 @@ public:
         auto path = path_helper::get_data_directory("/genuine/empty-no-string.xlsx");
         
         xlnt::workbook wb;
-        xlnt::excel_serializer serializer(wb);
+        TS_ASSERT_THROWS_NOTHING(wb.load(path));
-        
-        serializer.load_workbook(path);
-        
-        TS_ASSERT_DIFFERS(wb, nullptr);
     }
 
     void test_read_empty_file()
@@ -267,11 +263,7 @@ public:
         auto path = path_helper::get_data_directory("/genuine/libreoffice_nrt.xlsx");
         
         xlnt::workbook wb;
-        xlnt::excel_serializer serializer(wb);
+        TS_ASSERT_THROWS_NOTHING(wb.load(path));
-        
-        serializer.load_workbook(path);
-        
-        TS_ASSERT_DIFFERS(wb, nullptr);
     }
     
     void _test_read_complex_formulae()

source/workbook/tests/test_workbook.hpp

@@ -51,7 +51,7 @@ public:
         xlnt::workbook wb;
         auto new_sheet = wb.create_sheet();
         new_sheet.get_cell("A6").set_value(1.498);
-        wb.add_sheet(new_sheet);
+        wb.copy_sheet(new_sheet);
         TS_ASSERT(wb[1].compare(wb[2], false));
         wb.create_sheet().get_cell("A6").set_value(1.497);
         TS_ASSERT(!wb[1].compare(wb[3], false));
@@ -61,7 +61,7 @@ public:
     {
         xlnt::workbook wb1, wb2;
         auto new_sheet = wb1.get_active_sheet();
-        TS_ASSERT_THROWS(wb2.add_sheet(new_sheet), xlnt::value_error);
+        TS_ASSERT_THROWS(wb2.copy_sheet(new_sheet), xlnt::value_error);
         TS_ASSERT_THROWS(wb2.get_index(new_sheet), std::runtime_error);
     }
 
@@ -71,7 +71,7 @@ public:
         auto ws = wb.get_active_sheet();
         ws.get_cell("B3").set_value(2);
         ws.set_title("Active");
-        wb.add_sheet(ws, 0);
+        wb.copy_sheet(ws, 0);
         TS_ASSERT_EQUALS(wb.get_sheet_names().at(0), "Sheet");
         TS_ASSERT_EQUALS(wb.get_sheet_by_index(0).get_cell("B3").get_value<int>(), 2);
         TS_ASSERT_EQUALS(wb.get_sheet_names().at(1), "Active");

source/workbook/tests/test_write_workbook.hpp

@@ -91,8 +91,6 @@ public:
         
         xlnt::excel_serializer deserializer(new_wb);
         deserializer.load_virtual_workbook(wb_bytes);
-
-        TS_ASSERT(new_wb != nullptr);
     }
     
     void test_write_workbook_rels()

source/workbook/workbook.cpp

@@ -162,7 +162,7 @@ worksheet workbook::create_sheet()
     return worksheet(&d_->worksheets_.back());
 }
 
-void workbook::add_sheet(xlnt::worksheet worksheet)
+void workbook::copy_sheet(xlnt::worksheet worksheet)
 {
     if(worksheet.d_->parent_ != this) throw xlnt::value_error();
 
@@ -172,24 +172,27 @@ void workbook::add_sheet(xlnt::worksheet worksheet)
     *new_sheet.d_ = impl;
 }
 
-void workbook::add_sheet(xlnt::worksheet worksheet, std::size_t index)
+void workbook::copy_sheet(xlnt::worksheet worksheet, std::size_t index)
 {
-    add_sheet(worksheet);
+    copy_sheet(worksheet);
-    std::swap(d_->worksheets_[index], d_->worksheets_.back());
+
+    if (index != d_->worksheets_.size() - 1)
+    {
+		d_->worksheets_.insert(d_->worksheets_.begin() + index, d_->worksheets_.back());
+		d_->worksheets_.pop_back();
+    }
 }
 
-int workbook::get_index(xlnt::worksheet worksheet)
+std::size_t workbook::get_index(xlnt::worksheet worksheet)
 {
-    int i = 0;
+    auto match = std::find(begin(), end(), worksheet);
-    for (auto ws : *this)
+
+    if (match == end())
     {
-        if (worksheet == ws)
-        {
-            return i;
-        }
-        i++;
-    }
         throw std::runtime_error("worksheet isn't owned by this workbook");
+    }
+
+    return std::distance(begin(), match);
 }
 
 void workbook::create_named_range(const std::string &name, worksheet range_owner, const std::string &reference_string)
@@ -477,16 +480,16 @@ bool workbook::save(const std::string &filename)
     return true;
 }
 
-bool workbook::operator==(std::nullptr_t) const
-{
-    return d_.get() == nullptr;
-}
-
 bool workbook::operator==(const workbook &rhs) const
 {
     return d_.get() == rhs.d_.get();
 }
 
+bool workbook::operator!=(const workbook &rhs) const
+{
+    return d_.get() != rhs.d_.get();
+}
+
 const std::vector<relationship> &xlnt::workbook::get_relationships() const
 {
     return d_->relationships_;