From dbedb21676e35d6352c70dc000c1a760631cfad2 Mon Sep 17 00:00:00 2001 From: Benjamin Nauck Date: Sun, 27 Dec 2020 15:17:23 +0100 Subject: [PATCH 1/3] [Spreadsheet] Only evaluate cell values when prefixed with '=' This commit only changes the user interaction with spreadsheet and does not affect backwards compatibility (as valid cell expressions are prefixed with '=' when serialized). This fixes [#4156](https://tracker.freecadweb.org/view.php?id=4156), which is discussed in the forum thread: https://forum.freecadweb.org/viewtopic.php?f=3&t=39665 There has been additional logic added to handle numbers and simple fractions without using '='. The behaviour is what is expected by the spreadsheet test cases and in line with how other spreadsheet software works. The '-prefix can still be used to force the input to be handled as as string instead. Example of numbers and fractions handled are: 3 2mm 1/8 1mm/2 1/2mm 2/m 1mm/2s More complex expressions are not handled without '=' and will be stored as strings instead, for example: 2 / 3 / 2 1 + 1/3 --- src/Mod/Spreadsheet/App/Cell.cpp | 98 ++++++++++++++++++++++---------- 1 file changed, 69 insertions(+), 29 deletions(-) diff --git a/src/Mod/Spreadsheet/App/Cell.cpp b/src/Mod/Spreadsheet/App/Cell.cpp index 3af0131c24..459ba7db93 100644 --- a/src/Mod/Spreadsheet/App/Cell.cpp +++ b/src/Mod/Spreadsheet/App/Cell.cpp @@ -41,6 +41,7 @@ #include #include "Sheet.h" #include +#include FC_LOG_LEVEL_INIT("Spreadsheet",true,true) @@ -279,12 +280,12 @@ void Cell::afterRestore() { void Cell::setContent(const char * value) { PropertySheet::AtomicPropertyChange signaller(*owner); - App::Expression * expr = 0; + App::Expression * expr = nullptr; clearException(); - if (value != 0) { - if(owner->sheet()->isRestoring()) { - expression.reset(new App::StringExpression(owner->sheet(),value)); + if (value != nullptr) { + if (owner->sheet()->isRestoring()) { + expression.reset(new App::StringExpression(owner->sheet(), value)); setUsed(EXPRESSION_SET, true); return; } @@ -301,40 +302,80 @@ void Cell::setContent(const char * value) expr = new App::StringExpression(owner->sheet(), value + 1); } else if (*value != '\0') { + // check if value is just a number char * end; errno = 0; - double float_value = strtod(value, &end); - if (!*end && errno == 0) { - expr = new App::NumberExpression(owner->sheet(), Quantity(float_value)); + const double float_value = strtod(value, &end); + if (errno == 0) { + const bool isEndEmpty = *end == '\0' || strspn(end, " \t\n\r") == strlen(end); + if (isEndEmpty) { + expr = new App::NumberExpression(owner->sheet(), Quantity(float_value)); + } } - else { + + // if not a float, check if it is a quantity or compatible fraction + const bool isStartingWithNumber = value != end; + if (expr == nullptr && isStartingWithNumber) { try { - expr = ExpressionParser::parse(owner->sheet(), value); - if (expr) - delete expr->eval(); - } - catch (Base::Exception &) { - expr = new App::StringExpression(owner->sheet(), value); + auto parsedExpr = App::ExpressionParser::parse(owner->sheet(), value); + + if (const auto fraction = freecad_dynamic_cast(parsedExpr)) { + if (fraction->getOperator() == OperatorExpression::UNIT) { + const auto left = freecad_dynamic_cast(fraction->getLeft()); + const auto right = freecad_dynamic_cast(fraction->getRight()); + if (left && right) { + expr = parsedExpr; + } + } + else if (fraction->getOperator() == OperatorExpression::DIV) { + // only the following types of fractions are ok: + // 1/2, 1m/2, 1/2s, 1m/2s, 1/m + + // check for numbers in (de)nominator + const bool isNumberNom = freecad_dynamic_cast(fraction->getLeft()); + const bool isNumberDenom = freecad_dynamic_cast(fraction->getRight()); + + // check for numbers with units in (de)nominator + const auto opNom = freecad_dynamic_cast(fraction->getLeft()); + const auto opDenom = freecad_dynamic_cast(fraction->getRight()); + const bool isQuantityNom = opNom && opNom->getOperator() == OperatorExpression::UNIT; + const bool isQuantityDenom = opDenom && opDenom->getOperator() == OperatorExpression::UNIT; + + // check for units in denomainator + const auto uDenom = freecad_dynamic_cast(fraction->getRight()); + const bool isUnitDenom = uDenom && uDenom->getTypeId() == UnitExpression::getClassTypeId(); + + const bool isNomValid = isNumberNom || isQuantityNom; + const bool isDenomValid = isNumberDenom || isQuantityDenom || isUnitDenom; + if (isNomValid && isDenomValid) { + expr = parsedExpr; + } + } + } + else if (const auto number = freecad_dynamic_cast(parsedExpr)) { + // NumbersExpressions can accept more than can be parsed with strtod. + // Example: 12.34 and 12,34 are both valid NumberExpressions + expr = parsedExpr; + } + + if (expr == nullptr) { + delete parsedExpr; + } } + catch (...) {} } } - } - try { - setExpression(App::ExpressionPtr(expr)); - signaller.tryInvoke(); - } - catch (Base::Exception &e) { - if (value) { - std::string _value = value; - if (_value[0] != '=') { - _value.insert (0, 1, '='); - } - - setExpression(App::ExpressionPtr(new App::StringExpression(owner->sheet(), _value))); - setParseException(e.what()); + if (expr == nullptr && *value != '\0') { + expr = new App::StringExpression(owner->sheet(), value); } + + // trying to add an empty string will make expr = nullptr } + + // set expression, or delete the current expression by setting nullptr if empty string was entered + setExpression(App::ExpressionPtr(expr)); + signaller.tryInvoke(); } /** @@ -1020,4 +1061,3 @@ std::string Cell::getFormattedQuantity(void) return result; } - From 98414334ad7456d209275a62ccb8a16038d7af87 Mon Sep 17 00:00:00 2001 From: Benjamin Nauck Date: Tue, 29 Dec 2020 17:55:04 +0100 Subject: [PATCH 2/3] [Spreadsheet] Add unit tests for new input behaviour --- src/Mod/Spreadsheet/TestSpreadsheet.py | 27 ++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/Mod/Spreadsheet/TestSpreadsheet.py b/src/Mod/Spreadsheet/TestSpreadsheet.py index adda58466d..ba6f2a1ec1 100644 --- a/src/Mod/Spreadsheet/TestSpreadsheet.py +++ b/src/Mod/Spreadsheet/TestSpreadsheet.py @@ -668,6 +668,23 @@ class SpreadsheetCases(unittest.TestCase): self.assertEqual(sheet.A17, 0.5) self.assertEqual(sheet.A18, 0.5) + def testQuantitiesAndFractionsAsNumbers(self): + """ Test quantities and simple fractions as numbers """ + sheet = self.doc.addObject('Spreadsheet::Sheet','Spreadsheet') + sheet.set('A1', '1mm') + sheet.set('A2', '1/2') + sheet.set('A3', '4mm/2') + sheet.set('A4', '2/mm') + sheet.set('A5', '4/2mm') + sheet.set('A6', '6mm/3s') + self.doc.recompute() + self.assertEqual(sheet.A1, Units.Quantity('1 mm')) + self.assertEqual(sheet.A2, 0.5) + self.assertEqual(sheet.A3, Units.Quantity('2 mm')) + self.assertEqual(sheet.A4, Units.Quantity('2 1/mm')) + self.assertEqual(sheet.A5, Units.Quantity('2 1/mm')) + self.assertEqual(sheet.A6, Units.Quantity('2 mm/s')) + def testRemoveRows(self): """ Removing rows -- check renaming of internal cells """ sheet = self.doc.addObject('Spreadsheet::Sheet','Spreadsheet') @@ -1034,6 +1051,16 @@ class SpreadsheetCases(unittest.TestCase): self.doc.recompute() self.assertEqual(sheet.get('C1'), Units.Quantity('3 mm')) + def testIssue4156(self): + """ Regression test for issue 4156; necessarily use of leading '=' to enter an expression, creates inconsistent behavior depending on the spreadsheet state""" + sheet = self.doc.addObject('Spreadsheet::Sheet','Spreadsheet') + sheet.set('A3', 'A1') + sheet.set('A1', '1000') + self.doc.recompute() + sheet.set('A3', '') + sheet.set('A3', 'A1') + self.assertEqual(sheet.getContents('A3'), 'A1') + def testInsertRowsAlias(self): """ Regression test for issue 4429; insert rows to sheet with aliases""" sheet = self.doc.addObject('Spreadsheet::Sheet','Spreadsheet') From 24c61836619e46fa0ceabea677c10e58b03947b2 Mon Sep 17 00:00:00 2001 From: Benjamin Nauck Date: Tue, 19 Jan 2021 02:55:49 +0100 Subject: [PATCH 3/3] Spreadsheet: make setContent use unique_ptr and cleanup Make `Cell::setContent` use `unique_ptr` and `make_unique` for expressions to avoid potential memory leaks. Also renames `expo` to `newExpr` to avoid mixup with the member variable `expression`. Both changes was made at the request of @chennes. --- src/Mod/Spreadsheet/App/Cell.cpp | 39 +++++++++++++++----------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/src/Mod/Spreadsheet/App/Cell.cpp b/src/Mod/Spreadsheet/App/Cell.cpp index 459ba7db93..27768b4a8f 100644 --- a/src/Mod/Spreadsheet/App/Cell.cpp +++ b/src/Mod/Spreadsheet/App/Cell.cpp @@ -38,6 +38,7 @@ #include #include #include +#include #include #include "Sheet.h" #include @@ -280,10 +281,10 @@ void Cell::afterRestore() { void Cell::setContent(const char * value) { PropertySheet::AtomicPropertyChange signaller(*owner); - App::Expression * expr = nullptr; + ExpressionPtr newExpr; clearException(); - if (value != nullptr) { + if (value) { if (owner->sheet()->isRestoring()) { expression.reset(new App::StringExpression(owner->sheet(), value)); setUsed(EXPRESSION_SET, true); @@ -291,15 +292,15 @@ void Cell::setContent(const char * value) } if (*value == '=') { try { - expr = App::ExpressionParser::parse(owner->sheet(), value + 1); + newExpr = ExpressionPtr(App::ExpressionParser::parse(owner->sheet(), value + 1)); } catch (Base::Exception & e) { - expr = new App::StringExpression(owner->sheet(), value); + newExpr = std::make_unique(owner->sheet(), value); setParseException(e.what()); } } else if (*value == '\'') { - expr = new App::StringExpression(owner->sheet(), value + 1); + newExpr = std::make_unique(owner->sheet(), value + 1); } else if (*value != '\0') { // check if value is just a number @@ -309,22 +310,22 @@ void Cell::setContent(const char * value) if (errno == 0) { const bool isEndEmpty = *end == '\0' || strspn(end, " \t\n\r") == strlen(end); if (isEndEmpty) { - expr = new App::NumberExpression(owner->sheet(), Quantity(float_value)); + newExpr = std::make_unique(owner->sheet(), Quantity(float_value)); } } // if not a float, check if it is a quantity or compatible fraction const bool isStartingWithNumber = value != end; - if (expr == nullptr && isStartingWithNumber) { + if (!newExpr && isStartingWithNumber) { try { - auto parsedExpr = App::ExpressionParser::parse(owner->sheet(), value); + ExpressionPtr parsedExpr(App::ExpressionParser::parse(owner->sheet(), value)); - if (const auto fraction = freecad_dynamic_cast(parsedExpr)) { + if (const auto fraction = freecad_dynamic_cast(parsedExpr.get())) { if (fraction->getOperator() == OperatorExpression::UNIT) { const auto left = freecad_dynamic_cast(fraction->getLeft()); const auto right = freecad_dynamic_cast(fraction->getRight()); if (left && right) { - expr = parsedExpr; + newExpr = std::move(parsedExpr); } } else if (fraction->getOperator() == OperatorExpression::DIV) { @@ -348,33 +349,29 @@ void Cell::setContent(const char * value) const bool isNomValid = isNumberNom || isQuantityNom; const bool isDenomValid = isNumberDenom || isQuantityDenom || isUnitDenom; if (isNomValid && isDenomValid) { - expr = parsedExpr; + newExpr = std::move(parsedExpr); } } } - else if (const auto number = freecad_dynamic_cast(parsedExpr)) { + else if (const auto number = freecad_dynamic_cast(parsedExpr.get())) { // NumbersExpressions can accept more than can be parsed with strtod. // Example: 12.34 and 12,34 are both valid NumberExpressions - expr = parsedExpr; - } - - if (expr == nullptr) { - delete parsedExpr; + newExpr = std::move(parsedExpr); } } catch (...) {} } } - if (expr == nullptr && *value != '\0') { - expr = new App::StringExpression(owner->sheet(), value); + if (!newExpr && *value != '\0') { + newExpr = std::make_unique(owner->sheet(), value); } - // trying to add an empty string will make expr = nullptr + // trying to add an empty string will make newExpr = nullptr } // set expression, or delete the current expression by setting nullptr if empty string was entered - setExpression(App::ExpressionPtr(expr)); + setExpression(std::move(newExpr)); signaller.tryInvoke(); }