Improve error handling and component disposal

src/GridOS/Core/Configuration/MainConfig.cs → src/GridOS/Core/Configuration/BaseConfig.cs

@@ -4,7 +4,7 @@
 namespace IngameScript
 {
     // TODO: Finalize setting change notification implementation. Decide what way to go with configuration; separate static from per display instance. Try to move defaults to some plain DTO.
-    class MainConfig : IMenuPresentationConfig, IBreadcrumbConfig, IDisplayConfig
+    class BaseConfig : IMenuPresentationConfig, IBreadcrumbConfig, IDisplayConfig
     {
         public string PathSeparator { get; set; } = "›";
 

src/GridOS/Core/DisplaySystem/DisplayControls/Breadcrumb.cs

@@ -26,6 +26,9 @@ public Breadcrumb(IBreadcrumbConfig config)
             WidthUnit = SizeUnit.Percent;
         }
 
+        public override void Dispose()
+        {}
+
         public override StringBuilder GetContent(ContentGenerationHelper _, bool FlushCache = false)
         {
             if (FlushCache)

src/GridOS/Core/DisplaySystem/DisplayControls/Control.cs

@@ -34,7 +34,7 @@ abstract class Control : IControl
 
         public SizeUnit BorderUnit { get; set; }
         public Thickness Border { get; set; }
-        public Color BorderColor { get; set; } = Color.Transparent;
+        public Color? BorderColor { get; set; }
 
         public string FontName { get; set; } = null;
         public float FontSize { get; set; } = 1f;
@@ -51,5 +51,7 @@ protected virtual void OnRedrawRequired()
         {
             RedrawRequired?.Invoke(this);
         }
+
+        public abstract void Dispose();
     }
 }

src/GridOS/Core/DisplaySystem/DisplayControls/DisplayHeader.cs

