Skip to content

Commit

Permalink
Move the aliases before other content of cells
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@cheuksan @wwmayer
cheuksan authored and wwmayer committed Sep 13, 2020
1 parent 1a7fd88 commit 0aa759b
Show file tree
Hide file tree
Showing 3 changed files with 119 additions and 23 deletions.
81 changes: 61 additions & 20 deletions src/Mod/Spreadsheet/App/PropertySheet.cpp
View file
Expand Up @@ -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);

Expand All @@ -607,27 +617,35 @@ 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);
std::map<CellAddress, Cell*>::const_iterator j = data.find(newPos);

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;
Expand All @@ -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);
Expand All @@ -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());
Expand All @@ -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);

Expand Down Expand Up @@ -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);

Expand All @@ -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);
}
Expand All @@ -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);

Expand Down Expand Up @@ -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);

Expand All @@ -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);
}
Expand Down
10 changes: 7 additions & 3 deletions src/Mod/Spreadsheet/App/PropertySheet.h
View file
Expand Up @@ -98,7 +98,7 @@ class SpreadsheetExport PropertySheet : public App::PropertyExpressionContainer

void setSpans(App::CellAddress address, int rows, int columns);

void clear(App::CellAddress address);
void clear(App::CellAddress address, bool toClearAlias=true);

void clear();

Expand Down Expand Up @@ -126,8 +126,6 @@ class SpreadsheetExport PropertySheet : public App::PropertyExpressionContainer

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);
Expand Down Expand Up @@ -216,6 +214,12 @@ class SpreadsheetExport PropertySheet : public App::PropertyExpressionContainer
/*! 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
*/
Expand Down
51 changes: 51 additions & 0 deletions src/Mod/Spreadsheet/TestSpreadsheet.py
View file
Expand Up @@ -1017,6 +1017,57 @@ def testIssue3432(self):
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
Expand Down

0 comments on commit 0aa759b

Please sign in to comment.