diff --git a/src/AsyncNavigation.Avalonia.Floating/AvaloniaFloatingWindowHost.cs b/src/AsyncNavigation.Avalonia.Floating/AvaloniaFloatingWindowHost.cs index bd31e6a..542a5e6 100644 --- a/src/AsyncNavigation.Avalonia.Floating/AvaloniaFloatingWindowHost.cs +++ b/src/AsyncNavigation.Avalonia.Floating/AvaloniaFloatingWindowHost.cs @@ -31,7 +31,7 @@ public IFloatingWindowHost Create(FloatingWindowOptions options) internal sealed class AvaloniaFloatingWindowHost : IFloatingWindowHost { private readonly Window _window; - private bool _allowClose; + private readonly FloatingWindowCloseGuard _closeGuard = new(); private readonly ContentControl _content = new(); public AvaloniaFloatingWindowHost(FloatingWindowOptions options) @@ -71,7 +71,7 @@ public Task ActivateAsync(CancellationToken cancellationToken = default) => public Task CloseAsync(CancellationToken cancellationToken = default) => InvokeAsync(() => { - _allowClose = true; + _closeGuard.AllowClose(); _window.Close(); }, cancellationToken); @@ -82,7 +82,7 @@ await InvokeAsync(() => _window.Closing -= OnClosing; if (_window.IsVisible) { - _allowClose = true; + _closeGuard.AllowClose(); _window.Close(); } }, CancellationToken.None); @@ -90,7 +90,7 @@ await InvokeAsync(() => private void OnClosing(object? sender, WindowClosingEventArgs e) { - if (_allowClose) + if (!_closeGuard.ShouldCancelAndNotify()) return; e.Cancel = true; diff --git a/src/AsyncNavigation.Floating/FloatingWindowCloseGuard.cs b/src/AsyncNavigation.Floating/FloatingWindowCloseGuard.cs new file mode 100644 index 0000000..db4d63a --- /dev/null +++ b/src/AsyncNavigation.Floating/FloatingWindowCloseGuard.cs @@ -0,0 +1,24 @@ +namespace AsyncNavigation.Floating; + +/// +/// Encapsulates the "cancel the OS window's close, then raise CloseRequested instead" +/// dance shared by every backed by a real window: closing the +/// window via the OS chrome (or Alt+F4 etc.) must not close it outright - it has to be turned +/// into a request that the caller can still veto or handle asynchronously via +/// CloseAsync/DisposeAsync. Shared so this exact state transition - easy to get +/// subtly wrong per platform - only has to be implemented once. +/// +public sealed class FloatingWindowCloseGuard +{ + private volatile bool _allowClose; + + /// Call before actually closing the window from CloseAsync/DisposeAsync. + public void AllowClose() => _allowClose = true; + + /// + /// Call from the window's own Closing handler. Returns true when the close was not + /// explicitly requested via - the caller must then cancel the + /// window's own close and raise its CloseRequested event instead. + /// + public bool ShouldCancelAndNotify() => !_allowClose; +} diff --git a/src/AsyncNavigation.Wpf.Floating/WpfFloatingWindowHost.cs b/src/AsyncNavigation.Wpf.Floating/WpfFloatingWindowHost.cs index 93353f6..b925378 100644 --- a/src/AsyncNavigation.Wpf.Floating/WpfFloatingWindowHost.cs +++ b/src/AsyncNavigation.Wpf.Floating/WpfFloatingWindowHost.cs @@ -21,7 +21,7 @@ public IFloatingWindowHost Create(FloatingWindowOptions options) internal sealed class WpfFloatingWindowHost : IFloatingWindowHost { private readonly Window _window; - private bool _allowClose; + private readonly FloatingWindowCloseGuard _closeGuard = new(); private readonly ContentControl _content = new(); public WpfFloatingWindowHost(FloatingWindowOptions options) @@ -61,7 +61,7 @@ public Task ActivateAsync(CancellationToken cancellationToken = default) => public Task CloseAsync(CancellationToken cancellationToken = default) => InvokeAsync(() => { - _allowClose = true; + _closeGuard.AllowClose(); _window.Close(); }, cancellationToken); @@ -72,7 +72,7 @@ await InvokeAsync(() => _window.Closing -= OnClosing; if (_window.IsVisible) { - _allowClose = true; + _closeGuard.AllowClose(); _window.Close(); } }, CancellationToken.None); @@ -80,7 +80,7 @@ await InvokeAsync(() => private void OnClosing(object? sender, CancelEventArgs e) { - if (_allowClose) + if (!_closeGuard.ShouldCancelAndNotify()) return; e.Cancel = true; _ = _window.Dispatcher.InvokeAsync( diff --git a/src/AsyncNavigation/Abstractions/IViewManager.cs b/src/AsyncNavigation/Abstractions/IViewManager.cs index 5c11041..eb339ef 100644 --- a/src/AsyncNavigation/Abstractions/IViewManager.cs +++ b/src/AsyncNavigation/Abstractions/IViewManager.cs @@ -8,5 +8,7 @@ Task ResolveViewAsync(string key, Func? initialize = null); void AddView(string key, IView view); void Clear(); - void Remove(string key, bool dispose = false); + /// Removes the cache entry for . Returns false without removing + /// anything if the cached instance is currently floating (pinned) or the key isn't cached. + bool Remove(string key, bool dispose = false); } diff --git a/src/AsyncNavigation/RegionNavigationService.cs b/src/AsyncNavigation/RegionNavigationService.cs index ae2f6c6..bd9e9be 100644 --- a/src/AsyncNavigation/RegionNavigationService.cs +++ b/src/AsyncNavigation/RegionNavigationService.cs @@ -220,7 +220,14 @@ private async Task OnAfterNavigationAsync(NavigationContext navigationContext) { SubscribeUnload(navigationContext); await aware.OnNavigatedToAsync(navigationContext); - navigationContext.CancellationToken.ThrowIfCancellationRequested(); + if (navigationContext.CancellationToken.IsCancellationRequested) + { + // Activation succeeded but the operation is being cancelled/rolled back right + // after - notify the view it's leaving too, so its lifecycle state doesn't end + // up out of sync with the region reverting to whatever was active before. + await aware.OnNavigatedFromAsync(navigationContext); + navigationContext.CancellationToken.ThrowIfCancellationRequested(); + } _current = (view, navigationContext); } if (navigationContext.Target.Value is { } target) diff --git a/src/AsyncNavigation/ViewManager.cs b/src/AsyncNavigation/ViewManager.cs index ec21bd9..631a282 100644 --- a/src/AsyncNavigation/ViewManager.cs +++ b/src/AsyncNavigation/ViewManager.cs @@ -100,10 +100,10 @@ public async Task ResolveViewAsync(string key, } } - public void Remove(string cacheKey, bool dispose = false) + public bool Remove(string cacheKey, bool dispose = false) { if (_viewCache.TryGetValue(cacheKey, out var pinned) && - pinned.TryGetTarget(out var active) && _placement?.IsFloating(active) == true) return; + pinned.TryGetTarget(out var active) && _placement?.IsFloating(active) == true) return false; if (_viewCache.TryRemove(cacheKey, out var viewRef)) { lock (_lruLock) @@ -119,7 +119,9 @@ public void Remove(string cacheKey, bool dispose = false) { DisposeView(view); } + return true; } + return false; } public void RemoveInstance(IView view) @@ -169,12 +171,18 @@ private void AddToLru(string key) private void TrimCache(string protectedKey) { + // Resume scanning from wherever the previous eviction in this call left off, instead of + // restarting from the tail every outer-loop iteration - floating (pinned) entries don't + // change status mid-call, so re-walking past the same ones on every eviction is wasted + // work that turns this into O(evictions * pinned) when floating views sit at the tail. + LinkedListNode? node = null; while (_viewCache.Count > _maxCacheSize) { string? oldestKey = null; lock (_lruLock) { - var node = _lruList.Last; + if (node?.List != _lruList) + node = _lruList.Last; while (node is not null) { if (node.Value != protectedKey && @@ -185,8 +193,10 @@ private void TrimCache(string protectedKey) } if (node is null) break; // Active windows are pinned, even above the cache limit. oldestKey = node.Value; + var toRemove = node; + node = node.Previous; _lruIndex.Remove(oldestKey); - _lruList.Remove(node); + _lruList.Remove(toRemove); } if (oldestKey != null && _viewCache.TryRemove(oldestKey, out var viewRef)) diff --git a/src/AsyncNavigation/WeakUnloadObserver.cs b/src/AsyncNavigation/WeakUnloadObserver.cs index 90cf391..d644bfd 100644 --- a/src/AsyncNavigation/WeakUnloadObserver.cs +++ b/src/AsyncNavigation/WeakUnloadObserver.cs @@ -16,7 +16,7 @@ internal sealed class WeakUnloadObserver private static readonly ConditionalWeakTable> _subscriptions = new(); private static readonly object _subscriptionsLock = new(); - public static void Subscribe(INavigationAware navigationAware, Action onUnloadCallback) + public static void Subscribe(INavigationAware navigationAware, Func onUnloadCallback) { var weakReference = new WeakReference(navigationAware); @@ -32,7 +32,7 @@ async Task HandleRequestUnloadAsync(object? sender, AsyncEventArgs args) } await target.OnUnloadAsync(args.CancellationToken); - onUnloadCallback?.Invoke(target); + await onUnloadCallback(target); } // TryGetValue/Remove/Add aren't individually atomic against each other, and diff --git a/tests/AsyncNavigation.Tests/Mocks/GuardTestNavigationAware.cs b/tests/AsyncNavigation.Tests/Mocks/GuardTestNavigationAware.cs index ab16a5d..d9a32ba 100644 --- a/tests/AsyncNavigation.Tests/Mocks/GuardTestNavigationAware.cs +++ b/tests/AsyncNavigation.Tests/Mocks/GuardTestNavigationAware.cs @@ -7,9 +7,20 @@ public class GuardTestNavigationAware : INavigationAware, INavigationGuard { public static GuardTestNavigationAware? LastCreated { get; private set; } + /// Set before navigating (no view caching means a fresh instance is created per + /// navigation, so a static hook is what lets the next-created instance(s) pick it up) to run + /// on whichever instance gets created next. Not consumed/ + /// cleared on construction: GuardTestView's own constructor parameter causes the DI container + /// to construct a throwaway instance before the one actually wired as its DataContext, so + /// every instance built while this is set picks up the same reference - only the one that + /// ends up as the real DataContext ever has OnNavigatedToAsync invoked on it. Callers should + /// reset this back to null themselves once done to avoid leaking into other tests. + public static Action? NextActivatedCallback { get; set; } + public GuardTestNavigationAware() { LastCreated = this; + OnActivatedCallback = NextActivatedCallback; } public event AsyncEventHandler? AsyncRequestUnloadEvent; @@ -17,6 +28,10 @@ public GuardTestNavigationAware() public bool GuardWasCalled { get; private set; } public bool NavigatedFromWasCalled { get; private set; } + /// Invoked at the end of , before it returns - lets a + /// test simulate cancellation racing with a just-completed activation. + public Action? OnActivatedCallback { get; set; } + public Task CanNavigateAsync(NavigationContext context, CancellationToken cancellationToken) { GuardWasCalled = true; @@ -26,6 +41,10 @@ public Task CanNavigateAsync(NavigationContext context, CancellationToken public Task InitializeAsync(NavigationContext context) => Task.CompletedTask; public Task IsNavigationTargetAsync(NavigationContext context) => Task.FromResult(true); public Task OnNavigatedFromAsync(NavigationContext context) { NavigatedFromWasCalled = true; return Task.CompletedTask; } - public Task OnNavigatedToAsync(NavigationContext context) => Task.CompletedTask; + public Task OnNavigatedToAsync(NavigationContext context) + { + OnActivatedCallback?.Invoke(); + return Task.CompletedTask; + } public Task OnUnloadAsync(CancellationToken cancellationToken) => Task.CompletedTask; } diff --git a/tests/AsyncNavigation.Tests/RegionManagerTests.cs b/tests/AsyncNavigation.Tests/RegionManagerTests.cs index 93e2c5b..832ee55 100644 --- a/tests/AsyncNavigation.Tests/RegionManagerTests.cs +++ b/tests/AsyncNavigation.Tests/RegionManagerTests.cs @@ -207,4 +207,35 @@ public async Task NavigationGuard_WhenBlocksNavigation_DoesNotCallOnNavigatedFro Assert.True(guardVm.GuardWasCalled); Assert.False(guardVm.NavigatedFromWasCalled); } + + [Fact] + public async Task Navigation_CancelledRightAfterActivation_StillCallsOnNavigatedFrom() + { + var region = TestRegion.Build(_serviceProvider); + _regionManager.TryRemoveRegion("Guard4", out _); + _regionManager.AddRegion("Guard4", region); + + using var cts = new CancellationTokenSource(); + var callbackRan = false; + // Simulates cancellation racing in right after OnNavigatedToAsync succeeds but before the + // pipeline's own cancellation check - the view was told it's active, so it must also be + // told it's leaving when the operation is rolled back, or its lifecycle state goes out of + // sync with the region reverting. + GuardTestNavigationAware.NextActivatedCallback = () => + { + callbackRan = true; + cts.Cancel(); + }; + + var result = await _regionManager.RequestNavigateAsync("Guard4", "GuardTestView", cancellationToken: cts.Token); + var guardVm = GuardTestNavigationAware.LastCreated!; + GuardTestNavigationAware.NextActivatedCallback = null; + + _regionManager.TryRemoveRegion("Guard4", out _); + + Assert.True(callbackRan, "activation callback never ran"); + Assert.True(cts.IsCancellationRequested, "cts was not cancelled"); + Assert.True(result.IsCancelled, result.ToString()); + Assert.True(guardVm.NavigatedFromWasCalled); + } }