From c41b92f721bc61f3dd21e56f86557d7cb185197a Mon Sep 17 00:00:00 2001
From: Jesse Plamondon-Willard <Pathoschild@users.noreply.github.com>
Date: Tue, 16 Jun 2020 20:14:27 -0400
Subject: [PATCH] improve new event code

This commit...
* debounces the has-custom-priorities check;
* fixes collection-modified-during-enumeration errors if an event handler is added or removed while the event is being raised;
* fixes Remove(handler) removing all instances of the handler instead of the last one.
---
 src/SMAPI/Framework/Events/ManagedEvent.cs    | 56 +++++++++++--------
 .../Framework/Events/ManagedEventHandler.cs   |  6 --
 2 files changed, 33 insertions(+), 29 deletions(-)

diff --git a/src/SMAPI/Framework/Events/ManagedEvent.cs b/src/SMAPI/Framework/Events/ManagedEvent.cs
index b37fb376..08ac1131 100644
--- a/src/SMAPI/Framework/Events/ManagedEvent.cs
+++ b/src/SMAPI/Framework/Events/ManagedEvent.cs
@@ -14,23 +14,23 @@ namespace StardewModdingAPI.Framework.Events
         /*********
         ** Fields
         *********/
-        /// <summary>The underlying event handlers.</summary>
-        private readonly List<ManagedEventHandler<TEventArgs>> EventHandlers = new List<ManagedEventHandler<TEventArgs>>();
-
         /// <summary>The mod registry with which to identify mods.</summary>
         protected readonly ModRegistry ModRegistry;
 
         /// <summary>Tracks performance metrics.</summary>
         private readonly PerformanceMonitor PerformanceMonitor;
 
+        /// <summary>The underlying event handlers.</summary>
+        private readonly List<ManagedEventHandler<TEventArgs>> Handlers = new List<ManagedEventHandler<TEventArgs>>();
+
+        /// <summary>A cached snapshot of <see cref="Handlers"/>, or <c>null</c> to rebuild it next raise.</summary>
+        private ManagedEventHandler<TEventArgs>[] CachedHandlers = new ManagedEventHandler<TEventArgs>[0];
+
         /// <summary>The total number of event handlers registered for this events, regardless of whether they're still registered.</summary>
         private int RegistrationIndex;
 
-        /// <summary>Whether any registered event handlers have a custom priority value.</summary>
-        private bool HasCustomPriorities;
-
-        /// <summary>Whether event handlers should be sorted before the next invocation.</summary>
-        private bool NeedsSort;
+        /// <summary>Whether new handlers were added since the last raise.</summary>
+        private bool HasNewHandlers;
 
 
         /*********
@@ -62,7 +62,7 @@ namespace StardewModdingAPI.Framework.Events
         /// <summary>Get whether anything is listening to the event.</summary>
         public bool HasListeners()
         {
-            return this.EventHandlers.Count > 0;
+            return this.Handlers.Count > 0;
         }
 
         /// <summary>Add an event handler.</summary>
@@ -73,19 +73,25 @@ namespace StardewModdingAPI.Framework.Events
             EventPriority priority = handler.Method.GetCustomAttribute<EventPriorityAttribute>()?.Priority ?? EventPriority.Normal;
             var managedHandler = new ManagedEventHandler<TEventArgs>(handler, this.RegistrationIndex++, priority, mod);
 
-            this.EventHandlers.Add(managedHandler);
-            this.HasCustomPriorities = this.HasCustomPriorities || managedHandler.HasCustomPriority();
-
-            if (this.HasCustomPriorities)
-                this.NeedsSort = true;
+            this.Handlers.Add(managedHandler);
+            this.CachedHandlers = null;
+            this.HasNewHandlers = true;
         }
 
         /// <summary>Remove an event handler.</summary>
         /// <param name="handler">The event handler.</param>
         public void Remove(EventHandler<TEventArgs> handler)
         {
-            this.EventHandlers.RemoveAll(p => p.Handler == handler);
-            this.HasCustomPriorities = this.HasCustomPriorities && this.EventHandlers.Any(p => p.HasCustomPriority());
+            // match C# events: if a handler is listed multiple times, remove the last one added
+            for (int i = this.Handlers.Count - 1; i >= 0; i--)
+            {
+                if (this.Handlers[i].Handler != handler)
+                    continue;
+
+                this.Handlers.RemoveAt(i);
+                this.CachedHandlers = null;
+                break;
+            }
         }
 
         /// <summary>Raise the event and notify all handlers.</summary>
@@ -94,21 +100,25 @@ namespace StardewModdingAPI.Framework.Events
         public void Raise(TEventArgs args, Func<IModMetadata, bool> match = null)
         {
             // skip if no handlers
-            if (this.EventHandlers.Count == 0)
+            if (this.Handlers.Count == 0)
                 return;
 
-            // sort event handlers by priority
-            // (This is done here to avoid repeatedly sorting when handlers are added/removed.)
-            if (this.NeedsSort)
+            // update cached data
+            // (This is debounced here to avoid repeatedly sorting when handlers are added/removed,
+            // and keeping a separate cached list allows changes during enumeration.)
+            if (this.CachedHandlers == null)
             {
-                this.NeedsSort = false;
-                this.EventHandlers.Sort();
+                if (this.HasNewHandlers && this.Handlers.Any(p => p.Priority != EventPriority.Normal))
+                    this.Handlers.Sort();
+
+                this.CachedHandlers = this.Handlers.ToArray();
+                this.HasNewHandlers = false;
             }
 
             // raise event
             this.PerformanceMonitor.Track(this.EventName, () =>
             {
-                foreach (ManagedEventHandler<TEventArgs> handler in this.EventHandlers)
+                foreach (ManagedEventHandler<TEventArgs> handler in this.CachedHandlers)
                 {
                     if (match != null && !match(handler.SourceMod))
                         continue;
diff --git a/src/SMAPI/Framework/Events/ManagedEventHandler.cs b/src/SMAPI/Framework/Events/ManagedEventHandler.cs
index 87591f63..cf470c1e 100644
--- a/src/SMAPI/Framework/Events/ManagedEventHandler.cs
+++ b/src/SMAPI/Framework/Events/ManagedEventHandler.cs
@@ -39,12 +39,6 @@ namespace StardewModdingAPI.Framework.Events
             this.SourceMod = sourceMod;
         }
 
-        /// <summary>Get whether the event handler has a custom priority value.</summary>
-        public bool HasCustomPriority()
-        {
-            return this.Priority != EventPriority.Normal;
-        }
-
         /// <summary>Compares the current instance with another object of the same type and returns an integer that indicates whether the current instance precedes, follows, or occurs in the same position in the sort order as the other object.</summary>
         /// <param name="obj">An object to compare with this instance.</param>
         /// <exception cref="T:System.ArgumentException"><paramref name="obj" /> is not the same type as this instance.</exception>