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 #630
NatronGitHub · Jun 3, 2021 · 8b6f1c9 · 8b6f1c9
1 parent 4a79426
commit 8b6f1c9
Show file tree

Hide file tree

Showing 3 changed files with 56 additions and 27 deletions.
diff --git 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
@@ -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<KnobIPtr>::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<EffectInstance*>(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<SequenceTime> 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<KnobIWPtr>::const_iterator itClone = _clones.begin();
+    std::list<KnobIPtr>::iterator itClone = _clones.begin();
     for (std::list<KnobIWPtr>::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,21 +591,19 @@ 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
 RestoreDefaultsCommand::redo()
 {
     std::list<SequenceTime> 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<EffectInstance*>(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
@@ -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<EffectInstance*>(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<EffectInstance*>(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<KnobIWPtr> _knobs, _clones;
+    std::list<KnobIWPtr> _knobs;
+    std::list<KnobIPtr> _clones;  // owned by this
 };
 
 class SetExpressionCommand