Merge pull request #17315 from mwganson/ssclearall

[Spreadsheet] avoid removing user dynamic properties when clearing ce…

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<const char*> colstr = cm[1];
+            const boost::sub_match<const char*> 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<const char*> colstr = cm[1];
-            const boost::sub_match<const char*> 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

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<App::CellAddress> getUsedCells() const;
 
     std::tuple<App::CellAddress, App::CellAddress> getUsedRange() const;

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<std::string> 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();

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()
 
 # -------------------------
 

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()

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}
+)

tests/src/Mod/Spreadsheet/App/PropertySheet.cpp

@@ -0,0 +1,84 @@
+// SPDX-License-Identifier: LGPL-2.1-or-later
+
+#include <gtest/gtest.h>
+#include "src/App/InitApplication.h"
+
+#include <memory>
+
+#include <Mod/Spreadsheet/App/Sheet.h>
+#include <Mod/Spreadsheet/App/PropertySheet.h>
+
+class PropertySheetTest: public ::testing::Test
+{
+protected:
+    static void SetUpTestSuite()
+    {
+        tests::initApplication();
+    }
+    void SetUp() override
+    {
+        _sheet = std::make_unique<Spreadsheet::Sheet>();
+        _propertySheet = std::make_unique<Spreadsheet::PropertySheet>(_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<Spreadsheet::Sheet> _sheet;
+    std::unique_ptr<Spreadsheet::PropertySheet> _propertySheet;
+};
+
+TEST_F(PropertySheetTest, isValidCellAddressNameValidNames)  // NOLINT
+{
+    std::vector<std::string> 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<std::string> 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<std::string> 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<std::string> 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";
+    }
+}

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)