diff --git a/docs/manual-checks.md b/docs/manual-checks.md index a33fdd8..3a53cc7 100644 --- a/docs/manual-checks.md +++ b/docs/manual-checks.md @@ -795,6 +795,16 @@ With host A selected, right-click host B and choose Delete. wrong machine. `HostGridTests` covers both halves headlessly, so this is a confirmation that a real popup behaves as the headless one did. +**And the same on the group cards above.** Open a group, then right-click a card inside it and choose +Delete. + +**Pass:** the question names the **card**, not the group that is open — and Open on that menu goes into the +card, rather than back out to ALL HOSTS. Right-clicking the space around the group cards opens no menu. + +**Failure means:** the menu is reading `GroupTarget`'s fallback, which is the group whose contents are on +screen. That fallback is right for the EDIT and DELETE buttons beside the heading and wrong for a menu that +opened on a card. + ### 7.10 Clicking a host in the palette connects Ctrl+K, then click a result with the mouse rather than pressing Enter. diff --git a/src/DodoSSH.Client.App/Views/HostsScreen.axaml b/src/DodoSSH.Client.App/Views/HostsScreen.axaml index 77700f9..f57211e 100644 --- a/src/DodoSSH.Client.App/Views/HostsScreen.axaml +++ b/src/DodoSSH.Client.App/Views/HostsScreen.axaml @@ -282,6 +282,34 @@ + + + + + + + + + + + diff --git a/src/DodoSSH.Client.App/Views/HostsScreen.axaml.cs b/src/DodoSSH.Client.App/Views/HostsScreen.axaml.cs index e291c2e..35adc4f 100644 --- a/src/DodoSSH.Client.App/Views/HostsScreen.axaml.cs +++ b/src/DodoSSH.Client.App/Views/HostsScreen.axaml.cs @@ -96,6 +96,11 @@ internal sealed partial class HostsScreen : UserControl HostGrid.AddHandler(PointerPressedEvent, OnPointerPressed, RoutingStrategies.Tunnel); HostGrid.AddHandler(ContextRequestedEvent, OnContextRequested, RoutingStrategies.Tunnel); + // And the group cards answer a right click the same way, for the same reason: their menu's commands + // read the vault's group selection, and without this they would act on whichever card was selected + // before — or, with none, on the group the trail ends with, which is not on screen at all. + GroupGrid.AddHandler(ContextRequestedEvent, OnGroupContextRequested, RoutingStrategies.Tunnel); + HostGrid.PointerMoved += OnPointerMoved; HostGrid.PointerReleased += OnPointerReleased; HostGrid.PointerCaptureLost += OnPointerCaptureLost; @@ -204,6 +209,33 @@ internal sealed partial class HostsScreen : UserControl vault.SelectedSidebarRow = row; } + /// + /// Points the group menu at whatever was right-clicked. + /// + /// + /// + /// The host grid's rule, applied to the cards above it — see . What is + /// different is what an unaimed menu would have done: GroupTarget falls back to the open group + /// when no card is selected, so Edit and Delete over a card would have been offered about the group whose + /// contents are showing rather than the one the pointer is on. That fallback is right for a pair of + /// buttons that sit beside the heading and wrong for a menu that opened on a card. + /// + /// + /// Cancelled outright over the space around the cards, as the host grid's is. That is not a group, and + /// the fallback is exactly what would make the menu look like it worked there. + /// + /// + private void OnGroupContextRequested(object? sender, ContextRequestedEventArgs e) + { + if (Vault is not { } vault || RowUnder(e.Source) is not HostGroupRowViewModel row) + { + e.Handled = true; + return; + } + + vault.SelectedGroup = row; + } + /// /// Remembered rather than acted on. Whether this press is a click or the start of a drag is not known /// until the pointer moves, so this is the point at which both are still possible. diff --git a/tests/DodoSSH.Client.App.Layout.Tests/HostGridTests.cs b/tests/DodoSSH.Client.App.Layout.Tests/HostGridTests.cs index c193c7a..773eb43 100644 --- a/tests/DodoSSH.Client.App.Layout.Tests/HostGridTests.cs +++ b/tests/DodoSSH.Client.App.Layout.Tests/HostGridTests.cs @@ -29,10 +29,11 @@ namespace DodoSSH.Client.App.Layout.Tests; /// /// Separate from , which measures these controls rather than driving them. /// What is here is the one gesture that cannot be expressed as a binding and cannot be checked by -/// measuring: a right click has to move the selection before the menu opens, because all three of -/// that menu's commands read the vault's host selection. A menu that quietly acted on whichever host +/// measuring: a right click has to move the selection before the menu opens, because the commands +/// on both of that screen's menus read the vault's selection. A menu that quietly acted on whichever host /// happened to be selected would delete the wrong machine, which is the version of this mistake worth a -/// suite. +/// suite — and the group cards have the same menu with a fallback behind it that makes getting it wrong +/// quieter still. /// /// /// A real over a real unlocked vault, for the reason the other suites here use @@ -159,6 +160,82 @@ public sealed class HostGridTests : IAsyncLifetime }); } + /// + /// The same rule on the cards above, where getting it wrong is quieter and worse. + /// + /// + /// + /// The host grid's menu acts on nothing when it is not aimed; this one acts on the wrong group. + /// GroupTarget falls back to the group whose contents are on screen when no card is selected — the + /// right answer for the pair of buttons beside the heading, and the wrong one for a menu that opened on a + /// card, which would then offer to delete a group the pointer is nowhere near. + /// + /// + /// Open is the one entry that takes a parameter, because OpenGroupCommand's null is a real + /// argument — it is ALL HOSTS. That makes its CommandParameter binding the half most likely to + /// rot: a path that resolves to nothing compiles, draws, and quietly leaves the grid at the top level. + /// + /// + [Fact] + public async Task ARightClickSelectsTheGroupUnderThePointer() + { + await AddGroupAsync("staging"); + + await OnTheGridAsync((screen, window) => + { + var first = GroupRow(vault, "production"); + var other = GroupRow(vault, "staging"); + + vault.SelectedGroup = first; + + RightClick(CardFor(screen, other), window); + + vault.SelectedGroup.ShouldBeSameAs(other); + + var menu = screen.GroupGrid.ContextMenu.ShouldNotBeNull(); + menu.IsOpen.ShouldBeTrue(); + + var items = menu.Items.OfType().ToList(); + + var open = items.Single(item => item.Header is "Open"); + open.Command.ShouldBeSameAs(vault.OpenGroupCommand); + open.CommandParameter.ShouldBeSameAs(other, "the card under the pointer, not ALL HOSTS"); + + var edit = items.Single(item => item.Header is "Edit…"); + edit.Command.ShouldBeSameAs(vault.EditGroupCommand); + + edit.Command!.Execute(null); + + vault.IsEditingGroup.ShouldBeTrue(); + vault.GroupEditorLabel.ShouldBe( + other.Label, "the card that was right-clicked, not the one selected before"); + }); + } + + /// + /// The space around the group cards, where a menu would be at its most misleading: nothing is under the + /// pointer, so an unguarded one would open against the fallback and offer Delete about the group the + /// trail ends with — which, once it is open, is not a card on screen at all. + /// + [Fact] + public async Task ARightClickOffAnyGroupCardOpensNothingAndMovesNothing() + { + await OnTheGridAsync((screen, _) => + { + var selected = GroupRow(vault, "production"); + vault.SelectedGroup = selected; + + screen.GroupGrid.RaiseEvent(new ContextRequestedEventArgs + { + RoutedEvent = Control.ContextRequestedEvent, + Source = screen.GroupGrid, + }); + + vault.SelectedGroup.ShouldBeSameAs(selected, "the selection the menu would have acted on"); + screen.GroupGrid.ContextMenu.ShouldNotBeNull().IsOpen.ShouldBeFalse(); + }); + } + /// /// A host held over a group card would be filed there, and one held over another host card would not. /// @@ -474,14 +551,22 @@ public sealed class HostGridTests : IAsyncLifetime .OfType() .Single(item => item.DataContext is HostGroupRowViewModel); - private static ListBoxItem CardFor(Visual screen, HostRowViewModel host) => + /// Any row: a host card or a group card, which are both items of a list on this screen. + private static ListBoxItem CardFor(Visual screen, object row) => screen.GetVisualDescendants() .OfType() - .First(item => ReferenceEquals(item.DataContext, host)); + .First(item => ReferenceEquals(item.DataContext, row)); private static HostRowViewModel Row(VaultViewModel vault, string label) => vault.Hosts.First(row => string.Equals(row.Label, label, StringComparison.Ordinal)); + /// + /// Out of the cards on screen rather than out of every group, because that is what the card's own data + /// context is — Groups holds the same row objects, but only one level of them is drawn. + /// + private static HostGroupRowViewModel GroupRow(VaultViewModel vault, string label) => + vault.VisibleGroups.First(row => string.Equals(row.Label, label, StringComparison.Ordinal)); + private static Point Centre(Visual control, Visual window) => control.TranslatePoint(new Point(control.Bounds.Width / 2, control.Bounds.Height / 2), window) ?? throw new InvalidOperationException("the control is not in this window's tree");