feat: PubSub improvement: ISubscriber unsub [MTT-2765] (#612)

pdeschain · fernando-cortez · SamuelBellomo · web-flow · commit 5f1bcccfeb03 · 2022-09-29T20:52:38.000Z
* ISubscriber now contains Unsubscribe method

* Used the new API to unsubscribe directly from an ISubscriber - better readability in trivial cases

* added changelog

* Update Assets/Scripts/Gameplay/UI/IPConnectionWindow.cs

Co-authored-by: Fernando Cortez &lt;fernando.cortez@unity3d.com&gt;

* Addressing comments

* removed unnecessary assert in MessageChannel Unsubscribe and replaced it with conditional

Co-authored-by: Fernando Cortez &lt;fernando.cortez@unity3d.com&gt;
Co-authored-by: Sam Bellomo &lt;71790295+SamuelBellomo@users.noreply.github.com&gt;
diff --git a/Assets/Scripts/Gameplay/GameState/ServerBossRoomState.cs b/Assets/Scripts/Gameplay/GameState/ServerBossRoomState.cs
@@ -59,8 +59,6 @@ public class ServerBossRoomState : GameStateBehaviour
         /// </summary>
         [Inject] ISubscriber<LifeStateChangedEventMessage> m_LifeStateChangedEventMessageSubscriber;
 
-        IDisposable m_Subscription;
-
         [Inject] ConnectionManager m_ConnectionManager;
         [Inject] PersistentGameState m_PersistentGameState;
 
@@ -79,7 +77,7 @@ void OnNetworkSpawn()
                 return;
             }
             m_PersistentGameState.Reset();
-            m_Subscription = m_LifeStateChangedEventMessageSubscriber.Subscribe(OnLifeStateChangedEventMessage);
+            m_LifeStateChangedEventMessageSubscriber.Subscribe(OnLifeStateChangedEventMessage);
 
             NetworkManager.Singleton.OnClientDisconnectCallback += OnClientDisconnect;
             NetworkManager.Singleton.SceneManager.OnLoadEventCompleted += OnLoadEventCompleted;
