Преглед изворни кода

Fixes #3863 - Finalize IApplication interface and complete facade pattern (#4328)

* Initial plan

* Add missing properties to IApplication and move implementation to ApplicationImpl

- Added Force16Colors, ForceDriver, Sixel, Screen, and ClearScreenNextIteration to IApplication
- Moved implementation of these properties from Application static class to ApplicationImpl
- Updated Application.Driver.cs to delegate to ApplicationImpl.Instance
- Updated Application.Screen.cs to delegate to ApplicationImpl.Instance
- Added ResetScreen() method to ApplicationImpl for proper state cleanup
- Removed TODOs marked "Add to IApplication" in Application.Driver.cs
- All Application-related unit tests passing (180 passed)

Co-authored-by: tig <[email protected]>

* Rename Application.Initialization.cs to Application.Lifecycle.cs

- Changed terminology from "Initialization" to "Lifecycle" as requested
- Updated file comment to reflect lifecycle encompasses Init through Shutdown
- All tests still passing (193 Application-related tests)

Co-authored-by: tig <[email protected]>

* Address code review feedback in Application.Lifecycle.cs

- Fixed typo: 'cam' -> 'can' in XML documentation
- Fixed typo: 'Gets of list' -> 'Gets a list' in XML documentation
- Removed large block of commented-out legacy driver detection code
- Removed extra blank lines for cleaner code formatting
- All tests still passing (180 Application tests)

Co-authored-by: tig <[email protected]>

* Fix potential race condition in ApplicationImpl.Shutdown()

- Reset Screen before calling Application.ResetState() to avoid circular reference
- Application.ResetState() was trying to access ApplicationImpl.Instance while inside Shutdown()
- This could cause race conditions and crashes in test scenarios
- ResetScreen() is now called both in Shutdown() and ResetState() for safety
- All 3116 unit tests passing

Co-authored-by: tig <[email protected]>

---------

Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: tig <[email protected]>
Co-authored-by: Tig <[email protected]>
Copilot пре 1 месец
родитељ
комит
9530fb677a

+ 11 - 9
Terminal.Gui/App/Application.Driver.cs

@@ -6,7 +6,6 @@ public static partial class Application // Driver abstractions
 {
     internal static bool _forceFakeConsole;
 
-    // TODO: Add to IApplication
     /// <summary>Gets the <see cref="IConsoleDriver"/> that has been selected. See also <see cref="ForceDriver"/>.</summary>
     public static IConsoleDriver? Driver
     {
@@ -14,18 +13,18 @@ public static partial class Application // Driver abstractions
         internal set => ApplicationImpl.Instance.Driver = value;
     }
 
-    // TODO: Add to IApplication
-    // BUGBUG: Force16Colors should be nullable.
     /// <summary>
     ///     Gets or sets whether <see cref="Application.Driver"/> will be forced to output only the 16 colors defined in
     ///     <see cref="ColorName16"/>. The default is <see langword="false"/>, meaning 24-bit (TrueColor) colors will be output
     ///     as long as the selected <see cref="IConsoleDriver"/> supports TrueColor.
     /// </summary>
     [ConfigurationProperty (Scope = typeof (SettingsScope))]
-    public static bool Force16Colors { get; set; }
+    public static bool Force16Colors
+    {
+        get => ApplicationImpl.Instance.Force16Colors;
+        set => ApplicationImpl.Instance.Force16Colors = value;
+    }
 
-    // TODO: Add to IApplication
-    // BUGBUG: ForceDriver should be nullable.
     /// <summary>
     ///     Forces the use of the specified driver (one of "fake", "dotnet", "windows", or "unix"). If not
     ///     specified, the driver is selected based on the platform.
@@ -35,12 +34,15 @@ public static partial class Application // Driver abstractions
     ///     with either `driver` or `driverName` specified.
     /// </remarks>
     [ConfigurationProperty (Scope = typeof (SettingsScope))]
-    public static string ForceDriver { get; set; } = string.Empty;
+    public static string ForceDriver
+    {
+        get => ApplicationImpl.Instance.ForceDriver;
+        set => ApplicationImpl.Instance.ForceDriver = value;
+    }
 
-    // TODO: Add to IApplication
     /// <summary>
     /// Collection of sixel images to write out to screen when updating.
     /// Only add to this collection if you are sure terminal supports sixel format.
     /// </summary>
-    public static List<SixelToRender> Sixel { get; } = new List<SixelToRender> ();
+    public static List<SixelToRender> Sixel => ApplicationImpl.Instance.Sixel;
 }

