Keep the restore error and stop the tool once when tray start-up fails

README.md

@@ -238,6 +238,8 @@ These are the parts that look like they could be simplified away and cannot.
 | Behaviour | What breaks without it |
 |-----------|------------------------|
 | Catching **all** exceptions out of the Avalonia run call, and using `HasStarted` to tell "the tray ran and quit" from "the tray never came up" | Avalonia's `DBusTrayIconImpl.WatchAsync` is `async void` with an exception filter that stops applying once the icon is disposed, so an ordinary Linux shutdown throws `TaskCanceledException` *after* the tray has done its job. Without the flag, a clean quit restarts in the console. |
+| Tracking `ToolStarted` separately from `HasStarted`, and never falling back to the console once it is set | Start-up can still throw after the start action has run (a status getter, a missing icon resource). Falling back then starts the tool a second time and never stops the first start. |
+| Re-seeding a failed preference restore after a successful start action | `TrayMenu.Run` clears `LastError` on success, so the restore failure would be wiped before the menu was ever painted. |
 | Falling back to the console when the windowing stack refuses after `DesktopSession` said yes | A `DISPLAY` pointing at nothing, a missing libX11, or no status-notifier host kills a process someone started to hold a resource. |
 | Constructing menu items eagerly and the `TrayIcon` lazily | `TrayIcon`'s constructor reaches for a platform handle that does not exist until Avalonia has started. |
 | Keeping a failed action's message in a field rather than writing it into the menu item | The refresh that runs immediately after every action overwrites anything written directly. |

TrayApp.Test/TrayApplicationTests.cs

@@ -5,6 +5,7 @@ namespace ktsu.TrayApp.Test;
 using System.Linq;
 using Avalonia.Controls;
 using ktsu.TrayApp;
+using ktsu.TrayApp.Cli;
 using ktsu.TrayApp.Menu;
 using ktsu.TrayApp.Tray;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
@@ -237,4 +238,194 @@ public void TrayIconClick_WithNoToggleAtAll_DoesNothing()
 		// of all.
 		Assert.AreEqual(0, commandCalls);
 	}
