Proposal: Replace `Window.Visible` with cached/proxy value

[mqudsi]

Proposal: Replace Window.Visible with cached/proxy value

Instead of directly exposing the WinRT IWindow.Visible as a property, use a C# property instead that is updated on IWindow.Closed and IWindow.VisibilityChanged to protect against exceptions during teardown.

Summary

Currently, accessing Window.Visible directly maps to a WinRT/COM operation that throws a COMException if the Window has been closed. Since UI code may continue to be called until the state quiesces and operations that may query Window.Visible may already be taking place mid-flight, it would be safer for this property to provide cached/proxied window visibility state and for reading it to be a simple read operation that does not cross ABI boundaries and does not depend on the window handle still being valid.

Rationale

• As stated, operations may be mid-flight when the window is closed and generally one would not want to block the closing of the Window (in an event handler) for non-critical UI update code to complete. As such, it is "normal" for regular UI code to access properties of the window.
• Window.Visible is defined as a property and not a function; it is not the expectation that its execution will be crossing ABI boundaries and that it would be performing "meaningful" work;
• Generally C# class properties are expected to be non-throwing in idiomatic, well-designed code
• Developers coming from other UI frameworks (including WPF and SWF, if I'm not mistaken) expect that as long as the reference to the window itself remains live (i.e. the Window has not been cleaned up by the GC), access to its properties would be valid (though other operations might - of course - throw after the window has closed)
• The Window facade around IWindow should make every reasonable (non-expensive and straightforward) effort to make it seem like you are interacting with a native C# object and accessing a property leading to a COMException being thrown flies in the face of that.
• If a developer is accessing Window.Visible repeatedly in their UI update code, it is not an unreasonable expectation that this is a quick and fast operation that does not incur much cost. Crossing ABI boundaries to query a value that rarely changes contradicts that, especially when there exists an easy fix for this.

Important Notes

This is what I have in my own project to protect against this:

public sealed partial class MainWindow : Window
{
    private TypedEventHandler<object, WindowVisibilityChangedEventArgs> _visibilityHandler
        = static (sender, args) => ((MainWindow)sender).IsVisible = args.Visible;

    /// <summary>
    /// Caches the window visibility and updates on the <see cref="Window.VisibilityChanged"/> event;
    /// should be used in lieu of <see cref="Visible"/> to protect against COM exceptions when the
    /// window has been destroyed.
    /// </summary>
    public bool IsVisible { get; private set; }
    [Obsolete("Use " + nameof(IsVisible) + " instead to protect against COM exceptions")]
    public new bool Visible => IsVisible;

    public MainWindow()
    {
        this.InitializeComponent();

        IsVisible = true;
        VisibilityChanged += _visibilityHandler;
        Closed += static (sender, _args) =>
        {
            var @this = (MainWindow)sender;
            @this.VisibilityChanged -= @this._visibilityHandler;
            @this.IsVisible = false;
        };

        // Remainder of initialization code
    }
}

Obviously this is not a sufficient solution in that it doesn't protect against Window.Current.Visible being called and must be manually copied to each Window class in a project.

Open Questions

• Is this technically a breaking change? I don't think so, as I can't see any previously working code no longer compiling after this change, nor compiling but no longer working as expected. There is a slight concern about the reactivity latency here (the time it takes for Window.IsVisible in the sample code above to reflect a change if the call races with the execution of the VisibiltyChanged callback handler) but since the IWindow.Visible property is non-synchronized and non-atomic (the actual state of the window's visibility may change immediately after the call determines the return value, resulting in stale information being returned), this does not seem materially different.

[Scottj1s]

@mqudsi Thanks for your analysis and suggestion.

Unfortunately, this is not something we can support for several reasons:

1. Caching a value for Visible is misleading if there is no window, particularly if the cached value is True. But a cached value of False is also problematic. Attempting to map a third state (no Window) to the boolean Visible property is lossy and would likely break existing code.
2. It would be a runtime breaking change to no longer raise a COMException. Client code could be relying on that signal, such as for cleanup logic. Whether that's a good design choice is beside the point - exceptions are commonly considered part of an API contract.
3. All types in WinUI 3 (and Windows App SDK) are based on WinRT, and that does carry some overhead for frequent calls across the ABI. But it would be confusing to provide caching for a single property of a single class, and not for other Window properties (like Content, Bounds, Title, etc), much less for all WinRT types (including Windows.UI.Xaml.Window).
4. Other UI frameworks, such as WPF, have very different designs and implementations. It's not always possible to map behaviors or performance characteristics between them.

I appreciate the added complexity to client code, but I think your approach of handling Window.VisibilityChanged (or Window.Closed) to know when any window property is safe to access, is best.