From 322490b397836614e6859a08a0ad8f9bccb64808 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Tue, 10 Jul 2018 17:29:26 +1200 Subject: [PATCH 1/7] re-write workbook::reorder_relationships Issue #279 --- source/workbook/workbook.cpp | 151 ++++++++++++++++++----------------- 1 file changed, 76 insertions(+), 75 deletions(-) diff --git a/source/workbook/workbook.cpp b/source/workbook/workbook.cpp index 55c460c4..9ad48973 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)) { @@ -1025,7 +1025,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 +1391,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 +1564,69 @@ 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 +}; +} // namespace - if (!needs_reorder) +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 = manifest().relationship(path("/"), relationship_type::office_document); + const auto wb_path = wb_rel.target().path(); + auto rel_copy = manifest().relationships(wb_path); + // clear existing relations + for (const auto &rel : rel_copy) { manifest().unregister_relationship(uri(wb_path.string()), 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())); } } From cf8991a234d6ae1fba968081f01492a7309c2467 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Sun, 8 Jul 2018 15:44:12 +1200 Subject: [PATCH 2/7] no need to recreate the uri --- source/workbook/workbook.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/source/workbook/workbook.cpp b/source/workbook/workbook.cpp index 9ad48973..4ac1a4d2 100644 --- a/source/workbook/workbook.cpp +++ b/source/workbook/workbook.cpp @@ -1596,13 +1596,12 @@ void workbook::reorder_relationships() return; } // copy of existing relations - const auto wb_rel = manifest().relationship(path("/"), relationship_type::office_document); - const auto wb_path = wb_rel.target().path(); - auto rel_copy = manifest().relationships(wb_path); + const auto wb_rel_target = manifest().relationship(path("/"), relationship_type::office_document).target(); + auto rel_copy = manifest().relationships(wb_rel_target.path()); // 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()); } // create new relations std::size_t index = 0; From dcf50cb4cd701ce9f797a1275e3913453ac75be3 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Mon, 9 Jul 2018 14:17:04 +1200 Subject: [PATCH 3/7] return by const reference when possible --- include/xlnt/packaging/relationship.hpp | 4 ++-- include/xlnt/packaging/uri.hpp | 2 +- source/packaging/relationship.cpp | 4 ++-- source/packaging/uri.cpp | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/include/xlnt/packaging/relationship.hpp b/include/xlnt/packaging/relationship.hpp index 00d7e942..09e88ff2 100644 --- a/include/xlnt/packaging/relationship.hpp +++ b/include/xlnt/packaging/relationship.hpp @@ -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..07f7bafa 100644 --- a/source/packaging/relationship.cpp +++ b/source/packaging/relationship.cpp @@ -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_; } From 53c193433f15726091da43d27cb100fd0605c9be Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Mon, 9 Jul 2018 14:18:07 +1200 Subject: [PATCH 4/7] Ensure worksheets get a unique internal filename Issue #279 --- source/workbook/workbook.cpp | 24 ++++++++++++--- .../data/Issue279_workbook_delete_rename.xlsx | Bin 0 -> 11341 bytes tests/workbook/workbook_test_suite.cpp | 29 ++++++++++++++++-- 3 files changed, 46 insertions(+), 7 deletions(-) create mode 100644 tests/data/Issue279_workbook_delete_rename.xlsx diff --git a/source/workbook/workbook.cpp b/source/workbook/workbook.cpp index 4ac1a4d2..01b35e2e 100644 --- a/source/workbook/workbook.cpp +++ b/source/workbook/workbook.cpp @@ -738,22 +738,36 @@ 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"); diff --git a/tests/data/Issue279_workbook_delete_rename.xlsx b/tests/data/Issue279_workbook_delete_rename.xlsx new file mode 100644 index 0000000000000000000000000000000000000000..981af413c7412d4facf2aed93913c5688b218a39 GIT binary patch literal 11341 zcmeHtbzBwQ8t$fX)7^-4horQ$gmi~=cSwhz0@B?A(%l`hX{4p4rKG#-ZshaRbcOaBJ<6s}tLCuwle=MttMsq3 z;?0p*@aBbz=cL~4U|+ecEcdIS%@!V*E73-uX7ym&sd=e2K?G{QPeQEhougxXHUuY! zj{wO3)DG7sxri4~%Ytc7$0``Ooi?vcp*6LvtH*o7wTL1_bWX6RecGK$hKn>D1w2Zl z9f%B4@TzC_cCdcGg>80ep2(UPUDkAcMTuwRfEa3NSTz5+tCWm?fcxuGBN;y<_g8~a zw1;uG%YH7DZ5wW9Ow-+45Tp!JEq?L@2Ki{S;*3gwYw2|2gD#FeP4_sIVsGF8nN1yF zTjr`Tx}N)l9{N?^&cK97tToEaH~bB&tfw}5y~b;@%>cQ7p{q)nnQRM8lQbj%fB@dV zus5=FU}C)c4vFl)qsixvrsc~GYYF)DtOk<}6FOdZOe%+;D+-~2`3P=W@hqxe+b z=v@$gee@NdJ{yrNCc^&Kfh?Kchv?LFQCZ`(SUc6&8eA{dG!5plVRLwKcc^@RSDm)l z>~$o80N13+ELMi_wf2GX*|YFX;f+JK{Olq7&$=DFE4CsB&E2Lghhh!KV&1Yr7*yra z@FCF6TZb`r&rBP1zTILmuzFdpq6v^AmXNm9ATU~|T|obzS3zdT*nuc4hUe1mK=;SN*MooUfMG#YO zczIDr7U52W7sNi=k0^3!8=Jf^Q@0%k0BZiZ2CeeE)H>(mon zu~_zz`maMsC;7Ln75gNy zNW!Lv_&g_2Pwm7&92c7kbui1C{Wc%e+vECeqS}T@_@0ftyshQ~eY}mig8WzBW0dHu(@@8OTZ*)hTRfMKR8wu{)UARjW&}DmkXt6|OE4Leq3T|PBf%AtK80x%f zFU@YREys;>bT%%}&dxOkPV=SsTU^Z?-$=w}K(lcQZDb2EZ5nY42ck$LjN>6;ar=lL zUR>la$J#F~^xz>dO6)yjGn1_=k|XfV!gZhX)#nd-9lAKq)gh!+53AWZuMSn)eMqB% z8n63mF@w1(11%-mH%zj@VU`4yP#6yAxEF5-?Og$pe1L<3*n^_bV_RyA#^+DgbZQTM z**tjBOp!6_2TYI1>OER$rF} z`|t1a0Q{eeCDVQ9@S_?$piygG8avI95gY7p8dv}T|JQ2HHue_!HZ~S_e*fN|G>!I{ zgWnq11qTFAIM!2*>WZokNye;jizI%Z(k_HDii`wCuS+0KB(6rZ-~O{c`0ESY*Pmyd zr=e3#^Efnw2*ps-2BdX167Le+5ndpd1agYX zo19sehhekd+6C!4V{50CQzq0B~M}XRAGGL)t>YXH04)DlM)*ydtw773+d}7J_ z-q2X~>wY_{yT`5xUxmqbk}Mmm=?X+q_K{o`{i==StgM*HNbdGNea?{2Kiz*f+!iBT2Zn*& zPmcuvQ2p89Hvkq!eKYIrXg;YnIn1~7Y+g!4N~l_dBFUMeQcGRgS$P^}(xw`AcGMg~ zf>Dv#m<+(LA5pynG7QAMeJY~PptO!;to9w`R0lBz_;y`)`;p?vyL^H?q4(rX2ua%6 z=*JlwLy_0TLPMQA{6FU)&wvu;*088?TUP?GOB6!%Ik64qLh=Y%hnkx%ZK!wc3zJfN z#O7}HaQQxJzkfwOb%QgfXyR@~$Bjrg4 ziyyhYnoy!nWV%6uYNW;IBL^jfLiTZ*Rr&%pY@RV1XZlskCON?=2dErkngRs}$Z0;i z@L^rp;-xn+L&tn-#$`%<%TmZO*+rRQh#oCB0Sw9V`XE*HPhUmJ|(T5=CdkM;<;`I5l@{U@*p^RJx7Og+^p z0dqMG-tGb`a3gRq)w4G;RCENl1rvw6H;O&A34A!Wy1*1tUcPB1RdM!?1BQ^h&}qGi5evq zzNrYPh{zbJbH^*K35cvyN$Mo>Voa>Y#w}NM8i^#}9Qa9#B$8TBzx3~cgB@9G)o=!3 zBA*K7*t}Hnt=QnV&O2cFrgW3UPc^EEmX5oRr_K_9i9T~fOrKwI1}c5;VDqmJNTI2M zMhOQ1m^=jlkp6l=M^{Uu?@!jRr5*N_1Jko)^cGcggDh&dWSF_&6<$4^RSn&?(ssc! z#Hi=!8SPVVRIYccFeEG~eJY?Eg&Lrj+;{3uwdToTS9fN)Wui-%ne?MWl$XR*vEtuQ zzpGeLyIykTI8e@NAqf)neaxl8St>U_dwVsa{fhI!)dA!q6?hSq;4z$F)tW?=*G$YX z9Y`9W;Km9;S0J-%0#lWKj4&$JD76fm;)FH>j9vCPe2p13WTfv^l~#yQbTJ@w%?^D@ z=1mr@naibKLONj|hLDrhB*~1PD$`s`j$M2PB*PxktC1mnH2*z@UO@ylUq|m6K|IE2 z)EGRrS7JXyxH*{O4fz$ex4a5Ukuon4{mG%Zj&LuORB1gwJEV7xE+ z`!yCKEg!b$9LJ^4;Apd_UCpHRGyF8R7wf%N@t1nNmwLaP#-OV%R5If~4VKI{j0D0X5eJ5jPZ%sn%Y|;V0rJV@{T+Tr zdB@$?M0M{#%!9_uZ_4?+<_h6o1#8)kag8-%RKz`*#LXtNmP;cKr4Cw&E=tL}WJ}^# zW)5CYNP`}40DK!Ek4Hx4PJsmtrBQk!B3>*Eb+x29LB55YEhcxj<%Q{T#9HUnL4|H! zyl8$n8LKy~9Jw~VZifwG5AMw8QA?bAYzyz#asV|!F5)=tTs+W13eDTKPejISbud_q z?6F^k%^ z!h--ImG#B`VZj*%@{6&ihOrC$hW9QsC^E=a8sgO^oA0;di=Sj}Vv5-%5bw|CrItEV zwo3EWT6`GJ(|qrXJvKv2{uJtQ1=)5N{o8=D*{7Hx5oM@B5iF>|;C;xx2$pDWGL`N8 zpylf3#p#$7=fb|_`%2fO!6-^1JU+7#&T+;-N=nb8Jc3T4LTg+oP`w_#h{5t^Hb$~V zZq6aYjCT#(avxeN`MD72MuRN}CCM7T7)Meiy8_cY;G0g4kTbHg|8iypU!mOw&KEth z!?XxUl>)=VJn1#&ROQsd>Wm|$iRMHlmF-P`avfuuK3u=SC#}rwV_jY1=W1|gxO`{H zOY{5-^No?-Yd+pm<=*PY3)LRdcKpJR_cUN*31Pi@TVmTtO!2trp)RTY9C-2FHa{C_ zbH_QD5!$+fs8On3)O6YN1OY~D_+ovW>*7(WCYM$`>ag3ocPc!tmOM~5x9D#LGzQsc zpkDhDzaAiMIhqmVCG%TNGtxamT~WY9e1?hp2=Uq4i%`EzR%W_4nTpI(pa_vy%B|NB zTrQDZ%$OeMn1)UgVhK_;45GTA#StL(D&xR|jb-NI|atGHZe6mA~R)=I~_w7?3F zg<#<7r2}i>kWJlZhij%!60Y#V=rv_+`HVuJxIlK*bSI_s%#u{c3$-@~yDan${>mw> zxbzJIv17Wfys`5Sd*}3&dviKS*Ed5}*wm22R!+L?3F!GxLv?qy8Hr?42IGbw7K{zA zmL=b)6@~JLVI6guJF%V5XkM3T5NQ+lZibD0Gd-+xhIZI}xzRn5PoY*|ofGO&q~Tk& z@%;qD{fX34n9JAGd1UicmLUE`@Gom~XnogS;wyA>n1yGa+h-az&=#IA*qc8wfN19@ zf=X;OOPfz)zBasLm{dM-r%ct>WNyGf-9|n0Wr0zA4D(El(nseZo6zBkJ}|cT2tB;xDk5T<{L!N^D;j(6{vY7BoL&>M|BWyV8z5+q*TI>z2AhZX$jQNVJ(3i z3t{aP!DseMd`Z6Brx^5h$oSDsj8yUcvPX}bKns`e=B#}5gguy!eQC~`+NJT4=JIGJ z``#NQSBkh0`=)H+Epj*-lpsCOzj|;GtU^{ZcM7}|6xqH;tTPH2-!77lahCR0dhu~O zlnzbH=P_uoKZ*R^>RHW3gNHHero$E1;&$Q58QC>|sQmSgkL``x!iGA{p-bV-CELDl zS5Cr{gX2Jfa)xigkR}IQ_FL=A`*zshviYxNt-P1KJDIKuK8gtL)FWP2OLP}{5NyR> z#l5sFe9UBip?m|seQ`?*UzBE}>iuZ(#{0*B`p>ZOuUOsD)X2){w_x#7Kg;S93;>Wr zaIejP1B?4=-@jSz;)-o`nXqXQ+-9=k}^b z^G|-^>13qjEAkWejN|koq;c+S2MyyX!+SzqLJEuBZNcK17X5*h#aoS+14fNSkm%^p z2N5%!+H`WXbJbSstsJp9uF|BnoQtIGz}VIrzE@#SZMwJIipHyjrO6OPxuC0Rs4zCr z*(8-Tco0^gV7*BxqLAVOg*lBzDHIAhog$f@#CcKb)RB&n~f5)px*+OMM@um~#RWPwxRZV*)7HCtvz*F_|(7eglCiZMR|GKH* z+N;6WCg$t}=;dNoFkARlQLe?5P5UX;IsIcrLCR!p&Q>14EUK@W3S+JGRf|OwfSbZ z8_XuRo8{%jiYR}9WL`JNgFSb@1t-Gu`i0&Sf%EHLlwlj$k8bB@tgJd4XQ>}AzvVm> zV26r9_Sip?0^)rpj8EyRiDC7#b0J*F;OScU2Gqk4GI%)A8D~?}my>!r zq`Qc8Jq*9@FlD`D4oZ*AduAj+g2j4@XGm~KP70(rJ&Q*5o8*(EvzyH*UWk;%tQVI@ zQ8bVcb1du=nlOWRO-Mp~t)!}7;f!<4#hN!yA+Ai$25n3k^IEyp1!)iyIRtwte7!qK z4Th2oWv#UcT8p;3QWjSnDonyGvW{UD%fUuBUZk^fx6D;d6eyZQxh&IH7do>t;^ose zY04S`Np$%>StwAyaxi8XUS~Vp5XrS=_>1FwR%YZ*#qTiKEhw*7LJ(1O3FlzDfw^-!01p99Khjf67wxU z$ps=q5kiGTJwAJ^x-Et2@W2LVTw}He$t57yvZTtT1e#67eWtgX;NsFaol^T;fy$g5 z$U$MD<;MD$3d*{BwX)B$NO8)NA#sQkp6Vkd*S4D&(i>mA{m5DqAhy|)Q_LQKJNaOQ zfn&RwC2sY=l>EqKHNH(}^|MIF`c?e;aWh8( z1O&!5+@l^tCB0Q~?Ox?NIunk=^0w7-M{^6JV!3vqd~JF^Y1*b3gtFYqk8|dfc3cnP z5JyZ|>F|lxY${uIo--s!2NaBNMfYA2r_zujL+#>*Oi2^!mNq0P$B9$$(ryI3@jlrN$axD^5 zo#P@d3A3bmrG;HOB(fqkaxM?1t*t~|3nR2uMEq;D1G1O;bWdp4RIV3`=L@h6|7Z{XVT~=h-4c3z9Hvu{rC3}YTpy_2~x=#n*Vgs>9$WcVzX+GC*N`W)j zLb(M|rLK}vpE-rXQOH+NC&+YHrkC&mtLOS&Pk4-u_Y+TO5(tUTG%bR_CB#^qEy?oET9CE`(2Wy{|HlB_feye@kzkP**5wR3dL_r_g{2UkAw zMQ4P`JcGs`%yzDM#xQ10wt_qbE0j~qoSo?MegH@v=oO9y?;a_ojBb$i#jRdJFqaV^ z-P6=^YEVKhy-?gHa-HMf=MW2u>N}x;A7Pho2`pRM^i{^6p0bWiwo`M86Uxk=duAps zU?0piyqfSBimgMK&Ik$TK~>4BduI`@zgIw0wWjx&>>KK3!@>T~$dd$OyT&FYvC z(I(=}au~*$!*5^m^~Gs`c9=NayAoEiI6R)d+!xoo;9z~-0}NY-QR_~JLPEL9rV;O81)j|D#-qUesA&J{CiyR zUBN4g;FAR_cyFD_Z~UBiUPiuR8e4~yAW}=a0!-AAs}M~eXvL`IjY{}2C8E(t7cU;80kP8Kmo~6UGRm|0S4j}t4-_rAkz7aENvi!{d+T*I zJ7cGQNTEXXSbQxx1=D#I+LuLwRoMeo4ofrWyyn2*%;-~3uYQ}etR^wlPzWdeMYY7i5yI2&2s>dqq15q`th!^UVJ=q#g_+g1JYH*KaJE_Z$v?6G9< zdxZNBul@8_NR*7tG&ADcU8s|X(6a;`>J1!kw?CN5(fdQR)rVD(j#Q&Q8zrYOJJ}Sc zb3#qS9rDxrO#X16VrAo|1HalImAyU@r{amp0%rA>WA@wgrS*-_iug>6tfz470z%8D z{aq+y5N#aDfIu`*Mf6ylp>u;h*5Qa%Xg=j(NR~4lSJYJ;H zi+qS%+(o)?%uPU9800`K|{=yXOBAi(nf}>csRyTv?&W4`ApC*Tk8wPHBH!8O=xs>*avn-<<#~#tKu|r(!FZeDuUk3YiEzoQfGsHlg|NvH zrmAJ^KGmj*=eAACI<&%J>am!~JpI74T4EbkQHaMCiUF(jo9EX;Vd*Hc=QAb}JcEsy zBkzx2)C))rFq@($*_ik9y~WIYjzh#~@4t4NfBs7!d-q0pf!?hsf!l^7crESUw2jAi zF?L9#yi^-A=9_t#TjB1@L~CxRtZmdzTNK}bi}%@_R%(%&k$qOTb8HTzTShjgLs2Db zGnrb%!Ibk0jY68kX0eKpD!K)hs>6d}$IF4}4{NRJ#1OFfd~LOr2bDGMZ)Yam#BcZ$C%_?M!+Lr>&-6r+aZo=B}ypz!!_NXjJ*yYA@ zcgTe4aprDZx`9!e@OLRYjQ5n*uI$2cL374onX*bq)2S=e%Ep{tuX7{T~zmB}}mWkBR>h zCfNVS#D57B9RFkD{|6JR%DGG0;1T=vzaOzV@0pObfMh}K*nQ*WAKoBlWmQls`81Ol zn031|wY6o<+(X}^c0BF;vPfa?tCbGr;ZoBxB?4|U3sRzlFY9}`vrg;HWa9fkV+egRlo^$dk<5lIQG7@MKSRUfCPQzKrGW7i zXExl~oDG+ky2L76yp>I-t);vYHrBM77?S7_S~BHHs`+##Mt_optxi{e^*Y`W5}Exb zJrQ><=A86|a!am5SKVowyVdip=UNL*>}VLVqb4({!@YeN?DEoygTUSgU#f!OjVM$7 z75YtPJ8Tddj!LjI4^eD&;elRcG+$=d0+OohRx_URdGu z{+@qV*?(R!`B$@_n)|N+f6{910Z72RIGpd-{guq0#{azOJJI02-0>)d_qXxi62cD( z|7m+CBHWidTjzh+{*V-YAlyIu@1nvT!aZMk{|4dbl=-)2c@OyeeETQhJUH73%)u`z z$4{uAPPs?c|K}(PkN$}AJ+}ML wQOLlO&>y_}Judkj<@Ztar+16U|A_K^OqG*@0jG=s065^!DY&Z=q`W)&Ke9O 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 From ec02121c15dd31cf9d59f6061a135a3222cf87e2 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Mon, 9 Jul 2018 15:24:03 +1200 Subject: [PATCH 5/7] Add debug output for a relationship mismatch --- tests/helpers/xml_helper.hpp | 109 ++++++++++++++++++++++------------- 1 file changed, 69 insertions(+), 40 deletions(-) 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; + } }; From 7458426111881fa166a9c138e40917e2c0e1a55f Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Mon, 9 Jul 2018 15:54:28 +1200 Subject: [PATCH 6/7] return by const ref --- include/xlnt/packaging/relationship.hpp | 2 +- source/packaging/relationship.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/xlnt/packaging/relationship.hpp b/include/xlnt/packaging/relationship.hpp index 09e88ff2..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. diff --git a/source/packaging/relationship.cpp b/source/packaging/relationship.cpp index 07f7bafa..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_; } From 0d1bca3fd453712aa1c39e2f3824baf3120deced Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Mon, 9 Jul 2018 15:57:10 +1200 Subject: [PATCH 7/7] Remove hidden dependency on unordered map extraction order Issue #279 --- source/workbook/workbook.cpp | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/source/workbook/workbook.cpp b/source/workbook/workbook.cpp index 01b35e2e..c4739f03 100644 --- a/source/workbook/workbook.cpp +++ b/source/workbook/workbook.cpp @@ -754,8 +754,7 @@ worksheet workbook::create_sheet() auto workbook_rel = d_->manifest_.relationship(path("/"), relationship_type::office_document); 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 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; }); }; @@ -1597,6 +1596,20 @@ bool needs_reorder(const std::unordered_map &title_to_ } return !all_match; // if all are as expected, reorder not required }; + +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() @@ -1612,6 +1625,7 @@ void workbook::reorder_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) { @@ -1632,7 +1646,7 @@ void workbook::reorder_relationships() 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) + for (const auto &old_rel : rel_copy) { if (old_rel.type() == relationship_type::worksheet) {