From a944840aee2a27fb81289fcf7187f81f50270802 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Fri, 23 Oct 2026 09:09:41 +1100 Subject: [PATCH] Take the tray pieces from PlatformHeads DiffEngineTray takes PlatformHeads.WindowsForms. SessionEndWindow moves there, with the tray logging what its ending threw, and MenuBuilder builds the tracked items through MenuOnOpening. IssueLauncher and LinkLauncher sit on IssueLink and LinkOpener: a body is now plain text, encoded once, and PiperTest marks its message as raised through IssueLauncher.AlreadyRaised rather than by reflecting on a private field. BuildMonitor's tray takes the same pieces. All six PlatformHeads pins move to 0.5.0. --- .github/workflows/build-native.yml | 2 +- claude.md | 7 ++- native/CMakeLists.txt | 2 +- native/swift/Package.swift | 2 +- .../IssueLauncherTests.cs | 9 +-- src/DiffEngineTray.Tests/PiperTest.cs | 5 +- .../SessionEndWindowTest.cs | 36 +---------- src/DiffEngineTray/DiffEngineTray.csproj | 4 ++ src/DiffEngineTray/IssueLauncher.cs | 52 +++++++-------- src/DiffEngineTray/LinkLauncher.cs | 20 ++---- src/DiffEngineTray/MenuBuilder.cs | 52 ++++----------- src/DiffEngineTray/Program.cs | 4 +- src/DiffEngineTray/SessionEndWindow.cs | 63 ------------------- src/Directory.Packages.props | 6 +- .../Package.DiffEngineTray.verified.txt | 2 + 15 files changed, 68 insertions(+), 198 deletions(-) delete mode 100644 src/DiffEngineTray/SessionEndWindow.cs diff --git a/.github/workflows/build-native.yml b/.github/workflows/build-native.yml index 5b49f0f87..452a4172e 100644 --- a/.github/workflows/build-native.yml +++ b/.github/workflows/build-native.yml @@ -35,7 +35,7 @@ concurrency: jobs: native: - uses: SimonCropp/PlatformHeads/.github/workflows/build-native.yml@0.4.0 + uses: SimonCropp/PlatformHeads/.github/workflows/build-native.yml@0.5.0 permissions: contents: write pull-requests: write diff --git a/claude.md b/claude.md index 9ac174059..c9adfc5ad 100644 --- a/claude.md +++ b/claude.md @@ -752,8 +752,11 @@ the comment there about not caching "nothing staged" asks for. second being what `ViewerClient.FoundUnowned` is for. - `Program.Main` is a synchronous `[STAThread]` method that blocks on `Inner`. An attribute on an `async Task Main` lands on a method the runtime does not start, and the thread came up MTA. -- `SessionEndWindow` is a hidden top level window that hears `WM_ENDSESSION`, which neither the - notify icon nor a message filter does. An owning tray stages its queue from inside the message +- `SessionEndWindow`, PlatformHeads.WindowsForms', is a hidden top level window that hears + `WM_ENDSESSION`, which neither the notify icon nor a message filter does. So are + `MenuOnOpening`, which builds the menu's tracked items on every open beside its fixed ones and + disposes them at the next, and `IssueLink` and `LinkOpener` under `IssueLauncher` and + `LinkLauncher`: BuildMonitor's tray shares them, and a fix to any of them goes there. An owning tray stages its queue from inside the message (`OwnedInlineHost.SessionEnding`) and refuses patches from then on, because a logoff never comes back through `Application.Run()`. - A move that arrives for a tracked pair with no tool keeps the tool it was tracked with diff --git a/native/CMakeLists.txt b/native/CMakeLists.txt index d98a4b0c6..46ad56591 100644 --- a/native/CMakeLists.txt +++ b/native/CMakeLists.txt @@ -15,7 +15,7 @@ include(FetchContent) FetchContent_Declare( platformheads GIT_REPOSITORY https://github.com/SimonCropp/PlatformHeads.git - GIT_TAG 0.4.0 + GIT_TAG 0.5.0 GIT_SHALLOW ON) FetchContent_MakeAvailable(platformheads) include(${platformheads_SOURCE_DIR}/cmake/PlatformHeads.cmake) diff --git a/native/swift/Package.swift b/native/swift/Package.swift index ab7fad6da..f3c569182 100644 --- a/native/swift/Package.swift +++ b/native/swift/Package.swift @@ -12,7 +12,7 @@ let package = Package( dependencies: [ // What every AppKit head does underneath its drawing, shared with BuildMonitor. The sixth // pin of PlatformHeads, moved with the other five. - .package(url: "https://github.com/SimonCropp/PlatformHeads", exact: "0.4.0") + .package(url: "https://github.com/SimonCropp/PlatformHeads", exact: "0.5.0") ], targets: [ // Exists only to import the ABI. Swift does not guarantee struct layout, so the structs diff --git a/src/DiffEngineTray.Tests/IssueLauncherTests.cs b/src/DiffEngineTray.Tests/IssueLauncherTests.cs index 9268b8997..6f399c94f 100644 --- a/src/DiffEngineTray.Tests/IssueLauncherTests.cs +++ b/src/DiffEngineTray.Tests/IssueLauncherTests.cs @@ -30,15 +30,16 @@ public async Task An_ampersand_in_the_title_does_not_truncate_it() } /// - /// The body is encoded by its callers and handed over already escaped, so encoding the title - /// must not have changed what reaches GitHub as the body. + /// Handed over as plain text and encoded once, after the version and OS. /// [Test] public async Task Keeps_the_body_it_is_given() { - var url = IssueLauncher.BuildUrl("TheTitle", WebUtility.UrlEncode("\n * Action: TheAction")); + var url = IssueLauncher.BuildUrl("TheTitle", "\n * Action: R&D #1"); - await Assert.That(Query(url)["body"]).Contains("* Action: TheAction"); + var body = Query(url)["body"]; + await Assert.That(body).Contains("* DiffEngineTray Version:"); + await Assert.That(body).EndsWith("* Action: R&D #1"); } static Dictionary Query(string url) => diff --git a/src/DiffEngineTray.Tests/PiperTest.cs b/src/DiffEngineTray.Tests/PiperTest.cs index 5b85ce607..26bfcd418 100644 --- a/src/DiffEngineTray.Tests/PiperTest.cs +++ b/src/DiffEngineTray.Tests/PiperTest.cs @@ -315,10 +315,7 @@ public async Task AClientThatResetsBeforeItIsAcceptedIsNotReportedAsAnError() .CreateLogger(); // ExceptionHandler follows the log with a modal "open an issue?" box, which here would wait // for a click forever. It asks once per message, so this one is marked as already asked. - var asked = (ConcurrentBag) typeof(IssueLauncher) - .GetField("recorded", BindingFlags.NonPublic | BindingFlags.Static)! - .GetValue(null)!; - asked.Add("Failed to receive payload"); + IssueLauncher.AlreadyRaised("Failed to receive payload"); using var cancel = new CancelSource(); var held = new HeldContext(); diff --git a/src/DiffEngineTray.Tests/SessionEndWindowTest.cs b/src/DiffEngineTray.Tests/SessionEndWindowTest.cs index ee226aa42..2c6babc4e 100644 --- a/src/DiffEngineTray.Tests/SessionEndWindowTest.cs +++ b/src/DiffEngineTray.Tests/SessionEndWindowTest.cs @@ -1,6 +1,7 @@ /// /// The two messages a logoff or a shutdown sends every top level window, sent to the one the tray -/// keeps for hearing them. +/// keeps for hearing them, wired as Program wires it. Whether the window hears a session ending, +/// and not one that carried on, is PlatformHeads' and tested there. /// /// Sent from the thread that made the window, which calls its procedure directly, so no message /// loop is needed and nothing here awaits between making the window and destroying it. @@ -8,39 +9,6 @@ /// public class SessionEndWindowTest { - [Test] - public async Task TheSessionEndingIsHeard() - { - var heard = 0; - IntPtr query; - using (var window = new SessionEndWindow(() => heard++)) - { - query = SendMessage(window.Handle, queryEndSession, IntPtr.Zero, logoff); - SendMessage(window.Handle, endSession, new(1), logoff); - } - - // Agreed to. Refusing would hold up the logoff, and there is nothing here worth that - await Assert.That(query).IsEqualTo(new(1)); - await Assert.That(heard).IsEqualTo(1); - } - - /// - /// Another application refused the query, so everything that agreed is told the session goes - /// on. A tray that staged its queue for that would have it both on disk and in memory. - /// - [Test] - public async Task ASessionThatDidNotEndAfterAllIsNotHeard() - { - var heard = 0; - using (var window = new SessionEndWindow(() => heard++)) - { - SendMessage(window.Handle, queryEndSession, IntPtr.Zero, logoff); - SendMessage(window.Handle, endSession, IntPtr.Zero, logoff); - } - - await Assert.That(heard).IsEqualTo(0); - } - /// /// Wired the way Program wires it, over a queue this process holds. The session ending never /// reaches the disposal that stages a queue on a clean exit: the message loop does not return diff --git a/src/DiffEngineTray/DiffEngineTray.csproj b/src/DiffEngineTray/DiffEngineTray.csproj index 718107f82..74e937095 100644 --- a/src/DiffEngineTray/DiffEngineTray.csproj +++ b/src/DiffEngineTray/DiffEngineTray.csproj @@ -26,8 +26,12 @@ + + + diff --git a/src/DiffEngineTray/IssueLauncher.cs b/src/DiffEngineTray/IssueLauncher.cs index 0fa389e5c..ada13f027 100644 --- a/src/DiffEngineTray/IssueLauncher.cs +++ b/src/DiffEngineTray/IssueLauncher.cs @@ -1,29 +1,34 @@ static class IssueLauncher { - static ConcurrentBag recorded = []; - static string defaultBody; + static readonly IssueLink link = new("https://github.com/VerifyTests/DiffEngine"); - static IssueLauncher() => - defaultBody = WebUtility.UrlEncode( - $""" - * DiffEngineTray Version: {VersionReader.VersionString} - * OS: {Environment.OSVersion.VersionString} - """); + static readonly string defaultBody = + $""" + * DiffEngineTray Version: {VersionReader.VersionString} + * OS: {Environment.OSVersion.VersionString} + """; public static void Launch() => LinkLauncher.LaunchUrl(BuildUrl("TODO")); /// - /// The title is encoded like the body is. It is built from a message carrying a file path, and - /// a '#' in one started a fragment - dropping the body, and everything of the title after it - - /// while an '&' started a parameter GitHub does not have, truncating the title there. + /// Marks as already offered, so a test that makes it happen is not + /// left waiting on the modal box nobody is there to answer. + /// + internal static void AlreadyRaised(string message) => + link.FirstTime(message); + + /// + /// The title and body are encoded by , so a path's '#' or '&' in + /// the title neither drops the body nor cuts the title short. is + /// plain text. /// internal static string BuildUrl(string title, string extraBody = "") => - $"https://github.com/VerifyTests/DiffEngine/issues/new?title={WebUtility.UrlEncode(title)}&body={defaultBody}{extraBody}"; + link.Url(title, defaultBody + extraBody); public static void LaunchForException(string message, Exception exception) { - if (CheckRecorded(message)) + if (!link.FirstTime(message)) { return; } @@ -42,7 +47,7 @@ Open an issue on GitHub? return; } - var extraBody = WebUtility.UrlEncode( + var extraBody = $""" * Action: {message} @@ -50,13 +55,13 @@ Open an issue on GitHub? ``` {exception} ``` - """); + """; LinkLauncher.LaunchUrl(BuildUrl(message, extraBody)); } public static void LaunchForException(string message) { - if (CheckRecorded(message)) + if (!link.FirstTime(message)) { return; } @@ -73,11 +78,11 @@ Open an issue on GitHub? return; } - var extraBody = WebUtility.UrlEncode( + var extraBody = $""" * Action: {message} - """); + """; LinkLauncher.LaunchUrl(BuildUrl(message, extraBody)); } @@ -97,15 +102,4 @@ static bool AskIfOpenIssue(string text) MessageBoxIcon.Error); return result == DialogResult.No; } - - static bool CheckRecorded(string message) - { - if (recorded.Contains(message)) - { - return true; - } - - recorded.Add(message); - return false; - } } \ No newline at end of file diff --git a/src/DiffEngineTray/LinkLauncher.cs b/src/DiffEngineTray/LinkLauncher.cs index 2695d1eb5..aae422fe3 100644 --- a/src/DiffEngineTray/LinkLauncher.cs +++ b/src/DiffEngineTray/LinkLauncher.cs @@ -1,25 +1,15 @@ static class LinkLauncher { /// - /// Logged when it cannot be opened, and nothing more: no browser registered, or one that will - /// not start, is nothing the tray can do anything about. Thrown, it went onto the UI thread - /// from a click in the options form, and out of the handler that reports an error, in place - /// of the error being reported. + /// Logged when it cannot be opened, and nothing more: see . Thrown, it + /// went onto the UI thread from a click in the options form, and out of the handler that + /// reports an error, in place of the error being reported. /// public static void LaunchUrl(string url) { - var startInfo = new ProcessStartInfo + if (!LinkOpener.TryOpen(url, out var error)) { - UseShellExecute = true, - FileName = url - }; - try - { - using var process = Process.Start(startInfo); - } - catch (Exception exception) - { - Log.Error(exception, "Failed to open {Url}", url); + Log.Error(error, "Failed to open {Url}", url); } } } diff --git a/src/DiffEngineTray/MenuBuilder.cs b/src/DiffEngineTray/MenuBuilder.cs index bcb06ba5a..d8d858964 100644 --- a/src/DiffEngineTray/MenuBuilder.cs +++ b/src/DiffEngineTray/MenuBuilder.cs @@ -7,35 +7,6 @@ public static ContextMenuStrip Build(Action exit, Action launchOptions, Tracker DefaultDropDownDirection = ToolStripDropDownDirection.AboveLeft }; var items = menu.Items; - // The items that survive a close, held as themselves. They used to be told apart by their - // text, so a solution named after one of them - Options, say - gave that group a header - // that was never removed, and another one on every open after that - var fixedItems = new HashSet(); - // Removed while closing and disposed at the next open, rather than disposed while the - // close they were removed by is still running - var removed = new List(); - menu.Closed += delegate - { - foreach (var item in TrackingMenuItems(items, fixedItems)) - { - items.Remove(item); - removed.Add(item); - } - }; - menu.Opening += delegate - { - foreach (var item in removed) - { - item.Dispose(); - } - - removed.Clear(); - - foreach (var item in BuildTrackingMenuItems(tracker)) - { - items.Add(item); - } - }; menu.Font = new(menu.Font.FontFamily, 10); items.Add(new MenuButton("Exit", exit, Images.Exit)); items.Add(new MenuButton("Options", launchOptions, Images.Options)); @@ -43,20 +14,21 @@ public static ContextMenuStrip Build(Action exit, Action launchOptions, Tracker items.Add(new MenuButton("Open logs", Logging.OpenDirectory, Images.Folder)); items.Add(new MenuButton("Purge verified files", FilePurger.Launch, Images.Folder)); items.Add(new MenuButton("Raise issue", IssueLauncher.Launch, Images.Link)); - fixedItems.UnionWith(items.Cast()); + // After the fixed items, which it keeps as themselves: told apart by their text, a + // solution named after one of them - Options, say - gave that group a header that was + // never removed. The tracked ones are built again on every open, and disposed at the next. + MenuOnOpening.Rebuild( + menu, + _ => + { + foreach (var item in BuildTrackingMenuItems(tracker)) + { + _.Add(item); + } + }); return menu; } - /// - /// Everything the last open added, which is everything but the items put - /// there. Materialised, because the caller is removing them from the collection it reads. - /// - static List TrackingMenuItems(ToolStripItemCollection items, HashSet fixedItems) => - items - .Cast() - .Where(_ => !fixedItems.Contains(_)) - .ToList(); - internal static IEnumerable BuildTrackingMenuItems(Tracker tracker) { // Read everything first and decide from the counts. TrackingAny is backed by the scan diff --git a/src/DiffEngineTray/Program.cs b/src/DiffEngineTray/Program.cs index 5efc8c61b..30088ae3a 100644 --- a/src/DiffEngineTray/Program.cs +++ b/src/DiffEngineTray/Program.cs @@ -108,7 +108,9 @@ void Warn(string message) => // A logoff or a shutdown does not come back through Application.Run(), so nothing the // unwind below performs happens for one - using var sessionEnd = new SessionEndWindow(SessionEnding(owned, TrayVersionFile.Delete)); + using var sessionEnd = new SessionEndWindow( + SessionEnding(owned, TrayVersionFile.Delete), + _ => Log.Error(_, "Failed while the session was ending")); // Not a using. Anything throwing between here and the await below would dispose a task // that is still running, and Task.Dispose throws for one that has not completed - which diff --git a/src/DiffEngineTray/SessionEndWindow.cs b/src/DiffEngineTray/SessionEndWindow.cs deleted file mode 100644 index e3500067d..000000000 --- a/src/DiffEngineTray/SessionEndWindow.cs +++ /dev/null @@ -1,63 +0,0 @@ -/// -/// Hears that the Windows session is ending, which is how a tray started at login usually stops. -/// -/// Nothing else here would. A logoff or a shutdown sends every top level window -/// WM_QUERYENDSESSION and then WM_ENDSESSION, and Windows may end the process as soon as those are -/// answered. A Form turns them into a close, which is how the viewer hears of it. The tray has no -/// Form, only the notify icon's window, which agrees to the query and does nothing more: so -/// Application.Run() never returns, nothing after it in Program runs, and the queue an -/// owning tray holds in memory went with the process where a clean exit stages it. -/// -/// -/// A window of its own, because the two messages are sent rather than posted, which a message -/// filter never sees. Top level, because a message only window is not sent them. Never shown. -/// -/// -/// On WM_ENDSESSION rather than on the query before it. Another application can still refuse the -/// query, and a session that then carries on would have a queue both staged on disk and held here. -/// -/// -sealed class SessionEndWindow : - NativeWindow, - IDisposable -{ - const int endSession = 0x0016; - - readonly Action ending; - - /// - /// Run inside the message, on the thread this is created on, so whatever it does is done by - /// the time the message is answered: there may be no time after that. - /// - public SessionEndWindow(Action ending) - { - this.ending = ending; - CreateHandle(new()); - } - - protected override void WndProc(ref Message message) - { - // WParam is zero when the session is not ending after all, which everything that agreed - // to the query is told once something else has refused it - if (message.Msg == endSession && - message.WParam != IntPtr.Zero) - { - try - { - ending(); - } - catch (Exception exception) - { - // Logged and nothing more. Thrown out of here it reaches WinForms' exception - // dialog, which during a logoff is a window holding the session open with nobody - // left to answer it - Log.Error(exception, "Failed while the session was ending"); - } - } - - base.WndProc(ref message); - } - - public void Dispose() => - DestroyHandle(); -} diff --git a/src/Directory.Packages.props b/src/Directory.Packages.props index 9eeed2071..3308eface 100644 --- a/src/Directory.Packages.props +++ b/src/Directory.Packages.props @@ -19,9 +19,9 @@ --> - - - + + + diff --git a/src/Packaging.Tests/Package.DiffEngineTray.verified.txt b/src/Packaging.Tests/Package.DiffEngineTray.verified.txt index 6118a801d..ecac32b3a 100644 --- a/src/Packaging.Tests/Package.DiffEngineTray.verified.txt +++ b/src/Packaging.Tests/Package.DiffEngineTray.verified.txt @@ -11,6 +11,8 @@ tools/net10.0/any/DiffEngineTray.dll tools/net10.0/any/DiffEngineTray.runtimeconfig.json tools/net10.0/any/DotnetToolSettings.xml tools/net10.0/any/EmptyFiles.dll +tools/net10.0/any/PlatformHeads.Core.dll +tools/net10.0/any/PlatformHeads.WindowsForms.dll tools/net10.0/any/Serilog.Sinks.File.dll tools/net10.0/any/Serilog.dll tools/net10.0/any/viewer/documents/AngleSharp.dll