From abd10d0a808bf745da7ed77695268ba33e3ab20b Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 19:34:13 +0000 Subject: [PATCH 1/2] fix: do not activate a disabled toggle from a tray icon click [patch] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OnTrayIconClicked found the first TrayToggleItem in the menu and called Activate unconditionally, never reading model.IsEnabled. The native menu path is gated for free — the toolkit greys a disabled NativeMenuItem out and will not raise Click for it — so the tray icon was the one path from a click to the tool's setter with nothing in the way. A tool registering a toggle with an isEnabled predicate therefore saw its setter run on a left click while the menu showed the same action greyed out, invoking logic the tool had explicitly gated off. The click handler now skips disabled toggles. Activate also returns early for a disabled item, which covers the gap the native gating leaves: an item is greyed out as of the last refresh, and a tool's state can change between painting the menu and the user clicking it. IsEnabled is the value the paint used, so this agrees with what the user saw. OnTrayIconClicked becomes internal so it can be driven with no display, as the rest of this class's tests already are. Fixes ktsu-dev/TrayApp#7 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018VSTy8Ye7JXqeFqvhrmnRt --- TrayApp.Test/TrayApplicationTests.cs | 88 ++++++++++++++++++++++++++++ TrayApp/Tray/TrayApplication.cs | 35 ++++++++++- 2 files changed, 121 insertions(+), 2 deletions(-) diff --git a/TrayApp.Test/TrayApplicationTests.cs b/TrayApp.Test/TrayApplicationTests.cs index 8164323..f85792f 100644 --- a/TrayApp.Test/TrayApplicationTests.cs +++ b/TrayApp.Test/TrayApplicationTests.cs @@ -122,4 +122,92 @@ public void Dispose_IsIdempotent() // call throws, and the lifetime's Exit handler has usually disposed it already by then. Assert.AreEqual(0, stops); } + + [TestMethod] + public void TrayIconClick_WithTheToggleDisabled_DoesNotRunTheToolsSetter() + { + int setterCalls = 0; + bool available = false; + + TrayAppDefinition app = TrayAppBuilder.Create("demo") + .DisplayName("Demo") + .Toggle("Keep awake", () => false, _ => setterCalls++, isEnabled: () => available) + .Build(); + + using TrayApplication host = Host(app, out TrayMenu menu); + + // What painting the menu would have read. IsEnabled is a cache of the last refresh, so without this + // the item still carries its default of enabled and the assertion would pass for the wrong reason. + menu.Refresh(); + + host.OnTrayIconClicked(this, EventArgs.Empty); + + // The menu shows this greyed out, so a click on the icon has to agree with it. The native menu entry + // is refused by the toolkit; the icon is the path with nothing in the way. + Assert.AreEqual(0, setterCalls); + } + + [TestMethod] + public void TrayIconClick_WithTheToggleEnabled_RunsTheToolsSetter() + { + int setterCalls = 0; + + TrayAppDefinition app = TrayAppBuilder.Create("demo") + .DisplayName("Demo") + .Toggle("Keep awake", () => false, _ => setterCalls++, isEnabled: () => true) + .Build(); + + using TrayApplication host = Host(app, out TrayMenu menu); + menu.Refresh(); + + host.OnTrayIconClicked(this, EventArgs.Empty); + + // The fast path still has to work. Guarding it is worth nothing if it guards everything. + Assert.AreEqual(1, setterCalls); + } + + [TestMethod] + public void TrayIconClick_BecomingEnabledAgain_RunsTheToolsSetter() + { + int setterCalls = 0; + bool available = false; + + TrayAppDefinition app = TrayAppBuilder.Create("demo") + .DisplayName("Demo") + .Toggle("Keep awake", () => false, _ => setterCalls++, isEnabled: () => available) + .Build(); + + using TrayApplication host = Host(app, out TrayMenu menu); + + menu.Refresh(); + host.OnTrayIconClicked(this, EventArgs.Empty); + + available = true; + menu.Refresh(); + host.OnTrayIconClicked(this, EventArgs.Empty); + + // Refused while the tool said it was unavailable, served once it said otherwise. The guard reads the + // refreshed state rather than latching on the first answer. + Assert.AreEqual(1, setterCalls); + } + + [TestMethod] + public void TrayIconClick_WithNoToggleAtAll_DoesNothing() + { + int commandCalls = 0; + + TrayAppDefinition app = TrayAppBuilder.Create("demo") + .DisplayName("Demo") + .Command("Two", () => commandCalls++) + .Build(); + + using TrayApplication host = Host(app, out TrayMenu menu); + menu.Refresh(); + + host.OnTrayIconClicked(this, EventArgs.Empty); + + // The fast path is for toggles. A menu of commands must not have one picked for it, and quit least + // of all. + Assert.AreEqual(0, commandCalls); + } } diff --git a/TrayApp/Tray/TrayApplication.cs b/TrayApp/Tray/TrayApplication.cs index b3f1118..83c2d58 100644 --- a/TrayApp/Tray/TrayApplication.cs +++ b/TrayApp/Tray/TrayApplication.cs @@ -176,11 +176,25 @@ private void BuildNativeMenu() } } - private void OnTrayIconClicked(object? sender, EventArgs e) + /// + /// Flips the first toggle the user could have clicked in the menu. + /// + /// + /// A disabled toggle is skipped, because this is the only activation path with nothing between the + /// click and the tool's setter. The menu path is projected onto s that + /// the toolkit greys out and refuses to raise Click for, so the tray icon is where a tool's + /// isEnabled predicate would otherwise be silently ignored - the menu would show the action + /// as unavailable while a click on the icon ran it anyway. + /// + /// Internal so that it can be driven without a platform: everything above this reads menu state + /// that populates with no display attached. + /// + /// + internal void OnTrayIconClicked(object? sender, EventArgs e) { foreach ((TrayMenuItem model, _) in entries) { - if (model is TrayToggleItem) + if (model is TrayToggleItem { IsEnabled: true }) { Activate(model); return; @@ -192,8 +206,25 @@ private void OnTrayIconClicked(object? sender, EventArgs e) private void OnExit(object? sender, ControlledApplicationLifetimeExitEventArgs e) => Dispose(); + /// + /// Runs one item's action and brings the menu back in line with it. + /// + /// + /// The enabled check here is the backstop rather than the guard that matters. A native menu item is + /// greyed out at the last refresh, so the toolkit already refuses to raise Click for it - but + /// "at the last refresh" is the gap: a tool's state can change between the paint and the click, and + /// nothing repaints a menu the user is already looking at. is + /// the value that painting used, so this agrees with what the user saw rather than second-guessing + /// it. + /// + /// The item to activate. private void Activate(TrayMenuItem item) { + if (!item.IsEnabled) + { + return; + } + bool succeeded = menu.Activate(item); if (succeeded && item is TrayToggleItem toggle) From 650bb1e98d6b4024f920a568f05bfab37d147f0a Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 19:46:57 +0000 Subject: [PATCH 2/2] test: cover the disabled-item backstop in Activate [patch] SonarCloud failed the quality gate at 66.7% coverage on new code. The uncovered line was the early return in Activate: every test drove OnTrayIconClicked, which skips a disabled toggle before Activate is reached, so the backstop behind the native menu was never executed. The honest fix is to test it rather than delete it. The guard covers the gap native greying cannot - an item greyed out as of the last refresh, with a click already on its way - and dropping a correct check to raise a coverage number would trade the fix for the metric. NativeMenuItem exposes Click with no way to raise it, so the native path cannot be driven from a test at all. Activate becomes internal, as OnTrayIconClicked already is and for the same reason, and the new test calls it with an item the tool has since disabled. Verified the test fails without the guard (setter runs once against an expected zero), and against a local cobertura report that every executable line this PR adds is now hit. The two remaining uncovered lines in the file, OnQuitRequested and OnExit, are pre-existing and untouched by this diff. 113 tests: 112 pass, 1 pre-existing skip. Solution builds with 0 warnings. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018VSTy8Ye7JXqeFqvhrmnRt --- TrayApp.Test/TrayApplicationTests.cs | 27 +++++++++++++++++++++++++++ TrayApp/Tray/TrayApplication.cs | 7 ++++++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/TrayApp.Test/TrayApplicationTests.cs b/TrayApp.Test/TrayApplicationTests.cs index f85792f..bfe0e62 100644 --- a/TrayApp.Test/TrayApplicationTests.cs +++ b/TrayApp.Test/TrayApplicationTests.cs @@ -191,6 +191,33 @@ public void TrayIconClick_BecomingEnabledAgain_RunsTheToolsSetter() Assert.AreEqual(1, setterCalls); } + [TestMethod] + public void Activate_WithTheItemDisabled_DoesNotRunTheToolsSetter() + { + int setterCalls = 0; + bool available = true; + + TrayAppDefinition app = TrayAppBuilder.Create("demo") + .DisplayName("Demo") + .Toggle("Keep awake", () => false, _ => setterCalls++, isEnabled: () => available) + .Build(); + + using TrayApplication host = Host(app, out TrayMenu menu); + + menu.Refresh(); + available = false; + menu.Refresh(); + + TrayMenuItem toggle = menu.Items.First(item => item is TrayToggleItem); + host.Activate(toggle); + + // The backstop behind the native menu. The toolkit will not raise Click for a greyed-out item, but + // it greys it out as of the last refresh, and a click already on its way when the tool became + // unavailable still arrives here. NativeMenuItem exposes Click with no way to raise it, so this + // calls the handler's target rather than the event. + Assert.AreEqual(0, setterCalls); + } + [TestMethod] public void TrayIconClick_WithNoToggleAtAll_DoesNothing() { diff --git a/TrayApp/Tray/TrayApplication.cs b/TrayApp/Tray/TrayApplication.cs index 83c2d58..e0b315d 100644 --- a/TrayApp/Tray/TrayApplication.cs +++ b/TrayApp/Tray/TrayApplication.cs @@ -216,9 +216,14 @@ internal void OnTrayIconClicked(object? sender, EventArgs e) /// nothing repaints a menu the user is already looking at. is /// the value that painting used, so this agrees with what the user saw rather than second-guessing /// it. + /// + /// Internal for the same reason as : the native Click that + /// reaches this cannot be raised from a test, since exposes the event + /// and no way to fire it, so the backstop is exercised by calling it. + /// /// /// The item to activate. - private void Activate(TrayMenuItem item) + internal void Activate(TrayMenuItem item) { if (!item.IsEnabled) {