From 7b072bcdedcbc0288b355fd6bb35fa895c1117da Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Fri, 2 Oct 2026 15:07:08 +0200 Subject: [PATCH] Desktop: swap the host lists in whole instead of announcing every row to listeners that redo all their work per row --- .../ViewModels/ReplaceableCollection.cs | 44 ++++++++++++++++++ .../ViewModels/TransfersViewModel.cs | 46 ++++++++++++------- .../ViewModels/VaultViewModel.cs | 46 ++++++------------- .../ShellFlowTests.cs | 32 +++++++++++++ 4 files changed, 120 insertions(+), 48 deletions(-) create mode 100644 src/DodoSSH.Client.Shell/ViewModels/ReplaceableCollection.cs diff --git a/src/DodoSSH.Client.Shell/ViewModels/ReplaceableCollection.cs b/src/DodoSSH.Client.Shell/ViewModels/ReplaceableCollection.cs new file mode 100644 index 0000000..7b23f93 --- /dev/null +++ b/src/DodoSSH.Client.Shell/ViewModels/ReplaceableCollection.cs @@ -0,0 +1,44 @@ +using System.Collections.ObjectModel; +using System.Collections.Specialized; +using System.ComponentModel; + +namespace DodoSSH.Client.Shell.ViewModels; + +/// +/// An observable list whose whole contents can be swapped and announced once. +/// +/// +/// +/// For the lists rebuilt wholesale from the vault. They used to be emptied with Clear and refilled an +/// Add at a time, which announced every row separately — and two other view models listen to the host +/// list and redo their own work on every announcement: the transfers screen refilled its host picker, and the +/// shell walked every host against every tab. A reload of n hosts was n of each, so roughly n² rows touched; +/// three hundred hosts came to some ninety thousand notifications per reload. +/// +/// +/// One instead, which is also exactly what Clear has +/// always said first, so a listener that coped with the old sequence copes with this. Nothing listening to +/// these lists reads the items an announcement carries; they all re-read the list. +/// +/// +internal sealed class ReplaceableCollection : ObservableCollection +{ + /// Makes the whole of the list, and says so once. + internal void ReplaceAll(IEnumerable items) + { + ArgumentNullException.ThrowIfNull(items); + + CheckReentrancy(); + + Items.Clear(); + + foreach (var item in items) + { + Items.Add(item); + } + + OnPropertyChanged(new PropertyChangedEventArgs(nameof(Count))); + OnPropertyChanged(new PropertyChangedEventArgs("Item[]")); + OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset)); + } +} diff --git a/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs index 80047a9..26d6acf 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs @@ -867,14 +867,14 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo /// not this object's to clear. The rows are shared: they carry the decrypted host, and copying that would /// be a second decrypted copy of a secret for no gain. /// - internal ObservableCollection Hosts { get; } = []; + internal ReplaceableCollection Hosts { get; } = []; [ObservableProperty] private HostRowViewModel? selectedHost; /// The buckets that can be browsed, which is the vault's list. /// - internal ObservableCollection Buckets { get; } = []; + internal ReplaceableCollection Buckets { get; } = []; [ObservableProperty] private ObjectStoreRowViewModel? selectedBucket; @@ -3986,7 +3986,21 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo } } - private void OnVaultListChanged(object? sender, NotifyCollectionChangedEventArgs e) => RefreshHosts(); + /// + /// Only the picker the list feeds. Refilling both on either list's change made every reload refill the host + /// picker once for the hosts and again for the buckets. + /// + private void OnVaultListChanged(object? sender, NotifyCollectionChangedEventArgs e) + { + if (vault is { } open && ReferenceEquals(sender, open.ObjectStores)) + { + RefreshPickers(hosts: false, buckets: true); + } + else + { + RefreshPickers(hosts: true, buckets: false); + } + } /// /// Rebuilds both pickers from the vault's lists. @@ -4004,34 +4018,34 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo /// connected to. /// /// - private void RefreshHosts() + private void RefreshHosts() => RefreshPickers(hosts: true, buckets: true); + + private void RefreshPickers(bool hosts, bool buckets) { refreshingPickers = true; try { - Hosts.Clear(); - if (vault is not { } open) { + Hosts.ReplaceAll([]); return; } - foreach (var row in open.Hosts) + if (hosts) { - Hosts.Add(row); + Hosts.ReplaceAll(open.Hosts); + + SelectedHost = Hosts.FirstOrDefault(row => row.EntityId == chosenHost) ?? Hosts.FirstOrDefault(); } - SelectedHost = Hosts.FirstOrDefault(row => row.EntityId == chosenHost) ?? Hosts.FirstOrDefault(); - - Buckets.Clear(); - - foreach (var row in open.ObjectStores) + if (buckets) { - Buckets.Add(row); - } + Buckets.ReplaceAll(open.ObjectStores); - SelectedBucket = Buckets.FirstOrDefault(row => row.EntityId == chosenBucket) ?? Buckets.FirstOrDefault(); + SelectedBucket = + Buckets.FirstOrDefault(row => row.EntityId == chosenBucket) ?? Buckets.FirstOrDefault(); + } } finally { diff --git a/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs index 622ace1..a029eda 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs @@ -1663,7 +1663,7 @@ internal sealed partial class VaultViewModel( /// host key list checks itself against, so a filter applied here would change what the application can /// do rather than what it shows. is the filtered view. /// - internal ObservableCollection Hosts { get; } = []; + internal ReplaceableCollection Hosts { get; } = []; /// The hosts the grid is showing: one level of the group tree, narrowed by the box. /// @@ -1677,7 +1677,7 @@ internal sealed partial class VaultViewModel( /// alone: the phone draws , which is the same hosts flattened under headings. /// /// - internal ObservableCollection VisibleHosts { get; } = []; + internal ReplaceableCollection VisibleHosts { get; } = []; /// Whether the grid has anything to draw. /// @@ -1891,7 +1891,7 @@ internal sealed partial class VaultViewModel( /// groupless keychain is one flat grid and nothing above it. /// /// - internal ObservableCollection HostSections { get; } = []; + internal ReplaceableCollection HostSections { get; } = []; /// Every host on the board, in the order its cards are drawn — every section, one after another. /// @@ -2003,7 +2003,7 @@ internal sealed partial class VaultViewModel( internal ObservableCollection Credentials { get; } = []; /// The buckets to show, unpushed local state included. - internal ObservableCollection ObjectStores { get; } = []; + internal ReplaceableCollection ObjectStores { get; } = []; /// The tags to show, with the number of hosts wearing each. /// @@ -4714,18 +4714,14 @@ internal sealed partial class VaultViewModel( var (rows, unreadable) = await FetchHostRowsAsync(cancellationToken).ConfigureAwait(true); - Hosts.Clear(); - // Grouped by vault, with the one new items go into first, then by name inside each. Two vaults can // hold a host with the same label and both are shown: which vault it is in is what tells them - // apart, which is why the row carries the name rather than the list deduplicating. - foreach (var host in rows + // apart, which is why the row carries the name rather than the list deduplicating. Swapped in whole, + // and announced once: see ReplaceableCollection for what a row at a time cost. + Hosts.ReplaceAll(rows .OrderByDescending(row => row.VaultId == session.ActiveVaultId) .ThenBy(row => row.VaultName, StringComparer.CurrentCulture) - .ThenBy(row => row.Label, StringComparer.CurrentCulture)) - { - Hosts.Add(host); - } + .ThenBy(row => row.Label, StringComparer.CurrentCulture)); SelectedHost = SelectionAfterReload(selectedId); ApplyTheChosenHosts(); @@ -4816,13 +4812,9 @@ internal sealed partial class VaultViewModel( var selectedId = SelectedObjectStore?.EntityId; - ObjectStores.Clear(); - - foreach (var store in listing.Items - .OrderBy(store => store.Secret.Label, StringComparer.CurrentCulture)) - { - ObjectStores.Add(new ObjectStoreRowViewModel(store)); - } + ObjectStores.ReplaceAll(listing.Items + .OrderBy(store => store.Secret.Label, StringComparer.CurrentCulture) + .Select(store => new ObjectStoreRowViewModel(store))); SelectedObjectStore = ObjectStores.FirstOrDefault(row => row.EntityId == selectedId); @@ -5388,12 +5380,7 @@ internal sealed partial class VaultViewModel( // a walk up the group tree, and this is the map that walk is made against. var parents = EffectiveParents(); - VisibleHosts.Clear(); - - foreach (var host in Hosts.Where(host => Matches(host, parents))) - { - VisibleHosts.Add(host); - } + VisibleHosts.ReplaceAll(Hosts.Where(host => Matches(host, parents))); RebuildSidebarRows(); @@ -5403,7 +5390,7 @@ internal sealed partial class VaultViewModel( // property directly, and a host that is not on screen is not one any of them should act on. SelectedHost = selected is not null && VisibleHosts.Contains(selected) ? selected : null; - // After the host selection, not before: this mirrors it, and the ListBox's own answer to the Clear() + // After the host selection, not before: this mirrors it, and the ListBox's own answer to the reset // above is a null that has to be overwritten rather than read. SelectedSidebarRow = SelectedHost; @@ -5473,12 +5460,7 @@ internal sealed partial class VaultViewModel( /// private void RebuildHostSections() { - HostSections.Clear(); - - foreach (var built in BuildHostSections()) - { - HostSections.Add(built); - } + HostSections.ReplaceAll(BuildHostSections()); // Computed rather than stored, and none of the four has a change notification of its own — the same // arrangement RebuildVisibleHosts holds for HasVisibleHosts and NoVisibleHostsMessage, and for the diff --git a/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs b/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs index 0c0f4e3..dc53cd1 100644 --- a/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs +++ b/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs @@ -669,6 +669,38 @@ public sealed partial class ShellFlowTests : IAsyncLifetime shell.StatusMessage.ShouldBe("Locked."); } + /// + /// A reload used to empty the host list and refill it a row at a time, announcing each — and the transfers + /// screen refilled its own picker, announcing each of those, on every one of them. Three hundred hosts came + /// to some ninety thousand notifications. Counted here as announcements reaching the two lists, which is + /// what the transfers screen and the board both answer. + /// + [Fact] + public async Task ReloadingTheHosts_AnnouncesTheListOnce_NotOncePerHost() + { + await UnlockedAsync(); + var vault = shell.Vault!; + + foreach (var label in new[] { "prod-db", "prod-web", "stage-web", "dev-box" }) + { + await AddHostAsync(vault, label); + } + + var vaultAnnouncements = 0; + var pickerAnnouncements = 0; + + vault.Hosts.CollectionChanged += (_, _) => vaultAnnouncements++; + shell.Transfers.Hosts.CollectionChanged += (_, _) => pickerAnnouncements++; + + await vault.LoadAsync(Token); + + vault.Hosts.Count.ShouldBe(4); + shell.Transfers.Hosts.Count.ShouldBe(4); + + vaultAnnouncements.ShouldBe(1); + pickerAnnouncements.ShouldBe(1); + } + [Fact] public async Task AnAutomaticPass_SaysNothingWhenThereIsNothingToDo() {