+ 6 - 27
Terminal.Gui/App/Application.Initialization.cs → Terminal.Gui/App/Application.Lifecycle.cs

@@ -5,7 +5,7 @@ using System.Reflection;
 
 namespace Terminal.Gui.App;
 
-public static partial class Application // Initialization (Init/Shutdown)
+public static partial class Application // Lifecycle (Init/Shutdown)
 {
 
     /// <summary>Initializes a new instance of a Terminal.Gui Application. <see cref="Shutdown"/> must be called when the application is closing.</summary>
@@ -24,7 +24,7 @@ public static partial class Application // Initialization (Init/Shutdown)
     ///     The <see cref="Run{T}"/> function combines
     ///     <see cref="Init(IConsoleDriver,string)"/> and <see cref="Run(Toplevel, Func{Exception, bool})"/>
     ///     into a single
-    ///     call. An application cam use <see cref="Run{T}"/> without explicitly calling
+    ///     call. An application can use <see cref="Run{T}"/> without explicitly calling
     ///     <see cref="Init(IConsoleDriver,string)"/>.
     /// </para>
     /// <param name="driver">
@@ -118,28 +118,9 @@ public static partial class Application // Initialization (Init/Shutdown)
         // or go through the modern application architecture
         if (Driver is null)
         {
-            //// Try to find a legacy IConsoleDriver type that matches the driver name
-            //bool useLegacyDriver = false;
-            //if (!string.IsNullOrEmpty (ForceDriver))
-            //{
-            //    (List<Type?> drivers, List<string?> driverTypeNames) = GetDriverTypes ();
-            //    Type? driverType = drivers.FirstOrDefault (t => t!.Name.Equals (ForceDriver, StringComparison.InvariantCultureIgnoreCase));
-                
-            //    if (driverType is { } && !typeof (IConsoleDriverFacade).IsAssignableFrom (driverType))
-            //    {
-            //        // This is a legacy driver (not a ConsoleDriverFacade)
-            //        Driver = (IConsoleDriver)Activator.CreateInstance (driverType)!;
-            //        useLegacyDriver = true;
-            //    }
-            //}
-            
-            //// Use the modern application architecture
-            //if (!useLegacyDriver)
-            {
-                ApplicationImpl.Instance.Init (driver, driverName);
-                Debug.Assert (Driver is { });
-                return;
-            }
+            ApplicationImpl.Instance.Init (driver, driverName);
+            Debug.Assert (Driver is { });
+            return;
         }
 
         Debug.Assert (Navigation is null);
@@ -203,7 +184,7 @@ public static partial class Application // Initialization (Init/Shutdown)
     private static void Driver_KeyUp (object? sender, Key e) { RaiseKeyUpEvent (e); }
     private static void Driver_MouseEvent (object? sender, MouseEventArgs e) { RaiseMouseEvent (e); }
 
-    /// <summary>Gets of list of <see cref="IConsoleDriver"/> types and type names that are available.</summary>
+    /// <summary>Gets a list of <see cref="IConsoleDriver"/> types and type names that are available.</summary>
     /// <returns></returns>
     [RequiresUnreferencedCode ("AOT")]
     public static (List<Type?>, List<string?>) GetDriverTypes ()
@@ -229,8 +210,6 @@ public static partial class Application // Initialization (Init/Shutdown)
                                         .Union (["dotnet", "windows", "unix", "fake"])
                                         .ToList ()!;
 
-
-
         return (driverTypes, driverTypeNames);
     }
 

+ 7 - 28
Terminal.Gui/App/Application.Screen.cs

