From 03020cc7931d6103de69b90a465832ac7645bc3b Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Mon, 2 Jul 2018 21:06:12 +1200 Subject: [PATCH] improved hyperlink implementation - hyperlinks to cells and ranges are complete - hyperlink::display is now set as well as the cell value (in excel these can be different) -- if a cell is empty, display is equal to value text -- if a cell has a value, display can be just about anything - This version copies excel in that display is completely ignored once value is set - All hyperlink tests are now part of the cell test suite (not the worksheet test suite which the majority were previously located) --- include/xlnt/cell/cell.hpp | 11 +-- include/xlnt/cell/hyperlink.hpp | 14 +++- source/cell/cell.cpp | 68 +++++++++++-------- source/cell/hyperlink.cpp | 39 +++++++++-- .../detail/implementations/hyperlink_impl.hpp | 7 +- tests/cell/cell_test_suite.cpp | 53 ++++++++++++++- tests/workbook/serialization_test_suite.cpp | 2 +- tests/worksheet/worksheet_test_suite.cpp | 45 ------------ 8 files changed, 144 insertions(+), 95 deletions(-) diff --git a/include/xlnt/cell/cell.hpp b/include/xlnt/cell/cell.hpp index f77f6d31..8b85625b 100644 --- a/include/xlnt/cell/cell.hpp +++ b/include/xlnt/cell/cell.hpp @@ -262,26 +262,21 @@ public: /// class hyperlink hyperlink() const; - /// - /// Adds a hyperlink to this cell pointing to the URL of the given value. - /// - void hyperlink(const std::string &url); - /// /// Adds a hyperlink to this cell pointing to the URI of the given value and sets /// the text value of the cell to the given parameter. /// - void hyperlink(const std::string &url, const std::string &display); + void hyperlink(const std::string &url, const std::string &display = ""); /// /// Adds an internal hyperlink to this cell pointing to the given cell. /// - void hyperlink(xlnt::cell target); + void hyperlink(xlnt::cell target, const std::string& display = ""); /// /// Adds an internal hyperlink to this cell pointing to the given range. /// - void hyperlink(xlnt::range target); + void hyperlink(xlnt::range target, const std::string& display = ""); /// /// Returns true if this cell has a hyperlink set. diff --git a/include/xlnt/cell/hyperlink.hpp b/include/xlnt/cell/hyperlink.hpp index 8034098b..e77f8830 100644 --- a/include/xlnt/cell/hyperlink.hpp +++ b/include/xlnt/cell/hyperlink.hpp @@ -44,12 +44,22 @@ class XLNT_API hyperlink public: bool external() const; class relationship relationship() const; + // external target std::string url() const; + // internal target std::string target_range() const; + + bool has_display() const; void display(const std::string &value); - std::string display() const; + const std::string& display() const; + + bool has_tooltip() const; void tooltip(const std::string &value); - std::string tooltip() const; + const std::string& tooltip() const; + + bool has_location() const; + void location(const std::string &value); + const std::string& location() const; private: friend class cell; diff --git a/source/cell/cell.cpp b/source/cell/cell.cpp index 96c1a021..f080107b 100644 --- a/source/cell/cell.cpp +++ b/source/cell/cell.cpp @@ -372,7 +372,7 @@ hyperlink cell::hyperlink() const return xlnt::hyperlink(&d_->hyperlink_.get()); } -void cell::hyperlink(const std::string &url) +void cell::hyperlink(const std::string &url, const std::string &display) { if (url.empty() || std::find(url.begin(), url.end(), ':') == url.end()) { @@ -388,7 +388,8 @@ void cell::hyperlink(const std::string &url) auto relationships = manifest.relationships(ws.path(), relationship_type::hyperlink); auto relation = std::find_if(relationships.cbegin(), relationships.cend(), [&url](xlnt::relationship rel) { return rel.target().path().string() == url; }); - if (relation != relationships.end()) { + if (relation != relationships.end()) + { d_->hyperlink_.get().relationship = *relation; } else @@ -400,29 +401,20 @@ void cell::hyperlink(const std::string &url) target_mode::external); // TODO: make manifest::register_relationship return the created relationship instead of rel id d_->hyperlink_.get().relationship = manifest.relationship(ws.path(), rel_id); - } - - if (!has_value()) // hyperlink on an empty cell sets the value to the hyperlink string + } + // if a value is already present, the display string is ignored + if (has_value()) { - value(url); + d_->hyperlink_.get().display.set(to_string()); + } + else + { + d_->hyperlink_.get().display.set(display.empty() ? url : display); + value(hyperlink().display()); } } -void cell::hyperlink(const std::string &url, const std::string &display) -{ - if (!display.empty()) // if the display string isn't empty use that - { - value(display); - } - else // empty display string sets the value to the link text - { - value(url); - } - hyperlink(url); - d_->hyperlink_.get().display = display; -} - -void cell::hyperlink(xlnt::cell target) +void cell::hyperlink(xlnt::cell target, const std::string& display) { // TODO: should this computed value be a method on a cell? const auto cell_address = target.worksheet().title() + "!" + target.reference().to_string(); @@ -430,11 +422,19 @@ void cell::hyperlink(xlnt::cell target) d_->hyperlink_ = detail::hyperlink_impl(); d_->hyperlink_.get().relationship = xlnt::relationship("", relationship_type::hyperlink, uri(""), uri(cell_address), target_mode::internal); - d_->hyperlink_.get().display = cell_address; - value(cell_address); + // if a value is already present, the display string is ignored + if (has_value()) + { + d_->hyperlink_.get().display.set(to_string()); + } + else + { + d_->hyperlink_.get().display.set(display.empty() ? cell_address : display); + value(hyperlink().display()); + } } -void cell::hyperlink(xlnt::range target) +void cell::hyperlink(xlnt::range target, const std::string &display) { // TODO: should this computed value be a method on a cell? const auto range_address = target.worksheet().title() + "!" + target.reference().to_string(); @@ -442,8 +442,17 @@ void cell::hyperlink(xlnt::range target) d_->hyperlink_ = detail::hyperlink_impl(); d_->hyperlink_.get().relationship = xlnt::relationship("", relationship_type::hyperlink, uri(""), uri(range_address), target_mode::internal); - d_->hyperlink_.get().display = range_address; - value(range_address); + + // if a value is already present, the display string is ignored + if (has_value()) + { + d_->hyperlink_.get().display.set(to_string()); + } + else + { + d_->hyperlink_.get().display.set(display.empty() ? range_address : display); + value(hyperlink().display()); + } } void cell::formula(const std::string &formula) @@ -828,8 +837,11 @@ void cell::value(const std::string &value_string, bool infer_type) void cell::clear_format() { - format().d_->references -= format().d_->references > 0 ? 1 : 0; - d_->format_.clear(); + if (d_->format_.is_set()) + { + format().d_->references -= format().d_->references > 0 ? 1 : 0; + d_->format_.clear(); + } } void cell::clear_style() diff --git a/source/cell/hyperlink.cpp b/source/cell/hyperlink.cpp index 8d28d84e..28539107 100644 --- a/source/cell/hyperlink.cpp +++ b/source/cell/hyperlink.cpp @@ -67,24 +67,49 @@ bool hyperlink::external() const return d_->relationship.target_mode() == target_mode::external; } -void hyperlink::display(const std::string &value) +bool hyperlink::has_display() const { - d_->display = value; + return d_->display.is_set(); } -std::string hyperlink::display() const +void hyperlink::display(const std::string &value) { - return d_->display; + d_->display.set(value); +} + +const std::string& hyperlink::display() const +{ + return d_->display.get(); +} + +bool hyperlink::has_tooltip() const +{ + return d_->tooltip.is_set(); } void hyperlink::tooltip(const std::string &value) { - d_->tooltip = value; + d_->tooltip.set(value); } -std::string hyperlink::tooltip() const +const std::string& hyperlink::tooltip() const { - return d_->tooltip; + return d_->tooltip.get(); +} + +bool hyperlink::has_location() const +{ + return d_->location.is_set(); +} + +void hyperlink::location(const std::string &value) +{ + d_->location.set(value); +} + +const std::string &hyperlink::location() const +{ + return d_->location.get(); } } // namespace xlnt diff --git a/source/detail/implementations/hyperlink_impl.hpp b/source/detail/implementations/hyperlink_impl.hpp index 84172986..274997c3 100644 --- a/source/detail/implementations/hyperlink_impl.hpp +++ b/source/detail/implementations/hyperlink_impl.hpp @@ -26,15 +26,18 @@ #include #include +#include namespace xlnt { namespace detail { +// [serialised] struct hyperlink_impl { xlnt::relationship relationship; - std::string tooltip; - std::string display; + xlnt::optional location; + xlnt::optional tooltip; + xlnt::optional display; }; inline bool operator==(const hyperlink_impl &lhs, const hyperlink_impl &rhs) diff --git a/tests/cell/cell_test_suite.cpp b/tests/cell/cell_test_suite.cpp index a0753a49..8729134e 100644 --- a/tests/cell/cell_test_suite.cpp +++ b/tests/cell/cell_test_suite.cpp @@ -37,11 +37,13 @@ #include #include #include +#include #include #include #include #include + class cell_test_suite : public test_suite { public: @@ -665,13 +667,60 @@ private: { xlnt::workbook wb; auto cell = wb.active_sheet().cell("A1"); + cell.value(123); xlnt_assert(!cell.has_hyperlink()); xlnt_assert_throws(cell.hyperlink(), xlnt::invalid_attribute); xlnt_assert_throws(cell.hyperlink("notaurl"), xlnt::invalid_parameter); xlnt_assert_throws(cell.hyperlink(""), xlnt::invalid_parameter); - cell.hyperlink("http://example.com"); + // link without optional display + const std::string link1("http://example.com"); + cell.hyperlink(link1); xlnt_assert(cell.has_hyperlink()); - xlnt_assert_equals(cell.hyperlink().relationship().target().to_string(), "http://example.com"); + xlnt_assert(cell.hyperlink().external()); + xlnt_assert_equals(cell.hyperlink().url(), link1); + xlnt_assert_equals(cell.hyperlink().relationship().target().to_string(), link1); + xlnt_assert_equals(cell.hyperlink().display(), link1); + // link with display + const std::string link2("http://example2.com"); + const std::string display_txt("notaurl"); + cell.hyperlink(link2, display_txt); + xlnt_assert(cell.has_hyperlink()); + xlnt_assert(cell.hyperlink().external()); + xlnt_assert_equals(cell.hyperlink().url(), link2); + xlnt_assert_equals(cell.hyperlink().relationship().target().to_string(), link2); + xlnt_assert_equals(cell.hyperlink().display(), display_txt); + // cell overload without display + const std::string cell_target_str("A2"); + auto cell_target = wb.active_sheet().cell(cell_target_str); + std::string link3 = wb.active_sheet().title() + '!' + cell_target_str; // Sheet1!A2 + cell.hyperlink(cell_target); + xlnt_assert(cell.has_hyperlink()); + xlnt_assert(!cell.hyperlink().external()); + xlnt_assert_equals(cell.hyperlink().target_range(), link3); + xlnt_assert_equals(cell.hyperlink().display(), link3); + // cell overload with display + cell.hyperlink(cell_target, display_txt); + xlnt_assert(cell.has_hyperlink()); + xlnt_assert(!cell.hyperlink().external()); + xlnt_assert_equals(cell.hyperlink().target_range(), link3); + xlnt_assert_equals(cell.hyperlink().display(), display_txt); + // range overload without display + const std::string range_target_str("A2:A5"); + xlnt::range range_target(wb.active_sheet(), xlnt::range_reference(range_target_str)); + std::string link4 = wb.active_sheet().title() + '!' + range_target_str; // Sheet1!A2:A5 + cell.hyperlink(range_target); + xlnt_assert(cell.has_hyperlink()); + xlnt_assert(!cell.hyperlink().external()); + xlnt_assert_equals(cell.hyperlink().target_range(), link4); + xlnt_assert_equals(cell.hyperlink().display(), link4); + // range overload with display + cell.hyperlink(range_target, display_txt); + xlnt_assert(cell.has_hyperlink()); + xlnt_assert(!cell.hyperlink().external()); + xlnt_assert_equals(cell.hyperlink().target_range(), link4); + xlnt_assert_equals(cell.hyperlink().display(), display_txt); + // value not touched + xlnt_assert_equals(cell.value(), 123); } void test_comment() diff --git a/tests/workbook/serialization_test_suite.cpp b/tests/workbook/serialization_test_suite.cpp index 3533159c..744103ee 100644 --- a/tests/workbook/serialization_test_suite.cpp +++ b/tests/workbook/serialization_test_suite.cpp @@ -575,7 +575,7 @@ public: std::vector destination; source_workbook.save(destination); - source_workbook.save("temp.xlsx"); + source_workbook.save("temp" + source.filename()); #ifdef _MSC_VER std::ifstream source_stream(source.wstring(), std::ios::binary); diff --git a/tests/worksheet/worksheet_test_suite.cpp b/tests/worksheet/worksheet_test_suite.cpp index 84fa2693..b25e0d61 100644 --- a/tests/worksheet/worksheet_test_suite.cpp +++ b/tests/worksheet/worksheet_test_suite.cpp @@ -50,7 +50,6 @@ public: register_test(test_remove_named_range_bad); register_test(test_cell_alternate_coordinates); register_test(test_cell_range_name); - register_test(test_hyperlink_value); register_test(test_rows); register_test(test_no_rows); register_test(test_no_cols); @@ -225,50 +224,6 @@ public: xlnt_assert(c_range_coord[0][0] == c_cell); } - void test_hyperlink_value() - { - xlnt::workbook wb; - auto ws = wb.active_sheet(); - std::string test_link = "http://test.com"; - xlnt::cell_reference test_cell("A1"); - ws.cell(test_cell).hyperlink(test_link); - // when a hyperlink is added to an empty cell, the display text becomes == to the link - xlnt_assert_equals(ws.cell(test_cell).hyperlink().url(), test_link); - xlnt_assert_equals(ws.cell(test_cell).value(), test_link); - // if the display value changes, the hyperlink remains the same - std::string test_string = "test"; - ws.cell(test_cell).value(test_string); - xlnt_assert_equals(test_string, ws.cell(test_cell).value()); - xlnt_assert_equals(ws.cell(test_cell).hyperlink().url(), test_link); - // changing the link doesn't change the cell value - std::string test_link2 = "http://test-123.com"; - ws.cell(test_cell).hyperlink(test_link2); - xlnt_assert_equals(test_string, ws.cell(test_cell).value()); - xlnt_assert_equals(ws.cell(test_cell).hyperlink().url(), test_link2); - // and we can edit both value and hyperlink together - std::string test_string3 = "123test"; - std::string test_link3 = "http://123-test.com"; - ws.cell(test_cell).hyperlink(test_link3, test_string3); - xlnt_assert_equals(test_string3, ws.cell(test_cell).value()); - xlnt_assert_equals(ws.cell(test_cell).hyperlink().url(), test_link3); - // hyperlinks can also be applied to cells with non-string values - int numeric_test_val = 123; - std::string test_link4 = "http://test-numeric.com"; - xlnt::cell_reference numeric_test_cell("B1"); - ws.cell(numeric_test_cell).value(numeric_test_val); - ws.cell(numeric_test_cell).hyperlink(test_link4); - xlnt_assert_equals(ws.cell(numeric_test_cell).hyperlink().url(), test_link4); - xlnt_assert_equals(ws.cell(numeric_test_cell).value(), numeric_test_val); - // and there should be no issues if two cells use the same hyperlink - ws.cell(numeric_test_cell).hyperlink(test_link3); // still in use on 'A1' - // 'A1' - xlnt_assert_equals(test_string3, ws.cell(test_cell).value()); - xlnt_assert_equals(ws.cell(test_cell).hyperlink().url(), test_link3); - // 'B1' - xlnt_assert_equals(ws.cell(numeric_test_cell).hyperlink().url(), test_link3); - xlnt_assert_equals(ws.cell(numeric_test_cell).value(), numeric_test_val); - } - void test_rows() { xlnt::workbook wb;