@@ -90,16 +88,15 @@ void OnNetworkSpawn()
 
         void OnNetworkDespawn()
         {
-            m_Subscription?.Dispose();
-
+            m_LifeStateChangedEventMessageSubscriber?.Unsubscribe(OnLifeStateChangedEventMessage);
             NetworkManager.Singleton.OnClientDisconnectCallback -= OnClientDisconnect;
             NetworkManager.Singleton.SceneManager.OnLoadEventCompleted -= OnLoadEventCompleted;
             NetworkManager.Singleton.SceneManager.OnSynchronizeComplete -= OnSynchronizeComplete;
         }
 
         protected override void OnDestroy()
         {
-            m_Subscription?.Dispose();
+            m_LifeStateChangedEventMessageSubscriber?.Unsubscribe(OnLifeStateChangedEventMessage);
 
             if (m_NetcodeHooks)
             {
diff --git a/Assets/Scripts/Gameplay/UI/IPConnectionWindow.cs b/Assets/Scripts/Gameplay/UI/IPConnectionWindow.cs
@@ -20,12 +20,13 @@ public class IPConnectionWindow : MonoBehaviour
 
         [Inject] IPUIMediator m_IPUIMediator;
 
-        IDisposable m_Subscription;
+        ISubscriber<ConnectStatus> m_ConnectStatusSubscriber;
 
         [Inject]
         void InjectDependencies(ISubscriber<ConnectStatus> connectStatusSubscriber)
         {
-            m_Subscription = connectStatusSubscriber.Subscribe(OnConnectStatusMessage);
+            m_ConnectStatusSubscriber = connectStatusSubscriber;
+            m_ConnectStatusSubscriber.Subscribe(OnConnectStatusMessage);
         }
 
         void Awake()
@@ -35,7 +36,7 @@ void Awake()
 
         void OnDestroy()
         {
-            m_Subscription.Dispose();
+            m_ConnectStatusSubscriber?.Unsubscribe(OnConnectStatusMessage);
         }
 
         void OnConnectStatusMessage(ConnectStatus connectStatus)
diff --git a/Assets/Scripts/Gameplay/UI/IPUIMediator.cs b/Assets/Scripts/Gameplay/UI/IPUIMediator.cs
@@ -41,12 +41,13 @@ public class IPUIMediator : MonoBehaviour
 
         public IPHostingUI IPHostingUI => m_IPHostingUI;
 
-        IDisposable m_Subscription;
+        ISubscriber<ConnectStatus> m_ConnectStatusSubscriber;
 
         [Inject]
         void InjectDependencies(ISubscriber<ConnectStatus> connectStatusSubscriber)
         {
-            m_Subscription = connectStatusSubscriber.Subscribe(OnConnectStatusMessage);
+            m_ConnectStatusSubscriber = connectStatusSubscriber;
+            m_ConnectStatusSubscriber.Subscribe(OnConnectStatusMessage);
         }
 
         void Awake()
@@ -63,7 +64,7 @@ void Start()
 
         void OnDestroy()
         {
-            m_Subscription.Dispose();
+            m_ConnectStatusSubscriber?.Unsubscribe(OnConnectStatusMessage);
         }
 
         void OnConnectStatusMessage(ConnectStatus connectStatus)
diff --git a/Assets/Scripts/Gameplay/UI/Lobby/LobbyJoiningUI.cs b/Assets/Scripts/Gameplay/UI/Lobby/LobbyJoiningUI.cs
@@ -28,7 +28,8 @@ public class LobbyJoiningUI : MonoBehaviour
         IObjectResolver m_Container;
         LobbyUIMediator m_LobbyUIMediator;
         UpdateRunner m_UpdateRunner;
-        IDisposable m_Subscriptions;
+        ISubscriber<LobbyListFetchedMessage> m_LocalLobbiesRefreshedSub;
+
         List<LobbyListItemUI> m_LobbyListItems = new List<LobbyListItemUI>();
 
         void Awake()
@@ -40,13 +41,13 @@ void OnDisable()
         {
             if (m_UpdateRunner != null)
             {
-                m_UpdateRunner.Unsubscribe(PeriodicRefresh);
+                m_UpdateRunner?.Unsubscribe(PeriodicRefresh);
             }
         }
 
         void OnDestroy()
         {
-            m_Subscriptions?.Dispose();
+            m_LocalLobbiesRefreshedSub?.Unsubscribe(UpdateUI);
         }
 
         [Inject]
@@ -59,7 +60,8 @@ void InjectDependenciesAndInitialize(
             m_Container = container;
             m_LobbyUIMediator = lobbyUIMediator;
             m_UpdateRunner = updateRunner;
-            m_Subscriptions = localLobbiesRefreshedSub.Subscribe(UpdateUI);
+            m_LocalLobbiesRefreshedSub = localLobbiesRefreshedSub;
+            m_LocalLobbiesRefreshedSub.Subscribe(UpdateUI);
         }
 
         /// <summary>
diff --git a/Assets/Scripts/Gameplay/UI/Lobby/LobbyUIMediator.cs b/Assets/Scripts/Gameplay/UI/Lobby/LobbyUIMediator.cs
@@ -29,7 +29,7 @@ public class LobbyUIMediator : MonoBehaviour
         LocalLobby m_LocalLobby;
         NameGenerationData m_NameGenerationData;
         ConnectionManager m_ConnectionManager;
-        IDisposable m_Subscriptions;
+        ISubscriber<ConnectStatus> m_ConnectStatusSubscriber;
 
         const string k_DefaultLobbyName = "no-name";
 
@@ -50,10 +50,10 @@ ConnectionManager connectionManager
             m_LobbyServiceFacade = lobbyServiceFacade;
             m_LocalLobby = localLobby;
             m_ConnectionManager = connectionManager;
-
+            m_ConnectStatusSubscriber = connectStatusSub;
             RegenerateName();
 
-            m_Subscriptions = connectStatusSub.Subscribe(OnConnectStatus);
+            m_ConnectStatusSubscriber.Subscribe(OnConnectStatus);
         }
 
         void OnConnectStatus(ConnectStatus status)
@@ -66,7 +66,7 @@ void OnConnectStatus(ConnectStatus status)
 
         void OnDestroy()
         {
-            m_Subscriptions?.Dispose();
+            m_ConnectStatusSubscriber?.Unsubscribe(OnConnectStatus);
         }
 
         //Lobby and Relay calls done from UI
diff --git a/Assets/Scripts/Gameplay/UI/UnityServicesUIHandler.cs b/Assets/Scripts/Gameplay/UI/UnityServicesUIHandler.cs
@@ -10,7 +10,7 @@ namespace Unity.BossRoom.Gameplay.UI
 {
     public class UnityServicesUIHandler : MonoBehaviour
     {
-        IDisposable m_Subscriptions;
+        ISubscriber<UnityServiceErrorMessage> m_ServiceErrorSubscription;
 
         void Awake()
         {
@@ -20,7 +20,8 @@ void Awake()
         [Inject]
         void Initialize(ISubscriber<UnityServiceErrorMessage> serviceError)
         {
-            m_Subscriptions = serviceError.Subscribe(ServiceErrorHandler);
+            m_ServiceErrorSubscription = serviceError;
+            m_ServiceErrorSubscription.Subscribe(ServiceErrorHandler);
         }
 
         void ServiceErrorHandler(UnityServiceErrorMessage error)
@@ -86,7 +87,7 @@ void HandleLobbyError(UnityServiceErrorMessage error)
 
         void OnDestroy()
         {
-            m_Subscriptions?.Dispose();
+            m_ServiceErrorSubscription?.Unsubscribe(ServiceErrorHandler);
         }
     }
 }
diff --git a/Assets/Scripts/Infrastructure/PubSub/IMessageChannel.cs b/Assets/Scripts/Infrastructure/PubSub/IMessageChannel.cs
@@ -10,12 +10,12 @@ public interface IPublisher<T>
     public interface ISubscriber<T>
     {
         IDisposable Subscribe(Action<T> handler);
+        void Unsubscribe(Action<T> handler);
     }
 
     public interface IMessageChannel<T> : IPublisher<T>, ISubscriber<T>, IDisposable
     {
         bool IsDisposed { get; }
-        void Unsubscribe(Action<T> handler);
     }
 
     public interface IBufferedMessageChannel<T> : IMessageChannel<T>
diff --git a/Assets/Scripts/Infrastructure/PubSub/MessageChannel.cs b/Assets/Scripts/Infrastructure/PubSub/MessageChannel.cs
@@ -68,18 +68,19 @@ public virtual IDisposable Subscribe(Action<T> handler)
 
         public void Unsubscribe(Action<T> handler)
         {
-            Assert.IsTrue(IsSubscribed(handler), "Attempting to unsubscribe with a handler that is not subscribed");
-
-            if (m_PendingHandlers.ContainsKey(handler))
+            if (IsSubscribed(handler))
             {
-                if (m_PendingHandlers[handler])
+                if (m_PendingHandlers.ContainsKey(handler))
                 {
-                    m_PendingHandlers.Remove(handler);
+                    if (m_PendingHandlers[handler])
+                    {
+                        m_PendingHandlers.Remove(handler);
+                    }
+                }
+                else
+                {
+                    m_PendingHandlers[handler] = false;
                 }
-            }
-            else
-            {
-                m_PendingHandlers[handler] = false;
             }
         }
 
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -19,7 +19,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 * NetworkObjectSpawner handles dynamically spawning in-scene placed NetworkObjects (#717) - You can't place a NetworkObject in scene directly and destroy it at runtime. This PR showcases proper handling of NetworkObjects that you'd wish to place inside of scenes, but would still want to destroy at game-time. Examples of these are: Imps, VandalImps, ImpBoss. NetworkObjects such as doors, crystals, door switch, etc. remain the same, statically-placed in scene.
 * Quality levels settings set up for Desktop [MTT-4450] (#713)
 * Added custom RNSM config with graph for RTT instead of single value (#747)
-
+* Added Unsubscribe API for the ISubscriber<T> along with refactoring of the codebase to use this API instead of IDisposable handle when there is just one subscription (#612)
 ### Changed
 * Updated tools, authentication and relay packages (#690)
 * Replaced our dependency injection solution with VContainer. (#679)

Original file line number	Diff line number	Diff line change
`@@ -59,8 +59,6 @@ public class ServerBossRoomState : GameStateBehaviour`
`59`	`59`	`/// </summary>`
`60`	`60`	`[Inject] ISubscriber<LifeStateChangedEventMessage> m_LifeStateChangedEventMessageSubscriber;`
`61`	`61`
`62`		`- IDisposable m_Subscription;`
`63`		`-`
`64`	`62`	`[Inject] ConnectionManager m_ConnectionManager;`
`65`	`63`	`[Inject] PersistentGameState m_PersistentGameState;`
`66`	`64`
`@@ -79,7 +77,7 @@ void OnNetworkSpawn()`
`79`	`77`	`return;`
`80`	`78`	`}`
`81`	`79`	`m_PersistentGameState.Reset();`
`82`		`- m_Subscription = m_LifeStateChangedEventMessageSubscriber.Subscribe(OnLifeStateChangedEventMessage);`
	`80`	`+ m_LifeStateChangedEventMessageSubscriber.Subscribe(OnLifeStateChangedEventMessage);`
`83`	`81`
`84`	`82`	`NetworkManager.Singleton.OnClientDisconnectCallback += OnClientDisconnect;`
`85`	`83`	`NetworkManager.Singleton.SceneManager.OnLoadEventCompleted += OnLoadEventCompleted;`
`@@ -90,16 +88,15 @@ void OnNetworkSpawn()`
`90`	`88`
`91`	`89`	`void OnNetworkDespawn()`
`92`	`90`	`{`
`93`		`- m_Subscription?.Dispose();`
`94`		`-`
	`91`	`+ m_LifeStateChangedEventMessageSubscriber?.Unsubscribe(OnLifeStateChangedEventMessage);`
`95`	`92`	`NetworkManager.Singleton.OnClientDisconnectCallback -= OnClientDisconnect;`
`96`	`93`	`NetworkManager.Singleton.SceneManager.OnLoadEventCompleted -= OnLoadEventCompleted;`
`97`	`94`	`NetworkManager.Singleton.SceneManager.OnSynchronizeComplete -= OnSynchronizeComplete;`
`98`	`95`	`}`
`99`	`96`
`100`	`97`	`protected override void OnDestroy()`
`101`	`98`	`{`
`102`		`- m_Subscription?.Dispose();`
	`99`	`+ m_LifeStateChangedEventMessageSubscriber?.Unsubscribe(OnLifeStateChangedEventMessage);`
`103`	`100`
`104`	`101`	`if (m_NetcodeHooks)`
`105`	`102`	`{`
Original file line number	Diff line number	Diff line change
`@@ -20,12 +20,13 @@ public class IPConnectionWindow : MonoBehaviour`
`20`	`20`
`21`	`21`	`[Inject] IPUIMediator m_IPUIMediator;`
`22`	`22`
`23`		`- IDisposable m_Subscription;`
	`23`	`+ ISubscriber<ConnectStatus> m_ConnectStatusSubscriber;`
`24`	`24`
`25`	`25`	`[Inject]`
`26`	`26`	`void InjectDependencies(ISubscriber<ConnectStatus> connectStatusSubscriber)`
`27`	`27`	`{`
`28`		`- m_Subscription = connectStatusSubscriber.Subscribe(OnConnectStatusMessage);`
	`28`	`+ m_ConnectStatusSubscriber = connectStatusSubscriber;`
	`29`	`+ m_ConnectStatusSubscriber.Subscribe(OnConnectStatusMessage);`
`29`	`30`	`}`
`30`	`31`
`31`	`32`	`void Awake()`
`@@ -35,7 +36,7 @@ void Awake()`
`35`	`36`
`36`	`37`	`void OnDestroy()`
`37`	`38`	`{`
`38`		`- m_Subscription.Dispose();`
	`39`	`+ m_ConnectStatusSubscriber?.Unsubscribe(OnConnectStatusMessage);`
`39`	`40`	`}`
`40`	`41`
`41`	`42`	`void OnConnectStatusMessage(ConnectStatus connectStatus)`
Original file line number	Diff line number	Diff line change
`@@ -41,12 +41,13 @@ public class IPUIMediator : MonoBehaviour`
`41`	`41`
`42`	`42`	`public IPHostingUI IPHostingUI => m_IPHostingUI;`
`43`	`43`
`44`		`- IDisposable m_Subscription;`
	`44`	`+ ISubscriber<ConnectStatus> m_ConnectStatusSubscriber;`
`45`	`45`
`46`	`46`	`[Inject]`
`47`	`47`	`void InjectDependencies(ISubscriber<ConnectStatus> connectStatusSubscriber)`
`48`	`48`	`{`
`49`		`- m_Subscription = connectStatusSubscriber.Subscribe(OnConnectStatusMessage);`
	`49`	`+ m_ConnectStatusSubscriber = connectStatusSubscriber;`
	`50`	`+ m_ConnectStatusSubscriber.Subscribe(OnConnectStatusMessage);`
`50`	`51`	`}`
`51`	`52`
`52`	`53`	`void Awake()`
`@@ -63,7 +64,7 @@ void Start()`
`63`	`64`
`64`	`65`	`void OnDestroy()`
`65`	`66`	`{`
`66`		`- m_Subscription.Dispose();`
	`67`	`+ m_ConnectStatusSubscriber?.Unsubscribe(OnConnectStatusMessage);`
`67`	`68`	`}`
`68`	`69`
`69`	`70`	`void OnConnectStatusMessage(ConnectStatus connectStatus)`
Original file line number	Diff line number	Diff line change
`@@ -28,7 +28,8 @@ public class LobbyJoiningUI : MonoBehaviour`
`28`	`28`	`IObjectResolver m_Container;`
`29`	`29`	`LobbyUIMediator m_LobbyUIMediator;`
`30`	`30`	`UpdateRunner m_UpdateRunner;`
`31`		`- IDisposable m_Subscriptions;`
	`31`	`+ ISubscriber<LobbyListFetchedMessage> m_LocalLobbiesRefreshedSub;`
	`32`	`+`
`32`	`33`	`List<LobbyListItemUI> m_LobbyListItems = new List<LobbyListItemUI>();`
`33`	`34`
`34`	`35`	`void Awake()`
`@@ -40,13 +41,13 @@ void OnDisable()`
`40`	`41`	`{`
`41`	`42`	`if (m_UpdateRunner != null)`
`42`	`43`	`{`
`43`		`- m_UpdateRunner.Unsubscribe(PeriodicRefresh);`
	`44`	`+ m_UpdateRunner?.Unsubscribe(PeriodicRefresh);`
`44`	`45`	`}`
`45`	`46`	`}`
`46`	`47`
`47`	`48`	`void OnDestroy()`
`48`	`49`	`{`
`49`		`- m_Subscriptions?.Dispose();`
	`50`	`+ m_LocalLobbiesRefreshedSub?.Unsubscribe(UpdateUI);`
`50`	`51`	`}`
`51`	`52`
`52`	`53`	`[Inject]`
`@@ -59,7 +60,8 @@ void InjectDependenciesAndInitialize(`
`59`	`60`	`m_Container = container;`
`60`	`61`	`m_LobbyUIMediator = lobbyUIMediator;`
`61`	`62`	`m_UpdateRunner = updateRunner;`
`62`		`- m_Subscriptions = localLobbiesRefreshedSub.Subscribe(UpdateUI);`
	`63`	`+ m_LocalLobbiesRefreshedSub = localLobbiesRefreshedSub;`
	`64`	`+ m_LocalLobbiesRefreshedSub.Subscribe(UpdateUI);`
`63`	`65`	`}`
`64`	`66`
`65`	`67`	`/// <summary>`
Original file line number	Diff line number	Diff line change
`@@ -10,7 +10,7 @@ namespace Unity.BossRoom.Gameplay.UI`
`10`	`10`	`{`
`11`	`11`	`public class UnityServicesUIHandler : MonoBehaviour`
`12`	`12`	`{`
`13`		`- IDisposable m_Subscriptions;`
	`13`	`+ ISubscriber<UnityServiceErrorMessage> m_ServiceErrorSubscription;`
`14`	`14`
`15`	`15`	`void Awake()`
`16`	`16`	`{`
`@@ -20,7 +20,8 @@ void Awake()`
`20`	`20`	`[Inject]`
`21`	`21`	`void Initialize(ISubscriber<UnityServiceErrorMessage> serviceError)`
`22`	`22`	`{`
`23`		`- m_Subscriptions = serviceError.Subscribe(ServiceErrorHandler);`
	`23`	`+ m_ServiceErrorSubscription = serviceError;`
	`24`	`+ m_ServiceErrorSubscription.Subscribe(ServiceErrorHandler);`
`24`	`25`	`}`
`25`	`26`
`26`	`27`	`void ServiceErrorHandler(UnityServiceErrorMessage error)`
`@@ -86,7 +87,7 @@ void HandleLobbyError(UnityServiceErrorMessage error)`
`86`	`87`
`87`	`88`	`void OnDestroy()`
`88`	`89`	`{`
`89`		`- m_Subscriptions?.Dispose();`
	`90`	`+ m_ServiceErrorSubscription?.Unsubscribe(ServiceErrorHandler);`
`90`	`91`	`}`
`91`	`92`	`}`
`92`	`93`	`}`
Original file line number	Diff line number	Diff line change
`@@ -10,12 +10,12 @@ public interface IPublisher<T>`
`10`	`10`	`public interface ISubscriber<T>`
`11`	`11`	`{`
`12`	`12`	`IDisposable Subscribe(Action<T> handler);`
	`13`	`+ void Unsubscribe(Action<T> handler);`
`13`	`14`	`}`
`14`	`15`
`15`	`16`	`public interface IMessageChannel<T> : IPublisher<T>, ISubscriber<T>, IDisposable`
`16`	`17`	`{`
`17`	`18`	`bool IsDisposed { get; }`
`18`		`- void Unsubscribe(Action<T> handler);`
`19`	`19`	`}`
`20`	`20`
`21`	`21`	`public interface IBufferedMessageChannel<T> : IMessageChannel<T>`
Original file line number	Diff line number	Diff line change
`@@ -68,18 +68,19 @@ public virtual IDisposable Subscribe(Action<T> handler)`
`68`	`68`
`69`	`69`	`public void Unsubscribe(Action<T> handler)`
`70`	`70`	`{`
`71`		`- Assert.IsTrue(IsSubscribed(handler), "Attempting to unsubscribe with a handler that is not subscribed");`
`72`		`-`
`73`		`- if (m_PendingHandlers.ContainsKey(handler))`
	`71`	`+ if (IsSubscribed(handler))`
`74`	`72`	`{`
`75`		`- if (m_PendingHandlers[handler])`
	`73`	`+ if (m_PendingHandlers.ContainsKey(handler))`
`76`	`74`	`{`
`77`		`- m_PendingHandlers.Remove(handler);`
	`75`	`+ if (m_PendingHandlers[handler])`
	`76`	`+ {`
	`77`	`+ m_PendingHandlers.Remove(handler);`
	`78`	`+ }`
	`79`	`+ }`
	`80`	`+ else`
	`81`	`+ {`
	`82`	`+ m_PendingHandlers[handler] = false;`
`78`	`83`	`}`
`79`		`- }`
`80`		`- else`
`81`		`- {`
`82`		`- m_PendingHandlers[handler] = false;`
`83`	`84`	`}`
`84`	`85`	`}`
`85`	`86`