+
+	[TestMethod]
+	public void StartTool_WhenTheStartActionSucceeds_KeepsTheSeededRestoreError()
+	{
+		TrayAppDefinition app = TrayAppBuilder.Create("demo")
+			.DisplayName("Demo")
+			.Status(() => "idle")
+			.Toggle("One", () => false, _ => { })
+			.OnStart(() => { })
+			.Build();
+
+		using TrayApplication host = Host(app, out TrayMenu menu);
+		menu.SeedError("no inhibitor");
+
+		host.StartTool();
+
+		// The remembered setting was not applied, and a successful start does not change that. Clearing the
+		// message here would put a healthy status line in front of the user before the menu is ever painted.
+		Assert.AreEqual("no inhibitor", menu.LastError);
+		Assert.AreEqual("Demo failed: no inhibitor", menu.StatusItem!.Header);
+	}
+
+	[TestMethod]
+	public void StartTool_WhenTheStartActionFails_ReportsItsOwnError()
+	{
+		TrayAppDefinition app = TrayAppBuilder.Create("demo")
+			.DisplayName("Demo")
+			.Status(() => "idle")
+			.Toggle("One", () => false, _ => { })
+			.OnStart(() => throw new InvalidOperationException("device busy"))
+			.Build();
+
+		using TrayApplication host = Host(app, out TrayMenu menu);
+		menu.SeedError("no inhibitor");
+
+		host.StartTool();
+
+		// The start refusal is the newer and more pressing failure, so it wins the one status line.
+		Assert.AreEqual("device busy", menu.LastError);
+	}
+
+	[TestMethod]
+	public void Dispose_AfterTheToolStartedButBeforeTheTrayFinishedStarting_StopsTheTool()
+	{
+		int stops = 0;
+
+		TrayAppDefinition app = TrayAppBuilder.Create("demo")
+			.DisplayName("Demo")
+			.Toggle("One", () => false, _ => { })
+			.OnStart(() => { })
+			.OnStop(() => stops++)
+			.Build();
+
+		TrayApplication host = Host(app, out _);
+		host.StartTool();
+		host.Dispose();
+
+		// Whatever threw between the start action and the end of start-up, the tool is holding what it took,
+		// and nothing else will release it.
+		Assert.IsFalse(host.HasStarted);
+		Assert.AreEqual(1, stops);
+	}
+
+	[TestMethod]
+	public void RecoverFromTrayFailure_AfterTheToolStarted_DoesNotStartItAgain()
+	{
+		int starts = 0;
+		int stops = 0;
+
+		TrayAppDefinition app = TrayAppBuilder.Create("demo")
+			.DisplayName("Demo")
+			.Toggle("One", () => false, _ => { })
+			.OnStart(() => starts++)
+			.OnStop(() => stops++)
+			.Build();
+
+		using TrayApplication host = Host(app, out _);
+		host.StartTool();
+
+		// What a missing icon resource or a throwing status getter looks like from RunTray: start-up threw
+		// after the start action had run, so HasStarted is still false.
+		int exitCode = TrayAppBuilder.RecoverFromTrayFailure(
+			app,
+			new CommandLineOptions { Duration = TimeSpan.Zero },
+			host,
+			new InvalidOperationException("The tray icon 'nope-idle.png' is missing"));
+
+		Assert.AreEqual(1, starts);
+		Assert.AreEqual(1, stops);
+		Assert.AreEqual(1, exitCode);
+	}
+
+	[TestMethod]
+	public void RecoverFromTrayFailure_BeforeTheToolStarted_FallsBackToTheConsole()
+	{
+		int starts = 0;
+		int stops = 0;
+
+		TrayAppDefinition app = TrayAppBuilder.Create("demo")
+			.DisplayName("Demo")
+			.Toggle("One", () => false, _ => { })
+			.OnStart(() => starts++)
+			.OnStop(() => stops++)
+			.Build();
+
+		using TrayApplication host = Host(app, out _);
+
+		int exitCode = TrayAppBuilder.RecoverFromTrayFailure(
+			app,
+			new CommandLineOptions { Duration = TimeSpan.Zero },
+			host,
+			new InvalidOperationException("XOpenDisplay failed"));
+
+		// The windowing stack refused before anything was started, so the terminal run is the only start.
+		Assert.AreEqual(1, starts);
+		Assert.AreEqual(1, stops);
+		Assert.AreEqual(0, exitCode);
+	}
+
+	[TestMethod]
+	public void StartTool_WithNoStartAction_KeepsTheSeededErrorAndRecordsTheStart()
+	{
+		using TrayApplication host = Host(Definition(), out TrayMenu menu);
+		menu.SeedError("no inhibitor");
+
+		host.StartTool();
+
+		// There is no start action to succeed, so nothing clears the restore error; the tool still counts as
+		// started, so a later failure stops it rather than starting it again in the terminal.
+		Assert.IsTrue(host.ToolStarted);
+		Assert.AreEqual("no inhibitor", menu.LastError);
+	}
+
+	[TestMethod]
+	public void RecoverFromTrayFailure_WithNoHost_FallsBackToTheConsole()
+	{
+		int starts = 0;
+
+		TrayAppDefinition app = TrayAppBuilder.Create("demo")
+			.DisplayName("Demo")
+			.Toggle("One", () => false, _ => { })
+			.OnStart(() => starts++)
+			.Build();
+
+		// Avalonia can refuse before it ever asks for the application, leaving no host to dispose.
+		int exitCode = TrayAppBuilder.RecoverFromTrayFailure(
+			app,
+			new CommandLineOptions { Duration = TimeSpan.Zero },
+			host: null,
+			new InvalidOperationException("XOpenDisplay failed"));
+
+		Assert.AreEqual(1, starts);
+		Assert.AreEqual(0, exitCode);
+	}
+
+	[TestMethod]
+	public void RecoverFromTrayFailure_WithDebugOutputOn_StillStopsTheToolOnce()
+	{
+		int stops = 0;
+
+		TrayAppDefinition app = TrayAppBuilder.Create("demo")
+			.DisplayName("Demo")
+			.Toggle("One", () => false, _ => { })
+			.OnStop(() => stops++)
+			.Build();
+
+		using TrayApplication host = Host(app, out _);
+		host.StartTool();
+
+		string? previous = Environment.GetEnvironmentVariable("TRAYAPP_DEBUG");
+
+		try
+		{
+			Environment.SetEnvironmentVariable("TRAYAPP_DEBUG", "1");
+
+			int exitCode = TrayAppBuilder.RecoverFromTrayFailure(
+				app,
+				new CommandLineOptions { Duration = TimeSpan.Zero },
+				host,
+				new InvalidOperationException("status getter threw"));
+
+			// Printing the full exception is diagnostic only; it must not change what the run ends as.
+			Assert.AreEqual(1, exitCode);
+			Assert.AreEqual(1, stops);
+		}
+		finally
+		{
+			Environment.SetEnvironmentVariable("TRAYAPP_DEBUG", previous);
+		}
+	}
 }

