From 0aa759b5c55bb9449d6eef7d38cc3ab4ef3f404f Mon Sep 17 00:00:00 2001 From: Cheuksan Wang Date: Sat, 12 Sep 2020 17:38:03 -0700 Subject: [PATCH] Move the aliases before other content of cells When a user performs insert rows, remove rows, insert columns, or remove rows, we need to move multiple cells as a batch. The cells are moved sequentially. For each cell, its dependent alias positions are looked up and dependencies are added. However, those cells with aliases may be moved later in the batch. Thus the earlier dependencies become wrong. This commit fixes this bug by moving all the aliases before moving the cells. Unit tests are added to for this bug. fixes issue #4429 --- src/Mod/Spreadsheet/App/PropertySheet.cpp | 81 +++++++++++++++++------ src/Mod/Spreadsheet/App/PropertySheet.h | 10 ++- src/Mod/Spreadsheet/TestSpreadsheet.py | 51 ++++++++++++++ 3 files changed, 119 insertions(+), 23 deletions(-) diff --git a/src/Mod/Spreadsheet/App/PropertySheet.cpp b/src/Mod/Spreadsheet/App/PropertySheet.cpp index 4ec7464c6c..10643a2f5d 100644 --- a/src/Mod/Spreadsheet/App/PropertySheet.cpp +++ b/src/Mod/Spreadsheet/App/PropertySheet.cpp @@ -588,7 +588,17 @@ void PropertySheet::setSpans(CellAddress address, int rows, int columns) nonNullCellAt(address)->setSpans(rows, columns); } -void PropertySheet::clear(CellAddress address) +void PropertySheet::clearAlias(CellAddress address) +{ + // Remove alias if it exists + std::map::iterator j = aliasProp.find(address); + if (j != aliasProp.end()) { + revAliasProp.erase(j->second); + aliasProp.erase(j); + } +} + +void PropertySheet::clear(CellAddress address, bool toClearAlias) { std::map::iterator i = data.find(address); @@ -607,18 +617,24 @@ void PropertySheet::clear(CellAddress address) // Mark as dirty dirty.insert(i->first); - // Remove alias if it exists - std::map::iterator j = aliasProp.find(address); - if (j != aliasProp.end()) { - revAliasProp.erase(j->second); - aliasProp.erase(j); - } + if (toClearAlias) + clearAlias(address); // Erase from internal struct data.erase(i); signaller.tryInvoke(); } +void PropertySheet::moveAlias(CellAddress currPos, CellAddress newPos) +{ + std::map::iterator j = aliasProp.find(currPos); + if (j != aliasProp.end()) { + aliasProp[newPos] = j->second; + revAliasProp[j->second] = newPos; + aliasProp.erase(currPos); + } +} + void PropertySheet::moveCell(CellAddress currPos, CellAddress newPos, std::map & renames) { std::map::const_iterator i = data.find(currPos); @@ -626,8 +642,10 @@ void PropertySheet::moveCell(CellAddress currPos, CellAddress newPos, std::mapsecond; @@ -639,12 +657,6 @@ void PropertySheet::moveCell(CellAddress currPos, CellAddress newPos, std::mapgetAlias(alias)) { - owner->aliasRemoved(currPos, alias); - cell->setAlias(""); - } - // Remove from old removeDependencies(currPos); data.erase(currPos); @@ -664,9 +676,6 @@ void PropertySheet::moveCell(CellAddress currPos, CellAddress newPos, std::mapsetAlias(alias); - setDirty(newPos); renames[ObjectIdentifier(owner, currPos.toString())] = ObjectIdentifier(owner, newPos.toString()); @@ -689,6 +698,13 @@ void PropertySheet::insertRows(int row, int count) CellAddress(row, CellAddress::MAX_COLUMNS), count, 0); AtomicPropertyChange signaller(*this); + + // move all the aliases first so dependencies can be calculated correctly + for (std::vector::const_reverse_iterator i = keys.rbegin(); i != keys.rend(); ++i) { + if (i->row() >= row) + moveAlias(*i, CellAddress(i->row() + count, i->col())); + } + for (std::vector::const_reverse_iterator i = keys.rbegin(); i != keys.rend(); ++i) { std::map::iterator j = data.find(*i); @@ -740,6 +756,15 @@ void PropertySheet::removeRows(int row, int count) CellAddress(row + count - 1, CellAddress::MAX_COLUMNS), -count, 0); AtomicPropertyChange signaller(*this); + + // move all the aliases first so dependencies can be calculated correctly + for (std::vector::const_iterator i = keys.begin(); i != keys.end(); ++i) { + if (i->row() >= row && i->row() < row + count) + clearAlias(*i); + else if (i->row() >= row + count) + moveAlias(*i, CellAddress(i->row() - count, i->col())); + } + for (std::vector::const_iterator i = keys.begin(); i != keys.end(); ++i) { std::map::iterator j = data.find(*i); @@ -756,7 +781,7 @@ void PropertySheet::removeRows(int row, int count) } if (i->row() >= row && i->row() < row + count) - clear(*i); + clear(*i, false); // aliases were cleared earlier else if (i->row() >= row + count) moveCell(*i, CellAddress(i->row() - count, i->col()), renames); } @@ -781,6 +806,13 @@ void PropertySheet::insertColumns(int col, int count) CellAddress(CellAddress::MAX_ROWS, col), 0, count); AtomicPropertyChange signaller(*this); + + // move all the aliases first so dependencies can be calculated correctly + for (std::vector::const_reverse_iterator i = keys.rbegin(); i != keys.rend(); ++i) { + if (i->col() >= col) + moveAlias(*i, CellAddress(i->row(), i->col() + count)); + } + for (std::vector::const_reverse_iterator i = keys.rbegin(); i != keys.rend(); ++i) { std::map::iterator j = data.find(*i); @@ -832,6 +864,15 @@ void PropertySheet::removeColumns(int col, int count) CellAddress(CellAddress::MAX_ROWS, col + count - 1), 0, -count); AtomicPropertyChange signaller(*this); + + // move all the aliases first so dependencies can be calculated correctly + for (std::vector::const_iterator i = keys.begin(); i != keys.end(); ++i) { + if (i->col() >= col && i->col() < col + count) + clearAlias(*i); + else if (i->col() >= col + count) + moveAlias(*i, CellAddress(i->row(), i->col() - count)); + } + for (std::vector::const_iterator i = keys.begin(); i != keys.end(); ++i) { std::map::iterator j = data.find(*i); @@ -848,7 +889,7 @@ void PropertySheet::removeColumns(int col, int count) } if (i->col() >= col && i->col() < col + count) - clear(*i); + clear(*i, false); // aliases were cleared earlier else if (i->col() >= col + count) moveCell(*i, CellAddress(i->row(), i->col() - count), renames); } diff --git a/src/Mod/Spreadsheet/App/PropertySheet.h b/src/Mod/Spreadsheet/App/PropertySheet.h index 54844087e9..fa5327fe6d 100644 --- a/src/Mod/Spreadsheet/App/PropertySheet.h +++ b/src/Mod/Spreadsheet/App/PropertySheet.h @@ -98,7 +98,7 @@ public: void setSpans(App::CellAddress address, int rows, int columns); - void clear(App::CellAddress address); + void clear(App::CellAddress address, bool toClearAlias=true); void clear(); @@ -126,8 +126,6 @@ public: bool isDirty() const { return dirty.size() > 0; } - void moveCell(App::CellAddress currPos, App::CellAddress newPos, std::map &renames); - void pasteCells(const std::map &cells, int rowOffset, int colOffset); void insertRows(int row, int count); @@ -216,6 +214,12 @@ private: /*! Owner of this property */ Sheet * owner; + void clearAlias(App::CellAddress address); + + void moveAlias(App::CellAddress currPos, App::CellAddress newPos); + + void moveCell(App::CellAddress currPos, App::CellAddress newPos, std::map &renames); + /* * Cell dependency tracking */ diff --git a/src/Mod/Spreadsheet/TestSpreadsheet.py b/src/Mod/Spreadsheet/TestSpreadsheet.py index 0f347f973e..0b6cee4efd 100644 --- a/src/Mod/Spreadsheet/TestSpreadsheet.py +++ b/src/Mod/Spreadsheet/TestSpreadsheet.py @@ -1017,6 +1017,57 @@ class SpreadsheetCases(unittest.TestCase): self.doc.recompute() self.assertEqual(sheet.get('C1'), Units.Quantity('3 mm')) + def testInsertRowsAlias(self): + """ Regression test for issue 4429; insert rows to sheet with aliases""" + sheet = self.doc.addObject('Spreadsheet::Sheet','Spreadsheet') + sheet.set('A3', '1') + sheet.setAlias('A3', 'alias1') + sheet.set('A4', '=alias1 + 1') + sheet.setAlias('A4', 'alias2') + sheet.set('A5', '=alias2 + 1') + self.doc.recompute() + sheet.insertRows('1', 1) + self.doc.recompute() + self.assertEqual(sheet.A6, 3) + + def testInsertColumnsAlias(self): + """ Regression test for issue 4429; insert columns to sheet with aliases""" + sheet = self.doc.addObject('Spreadsheet::Sheet','Spreadsheet') + sheet.set('C1', '1') + sheet.setAlias('C1', 'alias1') + sheet.set('D1', '=alias1 + 1') + sheet.setAlias('D1', 'alias2') + sheet.set('E1', '=alias2 + 1') + self.doc.recompute() + sheet.insertColumns('A', 1) + self.doc.recompute() + self.assertEqual(sheet.F1, 3) + + def testRemoveRowsAlias(self): + """ Regression test for issue 4429; remove rows from sheet with aliases""" + sheet = self.doc.addObject('Spreadsheet::Sheet','Spreadsheet') + sheet.set('A3', '1') + sheet.setAlias('A3', 'alias1') + sheet.set('A5', '=alias1 + 1') + sheet.setAlias('A5', 'alias2') + sheet.set('A4', '=alias2 + 1') + self.doc.recompute() + sheet.removeRows('1', 1) + self.doc.recompute() + self.assertEqual(sheet.A3, 3) + + def testRemoveColumnsAlias(self): + """ Regression test for issue 4429; remove columns from sheet with aliases""" + sheet = self.doc.addObject('Spreadsheet::Sheet','Spreadsheet') + sheet.set('C1', '1') + sheet.setAlias('C1', 'alias1') + sheet.set('E1', '=alias1 + 1') + sheet.setAlias('E1', 'alias2') + sheet.set('D1', '=alias2 + 1') + self.doc.recompute() + sheet.removeColumns('A', 1) + self.doc.recompute() + self.assertEqual(sheet.C1, 3) def tearDown(self): #closing doc