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

@@ -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<CellAddress, std::string>::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<CellAddress, Cell*>::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<CellAddress, std::string>::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<CellAddress, std::string>::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<App::ObjectIdentifier, App::ObjectIdentifier> & renames)
 {
     std::map<CellAddress, Cell*>::const_iterator i = data.find(currPos);
@@ -626,8 +642,10 @@ void PropertySheet::moveCell(CellAddress currPos, CellAddress newPos, std::map<A
 
     AtomicPropertyChange signaller(*this);
 
-    if (j != data.end())
-        clear(newPos);
+    if (j != data.end()) {
+        // do not clear alias because we have moved them already
+        clear(newPos, false);
+    }
 
     if (i != data.end()) {
         Cell * cell = i->second;
@@ -639,12 +657,6 @@ void PropertySheet::moveCell(CellAddress currPos, CellAddress newPos, std::map<A
         // Remove merged cell data
         splitCell(currPos);
 
-        std::string alias;
-        if(cell->getAlias(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::map<A
 
         addDependencies(newPos);
 
-        if(alias.size())
-            cell->setAlias(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<CellAddress>::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<CellAddress>::const_reverse_iterator i = keys.rbegin(); i != keys.rend(); ++i) {
         std::map<CellAddress, Cell*>::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<CellAddress>::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<CellAddress>::const_iterator i = keys.begin(); i != keys.end(); ++i) {
         std::map<CellAddress, Cell*>::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<CellAddress>::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<CellAddress>::const_reverse_iterator i = keys.rbegin(); i != keys.rend(); ++i) {
         std::map<CellAddress, Cell*>::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<CellAddress>::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<CellAddress>::const_iterator i = keys.begin(); i != keys.end(); ++i) {
         std::map<CellAddress, Cell*>::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);
     }

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<App::ObjectIdentifier, App::ObjectIdentifier> &renames);
-
     void pasteCells(const std::map<App::CellAddress, std::string> &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<App::ObjectIdentifier, App::ObjectIdentifier> &renames);
+
     /*
      * Cell dependency tracking
      */

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