cleaned up element addition/removal code

CHANGELOG.md

@@ -68,6 +68,10 @@ Fixes
 - Fixed an exception when trying to force-update the area of an element without a ui system
 - Fixed the scroll bar of an empty panel being positioned incorrectly
 - Fixed UiControls maintaining old input states when input types are toggled off
+- Fixed an occasional deadlock when a game is disposed with a scrolling Panel present
+
+Removals
+- Marked Element.OnDisposed as obsolete in favor of the more predictable OnRemovedFromUi
 
 ### MLEM.Data
 Additions

MLEM.Ui/Elements/Element.cs

@@ -31,7 +31,7 @@ namespace MLEM.Ui.Elements {
         /// </summary>
         public UiSystem System {
             get => this.system;
-            internal set {
+            private set {
                 this.system = value;
                 this.Controls = value?.Controls;
                 this.Style = this.Style.OrStyle(value?.Style);
@@ -50,7 +50,7 @@ namespace MLEM.Ui.Elements {
         /// This element's <see cref="RootElement"/>.
         /// Note that this value is set even if this element has a <see cref="Parent"/>. To get the element that represents the root element, use <see cref="RootElement.Element"/>.
         /// </summary>
-        public RootElement Root { get; internal set; }
+        public RootElement Root { get; private set; }
         /// <summary>
         /// The scale that this ui element renders with
         /// </summary>
@@ -425,9 +425,10 @@ namespace MLEM.Ui.Elements {
         /// </summary>
         public GenericCallback OnRemovedFromUi;
         /// <summary>
-        /// Event that is called when this element's <see cref="Dispose"/> method is called, which also happens in <see cref="Finalize"/>.
+        /// Event that is called when this element's <see cref="Dispose"/> method is called.
         /// This event is useful for unregistering global event handlers when this object should be destroyed.
         /// </summary>
+        [Obsolete("OnDisposed will be removed in a future update. To unregister custom event handlers, use OnRemovedFromUi instead.")]
         public GenericCallback OnDisposed;
 
         /// <summary>
@@ -480,14 +481,15 @@ namespace MLEM.Ui.Elements {
             this.size = size;
 
             this.Children = new ReadOnlyCollection<Element>(this.children);
-            this.GetTabNextElement += (backward, next) => next;
-            this.GetGamepadNextElement += (dir, next) => next;
+            this.GetTabNextElement = (backward, next) => next;
+            this.GetGamepadNextElement = (dir, next) => next;
 
             this.SetAreaDirty();
             this.SetSortedChildrenDirty();
         }
 
         /// <inheritdoc />
+        [Obsolete("Dispose will be removed in a future update. To unregister custom event handlers, use OnRemovedFromUi instead.")]
         ~Element() {
             this.Dispose();
         }
@@ -504,12 +506,7 @@ namespace MLEM.Ui.Elements {
                 index = this.children.Count;
             this.children.Insert(index, element);
             element.Parent = this;
-            element.AndChildren(e => {
-                e.Root = this.Root;
-                e.System = this.System;
-                e.OnAddedToUi?.Invoke(e);
-                this.Root?.InvokeOnElementAdded(e);
-            });
+            element.AndChildren(e => e.AddedToUi(this.System, this.Root));
             this.OnChildAdded?.Invoke(this, element);
             this.SetSortedChildrenDirty();
             element.SetAreaDirty();
@@ -528,12 +525,7 @@ namespace MLEM.Ui.Elements {
             // upwards to us if the element is auto-positioned
             element.SetAreaDirty();
             element.Parent = null;
-            element.AndChildren(e => {
-                e.Root = null;
-                e.System = null;
-                e.OnRemovedFromUi?.Invoke(e);
-                this.Root?.InvokeOnElementRemoved(e);
-            });
+            element.AndChildren(e => e.RemovedFromUi());
             this.OnChildRemoved?.Invoke(this, element);
             this.SetSortedChildrenDirty();
         }
@@ -1098,6 +1090,7 @@ namespace MLEM.Ui.Elements {
         }
 
         /// <summary>Performs application-defined tasks associated with freeing, releasing, or resetting unmanaged resources.</summary>
+        [Obsolete("Dispose will be removed in a future update. To unregister custom event handlers, use OnRemovedFromUi instead.")]
         public virtual void Dispose() {
             this.OnDisposed?.Invoke(this);
             GC.SuppressFinalize(this);
@@ -1187,6 +1180,31 @@ namespace MLEM.Ui.Elements {
             return this.TransformInverse(position);
         }
 
+        /// <summary>
+        /// Called when this element is added to a <see cref="UiSystem"/> and, optionally, a given <see cref="RootElement"/>.
+        /// This method is called in <see cref="AddChild{T}"/> and <see cref="UiSystem.Add"/>.
+        /// </summary>
+        /// <param name="system">The ui system to add to.</param>
+        /// <param name="root">The root element to add to.</param>
+        protected internal virtual void AddedToUi(UiSystem system, RootElement root) {
+            this.Root = root;
+            this.System = system;
+            this.OnAddedToUi?.Invoke(this);
+            root?.InvokeOnElementAdded(this);
+        }
+
+        /// <summary>
+        /// Called when this element is removed from a <see cref="UiSystem"/> and <see cref="RootElement"/>.
+        /// This method is called in <see cref="RemoveChild"/> and <see cref="UiSystem.Remove"/>.
+        /// </summary>
+        protected internal virtual void RemovedFromUi() {
+            var root = this.Root;
+            this.Root = null;
+            this.System = null;
+            this.OnRemovedFromUi?.Invoke(this);
+            root?.InvokeOnElementRemoved(this);
+        }
+
         /// <summary>
         /// A delegate used for the <see cref="Element.OnTextInput"/> event.
         /// </summary>

MLEM.Ui/Elements/Panel.cs

@@ -177,15 +177,6 @@ namespace MLEM.Ui.Elements {
             return base.GetElementUnderPos(position);
         }
 
-        /// <inheritdoc />
-        public override void Dispose() {
-            if (this.renderTarget != null) {
-                this.renderTarget.Dispose();
-                this.renderTarget = null;
-            }
-            base.Dispose();
-        }
-
         /// <summary>
         /// Scrolls this panel's <see cref="ScrollBar"/> to the given <see cref="Element"/> in such a way that its center is positioned in the center of this panel.
         /// </summary>
@@ -236,6 +227,15 @@ namespace MLEM.Ui.Elements {
                 this.ScrollChildren();
         }
 
+        /// <inheritdoc />
+        protected internal override void RemovedFromUi() {
+            base.RemovedFromUi();
+            // we dispose our render target when removing so that it doesn't cause a memory leak
+            // if we're added back afterwards, it'll be recreated in ScrollSetup anyway
+            this.renderTarget?.Dispose();
+            this.renderTarget = null;
+        }
+
         /// <summary>
         /// Prepares the panel for auto-scrolling, creating the render target and setting up the scroll bar's maximum value.
         /// </summary>
@@ -274,11 +274,13 @@ namespace MLEM.Ui.Elements {
 
             // update the render target
             var targetArea = (Rectangle) this.GetRenderTargetArea();
-            if (targetArea.Width <= 0 || targetArea.Height <= 0)
+            if (targetArea.Width <= 0 || targetArea.Height <= 0) {
+                this.renderTarget?.Dispose();
+                this.renderTarget = null;
                 return;
+            }
             if (this.renderTarget == null || targetArea.Width != this.renderTarget.Width || targetArea.Height != this.renderTarget.Height) {
-                if (this.renderTarget != null)
-                    this.renderTarget.Dispose();
+                this.renderTarget?.Dispose();
                 this.renderTarget = targetArea.IsEmpty ? null : new RenderTarget2D(this.System.Game.GraphicsDevice, targetArea.Width, targetArea.Height);
                 this.relevantChildrenDirty = true;
             }

MLEM.Ui/Parsers/UiParser.cs

@@ -145,9 +145,15 @@ namespace MLEM.Ui.Parsers {
                 throw new NullReferenceException("A UI parser requires a GraphicsDevice for parsing images");
 
             TextureRegion image = null;
-            LoadImageAsync();
             return new Image(Anchor.AutoLeft, new Vector2(1, -1), _ => image) {
-                OnDisposed = e => image?.Texture.Dispose()
+                OnAddedToUi = e => {
+                    if (image == null)
+                        LoadImageAsync();
+                },
+                OnRemovedFromUi = e => {
+                    image?.Texture.Dispose();
+                    image = null;
+                }
             };
 
             async void LoadImageAsync() {

MLEM.Ui/UiSystem.cs

@@ -345,10 +345,7 @@ namespace MLEM.Ui {
             var root = new RootElement(name, element, this);
             this.rootElements.Add(root);
             root.Element.AndChildren(e => {
-                e.Root = root;
-                e.System = this;
-                e.OnAddedToUi?.Invoke(e);
-                root.InvokeOnElementAdded(e);
+                e.AddedToUi(this, root);
                 e.SetAreaDirty();
             });
             this.OnRootAdded?.Invoke(root);
@@ -368,10 +365,7 @@ namespace MLEM.Ui {
             this.rootElements.Remove(root);
             this.Controls.SelectElement(root, null);
             root.Element.AndChildren(e => {
-                e.Root = null;
-                e.System = null;
-                e.OnRemovedFromUi?.Invoke(e);
-                root.InvokeOnElementRemoved(e);
+                e.RemovedFromUi();
                 e.SetAreaDirty();
             });
             this.OnRootRemoved?.Invoke(root);

MLEM/Graphics/StaticSpriteBatch.cs

@@ -411,7 +411,6 @@ namespace MLEM.Graphics {
             this.indices?.Dispose();
             foreach (var buffer in this.vertexBuffers)
                 buffer.Dispose();
-            GC.SuppressFinalize(this);
         }
 
         private Item Add(Texture2D texture, Vector2 pos, Vector2 offset, Vector2 size, float sin, float cos, Color color, Vector2 texTl, Vector2 texBr, float depth) {