TrayApp/Tray/TrayApplication.cs

@@ -81,6 +81,16 @@ internal TrayApplication(TrayAppDefinition app, TrayMenu menu, TimeSpan? duratio
 	/// </remarks>
 	internal bool HasStarted { get; private set; }
 
+	/// <summary>
+	/// Gets a value indicating whether the tool's start action has run.
+	/// </summary>
+	/// <remarks>
+	/// Set before <see cref="HasStarted"/>, and not the same thing: start-up can still throw after the start
+	/// action - a status getter, a subscription, a missing icon resource - and by then the tool is holding
+	/// whatever it took. <see cref="Dispose"/> stops it on this, and the caller must not start it again.
+	/// </remarks>
+	internal bool ToolStarted { get; private set; }
+
 	/// <inheritdoc/>
 	public override void Initialize() => Name = app.DisplayName;
 
@@ -102,10 +112,7 @@ public override void OnFrameworkInitializationCompleted()
 		// Started here rather than before the run so that a tool whose start action throws still gets a tray
 		// icon, with the refusal in its status line, instead of a process that exits before the user sees
 		// anything. Run() puts the message where RefreshMenu will pick it up.
-		if (app.OnStart is not null)
-		{
-			menu.Run(app.OnStart);
-		}
+		StartTool();
 
 		// The tool's own state can change with nobody having clicked anything - a timer expiring, a device
 		// going away - and the menu only ever reads through getters, so all it needs is a nudge.
@@ -124,6 +131,34 @@ public override void OnFrameworkInitializationCompleted()
 		base.OnFrameworkInitializationCompleted();
 	}
 
+	/// <summary>
+	/// Runs the tool's start action, putting a refusal in the status line.
+	/// </summary>
+	/// <remarks>
+	/// Internal so that the start sequence can be driven without a platform: everything here reads and
+	/// writes menu state, and none of it needs the tray icon.
+	/// </remarks>
+	internal void StartTool()
+	{
+		ToolStarted = true;
+
+		if (app.OnStart is null)
+		{
+			return;
+		}
+
+		// A failed preference restore was seeded before the menu existed, and a successful start does not
+		// make the remembered setting any more applied. Run() clears the error on success, so put it back;
+		// a failed start reports its own error instead, which is the more pressing of the two.
+		string? seeded = menu.LastError;
+
+		if (menu.Run(app.OnStart) && seeded is not null)
+		{
+			menu.SeedError(seeded);
+			menu.Refresh();
+		}
+	}
+
 	/// <summary>
 	/// Stops the tray icon and runs the tool's stop action.
 	/// </summary>
@@ -139,7 +174,7 @@ public void Dispose()
 		expiryTimer?.Stop();
 		menu.QuitRequested -= OnQuitRequested;
 
-		if (HasStarted)
+		if (ToolStarted)
 		{
 			app.OnStop?.Invoke();
 		}

