From 0d61586d11c54dbbb941921c649a669ad72676bf Mon Sep 17 00:00:00 2001 From: hypercube <0hypercube@gmail.com> Date: Sat, 28 Dec 2024 19:52:34 +0000 Subject: [PATCH 1/5] Close organelle menu when leaving cell editor --- src/microbe_stage/editor/CellEditorComponent.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/microbe_stage/editor/CellEditorComponent.cs b/src/microbe_stage/editor/CellEditorComponent.cs index 7be89e7878..156deb51be 100644 --- a/src/microbe_stage/editor/CellEditorComponent.cs +++ b/src/microbe_stage/editor/CellEditorComponent.cs @@ -1084,6 +1084,11 @@ public void OnFinishEditing(bool shouldUpdatePosition) editedProperties.Colour = Colour; editedProperties.MembraneRigidity = Rigidity; + // The organelle menu may still be open if using keyboard/controler navigation. + // https://github.com/Revolutionary-Games/Thrive/issues/4470 + // Since the organelle menu will be reparented to the ModalManager, it is necessary close it manually. + organelleMenu.Close(); + if (!IsMulticellularEditor) { GD.Print("MicrobeEditor: updated organelles for species: ", editedSpecies.FormattedName); From 286a86a4f81fad7e64e08679160c760bfe438178 Mon Sep 17 00:00:00 2001 From: hypercube <0hypercube@gmail.com> Date: Mon, 30 Dec 2024 15:58:10 +0000 Subject: [PATCH 2/5] Check in the modal manager when a parent is deleted --- src/gui_common/ModalManager.cs | 30 +++++++++++++++++-- .../editor/CellEditorComponent.cs | 5 ---- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/gui_common/ModalManager.cs b/src/gui_common/ModalManager.cs index 611c1d044d..19ebbaf232 100644 --- a/src/gui_common/ModalManager.cs +++ b/src/gui_common/ModalManager.cs @@ -1,4 +1,4 @@ -using System.Collections.Generic; +using System.Collections.Generic; using System.Linq; using Godot; using Nito.Collections; @@ -85,7 +85,13 @@ public void MakeModal(TopLevelContainer popup) if (modalStack.Contains(popup)) return; - originalParents[popup] = popup.GetParent(); + var parent = popup.GetParent(); + originalParents[popup] = parent; + + // Listen for when the parent is removed from the tree so the modal can be removed as well. + parent.Connect(Node.SignalName.TreeExiting, Callable.From(() => OnParentLost(popup)), + (uint)ConnectFlags.OneShot); + modalStack.AddToFront(popup); modalsDirty = true; @@ -272,4 +278,24 @@ private void OnModalLost(TopLevelContainer popup) modalsDirty = true; } + + /// + /// Called when the parent of a is deleted from the scene tree. + /// + /// + /// If a popup is demoted but its parent is invalid, an exception will occur. + /// Deleting the modal here and removing references to it will avoid the exception. + /// + private void OnParentLost(TopLevelContainer popup) + { + // Remove the original parent since the reference will now be invalid + originalParents.Remove(popup); + + // Delete the popup + popup.Close(); + modalStack.Remove(popup); + popup.QueueFree(); + + modalsDirty = true; + } } diff --git a/src/microbe_stage/editor/CellEditorComponent.cs b/src/microbe_stage/editor/CellEditorComponent.cs index 156deb51be..7be89e7878 100644 --- a/src/microbe_stage/editor/CellEditorComponent.cs +++ b/src/microbe_stage/editor/CellEditorComponent.cs @@ -1084,11 +1084,6 @@ public void OnFinishEditing(bool shouldUpdatePosition) editedProperties.Colour = Colour; editedProperties.MembraneRigidity = Rigidity; - // The organelle menu may still be open if using keyboard/controler navigation. - // https://github.com/Revolutionary-Games/Thrive/issues/4470 - // Since the organelle menu will be reparented to the ModalManager, it is necessary close it manually. - organelleMenu.Close(); - if (!IsMulticellularEditor) { GD.Print("MicrobeEditor: updated organelles for species: ", editedSpecies.FormattedName); From 7cd6014bba3a214ce368f7a82bb329c64def7bcd Mon Sep 17 00:00:00 2001 From: hypercube <0hypercube@gmail.com> Date: Sat, 4 Jan 2025 18:05:36 +0000 Subject: [PATCH 3/5] Remove deleted popups from the demoted modals --- src/gui_common/ModalManager.cs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/gui_common/ModalManager.cs b/src/gui_common/ModalManager.cs index 19ebbaf232..03f9fa0d64 100644 --- a/src/gui_common/ModalManager.cs +++ b/src/gui_common/ModalManager.cs @@ -296,6 +296,15 @@ private void OnParentLost(TopLevelContainer popup) modalStack.Remove(popup); popup.QueueFree(); + // Remove the popup from the demoted modals since it is now destroyed + var demotedModalsCount = demotedModals.Count; + for (int i = 0; i < demotedModalsCount; i++) + { + var item = demotedModals.Dequeue(); + if (item != popup) + demotedModals.Enqueue(item); + } + modalsDirty = true; } } From cf757bd1f024102a4ecb9aecf7731d0552e93df0 Mon Sep 17 00:00:00 2001 From: hypercube <0hypercube@gmail.com> Date: Sat, 4 Jan 2025 18:08:04 +0000 Subject: [PATCH 4/5] Fix formatting --- src/gui_common/ModalManager.cs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/gui_common/ModalManager.cs b/src/gui_common/ModalManager.cs index 03f9fa0d64..289813b611 100644 --- a/src/gui_common/ModalManager.cs +++ b/src/gui_common/ModalManager.cs @@ -1,4 +1,4 @@ -using System.Collections.Generic; +using System.Collections.Generic; using System.Linq; using Godot; using Nito.Collections; @@ -283,8 +283,10 @@ private void OnModalLost(TopLevelContainer popup) /// Called when the parent of a is deleted from the scene tree. /// /// - /// If a popup is demoted but its parent is invalid, an exception will occur. - /// Deleting the modal here and removing references to it will avoid the exception. + /// + /// If a popup is demoted but its parent is invalid, an exception will occur. + /// Deleting the modal here and removing references to it will avoid the exception. + /// /// private void OnParentLost(TopLevelContainer popup) { From 1a65aff9df6c344464ad28fe1e8fb1f0a1695688 Mon Sep 17 00:00:00 2001 From: hypercube <0hypercube@gmail.com> Date: Sat, 11 Jan 2025 21:11:43 +0000 Subject: [PATCH 5/5] Code review --- src/gui_common/ModalManager.cs | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/src/gui_common/ModalManager.cs b/src/gui_common/ModalManager.cs index 289813b611..5583d32fd7 100644 --- a/src/gui_common/ModalManager.cs +++ b/src/gui_common/ModalManager.cs @@ -17,6 +17,11 @@ public partial class ModalManager : NodeWithInput /// private readonly Dictionary originalParents = new(); + /// + /// Contains the callable that is used to delete the modal when the parent leaves the tree. + /// + private readonly Dictionary parentLostCallables = new(); + private readonly Deque modalStack = new(); private readonly Queue demotedModals = new(); @@ -89,8 +94,10 @@ public void MakeModal(TopLevelContainer popup) originalParents[popup] = parent; // Listen for when the parent is removed from the tree so the modal can be removed as well. - parent.Connect(Node.SignalName.TreeExiting, Callable.From(() => OnParentLost(popup)), + var parentLostCallable = Callable.From(() => OnParentLost(popup)); + parent.Connect(Node.SignalName.TreeExiting, parentLostCallable, (uint)ConnectFlags.OneShot); + parentLostCallables[popup] = parentLostCallable; modalStack.AddToFront(popup); modalsDirty = true; @@ -159,12 +166,28 @@ protected override void Dispose(bool disposing) base.Dispose(disposing); } + /// + /// Parent lost signals are added when a modal is created so that the modal can be automatically deleted. + /// + private void DeleteParentLostSignal(TopLevelContainer modal) + { + if (!originalParents.TryGetValue(modal, out var parent)) + return; + if (!parentLostCallables.TryGetValue(modal, out var parentLostCallable)) + return; + + parent.Disconnect(Node.SignalName.TreeExiting, parentLostCallable); + parentLostCallables.Remove(modal); + } + private void UpdateModals() { while (demotedModals.Count > 0) { var modal = demotedModals.Dequeue(); + DeleteParentLostSignal(modal); + if (!originalParents.TryGetValue(modal, out var parent)) { modal.ReParent(this); @@ -291,16 +314,21 @@ private void OnModalLost(TopLevelContainer popup) private void OnParentLost(TopLevelContainer popup) { // Remove the original parent since the reference will now be invalid - originalParents.Remove(popup); + var popupWasInDictionary = originalParents.Remove(popup); + + // Guard against duplicate signals + if (!popupWasInDictionary) + return; // Delete the popup popup.Close(); modalStack.Remove(popup); popup.QueueFree(); + DeleteParentLostSignal(popup); // Remove the popup from the demoted modals since it is now destroyed var demotedModalsCount = demotedModals.Count; - for (int i = 0; i < demotedModalsCount; i++) + for (int i = 0; i < demotedModalsCount; ++i) { var item = demotedModals.Dequeue(); if (item != popup)