feat: report the outcome of a scene's openExplorerUi request back to it

PBExplorerUiEventsResult is appended to the scene root entity, so a scene that asked
for a panel learns that it actually appeared and, later, that it went away -- including
dismissals it never asked for, like the player pressing Escape. The set is grow-only, so
two events surviving the same tick is ordinary rather than a defect; the scene reads them
as a (prevMax, curMax] window the way input.ts does for pointer results, which is also
why timestamp carries the tick number and nothing finer.

The events are emitted from inside ExplorerUiActions.OpenSectionAsync rather than from
IMVCManager.OnViewShowed/OnViewClosed, and that is the decision worth spelling out. The
proto's ui field is a section -- EU_MAP, EU_BACKPACK, ... -- while MVC's life cycle unit
is the whole ExplorePanelController with seven tabs inside it. Switching tabs raises no
MVC event at all and the section on screen lives in a private field, so an MVC-sourced
implementation had nothing true to put in ui. Taken from the request, ui is correct by
construction. This does not close the door on reporting opens the user initiated: a
grow-only oneof takes new variants without a schema change.

OpenSection answers the scene on its JS thread, so it asks MVC a second time after
switching to the main thread and reports nothing if the user opened the panel in between.
ShowAsync returns without doing anything for a controller that is not hidden, so the
alternative is a reported open for a panel this scene never opened, followed by a close
that never comes. The closed event is enqueued from a finally around ShowAsync, which
resolves when the panel goes down however it got there.

The hand-off is a plain Queue<ExplorerUiEvent> on ECSWorldInstanceSharedDependencies,
built per scene next to EntityEventsBuilder and drained by WriteExplorerUiEventsSystem in
SyncedPreRenderingSystemGroup. Both ends are main-thread-only -- the producer enqueues
after SwitchToMainThread, the consumer is an ECS system -- so it needs no locking, and a
late enqueue cannot throw once ECSWorldFacade.Dispose has taken the world down. It sits in
ECS.Unity beside WriteEngineInfoSystem, the existing scene-root CRDT writer injected by
ECSWorldFactory, because DCL.Social does not reference the CRDT assembly and cannot see
anything placed next to IECSToCRDTWriter.

IExplorerUiActions.OpenSection now takes the protocol value alongside the section it maps
to. There is no reverse mapping and the two enums are not value-compatible, so the value
is carried down rather than recovered.

ExplorerUiActionsShould gains the pair, the two ways nothing is reported, and the close
that has to survive a show which does not end normally; its panel-state arrangement moved
off a positional Returns sequence because every accepted request now asks MVC twice.
WriteExplorerUiEventsSystemShould covers the drain, and ExplorerUiEventsResultShould
already pinned the append itself.

Author: popuz

Explorer/Assets/DCL/Infrastructure/CrdtEcsBridge/JsModulesImplementation/RestrictedActions/ExplorerUi/ExplorerUiActions.cs

@@ -2,11 +2,14 @@
 using DCL.Communities;
 using DCL.CrdtEcsBridge.JsModulesImplementation;
 using DCL.Diagnostics;
+using DCL.ECSComponents;
 using DCL.ExplorePanel;
 using DCL.UI;
 using Decentraland.Kernel.Apis;
+using ECS.Unity.ExplorerUiEvents;
 using MVC;
 using System;