TrayApp/TrayAppBuilder.cs

@@ -43,6 +43,7 @@ namespace ktsu.TrayApp;
 public sealed class TrayAppBuilder
 {
 	private const int ExitSuccess = 0;
+	private const int ExitFailure = 1;
 	private const int ExitUsage = 2;
 
 	/// <summary>Set to <c>1</c> to print the full exception behind a tray failure.</summary>
@@ -568,30 +569,57 @@ void Remember(TrayToggleItem toggle)
 		}
 		catch (Exception ex)
 		{
-			if (Environment.GetEnvironmentVariable(DebugEnvironmentVariable) == "1")
-			{
-				Console.Error.WriteLine(ex.ToString());
-			}
+			return RecoverFromTrayFailure(app, options, host, ex);
+		}
+	}
 
-			// The tray already ran and quit; this is Avalonia's teardown throwing after the fact (see
-			// TrayApplication.HasStarted). Restarting in the terminal here would leave a tool the user just
-			// quit still holding whatever it holds.
-			if (host?.HasStarted == true)
-			{
-				host.Dispose();
-				return ExitSuccess;
-			}
+	/// <summary>
+	/// Decides what a tray run that threw ends as.
+	/// </summary>
+	/// <param name="app">The tool being run.</param>
+	/// <param name="options">The options the run was started with.</param>
+	/// <param name="host">The tray host, or <see langword="null"/> if Avalonia never constructed it.</param>
+	/// <param name="ex">What the run threw.</param>
+	/// <returns>The process exit code.</returns>
+	internal static int RecoverFromTrayFailure(
+		TrayAppDefinition app,
+		CommandLineOptions options,
+		TrayApplication? host,
+		Exception ex)
+	{
+		if (Environment.GetEnvironmentVariable(DebugEnvironmentVariable) == "1")
+		{
+			Console.Error.WriteLine(ex.ToString());
+		}
 
-			// Otherwise the windowing stack refused after the session check passed - a DISPLAY pointing at
-			// nothing, a missing libX11, no status-notifier host on the bus - and it reports those as plain
-			// exceptions of whatever type the backend felt like (X11 throws Exception("XOpenDisplay failed")).
-			// There is no useful list to filter on, and every one of them means the same thing here: no tray,
-			// so use the terminal. Letting it escape instead would kill a process the user asked to keep
-			// running.
-			host?.Dispose();
-			Console.Error.WriteLine($"{app.Name}: could not show a tray icon ({ex.Message}). Staying in the terminal instead.");
-			return ConsoleRunner.Run(app, options);
+		// The tray already ran and quit; this is Avalonia's teardown throwing after the fact (see
+		// TrayApplication.HasStarted). Restarting in the terminal here would leave a tool the user just
+		// quit still holding whatever it holds.
+		if (host?.HasStarted == true)
+		{
+			host.Dispose();
+			return ExitSuccess;
 		}
+
+		// The icon came up and the tool was started, then something later in start-up threw - a status
+		// getter, a subscription, a missing icon resource. Disposing stops the tool; falling back to the
+		// terminal would start it a second time, and say the icon could not be shown when it was.
+		if (host?.ToolStarted == true)
+		{
+			host.Dispose();
+			Console.Error.WriteLine($"{app.Name}: the tray failed after starting ({ex.Message}).");
+			return ExitFailure;
+		}
+
+		// Otherwise the windowing stack refused after the session check passed - a DISPLAY pointing at
+		// nothing, a missing libX11, no status-notifier host on the bus - and it reports those as plain
+		// exceptions of whatever type the backend felt like (X11 throws Exception("XOpenDisplay failed")).
+		// There is no useful list to filter on, and every one of them means the same thing here: no tray,
+		// so use the terminal. Letting it escape instead would kill a process the user asked to keep
+		// running.
+		host?.Dispose();
+		Console.Error.WriteLine($"{app.Name}: could not show a tray icon ({ex.Message}). Staying in the terminal instead.");
+		return ConsoleRunner.Run(app, options);
 	}
 
 	/// <summary>