diff --git a/src/Mod/Spreadsheet/App/PropertySheet.cpp b/src/Mod/Spreadsheet/App/PropertySheet.cpp index 5933cdc5ae..9695a1d411 100644 --- a/src/Mod/Spreadsheet/App/PropertySheet.cpp +++ b/src/Mod/Spreadsheet/App/PropertySheet.cpp @@ -127,40 +127,47 @@ const Cell* PropertySheet::getValueFromAlias(const std::string& alias) const } } -bool PropertySheet::isValidAlias(const std::string& candidate) +bool PropertySheet::isValidCellAddressName(const std::string& candidate) { static const boost::regex gen("^[A-Za-z][_A-Za-z0-9]*$"); boost::cmatch cm; + /* Check if it matches a cell reference */ + if (boost::regex_match(candidate.c_str(), cm, gen)) { + static const boost::regex e("\\${0,1}([A-Z]{1,2})\\${0,1}([0-9]{1,5})"); + + if (boost::regex_match(candidate.c_str(), cm, e)) { + const boost::sub_match colstr = cm[1]; + const boost::sub_match rowstr = cm[2]; + + if (App::validRow(rowstr.str()) >= 0 && App::validColumn(colstr.str())) { + return true; + } + } + } + return false; +} + +bool PropertySheet::isValidAlias(const std::string& candidate) +{ + /* Check if it is used before */ if (getValueFromAlias(candidate)) { return false; } + /* check if it would be a valid cell address name, e.g. "A2" or "C3" */ + if (isValidCellAddressName(candidate)) { + return false; + } + /* Check to make sure it doesn't clash with a reserved name */ if (ExpressionParser::isTokenAUnit(candidate) || ExpressionParser::isTokenAConstant(candidate)) { return false; } - /* Check to make sure it doesn't match a cell reference */ - if (boost::regex_match(candidate.c_str(), cm, gen)) { - static const boost::regex e("\\${0,1}([A-Z]{1,2})\\${0,1}([0-9]{1,5})"); - - if (boost::regex_match(candidate.c_str(), cm, e)) { - const boost::sub_match colstr = cm[1]; - const boost::sub_match rowstr = cm[2]; - - // A valid cell address? - if (App::validRow(rowstr.str()) >= 0 && App::validColumn(colstr.str())) { - return false; - } - } - return true; - } - else { - return false; - } + return true; } namespace diff --git a/src/Mod/Spreadsheet/App/PropertySheet.h b/src/Mod/Spreadsheet/App/PropertySheet.h index 2e0cd268bb..676818ce16 100644 --- a/src/Mod/Spreadsheet/App/PropertySheet.h +++ b/src/Mod/Spreadsheet/App/PropertySheet.h @@ -124,6 +124,9 @@ public: bool isValidAlias(const std::string& candidate); + // checks whether candidate is of form A1, C4, etc. + bool isValidCellAddressName(const std::string& candidate); + std::vector getUsedCells() const; std::tuple getUsedRange() const; diff --git a/src/Mod/Spreadsheet/App/Sheet.cpp b/src/Mod/Spreadsheet/App/Sheet.cpp index 35a35eb45a..0171fd1eee 100644 --- a/src/Mod/Spreadsheet/App/Sheet.cpp +++ b/src/Mod/Spreadsheet/App/Sheet.cpp @@ -108,7 +108,11 @@ Sheet::~Sheet() } /** - * Clear all cells in the sheet. + * Clear all cells in the sheet. These are implemented as dynamic + * properties, for example "A1" is added as a dynamic property. Since + * now users may add dyanamic properties, we need to try to avoid + * removing those, too, so we check whether the dynamic property name + * is a valid cell address name before removing it. */ void Sheet::clearAll() @@ -118,7 +122,9 @@ void Sheet::clearAll() std::vector propNames = props.getDynamicPropertyNames(); for (const auto& propName : propNames) { - this->removeDynamicProperty(propName.c_str()); + if (cells.isValidCellAddressName(propName.c_str())) { + this->removeDynamicProperty(propName.c_str()); + } } propAddress.clear(); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index c60c20e7cc..1499ace2cb 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -117,6 +117,9 @@ endif(BUILD_POINTS) if(BUILD_SKETCHER) list (APPEND TestExecutables Sketcher_tests_run) endif(BUILD_SKETCHER) +if(BUILD_SPREADSHEET) + list (APPEND TestExecutables Spreadsheet_tests_run) +endif() # ------------------------- diff --git a/tests/src/Mod/CMakeLists.txt b/tests/src/Mod/CMakeLists.txt index faf7feabe1..4f7578400a 100644 --- a/tests/src/Mod/CMakeLists.txt +++ b/tests/src/Mod/CMakeLists.txt @@ -25,3 +25,6 @@ endif(BUILD_POINTS) if(BUILD_SKETCHER) add_subdirectory(Sketcher) endif(BUILD_SKETCHER) +if(BUILD_SPREADSHEET) + add_subdirectory(Spreadsheet) +endif() diff --git a/tests/src/Mod/Spreadsheet/App/CMakeLists.txt b/tests/src/Mod/Spreadsheet/App/CMakeLists.txt new file mode 100644 index 0000000000..76b1659d6e --- /dev/null +++ b/tests/src/Mod/Spreadsheet/App/CMakeLists.txt @@ -0,0 +1,11 @@ +target_sources( + Spreadsheet_tests_run + PRIVATE + ${CMAKE_CURRENT_SOURCE_DIR}/PropertySheet.cpp +) + +target_include_directories( + Spreadsheet_tests_run + PUBLIC + ${CMAKE_BINARY_DIR} +) diff --git a/tests/src/Mod/Spreadsheet/App/PropertySheet.cpp b/tests/src/Mod/Spreadsheet/App/PropertySheet.cpp new file mode 100644 index 0000000000..4e63b35d03 --- /dev/null +++ b/tests/src/Mod/Spreadsheet/App/PropertySheet.cpp @@ -0,0 +1,84 @@ +// SPDX-License-Identifier: LGPL-2.1-or-later + +#include +#include "src/App/InitApplication.h" + +#include + +#include +#include + +class PropertySheetTest: public ::testing::Test +{ +protected: + static void SetUpTestSuite() + { + tests::initApplication(); + } + void SetUp() override + { + _sheet = std::make_unique(); + _propertySheet = std::make_unique(_sheet.get()); + } + void TearDown() override + { + _sheet.reset(); + _propertySheet.reset(); + } + + /// Get a non-owning pointer to the internal PropertySheet for this test + Spreadsheet::PropertySheet* propertySheet() + { + return _propertySheet.get(); + } + +private: + std::unique_ptr _sheet; + std::unique_ptr _propertySheet; +}; + +TEST_F(PropertySheetTest, isValidCellAddressNameValidNames) // NOLINT +{ + std::vector validAddressNames {"A1", "Z1024", "AA42", "ZZ4096"}; + for (const auto& name : validAddressNames) { + EXPECT_TRUE(propertySheet()->isValidCellAddressName(name)) + << "\"" << name << "\" was not accepted as a cell name, and should be"; + } +} + +TEST_F(PropertySheetTest, isValidCellAddressNameInvalidNames) // NOLINT +{ + std::vector invalidAddressNames { + "Bork", + "Bork_de_bork", + "A", + "42", + "AAA1", // Too many characters to start, AAA is not a valid column + "ZZ123456" // Too large a number to end, 123456 is not a valid row + }; + for (const auto& name : invalidAddressNames) { + EXPECT_FALSE(propertySheet()->isValidCellAddressName(name)) + << "\"" << name << "\" was accepted as a cell name, and should not be"; + } +} + +TEST_F(PropertySheetTest, validAliases) // NOLINT +{ + std::vector validAliases {"Bork", + "Bork_de_bork" + "A", + "AA123456"}; + for (const auto& name : validAliases) { + EXPECT_TRUE(propertySheet()->isValidAlias(name)) + << "\"" << name << "\" was not accepted as an alias name, and should be"; + } +} + +TEST_F(PropertySheetTest, invalidAliases) // NOLINT +{ + std::vector invalidAliases {"A1", "ZZ1234", "mm"}; + for (const auto& name : invalidAliases) { + EXPECT_FALSE(propertySheet()->isValidAlias(name)) + << "\"" << name << "\" was accepted as an alias name, and should not be"; + } +} diff --git a/tests/src/Mod/Spreadsheet/CMakeLists.txt b/tests/src/Mod/Spreadsheet/CMakeLists.txt new file mode 100644 index 0000000000..3c19e5c372 --- /dev/null +++ b/tests/src/Mod/Spreadsheet/CMakeLists.txt @@ -0,0 +1,17 @@ + +target_include_directories(Spreadsheet_tests_run PUBLIC + ${EIGEN3_INCLUDE_DIR} + ${OCC_INCLUDE_DIR} + ${Python3_INCLUDE_DIRS} + ${SMESH_INCLUDE_DIR} + ${VTK_INCLUDE_DIRS} + ${XercesC_INCLUDE_DIRS} +) + +target_link_libraries(Spreadsheet_tests_run + gtest_main + ${Google_Tests_LIBS} + Spreadsheet +) + +add_subdirectory(App)