I am attempting to make an undo system, where when a property on an object that implements INotifyPropertyChanged is changed, the property name and its old value is pushed onto a stack via a KeyValuePair. When the user clicks "Undo" it then pops from the stack and uses reflection to set the property's value to its old value. The problem with this is that it calls OnPropertyChanged again, so the property and its restored value is added to the undo stack a second time. On the other hand, I still want it to call OnPropertyChanged since I want the view to update its bindings. There's obviously something wrong with how I'm designing it, but I can't seem to figure out another way of going about it.

Here's my model

internal class MyModel : INotifyPropertyChangedExtended
{
    private string testProperty1 = "";

    public string TestProperty1
    {
        get { return testProperty1; }
        set {
            var oldValue = testProperty1;
            testProperty1 = value;
            OnPropertyChanged(nameof(TestProperty1), oldValue);
        }
    }
    
    private string testProperty2 = "";

    public string TestProperty2
    {
        get { return testProperty2; }
        set {
            var oldValue = testProperty2;
            testProperty2 = value;
            OnPropertyChanged(nameof(TestProperty2), oldValue);
        }
    }
    
    public event PropertyChangedEventHandler? PropertyChanged;
    
    public void OnPropertyChanged(string propertyName, object oldValue)
    {
        if (PropertyChanged != null)
        {
            PropertyChanged(this, new PropertyChangedEventArgsExtended(propertyName, oldValue));
        }
    }
}

Here's my INotifyPropertyChangedExtended interface

public class PropertyChangedEventArgsExtended : PropertyChangedEventArgs
{
    public virtual object OldValue { get; private set; }

    public PropertyChangedEventArgsExtended(string propertyName, object oldValue)
           : base(propertyName)
    {
        OldValue = oldValue;
    }
}

public class INotifyPropertyChangedExtended : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler PropertyChanged;
    protected void NotifyPropertyChanged(string propertyName, object oldValue)
    {
        if (PropertyChanged != null)
            PropertyChanged(this, new PropertyChangedEventArgsExtended(propertyName, oldValue));
    }
}

And here's my view model

internal class MyViewModel
{
    public MyModel MyModel { get; set; } = new();
    
    public Stack<KeyValuePair<string, object>> PropertyStateStack = new();
    public RelayCommand Undo { get; set; }

    public MyViewModel()
    {
        SetupCommands();

        MyModel.PropertyChanged  = MyModel_PropertyChanged;
    }

    private void MyModel_PropertyChanged(object? sender, System.ComponentModel.PropertyChangedEventArgs e)
    {
        var args = e as PropertyChangedEventArgsExtended;

        if (args.OldValue != null)
        {
            PropertyStateStack.Push(new KeyValuePair<string, object>(args.PropertyName, args.OldValue));
        }
    }
    
    private void SetupCommands()
    {
        Undo = new RelayCommand(o =>
        {
            KeyValuePair<string, object> propertyState = PropertyStateStack.Pop();
            PropertyInfo? property = MyModel.GetType().GetProperty(propertyState.Key);

            if (property != null)
            {
                property.SetValue(MyModel, Convert.ChangeType(propertyState.Value, property.PropertyType), null);
            }
        });
    }
}

EDIT: I did research the "memento pattern" but I couldn't get it to work with INotifyPropertyChanged, since as soon as I set MyModel to a backup of it the bindings to the view stopped working.

CodePudding user response：

Implementing Memento or a variant is the right way. Opposed to storing the particular modifying undo action e.g., Action<T> (another good solution), Memento has a higher memory footprint (as it stores the complete object state), but allows random access to the stored states.

The key point is that when implementing Memento properly, you don't have to rely on reflection, which will only make your code slow and heavy.

The following example uses the IEditableObject interface to implement the Memento pattern (variant). The implementation supports undo and redo. The TextBox class is implementing undo/redo in a similar way using the same interface. The advantage is that you have full control over when to record the object's state. You can even cancel the ongoing modification.

This example clones the complete object to backup the state. Because objects can be quite expensive, for example when they allocate resources, it could make sense to introduce an immutable data model that actually stores the values of the public editable properties. Now, instead of cloning the complete object you would only clone the immutable data model. This can improve the performance in critical scenarios.
See the example provided by the IEditableObject link above to learn how to introduce an immutable data model that holds the object's data.