+using System.Collections.Generic;
 
 namespace DCL.Infrastructure.CrdtEcsBridge.JsModulesImplementation.RestrictedActions
 {
@@ -18,13 +21,15 @@ namespace DCL.Infrastructure.CrdtEcsBridge.JsModulesImplementation.RestrictedAct
     public class ExplorerUiActions : IExplorerUiActions
     {
         private readonly IMVCManager mvcManager;
+        private readonly Queue<ExplorerUiEvent> events;
 
-        public ExplorerUiActions(IMVCManager mvcManager)
+        public ExplorerUiActions(IMVCManager mvcManager, Queue<ExplorerUiEvent> events)
         {
             this.mvcManager = mvcManager;
+            this.events = events;
         }
 
-        public OpenExplorerUiResult OpenSection(ExploreSections section)
+        public OpenExplorerUiResult OpenSection(ExplorerUi ui, ExploreSections section)
         {
             // Communities availability depends on the user identity (feature flag + wallets allowlist),
             // so it cannot be gated through FeaturesRegistry like the other sections.
@@ -37,16 +42,28 @@ public OpenExplorerUiResult OpenSection(ExploreSections section)
             if (mvcManager.IsShowing<ExplorePanelView, ExplorePanelParameter>())
                 return OpenExplorerUiResult.WasAlreadyOpen;
 
-            OpenSectionAsync(section).Forget();
+            OpenSectionAsync(ui, section).Forget();
             return OpenExplorerUiResult.Opened;
         }
 
-        private async UniTask OpenSectionAsync(ExploreSections section)
+        private async UniTask OpenSectionAsync(ExplorerUi ui, ExploreSections section)
         {
             try
             {
                 await UniTask.SwitchToMainThread();
-                await mvcManager.ShowAsync(ExplorePanelController.IssueCommand(new ExplorePanelParameter(section)));
+
+                // The answer given to the scene was decided on its JS thread; by now the user may have opened
+                // the panel themselves, and ShowAsync does nothing for a controller that is not hidden.
+                if (mvcManager.IsShowing<ExplorePanelView, ExplorePanelParameter>())
+                    return;
+
+                // ShowAsync resolves when the panel closes, so the pair brackets its whole life cycle. The
+                // opened event goes out before the await because there is no later moment that still means
+                // "shown".
+                events.Enqueue(new ExplorerUiEvent(ui, ExplorerUiEventKind.Opened));
+
+                try { await mvcManager.ShowAsync(ExplorePanelController.IssueCommand(new ExplorePanelParameter(section))); }
+                finally { events.Enqueue(new ExplorerUiEvent(ui, ExplorerUiEventKind.Closed)); }
             }
             catch (OperationCanceledException) { }
             catch (Exception e) { ReportHub.LogException(e, ReportCategory.RESTRICTED_ACTIONS); }

Explorer/Assets/DCL/Infrastructure/CrdtEcsBridge/JsModulesImplementation/RestrictedActions/IExplorerUiActions.cs

@@ -1,10 +1,16 @@
+using DCL.ECSComponents;
 using DCL.UI;
 using Decentraland.Kernel.Apis;
 
 namespace DCL.CrdtEcsBridge.JsModulesImplementation
 {
     public interface IExplorerUiActions
     {
-        OpenExplorerUiResult OpenSection(ExploreSections section);
+        /// <summary>
+        ///     Opens the explore panel on <paramref name="section" />. <paramref name="ui" /> is the protocol
+        ///     value the request came in with: the section is what MVC needs, the protocol value is what the
+        ///     scene gets its life cycle events tagged with, and neither maps onto the other.
+        /// </summary>
+        OpenExplorerUiResult OpenSection(ExplorerUi ui, ExploreSections section);
     }
 }

Explorer/Assets/DCL/Infrastructure/CrdtEcsBridge/JsModulesImplementation/RestrictedActions/RestrictedActionsAPIImplementation.cs

@@ -236,7 +236,7 @@ public int TryOpenExplorerUi(int ui)
                 return (int)OpenExplorerUiResult.RejectedFeatureDisabled;
             }
 
-            return (int)explorerUiActions.OpenSection(section);
+            return (int)explorerUiActions.OpenSection((ExplorerUi)ui, section);
         }
 
         public void Dispose() { }

Explorer/Assets/DCL/Infrastructure/CrdtEcsBridge/JsModulesImplementation/RestrictedActions/Tests/ExplorerUiActionsShould.cs

@@ -1,61 +1,124 @@
+using Cysharp.Threading.Tasks;
+using DCL.ECSComponents;
 using DCL.ExplorePanel;
 using DCL.Infrastructure.CrdtEcsBridge.JsModulesImplementation.RestrictedActions;
 using DCL.UI;
 using Decentraland.Kernel.Apis;
+using ECS.Unity.ExplorerUiEvents;
 using MVC;
 using NSubstitute;
 using NUnit.Framework;
+using System.Collections.Generic;
+using System.Threading;
 
 namespace CrdtEcsBridge.RestrictedActions.Tests
 {
     /// <summary>
     ///     Covers how <see cref="ExplorerUiActions" /> chooses between opening the explore panel and answering
-    ///     the scene that it was already open. One instance exists per scene, so it cannot have witnessed the
-    ///     panel opening before it was built, and MVC skips OnViewClosed when a view's lifecycle is cancelled —
-    ///     the panel state therefore has to be read from MVC at the moment of the decision.
+    ///     the scene that it was already open, and the life cycle events it reports back for the requests it
+    ///     did accept. One instance exists per scene, so it cannot have witnessed the panel opening before it
+    ///     was built, and MVC skips OnViewClosed when a view's lifecycle is cancelled — the panel state
+    ///     therefore has to be read from MVC at the moment of the decision.
     /// </summary>
     [TestFixture]
     public class ExplorerUiActionsShould
     {
         private IMVCManager mvcManager;
+        private Queue<ExplorerUiEvent> events;
         private ExplorerUiActions explorerUiActions;
 
         [SetUp]
         public void SetUp()
         {
             mvcManager = Substitute.For<IMVCManager>();
+            events = new Queue<ExplorerUiEvent>();
 
             // Built before any panel state is arranged, the way a scene load builds it long after the user
             // could have opened a panel.
-            explorerUiActions = new ExplorerUiActions(mvcManager);
+            explorerUiActions = new ExplorerUiActions(mvcManager, events);
         }
 
         [Test]
         public void AnswerWasAlreadyOpenForAPanelOpenedBeforeTheSceneLoaded()
         {
             mvcManager.IsShowing<ExplorePanelView, ExplorePanelParameter>().Returns(true);
 
-            Assert.That(explorerUiActions.OpenSection(ExploreSections.Navmap), Is.EqualTo(OpenExplorerUiResult.WasAlreadyOpen));
+            Assert.That(explorerUiActions.OpenSection(ExplorerUi.EuMap, ExploreSections.Navmap), Is.EqualTo(OpenExplorerUiResult.WasAlreadyOpen));
+            Assert.That(events, Is.Empty);
         }
 
         [Test]
         public void OpenTheSectionWhileThePanelIsHidden()
         {
             mvcManager.IsShowing<ExplorePanelView, ExplorePanelParameter>().Returns(false);
 
-            Assert.That(explorerUiActions.OpenSection(ExploreSections.Navmap), Is.EqualTo(OpenExplorerUiResult.Opened));
+            Assert.That(explorerUiActions.OpenSection(ExplorerUi.EuMap, ExploreSections.Navmap), Is.EqualTo(OpenExplorerUiResult.Opened));
+            mvcManager.Received(1).ShowAsync(Arg.Any<ShowCommand<ExplorePanelView, ExplorePanelParameter>>(), Arg.Any<CancellationToken>());
         }
 
         [Test]
         public void FollowThePanelStateAcrossCalls()
         {
-            mvcManager.IsShowing<ExplorePanelView, ExplorePanelParameter>().Returns(false, true, false);
+            // Re-arranged before every call rather than queued up as a sequence: each accepted request asks
+            // MVC twice, once per thread it runs on, so the number of calls is not the test's business.
+            mvcManager.IsShowing<ExplorePanelView, ExplorePanelParameter>().Returns(false);
+            Assert.That(explorerUiActions.OpenSection(ExplorerUi.EuMap, ExploreSections.Navmap), Is.EqualTo(OpenExplorerUiResult.Opened));
 
-            Assert.That(explorerUiActions.OpenSection(ExploreSections.Navmap), Is.EqualTo(OpenExplorerUiResult.Opened));
-            Assert.That(explorerUiActions.OpenSection(ExploreSections.Places), Is.EqualTo(OpenExplorerUiResult.WasAlreadyOpen));
+            mvcManager.IsShowing<ExplorePanelView, ExplorePanelParameter>().Returns(true);
+            Assert.That(explorerUiActions.OpenSection(ExplorerUi.EuPlaces, ExploreSections.Places), Is.EqualTo(OpenExplorerUiResult.WasAlreadyOpen));
 
             // A cached flag would stay stuck on the middle answer; the panel closing has to be picked up.
-            Assert.That(explorerUiActions.OpenSection(ExploreSections.Places), Is.EqualTo(OpenExplorerUiResult.Opened));
+            mvcManager.IsShowing<ExplorePanelView, ExplorePanelParameter>().Returns(false);
+            Assert.That(explorerUiActions.OpenSection(ExplorerUi.EuPlaces, ExploreSections.Places), Is.EqualTo(OpenExplorerUiResult.Opened));
+        }
+
+        [Test]
+        public void ReportTheOpenedAndClosedPairOfItsOwnRequest()
+        {
+            mvcManager.IsShowing<ExplorePanelView, ExplorePanelParameter>().Returns(false);
+
+            explorerUiActions.OpenSection(ExplorerUi.EuBackpack, ExploreSections.Backpack);
+
+            // The events carry the protocol value the scene asked with, not the section MVC was driven by:
+            // the two enums are unrelated and only this direction of the mapping exists.
+            Assert.That(events, Is.EqualTo(new[]
+            {
+                new ExplorerUiEvent(ExplorerUi.EuBackpack, ExplorerUiEventKind.Opened),
+                new ExplorerUiEvent(ExplorerUi.EuBackpack, ExplorerUiEventKind.Closed),
+            }));
+        }
+
+        [Test]
+        public void ReportNothingWhenTheUserOpensThePanelFirst()
+        {
+            // False on the scene's JS thread, true once the request reaches the main thread. ShowAsync would
+            // silently do nothing from here on, so a reported pair would describe a panel this scene never
+            // opened and would never see closed either.
+            mvcManager.IsShowing<ExplorePanelView, ExplorePanelParameter>().Returns(false, true);
+
+            Assert.That(explorerUiActions.OpenSection(ExplorerUi.EuMap, ExploreSections.Navmap), Is.EqualTo(OpenExplorerUiResult.Opened));
+
+            Assert.That(events, Is.Empty);
+            mvcManager.DidNotReceive().ShowAsync(Arg.Any<ShowCommand<ExplorePanelView, ExplorePanelParameter>>(), Arg.Any<CancellationToken>());
+        }
+
+        [Test]
+        public void ReportClosedWhenTheShowDoesNotEndNormally()
+        {
+            mvcManager.IsShowing<ExplorePanelView, ExplorePanelParameter>().Returns(false);
+
+            mvcManager.ShowAsync(Arg.Any<ShowCommand<ExplorePanelView, ExplorePanelParameter>>(), Arg.Any<CancellationToken>())
+                      .Returns(UniTask.FromCanceled());
+
+            explorerUiActions.OpenSection(ExplorerUi.EuMap, ExploreSections.Navmap);
+
+            // Every way the show can end leaves the panel down, so the pair has to close on all of them: a
+            // reported open with no reported close is a scene waiting forever.
+            Assert.That(events, Is.EqualTo(new[]
+            {
+                new ExplorerUiEvent(ExplorerUi.EuMap, ExplorerUiEventKind.Opened),
+                new ExplorerUiEvent(ExplorerUi.EuMap, ExplorerUiEventKind.Closed),
+            }));
         }
     }
 }

Explorer/Assets/DCL/Infrastructure/CrdtEcsBridge/JsModulesImplementation/RestrictedActions/Tests/RestrictedActionsAPIImplementationShould.cs

@@ -58,7 +58,7 @@ public void SetUp()
             });
             systemClipboard = Substitute.For<ISystemClipboard>();
             explorerUiActions = Substitute.For<IExplorerUiActions>();