@@ -7,12 +7,9 @@ class DisplayHeader : Control
     {
         protected readonly StringBuilder _buffer = new StringBuilder();
         protected readonly ProgressIndicator2 _spinner = new ProgressIndicator2();
-        protected readonly IDiagnosticService _diagnostics;
 
-        public DisplayHeader(IDiagnosticService diagnostics)
+        public DisplayHeader()
         {
-            _diagnostics = diagnostics;
-
             FontSize = 0.6f;
             WidthUnit = SizeUnit.Percent;
             Width = 100;
@@ -22,6 +19,9 @@ public DisplayHeader(IDiagnosticService diagnostics)
             BackgroundColor = new Color(255, 255, 255, 90);
         }
 
+        public override void Dispose()
+        {}
+
         public override StringBuilder GetContent(ContentGenerationHelper _, bool FlushCache = false)
         {
             _buffer.Clear();

src/GridOS/Core/DisplaySystem/DisplayControls/IControl.cs

@@ -3,7 +3,7 @@
 
 namespace IngameScript
 {
-    interface IControl
+    interface IControl : IDisposable
     {
         bool Visible { get; set; }
 
@@ -29,7 +29,7 @@ interface IControl
 
         Thickness Border { get; set; }
         SizeUnit BorderUnit { get; set; }
-        Color BorderColor { get; set; }
+        Color? BorderColor { get; set; }
 
         string FontName { get; set; }
         float FontSize { get; set; }

src/GridOS/Core/DisplaySystem/DisplayInfrastructure/DisplayController.cs

@@ -13,7 +13,6 @@ class DisplayController : IDisposable, IMenuInstanceServices
         public IMenuPresentationConfig MenuConfig { get; }
 
         private readonly ICommandDispatcher _commandDispatcher;
-        private readonly IDiagnosticService _diagnostics;
         private readonly IView _view;
         private readonly IGlobalEvents _globalEvents;
         private readonly Breadcrumb _breadcrumb;
@@ -23,39 +22,54 @@ class DisplayController : IDisposable, IMenuInstanceServices
         private readonly CommandItem _downCommand;
         private readonly CommandItem _selectCommand;
 
-        public DisplayController(string name, ICommandDispatcher commandDispatcher, MainConfig config, IDiagnosticService diagnostics, IView view, IMenuGroup menuRoot, IGlobalEvents globalEvents)
+        public DisplayController(string name, ICommandDispatcher commandDispatcher, BaseConfig config, IMenuGroup menuRoot, IGlobalEvents globalEvents, IMyTextSurface surface)
         {
             Name = name;
             MenuConfig = config;
             DisplayConfig = config;
-            _view = view;
-            _diagnostics = diagnostics;
-            _commandDispatcher = commandDispatcher;
             _globalEvents = globalEvents;
+            _commandDispatcher = commandDispatcher;
 
-            _globalEvents.ExecutionWillFinish += DisplayTick;
-
-            _upCommand = new CommandItem($"{Name}Up", OnMoveUp);
-            _downCommand = new CommandItem($"{Name}Down", OnMoveDown);
-            _selectCommand = new CommandItem($"{Name}Select", OnSelect);
-
-            _menu = new Menu(
-                new MenuModel(menuRoot, this),
-                config);
-            _breadcrumb = new Breadcrumb(config);
-            _menu.NavigationPathChanged += _breadcrumb.OnPathChanged;
-
-            _view
-                .AddControl(new DisplayHeader(_diagnostics))
-                .AddControl(_breadcrumb)
-                .AddControl(_menu);
-
-            _commandDispatcher
-                .AddCommand_OverwriteExisting(_upCommand)
-                .AddCommand_OverwriteExisting(_downCommand)
-                .AddCommand_OverwriteExisting(_selectCommand);
-
-            _menu.PushUpdate();
+            try
+            {
+                _view = new DisplayView(
+                    surface,
+                    config,
+                    new TextSurfaceWordWrapper()
+                );
+
+                _globalEvents.ExecutionWillFinish += DisplayTick;
+
+                _upCommand = new CommandItem($"{Name}Up", OnMoveUp);
+                _downCommand = new CommandItem($"{Name}Down", OnMoveDown);
+                _selectCommand = new CommandItem($"{Name}Select", OnSelect);
+
+                _menu = new Menu(
+                    new MenuModel(menuRoot, this),
+                    config);
+                _breadcrumb = new Breadcrumb(config);
+                _menu.NavigationPathChanged += _breadcrumb.OnPathChanged;
+
+                _view
+                    .AddControl(new DisplayHeader())
+                    .AddControl(_breadcrumb)
+                    .AddControl(_menu);
+
+                _commandDispatcher
+                    .AddCommand_OverwriteExisting(_upCommand)
+                    .AddCommand_OverwriteExisting(_downCommand)
+                    .AddCommand_OverwriteExisting(_selectCommand);
+
+                _menu.PushUpdate();
+            }
+            catch (Exception)
+            {
+                _view?.Dispose();
+                _view = null;
+                _menu?.Dispose();
+                _menu = null;
+                throw;
+            }
         }
 
         public void SetFontType(string fontName)
@@ -73,9 +87,11 @@ public void SetBackgroundColor(Color color)
         public void Dispose()
         {
             _globalEvents.ExecutionWillFinish -= DisplayTick;
+            _menu.NavigationPathChanged -= _breadcrumb.OnPathChanged;
             _commandDispatcher.RemoveCommand(_upCommand);
             _commandDispatcher.RemoveCommand(_downCommand);
             _commandDispatcher.RemoveCommand(_selectCommand);
+            _view.Dispose();
         }
 
         private void OnMoveUp(CommandItem _, string __)

src/GridOS/Core/DisplaySystem/DisplayInfrastructure/DisplayView.cs

@@ -10,18 +10,17 @@ namespace IngameScript
     /// </summary>
     class DisplayView : IView
     {
-        protected IMyTextSurface _surface;
-
         protected bool _contentDirty = true;
         protected RectangleF _viewport;
 
         protected readonly List<KeyValuePair<IControl, List<MySprite>>> _content = new List<KeyValuePair<IControl, List<MySprite>>>();
         protected readonly IWordWrapperController _wordWrapper;
-        protected readonly MainConfig _config;
+        protected readonly IMyTextSurface _surface;
+        protected readonly BaseConfig _config;
 
         private readonly StringBuilder _buffer = new StringBuilder();
 
-        public DisplayView(IMyTextSurface surface, MainConfig config, IWordWrapperController wordWrapper)
+        public DisplayView(IMyTextSurface surface, BaseConfig config, IWordWrapperController wordWrapper)
         {
             _surface = surface;
             _config = config;
@@ -31,6 +30,12 @@ public DisplayView(IMyTextSurface surface, MainConfig config, IWordWrapperContro
             AdaptToSurface();
         }
 
+        public void Dispose()
+        {
+            ClearControls();
+            DecommissionSurface();
+        }
+
         public DisplayView AddControl(IControl control)
         {
             if (_content.Any(x => x.Key == control))
@@ -47,14 +52,18 @@ public void RemoveControl(IControl control)
             if (controlIndex > -1)
             {
                 control.RedrawRequired -= OnRedrawRequired;
+                control.Dispose();
                 _content.RemoveAt(controlIndex);
             }
         }
 
         public void ClearControls()
         {
-            foreach (var kvp in _content)
-                kvp.Key.RedrawRequired -= OnRedrawRequired;
+            foreach (var control in _content.Select(x => x.Key))
+            {
+                control.RedrawRequired -= OnRedrawRequired;
+                control.Dispose();
+            }
 
             _content.Clear();
         }
@@ -98,7 +107,7 @@ public void Redraw(bool flush = false)
                     if (controlEntry.Key.Visible)
                     {
                         // TODO: Add dirty flag to controls, and if not dirty, use cached.
-                        verticalWritingPosition = GenerateSpritesForControl(controlEntry.Key, controlEntry.Value, verticalWritingPosition);
+                        verticalWritingPosition = DrawControl(controlEntry.Key, controlEntry.Value, verticalWritingPosition);
                         frame.AddRange(controlEntry.Value);
                     }
                 }
@@ -111,7 +120,7 @@ public void Redraw(bool flush = false)
         /// Generates sprites for the specified control in the specified list.
         /// </summary>
         /// <returns>Returns the resulting vertical writing position after the control is rendered.</returns>
-        private float GenerateSpritesForControl(IControl control, List<MySprite> targetList, float verticalWritingPosition)
+        private float DrawControl(IControl control, List<MySprite> targetList, float verticalWritingPosition)
         {
             targetList.Clear();
 
@@ -248,6 +257,12 @@ private void SetupSurface()
             _surface.PreserveAspectRatio = true;
         }
 
+        private void DecommissionSurface()
+        {
+            _surface.ContentType = ContentType.TEXT_AND_IMAGE;
+            _surface.WriteText(string.Empty);
+        }
+
         private void OnRedrawRequired(IControl control)
         {
             _contentDirty = true;

src/GridOS/Core/DisplaySystem/DisplayInfrastructure/IView.cs

@@ -1,6 +1,8 @@
-namespace IngameScript
+using System;
+
+namespace IngameScript
 {
-    interface IView
+    interface IView : IDisposable
     {
         DisplayView AddControl(IControl control);
         void RemoveControl(IControl control);

src/GridOS/Core/DisplaySystem/DisplayOrchestrator.cs

@@ -9,69 +9,62 @@ namespace IngameScript
     /// </summary>
     class DisplayOrchestrator
     {
-        // TODO: Now that the menu system is dynamic... need to think about whether we need to dispose stuff when an item is removed.
-        // What to do if non-empty item is removed? Remove all children, or put them somewhere else? How to react if we're displaying items below a group node that is removed?
+        private const string _controllerNameTemplate = "Lcd";
 
-        // Root group displayed by default, parent of all other items/groups
+        // All menu content.
         private readonly IMenuGroup _menuRoot = new MenuGroup("Main");
 
-        private readonly List<IMyTextSurface> _registeredTextSurfaces = new List<IMyTextSurface>();
-        private readonly List<DisplayController> _controllers = new List<DisplayController>();
-        private const string _controllerNameTemplate = "Lcd";
-        private int _controllerCounter = 0;
-
+        // All displays used by the system, along with the corresponding controller.
+        private readonly Dictionary<IMyTextSurface, DisplayController> _registeredSurfaces = new Dictionary<IMyTextSurface, DisplayController>();
+
         private readonly ICommandDispatcher _commandDispatcher;
-        private readonly IDiagnosticService _diagnostics;
         private readonly IGlobalEvents _globalEvents;
+        private readonly ILogger _logger;
+        private int _controllerCounter = 0;
 
-        public DisplayOrchestrator(ICommandDispatcher commandDispatcher, IDiagnosticService diagnostics, IGlobalEvents globalEvents)
+        public DisplayOrchestrator(ICommandDispatcher commandDispatcher, IGlobalEvents globalEvents, ILogger logger)
         {
             _commandDispatcher = commandDispatcher;
-            _diagnostics = diagnostics;
             _globalEvents = globalEvents;
+            _logger = logger;
         }
 
         public void RegisterTextSurface(IMyTextSurface textSurface)
         {
-            if (_registeredTextSurfaces.Contains(textSurface))
+            if (_registeredSurfaces.ContainsKey(textSurface))
                 return;
 
+            DisplayController controller = null;
+
             try
             {
-                var config = new MainConfig();
+                var instanceConfig = new BaseConfig();
 
-                _controllers.Add(
-                new DisplayController(
+                controller = new DisplayController(
                     NextControllerName(),
                     _commandDispatcher,
-                    config,
-                    _diagnostics,
-                    new DisplayView(
-                        textSurface,
-                        config,
-                        new TextSurfaceWordWrapper()),
+                    instanceConfig,
                     _menuRoot,
-                    _globalEvents)
-                );
+                    _globalEvents, 
+                    textSurface);
+
+                _registeredSurfaces.Add(textSurface, controller);
             }
             catch (Exception e)
             {
-                throw new Exception(e.Message + "\n" + e.StackTrace);
+                _logger.Log(LogLevel.Error, $"Display Orchestrator failed to add LCD '{textSurface.DisplayName}' to the system.\r\n\r\nMessage: {e.Message}\r\n\r\nStack trace: {e.StackTrace}");
+                controller?.Dispose();
             }
-
-            _registeredTextSurfaces.Add(textSurface);
         }
 
         public void UnregisterTextSurface(IMyTextSurface textSurface)
         {
-            if (!_registeredTextSurfaces.Contains(textSurface))
-                return;
-
-            // TODO: Create proper removal infrastructure. CHECK REFERENCING to see if disposal is needed.
-            int sharedIndex = _registeredTextSurfaces.IndexOf(textSurface);
-            _controllers.Remove(_controllers[sharedIndex]);
-
-            _registeredTextSurfaces.Remove(textSurface);
+            DisplayController controller;
+            if (_registeredSurfaces.TryGetValue(textSurface, out controller))
+            {
+                controller.Dispose();
+                _registeredSurfaces.Remove(textSurface);
+            }
         }
 
         public void RegisterMenuItem(IMenuItem item)
@@ -83,10 +76,5 @@ private string NextControllerName()
         {
             return $"{_controllerNameTemplate}{++_controllerCounter}";
         }
-
-        public void ClearAll()
-        {
-            _registeredTextSurfaces.ForEach(x => x.WriteText(""));
-        }
     }
 }