The actual undo/redo logic is encapsulated in the example's abstract StateTracker<TStateObject> class. StateTracker<TStateObject> implements the aforementioned IEditableObject and the ICloneable interface. To add convenience, StateTracker<TStateObject> also implements a custom IUndoable interface (to enable anonymous usage of the public undo/redo API).

Every class that needs to support state tracking (undo/redo) must extend the abstract StateTracker<TStateObject> to provide a ICloneable.Clone and a StateTracker.UpdateState implementation.

The following example is very basic. It allows undo and redo, but does not support random access to undo/redo states. You would have to use an index based backing store like List<T> to implement such a feature.

IUndoable.cs
Enable anonymous access to the undo/redo API.

public interface IUndoable
{
  bool TryUndo();
  bool TryRedo();
}

StateTracker.cs
Encapsulates the actual undo/redo logic to avoid duplicate implementations
for each type that is supposed to support undo/redo.

You can consider to add a public UndoCommand and RedoCommand to this class and let the commands invoke TryUndo and TryRedo respectively.

public abstract class StateTracker<TStateObject> : IEditableObject, IUndoable, ICloneable
{
  public bool IsInEditMode { get; private set; }
  private Stack<TStateObject> UndoMemory { get; }
  private Stack<TStateObject> RedoMemory { get; }
  private TStateObject StateBeforeEdit { get; set; }
  private bool IsUpdatingState { get; set; }

  protected StateTracker()
  {
    this.UndoMemory = new Stack<TStateObject>();
    this.RedoMemory = new Stack<TStateObject>();
  }

  public abstract TStateObject Clone();
  protected abstract void UpdateState(TStateObject state);

  object ICloneable.Clone() => Clone();

  public bool TryUndo()
  {
    if (!this.UndoMemory.TryPop(out TStateObject previousState))
    {
      return false;
    }

    this.IsUpdatingState = true;

    this.StateBeforeEdit = Clone();
    this.RedoMemory.Push(this.StateBeforeEdit);

    UpdateState(previousState);

    this.IsUpdatingState = false;

    return true;
  }

  public bool TryRedo()
  {
    if (!this.RedoMemory.TryPop(out TStateObject nextState))
    {
      return false;
    }

    this.IsUpdatingState = true;

    this.StateBeforeEdit = Clone();
    this.UndoMemory.Push(this.StateBeforeEdit);

    UpdateState(nextState);

    this.IsUpdatingState = false;

    return true;
  }

  // Start recording the changes
  public void BeginEdit()
  {
    if (this.IsInEditMode || this.IsUpdatingState)
    {
      return;
    }

    this.IsInEditMode = true;

    // Create the snapshot before the instance is changed
    this.StateBeforeEdit = Clone();
  }

  // Abort recording the changes
  public void CancelEdit()
  {
    if (!this.IsInEditMode)
    {
      return;
    }

    // Restore the original state
    UpdateState(this.StateBeforeEdit);

    this.IsInEditMode = false;
  }

  // Commit recorded changes
  public void EndEdit()
  {
    if (!this.IsInEditMode || this.IsUpdatingState)
    {
      return;
    }

    // Commit the snapshot of the original state after the instance was changed without cancellation
    this.UndoMemory.Push(this.StateBeforeEdit);

    this.IsInEditMode = false;
  }
}

MyModel.cs

public class MyModel : StateTracker<MyModel>, INotifyPropertyChanged
{
  public event PropertyChangedEventHandler PropertyChanged;

  public MyModel()
  {
  }

  // Copy constructor
  private MyModel(MyModel originalInstance)
  {
    // Don't raise PropertyChanged to avoid the loop of death
    this.testProperty1 = originalInstance.TestProperty1;
    this.testProperty2 = originalInstance.TestProperty2;
  }

  // Create a deep copy using the copy constructor
  public override MyModel Clone()
  {
    var copyOfInstance = new MyModel(this);
    return copyOfInstance;
  }

  protected override void UpdateState(MyModel state)
  {
    // UpdateState() is called by the StateTracker
    // which internally guards against the infinite loop
    this.TestProperty1 = state.TestProperty1;
    this.TestProperty2 = state.TestProperty2;
  }