-            explorerUiActions.OpenSection(Arg.Any<ExploreSections>()).Returns(OpenExplorerUiResult.Opened);
+            explorerUiActions.OpenSection(Arg.Any<ExplorerUi>(), Arg.Any<ExploreSections>()).Returns(OpenExplorerUiResult.Opened);
             sceneWorld = World.Create();
             Entity scenePlayerEntity = sceneWorld.Create();
             restrictedActionsAPIImplementation = new RestrictedActionsAPIImplementation(
@@ -169,7 +169,7 @@ public void OpenExplorerUi_MapOpensNavmap()
 
             // Assert
             Assert.AreEqual((int)OpenExplorerUiResult.Opened, result);
-            explorerUiActions.Received(1).OpenSection(ExploreSections.Navmap);
+            explorerUiActions.Received(1).OpenSection(ExplorerUi.EuMap, ExploreSections.Navmap);
         }
 
         [Test]
@@ -183,7 +183,7 @@ public void OpenExplorerUi_NotCurrentScene_Rejects()
 
             // Assert
             Assert.AreEqual((int)OpenExplorerUiResult.RejectedNotCurrentScene, result);
-            explorerUiActions.DidNotReceive().OpenSection(Arg.Any<ExploreSections>());
+            explorerUiActions.DidNotReceive().OpenSection(Arg.Any<ExplorerUi>(), Arg.Any<ExploreSections>());
         }
 
         [Test]
