diff --git a/include/xlnt/packaging/relationship.hpp b/include/xlnt/packaging/relationship.hpp index 00d7e942..2dcbb4af 100644 --- a/include/xlnt/packaging/relationship.hpp +++ b/include/xlnt/packaging/relationship.hpp @@ -115,7 +115,7 @@ public: /// /// Returns a string of the form rId# that identifies the relationship. /// - std::string id() const; + const std::string& id() const; /// /// Returns the type of this relationship. @@ -130,12 +130,12 @@ public: /// /// Returns the URI of the package part this relationship points to. /// - uri source() const; + const uri &source() const; /// /// Returns the URI of the package part this relationship points to. /// - uri target() const; + const uri &target() const; /// /// Returns true if and only if rhs is equal to this relationship. diff --git a/include/xlnt/packaging/uri.hpp b/include/xlnt/packaging/uri.hpp index 7e945168..66d6358e 100644 --- a/include/xlnt/packaging/uri.hpp +++ b/include/xlnt/packaging/uri.hpp @@ -123,7 +123,7 @@ public: /// Returns the path of this URI. /// E.g. the path of http://example.com/document is "/document" /// - class path path() const; + const class path& path() const; /// /// Returns true if this URI has a non-null query string section. diff --git a/source/packaging/relationship.cpp b/source/packaging/relationship.cpp index 7788be5a..9c104e08 100644 --- a/source/packaging/relationship.cpp +++ b/source/packaging/relationship.cpp @@ -36,7 +36,7 @@ relationship::relationship( { } -std::string relationship::id() const +const std::string& relationship::id() const { return id_; } @@ -46,12 +46,12 @@ target_mode relationship::target_mode() const return mode_; } -uri relationship::source() const +const uri &relationship::source() const { return source_; } -uri relationship::target() const +const uri &relationship::target() const { return target_; } diff --git a/source/packaging/uri.cpp b/source/packaging/uri.cpp index 10b43777..850234a3 100644 --- a/source/packaging/uri.cpp +++ b/source/packaging/uri.cpp @@ -16,7 +16,7 @@ std::string uri::to_string() const return path_.string(); } -path uri::path() const +const path& uri::path() const { return path_; } diff --git a/source/workbook/workbook.cpp b/source/workbook/workbook.cpp index 55c460c4..c4739f03 100644 --- a/source/workbook/workbook.cpp +++ b/source/workbook/workbook.cpp @@ -28,16 +28,6 @@ #include #include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include #include #include #include @@ -61,12 +51,22 @@ #include #include #include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include namespace { using xlnt::detail::open_stream; -template +template std::vector keys(const std::vector> &container) { auto result = std::vector(); @@ -80,7 +80,7 @@ std::vector keys(const std::vector> &container) return result; } -template +template bool contains(const std::vector> &container, const T key) { for (const auto &iter : container) @@ -451,29 +451,29 @@ workbook workbook::empty() stylesheet.parent = &wb; auto default_border = border() - .side(border_side::bottom, border::border_property()) - .side(border_side::top, border::border_property()) - .side(border_side::start, border::border_property()) - .side(border_side::end, border::border_property()) - .side(border_side::diagonal, border::border_property()); + .side(border_side::bottom, border::border_property()) + .side(border_side::top, border::border_property()) + .side(border_side::start, border::border_property()) + .side(border_side::end, border::border_property()) + .side(border_side::diagonal, border::border_property()); wb.d_->stylesheet_.get().borders.push_back(default_border); auto default_fill = fill(pattern_fill() - .type(pattern_fill_type::none)); + .type(pattern_fill_type::none)); stylesheet.fills.push_back(default_fill); auto gray125_fill = pattern_fill() - .type(pattern_fill_type::gray125); + .type(pattern_fill_type::gray125); stylesheet.fills.push_back(gray125_fill); auto default_font = font() - .name("Calibri") - .size(12) - .scheme("minor") - .family(2) - .color(theme_color(1)); + .name("Calibri") + .size(12) + .scheme("minor") + .family(2) + .color(theme_color(1)); stylesheet.fonts.push_back(default_font); - wb.create_builtin_style(0) + wb.create_builtin_style(0) .border(default_border) .fill(default_fill) .font(default_font) @@ -550,7 +550,7 @@ void workbook::register_package_part(relationship_type type) void workbook::register_workbook_part(relationship_type type) { auto wb_rel = manifest().relationship(path("/"), relationship_type::office_document); - auto wb_path = manifest().canonicalize({ wb_rel }); + auto wb_path = manifest().canonicalize({wb_rel}); if (!manifest().has_relationship(wb_path, type)) { @@ -738,22 +738,35 @@ worksheet workbook::create_sheet() std::string title = "Sheet1"; int index = 1; + // make a unique sheet name. Sheet<1...n> while (contains(title)) { title = "Sheet" + std::to_string(++index); } - + // unique sheet id size_t sheet_id = 1; for (const auto ws : *this) { sheet_id = std::max(sheet_id, ws.id() + 1); } - std::string sheet_filename = "sheet" + std::to_string(sheet_id) + ".xml"; - d_->worksheets_.push_back(detail::worksheet_impl(this, sheet_id, title)); - + // unique sheet file name auto workbook_rel = d_->manifest_.relationship(path("/"), relationship_type::office_document); - uri relative_sheet_uri(path("worksheets").append(sheet_filename).string()); + auto workbook_files = d_->manifest_.relationships(workbook_rel.target().path()); + auto rel_vec_contains = [&workbook_files](const xlnt::path &new_file_id) { + return workbook_files.end() != std::find_if(workbook_files.begin(), workbook_files.end(), [&new_file_id](const xlnt::relationship &rel) { + return rel.target().path() == new_file_id; + }); + }; + + size_t file_id = sheet_id; + xlnt::path sheet_relative_path; + do + { + sheet_relative_path = path("worksheets").append("sheet" + std::to_string(file_id++) + ".xml"); + } while (rel_vec_contains(sheet_relative_path)); + + uri relative_sheet_uri(sheet_relative_path.string()); auto absolute_sheet_path = path("/xl").append(relative_sheet_uri.path()); d_->manifest_.register_override_type( absolute_sheet_path, "application/vnd.openxmlformats-officedocument.spreadsheetml.worksheet+xml"); @@ -1025,7 +1038,8 @@ void workbook::remove_sheet(worksheet ws) for (auto &title_rel_id_pair : d_->sheet_title_rel_id_map_) { title_rel_id_pair.second = rel_id_map.count(title_rel_id_pair.second) > 0 - ? rel_id_map[title_rel_id_pair.second] : title_rel_id_pair.second; + ? rel_id_map[title_rel_id_pair.second] + : title_rel_id_pair.second; } update_sheet_properties(); @@ -1390,7 +1404,7 @@ style workbook::create_style(const std::string &name) style workbook::create_builtin_style(const std::size_t builtin_id) { - return d_->stylesheet_.get().create_builtin_style(builtin_id); + return d_->stylesheet_.get().create_builtin_style(builtin_id); } style workbook::style(const std::string &name) @@ -1563,69 +1577,83 @@ void workbook::update_sheet_properties() } } -void workbook::reorder_relationships() +namespace { +// true if a sheet index is != worksheet relationship index +bool needs_reorder(const std::unordered_map &title_to_rels, + const std::vector &titles, + std::vector &relation_ids) { - const auto wb_rel = manifest().relationship(path("/"), relationship_type::office_document); - const auto wb_path = wb_rel.target().path(); - const auto relationships = manifest().relationships(wb_path); - std::unordered_map rel_map; - const auto titles = sheet_titles(); - const auto title_rel_id_map = d_->sheet_title_rel_id_map_; - bool needs_reorder = false; - - for (const auto &rel : relationships) + bool all_match = true; + for (std::size_t title_index = 0; title_index < titles.size(); ++title_index) { - rel_map[rel.id()] = rel; - - if (rel.type() == relationship_type::worksheet) + const auto &rel = title_to_rels.at(titles[title_index]); + relation_ids.push_back(rel); + const auto expected_rel_id = "rId" + std::to_string(title_index + 1); + if (rel != expected_rel_id) { - for (auto title_index = std::size_t(0); title_index < titles.size(); ++title_index) - { - const auto title = titles[title_index]; - - if (title_rel_id_map.at(title) == rel.id()) - { - const auto expected_rel_id = "rId" + std::to_string(title_index + 1); - - if (expected_rel_id != rel.id()) - { - needs_reorder = true; - } - - break; - } - } + all_match = false; } } + return !all_match; // if all are as expected, reorder not required +}; - if (!needs_reorder) +struct rel_id_sorter +{ + // true if lhs < rhs + bool operator()(const xlnt::relationship &lhs, const xlnt::relationship &rhs) + { + // format is rTd + if (lhs.id().size() < rhs.id().size()) // a number with more digits will be larger + { + return true; + } + return lhs.id() < rhs.id(); + } +}; +} // namespace + +void workbook::reorder_relationships() +{ + const auto titles = sheet_titles(); + // the relation ID corresponding to the title at the same index is copied into here + std::vector worksheet_rel_ids; + worksheet_rel_ids.reserve(titles.size()); + if (!needs_reorder(d_->sheet_title_rel_id_map_, titles, worksheet_rel_ids)) { return; } - - for (const auto &rel : relationships) + // copy of existing relations + const auto wb_rel_target = manifest().relationship(path("/"), relationship_type::office_document).target(); + auto rel_copy = manifest().relationships(wb_rel_target.path()); + std::sort(rel_copy.begin(), rel_copy.end(), rel_id_sorter{}); + // clear existing relations + for (const auto &rel : rel_copy) { - manifest().unregister_relationship(uri(wb_path.string()), rel.id()); + manifest().unregister_relationship(wb_rel_target, rel.id()); } - - for (auto index = std::size_t(0); index < rel_map.size(); ++index) + // create new relations + std::size_t index = 0; + auto new_id = [&index]() { return "rId" + std::to_string(++index); }; // ids start from 1 + // worksheets first + while (index < worksheet_rel_ids.size()) { - auto rel_id = "rId" + std::to_string(index + 1); - auto old_rel_id = std::string(); + auto rel_it = std::find_if(rel_copy.begin(), rel_copy.end(), + [&](const relationship &rel) { return rel.id() == worksheet_rel_ids[index]; }); - if (index < titles.size()) + std::string rel_id = new_id(); + d_->sheet_title_rel_id_map_.at(titles[index - 1]) = rel_id; // update title -> relation mapping + manifest().register_relationship(relationship(rel_id, rel_it->type(), + rel_it->source(), rel_it->target(), rel_it->target_mode())); + } + // then all the other relations in the same order they started (just new indices) + for (const auto &old_rel : rel_copy) + { + if (old_rel.type() == relationship_type::worksheet) { - auto title = titles[index]; - old_rel_id = title_rel_id_map.at(title); - d_->sheet_title_rel_id_map_[title] = rel_id; - } else { - old_rel_id = "rId" + std::to_string(index - titles.size() + 2); + continue; } - - auto old_rel = rel_map[old_rel_id]; - auto new_rel = relationship(rel_id, old_rel.type(), - old_rel.source(), old_rel.target(), old_rel.target_mode()); - manifest().register_relationship(new_rel); + manifest().register_relationship(relationship(new_id(), old_rel.type(), + old_rel.source(), old_rel.target(), old_rel.target_mode())); } } diff --git a/tests/data/Issue279_workbook_delete_rename.xlsx b/tests/data/Issue279_workbook_delete_rename.xlsx new file mode 100644 index 00000000..981af413 Binary files /dev/null and b/tests/data/Issue279_workbook_delete_rename.xlsx differ diff --git a/tests/helpers/xml_helper.hpp b/tests/helpers/xml_helper.hpp index 9c051536..9a90e9e3 100644 --- a/tests/helpers/xml_helper.hpp +++ b/tests/helpers/xml_helper.hpp @@ -3,17 +3,17 @@ #include #include +#include +#include #include #include #include -#include -#include class xml_helper { public: static bool compare_files(const std::string &left, - const std::string &right, const std::string &content_type) + const std::string &right, const std::string &content_type) { // content types are stored in unordered maps, too complicated to compare if (content_type == "[Content_Types].xml") @@ -34,7 +34,7 @@ public: } auto is_xml = (content_type.substr(0, 12) == "application/" - && content_type.substr(content_type.size() - 4) == "+xml") + && content_type.substr(content_type.size() - 4) == "+xml") || content_type == "application/xml" || content_type == "[Content_Types].xml" || content_type == "application/vnd.openxmlformats-officedocument.vmlDrawing"; @@ -63,8 +63,7 @@ public: bool difference = false; auto right_iter = right_parser.begin(); - auto is_whitespace = [](const std::string &v) - { + auto is_whitespace = [](const std::string &v) { return v.find_first_not_of("\n\r\t ") == std::string::npos; }; @@ -198,26 +197,26 @@ public: ++right_iter; } - if (difference && !suppress_debug_info) - { - std::cout << "documents don't match" << std::endl; + if (difference && !suppress_debug_info) + { + std::cout << "documents don't match" << std::endl; - std::cout << "left:" << std::endl; + std::cout << "left:" << std::endl; for (auto c : left) { std::cout << c << std::flush; } - std::cout << std::endl; + std::cout << std::endl; - std::cout << "right:" << std::endl; + std::cout << "right:" << std::endl; for (auto c : right) { std::cout << c << std::flush; } - std::cout << std::endl; - } + std::cout << std::endl; + } - return !difference; + return !difference; } static bool compare_relationships(const xlnt::manifest &left, @@ -271,27 +270,26 @@ public: return true; } - static bool xlsx_archives_match(const std::vector &left, + static bool xlsx_archives_match(const std::vector &left, const std::vector &right) - { + { xlnt::detail::vector_istreambuf left_buffer(left); std::istream left_stream(&left_buffer); xlnt::detail::izstream left_archive(left_stream); - const auto left_info = left_archive.files(); + const auto left_info = left_archive.files(); xlnt::detail::vector_istreambuf right_buffer(right); std::istream right_stream(&right_buffer); xlnt::detail::izstream right_archive(right_stream); - const auto right_info = right_archive.files(); + const auto right_info = right_archive.files(); auto difference_is_missing_calc_chain = false; if (std::abs(int(left_info.size()) - int(right_info.size())) == 1) { - auto is_calc_chain = [](const xlnt::path &p) - { + auto is_calc_chain = [](const xlnt::path &p) { return p.filename() == "calcChain.xml"; }; @@ -306,7 +304,7 @@ public: } } - if (left_info.size() != right_info.size() && ! difference_is_missing_calc_chain) + if (left_info.size() != right_info.size() && !difference_is_missing_calc_chain) { std::cout << "left has a different number of files than right" << std::endl; @@ -338,12 +336,41 @@ public: if (!compare_relationships(left_manifest, right_manifest)) { + std::cout << "relationship mismatch\n" + << "Left:\n"; + for (const auto &part : left_manifest.parts()) + { + std::cout << "-part: " << part.string() << '\n'; + auto rels = left_manifest.relationships(part); + for (auto &rel : rels) + { + std::cout << rel.id() << ':' + << static_cast(rel.type()) + << ':' << static_cast(rel.target_mode()) + << ':' << rel.source().path().string() + << ':' << rel.target().path().string() << '\n'; + } + } + std::cout << "\nRight:\n"; + for (const auto &part : right_manifest.parts()) + { + std::cout << "-part: " << part.string() << '\n'; + auto rels = right_manifest.relationships(part); + for (auto &rel : rels) + { + std::cout << rel.id() + << ':' << static_cast(rel.type()) + << ':' << static_cast(rel.target_mode()) + << ':' << rel.source().path().string() + << ':' << rel.target().path().string() << '\n'; + } + } return false; } - for (auto left_member : left_info) - { - if (!right_archive.has_file(left_member)) + for (auto left_member : left_info) + { + if (!right_archive.has_file(left_member)) { if (difference_is_missing_calc_chain) { @@ -357,32 +384,34 @@ public: } auto left_content_type = left_member.string() == "[Content_Types].xml" - ? "[Content_Types].xml" : left_manifest.content_type(left_member); + ? "[Content_Types].xml" + : left_manifest.content_type(left_member); auto right_content_type = left_member.string() == "[Content_Types].xml" - ? "[Content_Types].xml" : right_manifest.content_type(left_member); + ? "[Content_Types].xml" + : right_manifest.content_type(left_member); if (left_content_type != right_content_type) { std::cout << "content types differ: " - << left_member.string() - << " " - << left_content_type - << " " - << right_content_type - << std::endl; + << left_member.string() + << " " + << left_content_type + << " " + << right_content_type + << std::endl; match = false; break; } if (!compare_files(left_archive.read(left_member), - right_archive.read(left_member), left_content_type)) - { - std::cout << left_member.string() << std::endl; + right_archive.read(left_member), left_content_type)) + { + std::cout << left_member.string() << std::endl; match = false; break; - } - } + } + } - return match; - } + return match; + } }; diff --git a/tests/workbook/workbook_test_suite.cpp b/tests/workbook/workbook_test_suite.cpp index 86e8ae1e..b1a83df8 100644 --- a/tests/workbook/workbook_test_suite.cpp +++ b/tests/workbook/workbook_test_suite.cpp @@ -66,6 +66,7 @@ public: register_test(test_comparison); register_test(test_id_gen); register_test(test_load_file); + register_test(test_Issue279); } void test_active_sheet() @@ -460,11 +461,35 @@ public: // load with vector std::ifstream file_reader3(file.string(), std::ios::binary); file_reader3.unsetf(std::ios::skipws); - std::vector data(std::istream_iterator{file_reader3}, - std::istream_iterator()); + std::vector data(std::istream_iterator{file_reader3}, + std::istream_iterator()); xlnt::workbook wb_load5; wb_load5.load(data); xlnt_assert_equals(wb_path, wb_load5); } + + void test_Issue279() + { + xlnt::workbook wb(path_helper::test_file("Issue279_workbook_delete_rename.xlsx")); + while (wb.sheet_count() > 1) + { + if (wb[1].title() != "BOM") + { + wb.remove_sheet(wb[1]); + } + else + { + wb.remove_sheet(wb[0]); + } + } + // get sheet bom change title + auto ws1 = wb.sheet_by_index(0); + ws1.title("checkedBom"); + // report sheet + auto ws2 = wb.create_sheet(1); + ws2.title("REPORT"); + //save a copy file + wb.save("temp.xlsx"); + } }; static workbook_test_suite x; \ No newline at end of file