  private void OnPropertyChanged([CallerMemberName] string propertyName = null)
    => this.PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));

  private string testProperty1;
  public string TestProperty1
  {
    get => this.testProperty1;
    set
    {
      this.testProperty1 = value;
      OnPropertyChanged();
    }
  }

  private string testProperty2;
  public string TestProperty2
  {
    get => this.testProperty2;
    set
    {
      this.testProperty2 = value;
      OnPropertyChanged();
    }
  }
}

Example

The following example stores the state of a TextBox, that binds to a MyModel instance. When the TextBox receives focus, the MyModel.BeginEdit method is called to start recording the input. When the TextBox loses focus the recorded state is pushed onto the undo stack by calling the MyModel.EndEdit method.

MainWindow.xaml

<Window>
  <Window.DataContext>
    <local:MyModel />
  </Window.DataContext>

  <StackPanel>
    <Button Content="Undo"
            Click="OnUndoButtonClick" />
    <Button Content="Redo"
            Click="OnRedoButtonClick" />

    <TextBox Text="{Binding TestProperty1, UpdateSourceTrigger=PropertyChanged}" 
             GotFocus="OnTextBoxGotFocus" 
             LostFocus="OnTextBoxLostFocus" />
  </StackPanel>
</Window>

MainWindow.xaml.cs
Because of the defined interfaces we can handle undo/redo without knowing the actual data type.

private void OnTextBoxGotFocus(object sender, RoutedEventArgs e) 
  => ((sender as FrameworkElement).DataContext as IEditableObject).BeginEdit();

private void OnTextBoxLostFocus(object sender, RoutedEventArgs e) 
  => ((sender as FrameworkElement).DataContext as IEditableObject).EndEdit();

private void OnUndoButtonClick(object sender, RoutedEventArgs e) 
  => _ = ((sender as FrameworkElement).DataContext as IUndoable).TryUndo();

private void OnRedoButtonClick(object sender, RoutedEventArgs e) 
  => _ = ((sender as FrameworkElement).DataContext as IUndoable).TryRedo();

An alternative flow could be that the MyModel class internally calls BeginEdit and EndEdit inside the relevant property setters (before accepting the new value and after accepting the new value). In case of the TextBox, the advantage of this solution is that it allows to record every single input.
In this scenario, the GotFocus and LostFocus event handlers previously defined on the TextBox (example above) are not needed and related code must be removed:

MyModel.cs

public class MyModel : StateTracker<MyModel>, INotifyPropertyChanged
{
  public event PropertyChangedEventHandler PropertyChanged;

  public MyModel()
  {
  }

  // Copy constructor
  private MyModel(MyModel originalInstance)
  {
    // Don't raise PropertyChanged to avoid the loop of death
    this.testProperty1 = originalInstance.TestProperty1;
    this.testProperty2 = originalInstance.TestProperty2;
  }

  // Create a deep copy using the copy constructor
  public override MyModel Clone()
  {
    var copyOfInstance = new MyModel(this);
    return copyOfInstance;
  }

  protected override void UpdateState(MyModel state)
  {
    // UpdateState() is called by the StateTracker
    // which internally guards against the infinite loop
    this.TestProperty1 = state.TestProperty1;
    this.TestProperty2 = state.TestProperty2;
  }

  private void RecordPropertyChange<TValue>(ref TValue backingField, TValue newValue)
  {
    BeginEdit();
    backingField = newValue;
    EndEdit();
  }

  private void OnPropertyChanged([CallerMemberName] string propertyName = null)
    => this.PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));

  private string testProperty1;
  public string TestProperty1
  {
    get => this.testProperty1;
    set
    {
      RecordPropertyChange(ref this.testProperty1, value);
      OnPropertyChanged();
    }
  }

  private string testProperty2;
  public string TestProperty2
  {
    get => this.testProperty2;
    set
    {
      RecordPropertyChange(ref this.testProperty2, value);
      OnPropertyChanged();
    }
  }
}

Remarks

If extending StateTracker is not an option (e.g., because it would introduce a multi-inheritance issue), you can always make use of composition (for example add a private property of type StateTracker to your undoable model to replace inheritance).
Just create a new class that extends StateTracker to implement the abstract members. Then define a private property of this new type in your undoable model. Now, let the model reference this private property to access the undo/redo API.

While composition is to be favored, this example chooses inheritance as this concept feels more natural to most. It may helps to understand the basic idea.