@@ -200,14 +200,14 @@ public void OpenExplorerUi_NoRecentGesture_Rejects(int lastUserInputTick)
 
             // Assert
             Assert.AreEqual((int)OpenExplorerUiResult.RejectedNoUserGesture, result);
-            explorerUiActions.DidNotReceive().OpenSection(Arg.Any<ExploreSections>());
+            explorerUiActions.DidNotReceive().OpenSection(Arg.Any<ExplorerUi>(), Arg.Any<ExploreSections>());
         }
 
         [Test]
         public void OpenExplorerUi_AlreadyOpen_ReturnsWasAlreadyOpen()
         {
             // Arrange
-            explorerUiActions.OpenSection(Arg.Any<ExploreSections>()).Returns(OpenExplorerUiResult.WasAlreadyOpen);
+            explorerUiActions.OpenSection(Arg.Any<ExplorerUi>(), Arg.Any<ExploreSections>()).Returns(OpenExplorerUiResult.WasAlreadyOpen);
 
             // Act
             int result = restrictedActionsAPIImplementation.TryOpenExplorerUi((int)ExplorerUi.EuMap);
@@ -224,7 +224,7 @@ public void OpenExplorerUi_UnknownUiValue_Rejects()
 
             // Assert
             Assert.AreEqual((int)OpenExplorerUiResult.RejectedFeatureDisabled, result);
-            explorerUiActions.DidNotReceive().OpenSection(Arg.Any<ExploreSections>());
+            explorerUiActions.DidNotReceive().OpenSection(Arg.Any<ExplorerUi>(), Arg.Any<ExploreSections>());
         }
 
         [Test]
@@ -241,7 +241,7 @@ public void OpenExplorerUi_FeatureDisabled_Rejects()
 
             // Assert
             Assert.AreEqual((int)OpenExplorerUiResult.RejectedFeatureDisabled, result);
-            explorerUiActions.DidNotReceive().OpenSection(Arg.Any<ExploreSections>());
+            explorerUiActions.DidNotReceive().OpenSection(Arg.Any<ExplorerUi>(), Arg.Any<ExploreSections>());
         }
 
         [Test]
@@ -250,7 +250,7 @@ public void OpenExplorerUi_CommunitiesRejectionPropagates()
             // Arrange
             // Communities availability is identity-dependent, so its gate lives inside the
             // IExplorerUiActions implementation; the API must return that rejection to the scene.
-            explorerUiActions.OpenSection(ExploreSections.Communities).Returns(OpenExplorerUiResult.RejectedFeatureDisabled);
+            explorerUiActions.OpenSection(ExplorerUi.EuCommunities, ExploreSections.Communities).Returns(OpenExplorerUiResult.RejectedFeatureDisabled);
 
             // Act
             int result = restrictedActionsAPIImplementation.TryOpenExplorerUi((int)ExplorerUi.EuCommunities);