diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Dock/DockBandViewModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Dock/DockBandViewModel.cs index 294e20197f..9c8e327b81 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Dock/DockBandViewModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Dock/DockBandViewModel.cs @@ -5,6 +5,7 @@ using System.Collections.Immutable; using System.Collections.ObjectModel; using System.Threading; +using Microsoft.CmdPal.Common; using Microsoft.CmdPal.Common.Helpers; using Microsoft.CmdPal.UI.ViewModels.Models; using Microsoft.CmdPal.UI.ViewModels.Services; @@ -24,6 +25,9 @@ public sealed partial class DockBandViewModel : ExtensionObjectViewModel private readonly Lock _subscriptionLock = new(); private DockBandSettings _bandSettings; + private Dictionary _viewModelCache = new(ReferenceEqualityComparer.Instance); + private InterlockedBoolean _refreshInFlight; + private InterlockedBoolean _refreshRequested; private InterlockedBoolean _cleanupStarted; private IListPage? _subscribedList; @@ -235,24 +239,165 @@ public sealed partial class DockBandViewModel : ExtensionObjectViewModel private void InitializeFromList(IListPage list) { - var items = list.GetItems(); - var newViewModels = new List(); - foreach (var item in items) + if (_cleanupStarted.Value) { - var newItemVm = new DockItemViewModel(new(item), this.PageContext, _showTitles, _showSubtitles, _contextMenuFactory); - newItemVm.SlowInitializeProperties(); - newViewModels.Add(newItemVm); + return; } - List removed = new(); - DoOnUiThread(() => + _refreshRequested.Set(); + if (!_refreshInFlight.Set()) { - ListHelpers.InPlaceUpdateList(Items, newViewModels, out removed); - }); + return; + } - foreach (var removedItem in removed) + BuildRefresh(list); + } + + private void BuildRefresh(IListPage list) + { + List createdViewModels = []; + try { - removedItem.SafeCleanup(); + _refreshRequested.Clear(); + if (_cleanupStarted.Value) + { + CompleteRefresh(list); + return; + } + + var items = list.GetItems(); + var currentCache = Volatile.Read(ref _viewModelCache); + var nextCache = new Dictionary(items.Length, ReferenceEqualityComparer.Instance); + var newViewModels = new List(items.Length); + + foreach (var item in items) + { + if (item is null) + { + continue; + } + + if (!nextCache.TryGetValue(item, out var itemViewModel) && + !currentCache.TryGetValue(item, out itemViewModel)) + { + itemViewModel = new DockItemViewModel(new(item), PageContext, _showTitles, _showSubtitles, _contextMenuFactory); + createdViewModels.Add(itemViewModel); + itemViewModel.SlowInitializeProperties(); + } + + nextCache[item] = itemViewModel; + newViewModels.Add(itemViewModel); + } + + if (!TryDoOnUiThread(() => ApplyRefresh(list, nextCache, newViewModels, createdViewModels))) + { + QueueCleanup(createdViewModels); + CompleteRefresh(list); + } + } + catch + { + // Clear the gate directly rather than through CompleteRefresh: any pending _refreshRequested is dropped + // on purpose, because re-arming against a GetItems() that keeps throwing would spin a tight retry loop. + // The next ItemsChanged starts a fresh refresh. + QueueCleanup(createdViewModels); + _refreshInFlight.Clear(); + throw; + } + } + + private void ApplyRefresh( + IListPage list, + Dictionary nextCache, + IReadOnlyList newViewModels, + IReadOnlyList createdViewModels) + { + List removedItems = []; + try + { + if (_cleanupStarted.Value) + { + return; + } + + foreach (var viewModel in newViewModels) + { + viewModel.ShowTitle = _showTitles; + viewModel.ShowSubtitle = _showSubtitles; + } + + ListHelpers.InPlaceUpdateList(Items, newViewModels, out removedItems); + Volatile.Write(ref _viewModelCache, nextCache); + } + finally + { + CleanupDiscardedViewModels(removedItems, createdViewModels); + CompleteRefresh(list); + } + } + + private void CompleteRefresh(IListPage list) + { + _refreshInFlight.Clear(); + if (_cleanupStarted.Value || + !_refreshRequested.Value) + { + return; + } + + _ = Task.Run( + () => + { + try + { + InitializeFromList(list); + } + catch (Exception ex) + { + CoreLogger.LogError("Failed to refresh a Dock band.", ex); + } + }); + } + + private void CleanupDiscardedViewModels( + IEnumerable removedItems, + IEnumerable createdViewModels) + { + var retained = new HashSet(Items, ReferenceEqualityComparer.Instance); + var discarded = new HashSet(ReferenceEqualityComparer.Instance); + + foreach (var item in removedItems) + { + if (!retained.Contains(item)) + { + discarded.Add(item); + } + } + + foreach (var item in createdViewModels) + { + if (!retained.Contains(item)) + { + discarded.Add(item); + } + } + + QueueCleanup(discarded); + } + + private static void QueueCleanup(IEnumerable viewModels) + { + var pendingCleanup = viewModels.ToArray(); + if (pendingCleanup.Length != 0) + { + _ = Task.Run( + () => + { + foreach (var viewModel in pendingCleanup) + { + viewModel.SafeCleanup(); + } + }); } } @@ -267,7 +412,6 @@ public sealed partial class DockBandViewModel : ExtensionObjectViewModel var list = command.Model.Unsafe as IListPage; if (list is not null) { - InitializeFromList(list); lock (_subscriptionLock) { if (_cleanupStarted.Value || _subscribedList is not null) @@ -278,15 +422,32 @@ public sealed partial class DockBandViewModel : ExtensionObjectViewModel list.ItemsChanged += HandleItemsChanged; _subscribedList = list; } + + if (_cleanupStarted.Value) + { + return; + } + + InitializeFromList(list); } else { var dockItem = new DockItemViewModel(_rootItem, _showTitles, _showSubtitles, _contextMenuFactory); dockItem.SlowInitializeProperties(); - DoOnUiThread(() => + if (!TryDoOnUiThread(() => { - Items.Add(dockItem); - }); + if (!_cleanupStarted.Value) + { + Items.Add(dockItem); + } + else + { + QueueCleanup([dockItem]); + } + })) + { + QueueCleanup([dockItem]); + } } } @@ -324,11 +485,22 @@ public sealed partial class DockBandViewModel : ExtensionObjectViewModel subscribedList.ItemsChanged -= HandleItemsChanged; } - foreach (var item in Items) + Volatile.Write( + ref _viewModelCache, + new Dictionary(ReferenceEqualityComparer.Instance)); + + if (!TryDoOnUiThread(DrainItems)) { - item.SafeCleanup(); + QueueCleanup(Items.ToArray()); } } + + private void DrainItems() + { + var items = Items.ToArray(); + Items.Clear(); + QueueCleanup(items); + } } public partial class DockItemViewModel : CommandItemViewModel @@ -418,6 +590,10 @@ public partial class DockItemViewModel : CommandItemViewModel } }; } + + public override bool Equals(object? obj) => obj is DockItemViewModel viewModel && viewModel.Model.Equals(Model); + + public override int GetHashCode() => Model.GetHashCode(); } diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ExtensionObjectViewModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ExtensionObjectViewModel.cs index 4bb1da97a1..099e7eeff3 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ExtensionObjectViewModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ExtensionObjectViewModel.cs @@ -255,6 +255,9 @@ public abstract partial class ExtensionObjectViewModel : ObservableObject, IBatc private static PropertyChangedEventArgs Args(string name) => new(name); protected void DoOnUiThread(Action action) + => _ = TryDoOnUiThread(action); + + protected bool TryDoOnUiThread(Action action) { if (PageContext.TryGetTarget(out var pageContext)) { @@ -263,7 +266,10 @@ public abstract partial class ExtensionObjectViewModel : ObservableObject, IBatc CancellationToken.None, TaskCreationOptions.None, pageContext.Scheduler); + return true; } + + return false; } protected virtual void UnsafeCleanup() diff --git a/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/DockItemViewModelTests.cs b/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/DockItemViewModelTests.cs new file mode 100644 index 0000000000..f59f5b6f77 --- /dev/null +++ b/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/DockItemViewModelTests.cs @@ -0,0 +1,314 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Diagnostics; +using System.Reflection; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.CmdPal.UI.ViewModels.Dock; +using Microsoft.CmdPal.UI.ViewModels.Models; +using Microsoft.CmdPal.UI.ViewModels.Services; +using Microsoft.CmdPal.UI.ViewModels.Settings; +using Microsoft.CommandPalette.Extensions; +using Microsoft.CommandPalette.Extensions.Toolkit; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; + +namespace Microsoft.CmdPal.UI.ViewModels.UnitTests; + +[TestClass] +public partial class DockItemViewModelTests +{ + private sealed class TestPageContext(TaskScheduler scheduler) : IPageContext + { + public TaskScheduler Scheduler { get; } = scheduler; + + public ICommandProviderContext ProviderContext => CommandProviderContext.Empty; + + public void ShowException(Exception ex, string? extensionHint = null) + { + throw new AssertFailedException($"Unexpected exception from view model: {ex}"); + } + } + + private sealed class QueuedTaskScheduler : TaskScheduler + { + private readonly ConcurrentQueue _tasks = []; + + protected override IEnumerable GetScheduledTasks() => _tasks.ToArray(); + + protected override void QueueTask(Task task) => _tasks.Enqueue(task); + + protected override bool TryExecuteTaskInline(Task task, bool taskWasPreviouslyQueued) => false; + + public void ExecuteAllAvailable() + { + while (_tasks.TryDequeue(out var task)) + { + TryExecuteTask(task); + } + } + + public void ExecuteUntil(Func condition) + { + var timeout = Stopwatch.StartNew(); + while (!condition()) + { + ExecuteAllAvailable(); + if (timeout.Elapsed > TimeSpan.FromSeconds(2)) + { + Assert.Fail("Timed out waiting for scheduled Dock work."); + } + + Thread.Sleep(5); + } + } + } + + private sealed partial class TestDockPage : ListPage + { + private IListItem[] _items; + private int _getItemsCallCount; + private Action? _getItemsCallback; + + public TestDockPage(IListItem item) + { + _items = [item]; + } + + public int GetItemsCallCount => Volatile.Read(ref _getItemsCallCount); + + public override IListItem[] GetItems() + { + Interlocked.Increment(ref _getItemsCallCount); + Interlocked.Exchange(ref _getItemsCallback, null)?.Invoke(); + return Volatile.Read(ref _items); + } + + public int ItemsChangedSubscriberCount => + ((Delegate?)typeof(ListPage) + .GetField(nameof(ItemsChanged), BindingFlags.Instance | BindingFlags.NonPublic)! + .GetValue(this))? + .GetInvocationList() + .Length ?? 0; + + public void SetItem(IListItem item) => SetItems(item); + + public void SetItems(params IListItem[] items) => Volatile.Write(ref _items, items); + + public void OnNextGetItems(Action callback) => Volatile.Write(ref _getItemsCallback, callback); + + public void TriggerItemsChanged() => RaiseItemsChanged(_items.Length); + } + + private sealed record BandFixture( + DockBandViewModel Band, + CommandItemViewModel Root, + TestDockPage Page, + TestPageContext Context, + QueuedTaskScheduler Scheduler); + + [TestMethod] + public void ItemsChanged_CleansReplacedViewModelAfterUiUpdate() + { + var fixture = CreateBandFixture(); + try + { + var original = fixture.Band.Items[0]; + fixture.Page.SetItem(CreateItem("Replacement")); + + fixture.Page.TriggerItemsChanged(); + fixture.Scheduler.ExecuteUntil(() => !ReferenceEquals(original, fixture.Band.Items[0])); + + Assert.IsTrue( + SpinWait.SpinUntil( + () => original.Initialized.HasFlag(InitializedState.CleanedUp), + TimeSpan.FromSeconds(2)), + "The replaced Dock item was not cleaned."); + } + finally + { + CleanupFixture(fixture); + } + } + + [TestMethod] + public void RepeatedItemsChanged_ReusesStableViewModel() + { + var fixture = CreateBandFixture(); + try + { + var original = fixture.Band.Items[0]; + + for (var i = 0; i < 100; i++) + { + fixture.Page.TriggerItemsChanged(); + fixture.Scheduler.ExecuteAllAvailable(); + } + + Assert.AreSame(original, fixture.Band.Items[0]); + Assert.IsFalse(original.Initialized.HasFlag(InitializedState.CleanedUp)); + } + finally + { + CleanupFixture(fixture); + } + } + + [TestMethod] + public void DuplicateItems_ReuseSingleViewModel() + { + var fixture = CreateBandFixture(); + try + { + var duplicate = CreateItem("Duplicate"); + fixture.Page.SetItems(duplicate, duplicate); + + fixture.Page.TriggerItemsChanged(); + fixture.Scheduler.ExecuteUntil(() => fixture.Band.Items.Count == 2); + + Assert.AreSame(fixture.Band.Items[0], fixture.Band.Items[1]); + + var sharedViewModel = fixture.Band.Items[0]; + fixture.Page.SetItem(duplicate); + fixture.Page.TriggerItemsChanged(); + fixture.Scheduler.ExecuteUntil(() => fixture.Band.Items.Count == 1); + + Assert.AreSame(sharedViewModel, fixture.Band.Items[0]); + Assert.IsFalse(sharedViewModel.Initialized.HasFlag(InitializedState.CleanedUp)); + } + finally + { + CleanupFixture(fixture); + } + } + + [TestMethod] + public void BurstyItemsChanged_CoalescesRefreshWork() + { + var fixture = CreateBandFixture(); + try + { + var initialCalls = fixture.Page.GetItemsCallCount; + + for (var i = 0; i < 1000; i++) + { + fixture.Page.TriggerItemsChanged(); + } + + Assert.AreEqual(initialCalls + 1, fixture.Page.GetItemsCallCount); + + fixture.Scheduler.ExecuteAllAvailable(); + Assert.IsTrue( + SpinWait.SpinUntil( + () => fixture.Page.GetItemsCallCount == initialCalls + 2, + TimeSpan.FromSeconds(2)), + "The coalesced follow-up refresh did not run."); + fixture.Scheduler.ExecuteAllAvailable(); + + Assert.AreEqual(initialCalls + 2, fixture.Page.GetItemsCallCount); + Assert.AreEqual(1, fixture.Band.Items.Count); + } + finally + { + CleanupFixture(fixture); + } + } + + [TestMethod] + public void CleanupWithQueuedRefresh_DoesNotRepopulateItems() + { + var fixture = CreateBandFixture(); + try + { + var original = fixture.Band.Items[0]; + fixture.Page.SetItem(CreateItem("Replacement")); + fixture.Page.TriggerItemsChanged(); + + fixture.Band.SafeCleanup(); + fixture.Scheduler.ExecuteAllAvailable(); + + Assert.AreEqual(0, fixture.Band.Items.Count); + Assert.IsTrue( + SpinWait.SpinUntil( + () => original.Initialized.HasFlag(InitializedState.CleanedUp), + TimeSpan.FromSeconds(2)), + "The disposed Dock item was not cleaned."); + } + finally + { + CleanupFixture(fixture); + } + } + + [TestMethod] + public void CleanupDuringInitialization_DoesNotLeaveItemsChangedSubscription() + { + var fixture = CreateBandFixture(cleanupDuringInitialization: true); + try + { + Assert.AreEqual(0, fixture.Page.ItemsChangedSubscriberCount); + } + finally + { + CleanupFixture(fixture); + } + } + + private static BandFixture CreateBandFixture(bool cleanupDuringInitialization = false) + { + var scheduler = new QueuedTaskScheduler(); + var context = new TestPageContext(scheduler); + var page = new TestDockPage(CreateItem("Initial")) + { + Id = "test.dock.page", + Name = "Test Dock Page", + Title = "Test Dock Page", + }; + var root = new CommandItemViewModel( + new(new CommandItem(page) { Title = page.Title }), + new(context), + DefaultContextMenuFactory.Instance); + root.SlowInitializeProperties(); + + var settingsService = new Mock(); + settingsService.SetupGet(service => service.Settings).Returns(new SettingsModel()); + + var band = new DockBandViewModel( + root, + new(context), + new DockBandSettings { ProviderId = "test", CommandId = page.Id }, + settingsService.Object, + DefaultContextMenuFactory.Instance); + if (cleanupDuringInitialization) + { + page.OnNextGetItems(band.SafeCleanup); + } + + band.InitializeProperties(); + if (cleanupDuringInitialization) + { + scheduler.ExecuteAllAvailable(); + } + else + { + scheduler.ExecuteUntil(() => band.Items.Count == 1); + } + + return new(band, root, page, context, scheduler); + } + + private static ListItem CreateItem(string title) => + new(new NoOpCommand { Name = title }) { Title = title }; + + private static void CleanupFixture(BandFixture fixture) + { + fixture.Band.SafeCleanup(); + fixture.Root.SafeCleanup(); + fixture.Scheduler.ExecuteAllAvailable(); + } +}