From dddeab8e12a7cc062c9c483a09f236c8f9bad4a5 Mon Sep 17 00:00:00 2001
From: YoshiRulz <OSSYoshiRulz@gmail.com>
Date: Tue, 30 Jun 2020 14:25:02 +1000
Subject: [PATCH] Refactor UndoHistory

---
 .../tools/RamSearchEngine/RamSearchEngine.cs  |   8 +-
 src/BizHawk.Common/UndoHistory.cs             | 102 +++++++-----------
 2 files changed, 45 insertions(+), 65 deletions(-)

diff --git a/src/BizHawk.Client.Common/tools/RamSearchEngine/RamSearchEngine.cs b/src/BizHawk.Client.Common/tools/RamSearchEngine/RamSearchEngine.cs
index 42e2adf824..f8093331eb 100644
--- a/src/BizHawk.Client.Common/tools/RamSearchEngine/RamSearchEngine.cs
+++ b/src/BizHawk.Client.Common/tools/RamSearchEngine/RamSearchEngine.cs
@@ -16,7 +16,7 @@ namespace BizHawk.Client.Common.RamSearchEngine
 
 		private List<IMiniWatch> _watchList = new List<IMiniWatch>();
 		private readonly SearchEngineSettings _settings;
-		private readonly UndoHistory<IMiniWatch> _history = new UndoHistory<IMiniWatch>(true);
+		private readonly UndoHistory<IEnumerable<IMiniWatch>> _history = new UndoHistory<IEnumerable<IMiniWatch>>(true, new List<IMiniWatch>()); //TODO use IList instead of IEnumerable and stop calling `.ToList()` (i.e. cloning) on reads and writes?
 		private bool _isSorted = true; // Tracks whether or not the list is sorted by address, if it is, binary search can be used for finding watches
 
 		public RamSearchEngine(SearchEngineSettings settings, IMemoryDomains memoryDomains)
@@ -141,7 +141,7 @@ namespace BizHawk.Client.Common.RamSearchEngine
 
 			if (UndoEnabled)
 			{
-				_history.AddState(_watchList);
+				_history.AddState(_watchList.ToList());
 			}
 
 			return before - _watchList.Count;
@@ -244,7 +244,7 @@ namespace BizHawk.Client.Common.RamSearchEngine
 		{
 			if (UndoEnabled)
 			{
-				_history.AddState(_watchList);
+				_history.AddState(_watchList.ToList());
 			}
 
 			var addresses = watches.Select(w => w.Address);
@@ -255,7 +255,7 @@ namespace BizHawk.Client.Common.RamSearchEngine
 		{
 			if (UndoEnabled)
 			{
-				_history.AddState(_watchList);
+				_history.AddState(_watchList.ToList());
 			}
 
 			var removeList = indices.Select(i => _watchList[i]); // This will fail after int.MaxValue but RAM Search fails on domains that large anyway
diff --git a/src/BizHawk.Common/UndoHistory.cs b/src/BizHawk.Common/UndoHistory.cs
index 66bedf1354..88969c92fb 100644
--- a/src/BizHawk.Common/UndoHistory.cs
+++ b/src/BizHawk.Common/UndoHistory.cs
@@ -1,81 +1,61 @@
 using System.Collections.Generic;
-using System.Linq;
 
 namespace BizHawk.Common
 {
 	public class UndoHistory<T>
 	{
-		private List<List<T>> _history = new List<List<T>>();
-		private int _curPos; // 1-based
+		private readonly T _blankState;
 
-		public int MaxUndoLevels { get; }
+		/// <remarks>
+		/// <c>1</c>-based, so the "current timeline" includes all of <see cref="_history"/> up to and not including <c>_history[_curPos]</c>
+		/// (that is, all of <see cref="_history"/> has been redo'd when <c>_curPos == _history.Count</c>)
+		/// </remarks>
+		private int _curPos;
 
-		public UndoHistory(bool enabled)
-		{
-			MaxUndoLevels = 5;
-			Enabled = enabled;
-		}
-
-		public bool Enabled { get; }
-
-		public bool CanUndo => Enabled && _curPos > 1;
+		private readonly IList<T> _history = new List<T>();
 
 		public bool CanRedo => Enabled && _curPos < _history.Count;
 
-		public bool HasHistory => Enabled && _history.Any();
+		public bool CanUndo => Enabled && _curPos > 1;
+
+		public bool Enabled { get; }
+
+		public bool HasHistory => Enabled && _history.Count != 0;
+
+		/// <remarks>
+		/// <see cref="_history"/> can actually grow to this + 1<br/>
+		/// TODO fix that by moving the <c>.RemoveAt(0)</c> loop in <see cref="AddState"/> to AFTER the insertion<br/>
+		/// TODO old code assumed the setter was public, so pruning multiple states from start may have been required if this changed between insertions
+		/// </remarks>
+		public int MaxUndoLevels { get; } = 5;
+
+		/// <param name="blankState">
+		/// returned from calls to <see cref="Undo"/>/<see cref="Redo"/> when there is nothing to undo/redo, or
+		/// when either method is called while disabled
+		/// </param>
+		public UndoHistory(bool enabled, T blankState)
+		{
+			_blankState = blankState;
+			Enabled = enabled;
+		}
+
+		public void AddState(T newState)
+		{
+			if (!Enabled) return;
+			while (_history.Count > _curPos) _history.RemoveAt(_history.Count - 1); // prune "alternate future timeline" that we're about to overwrite
+			while (_history.Count > MaxUndoLevels) _history.RemoveAt(0); // prune from start when at user-defined limit
+			_history.Add(newState);
+			_curPos = _history.Count;
+		}
 
 		public void Clear()
 		{
-			_history = new List<List<T>>();
+			_history.Clear();
 			_curPos = 0;
 		}
 
-		public void AddState(IEnumerable<T> newState)
-		{
-			if (Enabled)
-			{
-				if (_curPos < _history.Count)
-				{
-					for (var i = _curPos + 1; i <= _history.Count; i++)
-					{
-						_history.Remove(_history[i - 1]);
-					}
-				}
+		public T Redo() => CanRedo && Enabled ? _history[++_curPos - 1] : _blankState;
 
-				// TODO: don't assume we are one over, since MaxUndoLevels is public, it could be set to a small number after a large list has occured
-				if (_history.Count > MaxUndoLevels)
-				{
-					foreach (var item in _history.Take(_history.Count - MaxUndoLevels))
-					{
-						_history.Remove(item);
-					}
-				}
-
-				_history.Add(newState.ToList());
-				_curPos = _history.Count;
-			}
-		}
-
-		public IEnumerable<T> Undo()
-		{
-			if (CanUndo && Enabled)
-			{
-				_curPos--;
-				return _history[_curPos - 1];
-			}
-
-			return Enumerable.Empty<T>();
-		}
-
-		public IEnumerable<T> Redo()
-		{
-			if (CanRedo && Enabled)
-			{
-				_curPos++;
-				return _history[_curPos - 1];
-			}
-			
-			return Enumerable.Empty<T>();
-		}
+		public T Undo() => CanUndo && Enabled ? _history[--_curPos - 1] : _blankState;
 	}
 }