@@ -4,9 +4,6 @@ namespace Terminal.Gui.App;
 
 public static partial class Application // Screen related stuff; intended to hide Driver details
 {
-    private static readonly object _lockScreen = new ();
-    private static Rectangle? _screen;
-
     /// <summary>
     ///     Gets or sets the size of the screen. By default, this is the size of the screen as reported by the <see cref="IConsoleDriver"/>.
     /// </summary>
@@ -17,30 +14,8 @@ public static partial class Application // Screen related stuff; intended to hid
     /// </remarks>
     public static Rectangle Screen
     {
-        get
-        {
-            lock (_lockScreen)
-            {
-                if (_screen == null)
-                {
-                    _screen = Driver?.Screen ?? new (new (0, 0), new (2048, 2048));
-                }
-
-                return _screen.Value;
-            }
-        }
-        set
-        {
-            if (value is {} && (value.X != 0 || value.Y != 0))
-            {
-                throw new NotImplementedException ($"Screen locations other than 0, 0 are not yet supported");
-            }
-
-            lock (_lockScreen)
-            {
-                _screen = value;
-            }
-        }
+        get => ApplicationImpl.Instance.Screen;
+        set => ApplicationImpl.Instance.Screen = value;
     }
 
     /// <summary>Invoked when the terminal's size changed. The new size of the terminal is provided.</summary>
@@ -85,5 +60,9 @@ public static partial class Application // Screen related stuff; intended to hid
     ///     This is typical set to true when a View's <see cref="View.Frame"/> changes and that view has no
     ///     SuperView (e.g. when <see cref="Application.Top"/> is moved or resized.
     /// </remarks>
-    internal static bool ClearScreenNextIteration { get; set; }
+    internal static bool ClearScreenNextIteration
+    {
+        get => ApplicationImpl.Instance.ClearScreenNextIteration;
+        set => ApplicationImpl.Instance.ClearScreenNextIteration = value;
+    }
 }

+ 7 - 1
Terminal.Gui/App/Application.cs

@@ -232,7 +232,13 @@ public static partial class Application
             Driver = null;
         }
 
-        _screen = null;
+        // Reset Screen to null so it will be recalculated on next access
+        // Note: ApplicationImpl.Shutdown() also calls ResetScreen() before calling this method
+        // to avoid potential circular reference issues. Calling it twice is harmless.
+        if (ApplicationImpl.Instance is ApplicationImpl impl)
+        {
+            impl.ResetScreen ();
+        }
 
         // Don't reset ForceDriver; it needs to be set before Init is called.
         //ForceDriver = string.Empty;

+ 80 - 6
Terminal.Gui/App/ApplicationImpl.cs

@@ -24,6 +24,12 @@ public class ApplicationImpl : IApplication
     private Toplevel? _top;
     private readonly ConcurrentStack<Toplevel> _topLevels = new ();
     private int _mainThreadId = -1;
+    private bool _force16Colors;
+    private string _forceDriver = string.Empty;
+    private readonly List<SixelToRender> _sixel = new ();
+    private readonly object _lockScreen = new ();
+    private Rectangle? _screen;
+    private bool _clearScreenNextIteration;
 
     // Private static readonly Lazy instance of Application
     private static Lazy<IApplication> _lazyInstance = new (() => new ApplicationImpl ());
@@ -94,6 +100,59 @@ public class ApplicationImpl : IApplication
         set => _initialized = value;
     }
 
