Skip to content

Commit

Permalink
App: fix dynamic property undo/redo
Browse files Browse the repository at this point in the history
Instead of enforce property type match when undo/redo, modify various
property Paste() to make it type safe.
  • Loading branch information
realthunder authored and wwmayer committed Oct 7, 2019
1 parent 890bc90 commit 41387fd
Show file tree
Hide file tree
Showing 6 changed files with 37 additions and 24 deletions.
6 changes: 3 additions & 3 deletions src/App/PropertyExpressionEngine.cpp
Expand Up @@ -147,17 +147,17 @@ void PropertyExpressionEngine::hasSetValue()

void PropertyExpressionEngine::Paste(const Property &from)
{
const PropertyExpressionEngine * fromee = static_cast<const PropertyExpressionEngine*>(&from);
const PropertyExpressionEngine &fromee = dynamic_cast<const PropertyExpressionEngine&>(from);

AtomicPropertyChange signaller(*this);

expressions.clear();
for(auto &e : fromee->expressions) {
for(auto &e : fromee.expressions) {
expressions[e.first] = ExpressionInfo(
boost::shared_ptr<Expression>(e.second.expression->copy()));
expressionChanged(e.first);
}
validator = fromee->validator;
validator = fromee.validator;
signaller.tryInvoke();
}

Expand Down
36 changes: 24 additions & 12 deletions src/App/Transactions.cpp
Expand Up @@ -292,7 +292,7 @@ void TransactionObject::applyNew(Document & /*Doc*/, TransactionalObject * /*pcO
{
}

void TransactionObject::applyChn(Document & /*Doc*/, TransactionalObject *pcObj, bool Forward)
void TransactionObject::applyChn(Document & /*Doc*/, TransactionalObject *pcObj, bool /* Forward */)
{
if (status == New || status == Chn) {
// Property change order is not preserved, as it is recursive in nature
Expand Down Expand Up @@ -333,17 +333,29 @@ void TransactionObject::applyChn(Document & /*Doc*/, TransactionalObject *pcObj,
prop->setStatusValue(data.property->getStatus());
}
}
// Because we now allow undo/redo dynamic property adding/removing,
// we have to enforce property type checking before calling Copy/Paste.
if(data.propertyType != prop->getTypeId()) {
FC_WARN("Cannot " << (Forward?"redo":"undo")
<< " change of property " << prop->getName()
<< " because of type change: "
<< data.propertyType.getName()
<< " -> " << prop->getTypeId().getName());
continue;
}
prop->Paste(*data.property);

// Many properties do not bother implement Copy() and accepts
// derived types just fine in Paste(). So we do not enforce type
// matching here. But instead, strengthen type checking in all
// Paste() implementation.
//
// if(data.propertyType != prop->getTypeId()) {
// FC_WARN("Cannot " << (Forward?"redo":"undo")
// << " change of property " << prop->getName()
// << " because of type change: "
// << data.propertyType.getName()
// << " -> " << prop->getTypeId().getName());
// continue;
// }
try {
prop->Paste(*data.property);
} catch (Base::Exception &e) {
e.ReportException();
FC_ERR("exception while restoring " << prop->getFullName() << ": " << e.what());
} catch (std::exception &e) {
FC_ERR("exception while restoring " << prop->getFullName() << ": " << e.what());
} catch (...)
{}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/Mod/Spreadsheet/App/PropertyColumnWidths.cpp
Expand Up @@ -63,7 +63,7 @@ App::Property *PropertyColumnWidths::Copy() const

void PropertyColumnWidths::Paste(const App::Property &from)
{
setValues(static_cast<const PropertyColumnWidths&>(from).getValues());
setValues(dynamic_cast<const PropertyColumnWidths&>(from).getValues());
}

void PropertyColumnWidths::setValues(const std::map<int,int> &values) {
Expand Down
2 changes: 1 addition & 1 deletion src/Mod/Spreadsheet/App/PropertyRowHeights.cpp
Expand Up @@ -56,7 +56,7 @@ App::Property *PropertyRowHeights::Copy() const

void PropertyRowHeights::Paste(const Property &from)
{
setValues(static_cast<const PropertyRowHeights&>(from).getValues());
setValues(dynamic_cast<const PropertyRowHeights&>(from).getValues());
}

void PropertyRowHeights::setValues(const std::map<int,int> &values) {
Expand Down
10 changes: 5 additions & 5 deletions src/Mod/Spreadsheet/App/PropertySheet.cpp
Expand Up @@ -224,9 +224,9 @@ App::Property *PropertySheet::Copy(void) const

void PropertySheet::Paste(const Property &from)
{
AtomicPropertyChange signaller(*this);
const PropertySheet &froms = dynamic_cast<const PropertySheet&>(from);

const PropertySheet * froms = static_cast<const PropertySheet*>(&from);
AtomicPropertyChange signaller(*this);

std::map<CellAddress, Cell* >::iterator icurr = data.begin();

Expand All @@ -236,8 +236,8 @@ void PropertySheet::Paste(const Property &from)
++icurr;
}

std::map<CellAddress, Cell* >::const_iterator ifrom = froms->data.begin();
while (ifrom != froms->data.end()) {
std::map<CellAddress, Cell* >::const_iterator ifrom = froms.data.begin();
while (ifrom != froms.data.end()) {
std::map<CellAddress, Cell* >::iterator i = data.find(ifrom->first);

if (i != data.end()) {
Expand Down Expand Up @@ -269,7 +269,7 @@ void PropertySheet::Paste(const Property &from)
++icurr;
}

mergedCells = froms->mergedCells;
mergedCells = froms.mergedCells;
signaller.tryInvoke();
}

Expand Down
5 changes: 3 additions & 2 deletions src/Mod/Spreadsheet/App/Sheet.cpp
Expand Up @@ -1470,9 +1470,10 @@ Property *PropertySpreadsheetQuantity::Copy() const

void PropertySpreadsheetQuantity::Paste(const Property &from)
{
const auto &src = dynamic_cast<const PropertySpreadsheetQuantity&>(from);
aboutToSetValue();
_dValue = static_cast<const PropertySpreadsheetQuantity*>(&from)->_dValue;
_Unit = static_cast<const PropertySpreadsheetQuantity*>(&from)->_Unit;
_dValue = src._dValue;
_Unit = src._Unit;
hasSetValue();
}

Expand Down

0 comments on commit 41387fd

Please sign in to comment.