From 67e5b8cff29117bf7c83c086cccd5b5d99f316ac Mon Sep 17 00:00:00 2001 From: Erik Date: Tue, 16 Jun 2026 17:27:30 +0200 Subject: [PATCH] =?UTF-8?q?fix(D.2b):=20UiMenu=20=E2=80=94=20controller=20?= =?UTF-8?q?owns=20Selected=20(review=20fix=20for=20Task=204)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review caught a behavior divergence: the generic UiMenu auto-set its own Selected on any enabled pick, while the controller's EnabledProvider keeps the null-payload specials (Squelch / Tell-to-Selected) enabled/white like retail. So a special-item click set Selected=null and shifted the highlight onto the deferred placeholders — and the menu tests masked it by using a different (specials-disabled) gate than the controller ships. Fix: clean MVC contract mirroring retail UIElement_Menu::NewSelection — the widget REPORTS the pick via OnSelect; the controller OWNS Selected (it sets it only for talk-channel payloads). A special-item click now fires OnSelect(null), the controller ignores it, and the active channel + highlight stay put — observably identical to the pre-generalization widget, and extensible for when Squelch lands. Tests realigned to the controller's gate (specials white) and to the controller-owns-Selected contract. Full suite: 403 passed, 2 skipped, 0 failed. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../UI/Layout/ChatWindowController.cs | 5 +++ src/AcDream.App/UI/UiMenu.cs | 9 +++-- tests/AcDream.App.Tests/UI/UiMenuTests.cs | 36 +++++++++++-------- 3 files changed, 34 insertions(+), 16 deletions(-) diff --git a/src/AcDream.App/UI/Layout/ChatWindowController.cs b/src/AcDream.App/UI/Layout/ChatWindowController.cs index a6281eaa..527e1fad 100644 --- a/src/AcDream.App/UI/Layout/ChatWindowController.cs +++ b/src/AcDream.App/UI/Layout/ChatWindowController.cs @@ -266,8 +266,13 @@ public sealed class ChatWindowController menu.Items = System.Array.ConvertAll(ChannelItems, t => new UiMenu.MenuItem(t.Label, (object?)t.Channel)); menu.Selected = (object?)c._activeChannel; + // Specials (Squelch / Tell-to-Selected, null payload) render WHITE/enabled like + // retail; only the talk-CHANNEL items grey when unavailable. menu.EnabledProvider = p => p is not ChatChannelKind ch || ChannelAvailable(ch); menu.ButtonLabelProvider = () => ChannelButtonLabel(c._activeChannel); + // The widget reports the pick; the controller owns Selected. Only a talk-channel + // payload updates the active channel + highlight — the null-payload specials are + // deferred no-ops (see the chat re-drive deferred list) and leave selection intact. menu.OnSelect = p => { if (p is ChatChannelKind ch) { c._activeChannel = ch; menu.Selected = p; } diff --git a/src/AcDream.App/UI/UiMenu.cs b/src/AcDream.App/UI/UiMenu.cs index b3e1595d..85241a68 100644 --- a/src/AcDream.App/UI/UiMenu.cs +++ b/src/AcDream.App/UI/UiMenu.cs @@ -223,8 +223,13 @@ public sealed class UiMenu : UiElement if (row >= 0 && row < RowsPerColumn && idx >= 0 && idx < Items.Count && (EnabledProvider?.Invoke(Items[idx].Payload) ?? true)) { - Selected = Items[idx].Payload; - OnSelect?.Invoke(Selected); + // The widget REPORTS the pick; the controller owns Selected (it sets + // Selected only for payloads it acts on). This mirrors retail + // UIElement_Menu::NewSelection delegating to the owner rather than + // self-selecting — so a deferred/no-op item (e.g. the Squelch / + // Tell-to-Selected specials, null payload) leaves the current + // selection + highlight unchanged when the controller ignores it. + OnSelect?.Invoke(Items[idx].Payload); } } _open = false; diff --git a/tests/AcDream.App.Tests/UI/UiMenuTests.cs b/tests/AcDream.App.Tests/UI/UiMenuTests.cs index 1e4e1bd5..4b1a16fe 100644 --- a/tests/AcDream.App.Tests/UI/UiMenuTests.cs +++ b/tests/AcDream.App.Tests/UI/UiMenuTests.cs @@ -31,12 +31,14 @@ public class UiMenuTests new("Tell to Olthoi Chat", (object?)ChatChannelKind.Olthoi), }; - // Availability gate identical to ChatWindowController.ChannelAvailable. + // Availability gate identical to ChatWindowController's EnabledProvider: the null-payload + // specials (Squelch/Tell-to-Selected) are ENABLED/white like retail; only talk-CHANNEL + // items grey when unavailable. (The widget reports any enabled pick via OnSelect; the + // controller decides whether to update Selected, so specials are inert no-ops anyway.) private static bool ChannelAvailable(object? p) - => p is ChatChannelKind ch - ? ch is ChatChannelKind.Say or ChatChannelKind.General - or ChatChannelKind.Trade or ChatChannelKind.Lfg - : false; // null-payload (Squelch/Tell-to-Selected) = inert + => p is not ChatChannelKind ch + || ch is ChatChannelKind.Say or ChatChannelKind.General + or ChatChannelKind.Trade or ChatChannelKind.Lfg; private UiMenu MakeMenu() => new UiMenu { @@ -96,7 +98,8 @@ public class UiMenuTests Assert.True(menu.OnEvent(new UiEvent(0, menu, UiEventType.MouseDown, 0, 10, 5))); // open object? fired = null; - menu.OnSelect = p => fired = p; + // Mirror the controller: the widget reports the pick, the controller sets Selected. + menu.OnSelect = p => { fired = p; if (p is ChatChannelKind) menu.Selected = p; }; // "Chat to All" (Say) is index 2 = left col, row 2: y in [-85,-68). Say is available. Assert.True(menu.OnEvent(new UiEvent(0, menu, UiEventType.MouseDown, 0, 10, -76))); @@ -111,7 +114,8 @@ public class UiMenuTests Assert.True(menu.OnEvent(new UiEvent(0, menu, UiEventType.MouseDown, 0, 10, 5))); // open object? fired = null; - menu.OnSelect = p => fired = p; + // Mirror the controller: the widget reports the pick, the controller sets Selected. + menu.OnSelect = p => { fired = p; if (p is ChatChannelKind) menu.Selected = p; }; // "Tell to Trade Chat" (Trade) is index 11 = right col (lx>=191), row 4: y in [-51,-34). Assert.True(menu.OnEvent(new UiEvent(0, menu, UiEventType.MouseDown, 0, 200, -42))); @@ -120,17 +124,21 @@ public class UiMenuTests } [Fact] - public void Select_SpecialItem_DoesNotFire() + public void Select_SpecialItem_FiresNull_LeavesSelectionUnchanged() { - var menu = MakeMenu(); + var menu = MakeMenu(); // Selected = Say Assert.True(menu.OnEvent(new UiEvent(0, menu, UiEventType.MouseDown, 0, 10, 5))); // open - int fired = 0; - menu.OnSelect = _ => fired++; - // "Squelch (ignore)" is index 0 = left col, row 0 (null payload): y in [-119,-102). - // null payload → ChannelAvailable returns false → inert. + // Mirror the controller: only channel payloads update Selected; the null-payload + // specials are deferred no-ops that leave the active channel + highlight unchanged. + bool fired = false; object? firedPayload = "sentinel"; + menu.OnSelect = p => { fired = true; firedPayload = p; if (p is ChatChannelKind) menu.Selected = p; }; + + // "Squelch (ignore)" is index 0 = left col, row 0 (null payload), white/enabled. Assert.True(menu.OnEvent(new UiEvent(0, menu, UiEventType.MouseDown, 0, 10, -110))); - Assert.Equal(0, fired); + Assert.True(fired); // the pick IS reported... + Assert.Null(firedPayload); // ...with the special's null payload + Assert.Equal(ChatChannelKind.Say, menu.Selected); // ...but selection is unchanged (deferred no-op) } [Fact]