From be012585b352cdec40d18f2aed01fceffbba9efd Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Tue, 4 Aug 2026 16:27:01 +0200 Subject: [PATCH] Let a group be right-clicked, as a host already can MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The host cards have had a menu since the grid replaced the sidebar; the group cards above them had a double-click and two buttons beside the heading, and nothing that named the card under the pointer. Open, Edit and Delete are on them now, drawn and aimed the same way. On the list rather than in the item template, for the reason the host grid's is: the three commands are the vault's, and a ContextMenu inside a DataTemplate has the row for its data context, so every binding in it would silently resolve to nothing. The code-behind selects whatever was right-clicked before the menu opens, and that is what makes one menu act on the card under the pointer rather than on whichever was selected before. **Cancelled over the space around the cards, and here that guard is doing more than the host grid's.** GroupTarget falls back to the group whose contents are on screen when no card is selected — the right answer for a pair of buttons beside the heading, which would otherwise have no subject the moment a group with nothing inside it is opened, and the wrong one for a menu that opened on a card. Without the guard, right-clicking the gap beside the cards would offer to delete the group the trail ends with: a question about something the user is not pointing at, in the one menu where the answer is a deletion. Only Open takes a parameter, and it has to. OpenGroupCommand's null is a real argument rather than a missing one — it is the trail's first crumb, ALL HOSTS — so an entry with no parameter would not open the card, it would leave the group the user right-clicked and go back to the top level. Two tests beside the two the host menu already had. What they hold that a build cannot is the CommandParameter binding: a path that resolves to nothing compiles and draws, and the entry would then quietly do the opposite of what it says. The popup itself is still the platform's, so manual-checks 7.9 gained the group half of the same check. --- docs/manual-checks.md | 10 ++ .../Views/HostsScreen.axaml | 28 ++++++ .../Views/HostsScreen.axaml.cs | 32 +++++++ .../HostGridTests.cs | 95 ++++++++++++++++++- 4 files changed, 160 insertions(+), 5 deletions(-) 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");