+    /// <inheritdoc/>
+    public bool Force16Colors
+    {
+        get => _force16Colors;
+        set => _force16Colors = value;
+    }
+
+    /// <inheritdoc/>
+    public string ForceDriver
+    {
+        get => _forceDriver;
+        set => _forceDriver = value;
+    }
+
+    /// <inheritdoc/>
+    public List<SixelToRender> Sixel => _sixel;
+
+    /// <inheritdoc/>
+    public Rectangle Screen
+    {
+        get
+        {
+            lock (_lockScreen)
+            {
+                if (_screen == null)
+                {
+                    _screen = Driver?.Screen ?? new (new (0, 0), new (2048, 2048));
+                }
+
+                return _screen.Value;
+            }
+        }
+        set
+        {
+            if (value is {} && (value.X != 0 || value.Y != 0))
+            {
+                throw new NotImplementedException ($"Screen locations other than 0, 0 are not yet supported");
+            }
+
+            lock (_lockScreen)
+            {
+                _screen = value;
+            }
+        }
+    }
+
+    /// <inheritdoc/>
+    public bool ClearScreenNextIteration
+    {
+        get => _clearScreenNextIteration;
+        set => _clearScreenNextIteration = value;
+    }
+
     /// <inheritdoc/>
     public ApplicationPopover? Popover
     {
@@ -381,6 +440,9 @@ public class ApplicationImpl : IApplication
         
         bool wasInitialized = _initialized;
         
+        // Reset Screen before calling Application.ResetState to avoid circular reference
+        ResetScreen ();
+        
         // Call ResetState FIRST so it can properly dispose Popover and other resources
         // that are accessed via Application.* static properties that now delegate to instance fields
         Application.ResetState ();
@@ -396,6 +458,10 @@ public class ApplicationImpl : IApplication
         _top = null;
         _topLevels.Clear ();
         _mainThreadId = -1;
+        _screen = null;
+        _clearScreenNextIteration = false;
+        _sixel.Clear ();
+        // Don't reset ForceDriver and Force16Colors; they need to be set before Init is called
 
         if (wasInitialized)
         {
@@ -469,15 +535,12 @@ public class ApplicationImpl : IApplication
             tops.Insert (0, visiblePopover);
         }
 
-        // BUGBUG: Application.Screen needs to be moved to IApplication
-        bool neededLayout = View.Layout (tops.ToArray ().Reverse (), Application.Screen.Size);
+        bool neededLayout = View.Layout (tops.ToArray ().Reverse (), Screen.Size);
 
-        // BUGBUG: Application.ClearScreenNextIteration needs to be moved to IApplication
-        if (Application.ClearScreenNextIteration)
+        if (ClearScreenNextIteration)
         {
             forceRedraw = true;
-            // BUGBUG: Application.Screen needs to be moved to IApplication
-            Application.ClearScreenNextIteration = false;
+            ClearScreenNextIteration = false;
         }
 
         if (forceRedraw)
@@ -490,4 +553,15 @@ public class ApplicationImpl : IApplication
         View.SetClipToScreen ();
         _driver?.Refresh ();
     }
+
+    /// <summary>
+    /// Resets the Screen field to null so it will be recalculated on next access.
+    /// </summary>
+    internal void ResetScreen ()
+    {
+        lock (_lockScreen)
+        {
+            _screen = null;
+        }
+    }
 }

+ 29 - 0
Terminal.Gui/App/IApplication.cs

@@ -33,6 +33,35 @@ public interface IApplication
     /// <summary>Gets or sets whether the application has been initialized.</summary>
     bool Initialized { get; set; }
 
+    /// <summary>
+    ///     Gets or sets whether <see cref="Driver"/> will be forced to output only the 16 colors defined in
+    ///     <see cref="ColorName16"/>. The default is <see langword="false"/>, meaning 24-bit (TrueColor) colors will be output
+    ///     as long as the selected <see cref="IConsoleDriver"/> supports TrueColor.
+    /// </summary>
+    bool Force16Colors { get; set; }
+
+    /// <summary>
+    ///     Forces the use of the specified driver (one of "fake", "dotnet", "windows", or "unix"). If not
+    ///     specified, the driver is selected based on the platform.
+    /// </summary>
+    string ForceDriver { get; set; }
+
+    /// <summary>
+    /// Collection of sixel images to write out to screen when updating.
+    /// Only add to this collection if you are sure terminal supports sixel format.
+    /// </summary>
+    List<SixelToRender> Sixel { get; }
+
+    /// <summary>
+    ///     Gets or sets the size of the screen. By default, this is the size of the screen as reported by the <see cref="IConsoleDriver"/>.
+    /// </summary>
+    Rectangle Screen { get; set; }
+
+    /// <summary>
+    ///     Gets or sets whether the screen will be cleared, and all Views redrawn, during the next Application iteration.
+    /// </summary>
+    bool ClearScreenNextIteration { get; set; }
+
     /// <summary>Gets or sets the popover manager.</summary>
     ApplicationPopover? Popover { get; set; }