From 8b6f1c994fb1857545c620c8f38a8c2cdb6e8ba8 Mon Sep 17 00:00:00 2001 From: Frederic Devernay Date: Wed, 2 Jun 2021 18:38:23 -0700 Subject: [PATCH] KnobUndoCommand: fix bug + prettier command names bug fixes: - _clones and _knobs were in reverse order - _clones was a list of weak_ptr that were not owned fixes https://github.com/NatronGitHub/Natron/issues/630 --- CHANGELOG.md | 2 ++ Gui/KnobUndoCommand.cpp | 59 ++++++++++++++++++++++++++--------------- Gui/KnobUndoCommand.h | 22 +++++++++++---- 3 files changed, 56 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a8e67ce78..9eb53816d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,9 +16,11 @@ ### Changes - Fix checkerboard drawing on macOS Catalina and later. #614 +- Fix undoing "Reset to default" on parameters #630 ### Plugins +- Transform node now displays the motion path ## Version 2.4.0 diff --git a/Gui/KnobUndoCommand.cpp b/Gui/KnobUndoCommand.cpp index de2c8260be..179c84a60b 100644 --- a/Gui/KnobUndoCommand.cpp +++ b/Gui/KnobUndoCommand.cpp @@ -256,7 +256,7 @@ MultipleKnobEditsUndoCommand::MultipleKnobEditsUndoCommand(const KnobGuiPtr& kno holderName = QString::fromUtf8( effect->getNode()->getLabel().c_str() ); } - setText( tr("Multiple edits for %1").arg(holderName) ); + setText( tr("Multiple edits of %1").arg(holderName) ); } MultipleKnobEditsUndoCommand::~MultipleKnobEditsUndoCommand() @@ -528,9 +528,23 @@ RestoreDefaultsCommand::RestoreDefaultsCommand(bool isNodeReset, , _knobs() { for (std::list::const_iterator it = knobs.begin(); it != knobs.end(); ++it) { - _knobs.push_front(*it); + _knobs.push_back(*it); _clones.push_back( MultipleKnobEditsUndoCommand::createCopyForKnob(*it) ); } + + KnobIPtr first = _knobs.front().lock(); + KnobHolder* holder = first ? first->getHolder() : nullptr; + EffectInstance* effect = dynamic_cast(holder); + QString holderName; + if (effect) { + holderName = QString::fromUtf8( effect->getNode()->getLabel().c_str() ); + } + + if (_knobs.size() == 1) { + setText( tr("Reset %1.%2 to default").arg(holderName).arg( QString::fromUtf8( first->getLabel().c_str() ) ) ); + } else { + setText( tr("Reset %1 to default").arg(holderName) ); + } } void @@ -540,19 +554,25 @@ RestoreDefaultsCommand::undo() std::list times; KnobIPtr first = _knobs.front().lock(); - AppInstancePtr app = first->getHolder()->getApp(); + KnobHolder* holder = first ? first->getHolder() : nullptr; + AppInstancePtr app = holder ? holder->getApp() : nullptr; assert(app); - std::list::const_iterator itClone = _clones.begin(); + std::list::iterator itClone = _clones.begin(); for (std::list::const_iterator it = _knobs.begin(); it != _knobs.end(); ++it, ++itClone) { KnobIPtr itKnob = it->lock(); if (!itKnob) { + // The Knob probably doesn't exist anymore. + // We won't need the clone, so we may as well release it (we own it). + itClone->reset(); + continue; } - KnobIPtr itCloneKnob = itClone->lock(); - if (!itCloneKnob) { + if (!*itClone) { + assert(false); // We own the clone, so it should always be valid + continue; } - itKnob->cloneAndUpdateGui( itCloneKnob.get() ); + itKnob->cloneAndUpdateGui( itClone->get() ); if ( itKnob->getHolder()->getApp() ) { int dim = itKnob->getDimension(); @@ -571,12 +591,10 @@ RestoreDefaultsCommand::undo() } app->addMultipleKeyframeIndicatorsAdded(times, true); - first->getHolder()->incrHashAndEvaluate(true, true); - if ( first->getHolder()->getApp() ) { - first->getHolder()->getApp()->redrawAllViewers(); + holder->incrHashAndEvaluate(true, true); + if ( app ) { + app->redrawAllViewers(); } - - setText( tr("Restore default value(s)") ); } void @@ -584,8 +602,8 @@ RestoreDefaultsCommand::redo() { std::list times; KnobIPtr first = _knobs.front().lock(); - AppInstancePtr app; - KnobHolder* holder = first->getHolder(); + AppInstancePtr app = nullptr; + KnobHolder* holder = first ? first->getHolder() : nullptr; EffectInstance* isEffect = dynamic_cast(holder); if (holder) { @@ -666,14 +684,12 @@ RestoreDefaultsCommand::redo() isEffect->purgeCaches(); } - - if ( first->getHolder() ) { - first->getHolder()->incrHashAndEvaluate(true, true); - if ( first->getHolder()->getApp() ) { - first->getHolder()->getApp()->redrawAllViewers(); + if ( holder ) { + holder->incrHashAndEvaluate(true, true); + if ( app ) { + app->redrawAllViewers(); } } - setText( tr("Restore default value(s)") ); } // RestoreDefaultsCommand::redo SetExpressionCommand::SetExpressionCommand(const KnobIPtr & knob, @@ -693,6 +709,7 @@ SetExpressionCommand::SetExpressionCommand(const KnobIPtr & knob, _oldExprs.push_back( knob->getExpression(i) ); _hadRetVar.push_back( knob->isExpressionUsingRetVariable(i) ); } + setText( tr("Set expression") ); } void @@ -713,7 +730,6 @@ SetExpressionCommand::undo() } knob->evaluateValueChange(_dimension == -1 ? 0 : _dimension, knob->getCurrentTime(), ViewIdx(0), eValueChangedReasonNatronGuiEdited); - setText( tr("Set expression") ); } void @@ -741,7 +757,6 @@ SetExpressionCommand::redo() } } knob->evaluateValueChange(_dimension == -1 ? 0 : _dimension, knob->getCurrentTime(), ViewIdx(0), eValueChangedReasonNatronGuiEdited); - setText( tr("Set expression") ); } NATRON_NAMESPACE_EXIT diff --git a/Gui/KnobUndoCommand.h b/Gui/KnobUndoCommand.h index 71ae6b6dfa..13d969ce97 100644 --- a/Gui/KnobUndoCommand.h +++ b/Gui/KnobUndoCommand.h @@ -49,6 +49,7 @@ CLANG_DIAG_ON(uninitialized) #include "Engine/TimeLine.h" #include "Engine/Curve.h" #include "Engine/Knob.h" +#include "Engine/Node.h" #include "Engine/EffectInstance.h" #include "Engine/ViewIdx.h" @@ -184,8 +185,13 @@ class KnobUndoCommand knobUI->getGui()->getCurveEditor()->getCurveWidget()->refreshSelectedKeysAndUpdate(); } - setText( tr("Set value of %1") - .arg( QString::fromUtf8( knob->getLabel().c_str() ) ) ); + EffectInstance* effect = dynamic_cast(holder); + QString holderName; + if (effect) { + holderName = QString::fromUtf8( effect->getNode()->getLabel().c_str() ); + } + + setText( tr("Set value of %1.%2").arg(holderName).arg( QString::fromUtf8( knob->getLabel().c_str() ) ) ); } // undo virtual void redo() OVERRIDE FINAL @@ -263,8 +269,13 @@ class KnobUndoCommand knobUI->getGui()->getCurveEditor()->getCurveWidget()->refreshSelectedKeysAndUpdate(); } - setText( tr("Set value of %1") - .arg( QString::fromUtf8( knob->getLabel().c_str() ) ) ); + EffectInstance* effect = dynamic_cast(holder); + QString holderName; + if (effect) { + holderName = QString::fromUtf8( effect->getNode()->getLabel().c_str() ); + } + + setText( tr("Set value of %1.%2").arg(holderName).arg( QString::fromUtf8( knob->getLabel().c_str() ) ) ); _firstRedoCalled = true; } // redo @@ -402,7 +413,8 @@ class RestoreDefaultsCommand bool _isNodeReset; int _targetDim; - std::list _knobs, _clones; + std::list _knobs; + std::list _clones; // owned by this }; class SetExpressionCommand