Fix Firefox handling with custom window event logic (#957)

Previously, Whim would enqueue a `DoLayout` whenever a window moved. This was done to account for windows doing atypical things, like trying to restore their own position.

This worked prior to Whim becoming multi-threaded. Now that it is, this is no longer a viable approach. The `WindowMovedTransform` is triggered many times by Whim. Accordingly, the enqueueing of `DoLayout` similarly is done many times, sometimes overloading the main thread (related issues: #941, #944).

This PR allows custom logic for handling events for specific types of windows. `IWindowProcessor` s specify whether to ignore the current Windows event. A `FirefoxWindowProcessor` has been implemented to account for atypical behavior by Firefox during startup.

docs/docs/getting-started/troubleshooting.md

@@ -2,13 +2,9 @@
 
 ## Managing troublesome windows
 
-Some windows like to remember their size and position. This can be a problem for Whim, because it will try to manage the window's size and position, and the window will fight back.
+Some applications are difficult to manage. Whim will try to manage them as best as it can, but there are some limitations. For example, Firefox will try to remember its size and position, and will fight against Whim's attempts to manage it on first load.
 
-The <xref:Whim.IWindowManager> exposes an <xref:Whim.IFilterManager> called <xref:Whim.IWindowManager.LocationRestoringFilterManager>. `LocationRestoringFilterManager` listens to <xref:Whim.IWindowManager.WindowMoved> events for these windows and will force their parent <xref:Whim.IWorkspace> to do a layout two seconds after their first `WindowMoved` event, attempting to restore the window to its correct position.
-
-If this doesn't work, dragging a window's edge will force a layout, which should fix the window's position. This is an area which could use further improvement.
-
-Examples of troublesome windows include Firefox and JetBrains Gateway.
+To get around this, Whim has <xref:Whim.IWindowProcessor>s. These are used to tell Whim to ignore specific window messages (see [Event Constants](https://learn.microsoft.com/en-us/windows/win32/winauto/event-constants)). For example, the <xref:Whim.FirefoxWindowProcessor> ignores all events until the first [`EVENT_OBJECT_CLOAKED`](https://learn.microsoft.com/en-us/windows/win32/winauto/event-constants#:~:text=EVENT_OBJECT_CLOAKED) event is received.
 
 ## Window launch locations
 
@@ -18,7 +14,7 @@ To counteract this, the <xref:Whim.IRouterManager> has a <xref:Whim.IRouterManag
 
 ## Adding/removing monitors
 
-When adding and removing monitors, Windows will very helpfully move windows between monitors. However, this conflicts with Whim. To work around Windows' helpfulness, Whim (in the `WindowManager` and `ButlerEventHandlers` will) ignore [`WinEvents`](../architecture/events.md) for 3 seconds for tracked windows. After the 3 seconds have elapsed, Whim will layout all the active workspaces.
+When adding and removing monitors, Windows will very helpfully move windows between monitors. However, this conflicts with Whim. To work around Windows' helpfulness, Whim (in the `WindowManager` and `ButlerEventHandlers`) will ignore [`WinEvents`](../architecture/events.md) for 3 seconds for tracked windows. After the 3 seconds have elapsed, Whim will lay out all the active workspaces.
 
 ## Window overflows given area
 

docs/docs/plugins/focus-indicator.md

@@ -3,7 +3,7 @@
 The <xref:Whim.FocusIndicator.FocusIndicatorPlugin> adds a border around the current window which Whim has tracked as having focus. Various options can be configured using the <xref:Whim.FocusIndicator.FocusIndicatorConfig>.
 
 > [!WARNING]
-> The focus may remain on a window despite the focus shifting to an untracked window (e.g., the Start Menu). This depends on the config option <xref:Whim.IRouterManager.RouteToActiveWorkspace>.
+> The focus may remain on a window despite the focus shifting to an untracked window (e.g., the Start Menu). This depends on the config option <xref:Whim.IRouterManager.RouterOptions>.
 
 ![Focus indicator demo](../../images/focus-indicator-demo.gif)
 

docs/docs/toc.yml

@@ -5,6 +5,7 @@ items:
   - href: getting-started/customize.md
   - href: getting-started/plugins.md
   - href: getting-started/inspiration.md
+  - href: getting-started/troubleshooting.md
 
   - name: Customize
   - href: customize/scripting.md

src/Whim.Tests/Store/WindowSector/Processors/FirefoxWindowProcessorTests.cs

@@ -0,0 +1,112 @@
+using Windows.Win32;
+
+namespace Whim.Tests;
+
+public class FirefoxWindowProcessorTests
+{
+	[Theory, AutoSubstituteData]
+	public void Create_Failure(IContext ctx, IWindow window)
+	{
+		// Given a window which is not a Firefox window
+		window.ProcessFileName.Returns("chrome.exe");
+
+		// When `Create` is called
+		IWindowProcessor? processor = FirefoxWindowProcessor.Create(ctx, window);
+
+		// Then the processor should be null
+		Assert.Null(processor);
+	}
+
+	[Theory, AutoSubstituteData]
+	public void Create_Success(IContext ctx, IWindow window)
+	{
+		// Given a window which is a Firefox window
+		window.ProcessFileName.Returns("firefox.exe");
+
+		// When `Create` is called
+		IWindowProcessor? processor = FirefoxWindowProcessor.Create(ctx, window);
+
+		// Then the processor should not be null
+		Assert.NotNull(processor);
+	}
+
+	[Theory]
+	[InlineAutoSubstituteData(PInvoke.EVENT_OBJECT_SHOW)]
+	[InlineAutoSubstituteData(PInvoke.EVENT_SYSTEM_FOREGROUND)]
+	[InlineAutoSubstituteData(PInvoke.EVENT_OBJECT_UNCLOAKED)]
+	[InlineAutoSubstituteData(PInvoke.EVENT_OBJECT_HIDE)]
+	[InlineAutoSubstituteData(PInvoke.EVENT_SYSTEM_MOVESIZESTART)]
+	[InlineAutoSubstituteData(PInvoke.EVENT_SYSTEM_MOVESIZEEND)]
+	[InlineAutoSubstituteData(PInvoke.EVENT_OBJECT_LOCATIONCHANGE)]
+	[InlineAutoSubstituteData(PInvoke.EVENT_SYSTEM_MINIMIZESTART)]
+	[InlineAutoSubstituteData(PInvoke.EVENT_SYSTEM_MINIMIZEEND)]
+	public void ShouldBeIgnored_UntilCloaked(uint eventType, IContext ctx, IWindow window)
+	{
+		// Given an event which isn't `EVENT_OBJECT_CLOAKED` or `EVENT_OBJECT_DESTROY`
+		window.ProcessFileName.Returns("firefox.exe");
+		IWindowProcessor processor = FirefoxWindowProcessor.Create(ctx, window)!;
+
+		// When the event is passed to `ShouldBeIgnored`
+		WindowProcessorResult result = processor.ProcessEvent(eventType, 0, 0, 0, 0);
+
+		// Then the event should be ignored
+		Assert.Equal(WindowProcessorResult.Ignore, result);
+	}
+
+	[Theory, AutoSubstituteData]
+	public void ShouldBeIgnored_FirstCloaked(IContext ctx, IWindow window)
+	{
+		// Given the first `EVENT_OBJECT_CLOAKED` event
+		window.ProcessFileName.Returns("firefox.exe");
+		IWindowProcessor processor = FirefoxWindowProcessor.Create(ctx, window)!;
+
+		// When the event is passed to `ShouldBeIgnored`
+		WindowProcessorResult result = processor.ProcessEvent(PInvoke.EVENT_OBJECT_CLOAKED, 0, 0, 0, 0);
+
+		// Then the event should be ignored
+		Assert.Equal(WindowProcessorResult.Ignore, result);
+	}
+
+	[Theory, AutoSubstituteData]
+	public void ShouldNotBeIgnored_SecondCloaked(IContext ctx, IWindow window)
+	{
+		// Given the second `EVENT_OBJECT_CLOAKED` event
+		window.ProcessFileName.Returns("firefox.exe");
+		IWindowProcessor processor = FirefoxWindowProcessor.Create(ctx, window)!;
+		processor.ProcessEvent(PInvoke.EVENT_OBJECT_CLOAKED, 0, 0, 0, 0);
+
+		// When the event is passed to `ShouldBeIgnored`
+		WindowProcessorResult result = processor.ProcessEvent(PInvoke.EVENT_OBJECT_CLOAKED, 0, 0, 0, 0);
+
+		// Then the event should not be ignored
+		Assert.Equal(WindowProcessorResult.Process, result);
+	}
+
+	[Theory, AutoSubstituteData]
+	public void ShouldBeRemoved(IContext ctx, IWindow window)
+	{
+		// Given an `EVENT_OBJECT_DESTROY` event
+		window.ProcessFileName.Returns("firefox.exe");
+		IWindowProcessor processor = FirefoxWindowProcessor.Create(ctx, window)!;
+
+		// When the event is passed to `ShouldBeIgnored`
+		WindowProcessorResult result = processor.ProcessEvent(PInvoke.EVENT_OBJECT_DESTROY, 0, 0, 0, 0);
+
+		// Then the processor should be removed
+		Assert.Equal(WindowProcessorResult.RemoveProcessor, result);
+	}
+
+	[Theory, AutoSubstituteData]
+	public void Window(IContext ctx, IWindow window)
+	{
+		// Given a window which is a Firefox window
+		window.ProcessFileName.Returns("firefox.exe");
+		IWindowProcessor processor = FirefoxWindowProcessor.Create(ctx, window)!;
+
+		// When `Window` is called
+		IWindow result = processor.Window;
+
+		// Then the result should be the window
+		Assert.Equal(window, result);
+	}
+}

src/Whim.Tests/Store/WindowSector/Processors/WindowProcessorManagerTests.cs

@@ -0,0 +1,66 @@
+using Windows.Win32;
+
+namespace Whim.Tests;
+
+public class WindowProcessorManagerTests
+{
+	[Theory, AutoSubstituteData]
+	public void ShouldBeIgnored_CreateProcessor_Failure(IContext ctx, IWindow window)
+	{
+		// Given a window which is not a Firefox window
+		window.ProcessFileName.Returns("chrome.exe");
+		WindowProcessorManager sut = new(ctx);
+
+		// When ShouldBeIgnored is called
+		bool result = sut.ShouldBeIgnored(window, default, default, default, default, default, default);
+
+		// Then the result should be false
+		Assert.False(result);
+	}
+
+	[Theory, AutoSubstituteData]
+	public void ShouldBeIgnored_CreateProcessor_Success(IContext ctx, IWindow window)
+	{
+		// Given a window which is a Firefox window
+		window.ProcessFileName.Returns("firefox.exe");
+		WindowProcessorManager sut = new(ctx);
+
+		// When ShouldBeIgnored is called for the first time
+		bool result = sut.ShouldBeIgnored(window, default, default, default, default, default, default);
+
+		// Then the result should be true
+		Assert.True(result);
+	}
+
+	[Theory, AutoSubstituteData]
+	public void ShouldBeIgnored_ProcessorExists_Process(IContext ctx, IWindow window)
+	{
+		// Given a window which is a Firefox window
+		window.ProcessFileName.Returns("firefox.exe");
+		WindowProcessorManager sut = new(ctx);
+
+		// When ShouldBeIgnored is called for the second time
+		sut.ShouldBeIgnored(window, default, PInvoke.EVENT_OBJECT_CLOAKED, 0, 0, 0, 0);
+		bool result = sut.ShouldBeIgnored(window, default, 0, 0, 0, 0, 0);
+
+		// Then the processor should have been created by the second call, and the window should not be ignored
+		Assert.False(result);
+	}
+
+	[Theory, AutoSubstituteData]
+	public void ShouldBeIgnored_ProcessorExists_RemoveProcessor(IContext ctx, IWindow window)
+	{
+		// Given a window which is a Firefox window
+		window.ProcessFileName.Returns("firefox.exe");
+		WindowProcessorManager sut = new(ctx);
+
+		// When ShouldBeIgnored is called for the second time
+		sut.ShouldBeIgnored(window, default, PInvoke.EVENT_OBJECT_CLOAKED, 0, 0, 0, 0);
+		bool firstProcessorResult = sut.ShouldBeIgnored(window, default, PInvoke.EVENT_OBJECT_DESTROY, 0, 0, 0, 0);
+		bool secondProcessorResult = sut.ShouldBeIgnored(window, default, 0, 0, 0, 0, 0);
+
+		// Then the processor should have been removed by the second call, and the window should be ignored in the next call
+		Assert.False(firstProcessorResult);
+		Assert.True(secondProcessorResult);
+	}
+}

src/Whim.Tests/Store/WindowSector/Transforms/WindowMovedTransformTests.cs

@@ -56,46 +56,6 @@ internal void IsNotMoving_NullProcessFileName(IContext ctx, MutableRootSector mu
 		// Given the window is not moving and the process file name is null
 		mutableRootSector.WindowSector.IsMovingWindow = false;
 		window.ProcessFileName.ReturnsNull();
-		ctx.WindowManager.LocationRestoringFilterManager.ShouldBeIgnored(window).Returns(true);
-
-		WindowMovedTransform sut = new(window);
-
-		// When we dispatch the transform
-		var result = AssertDoesNotRaise(ctx, mutableRootSector, sut);
-
-		// Then we get an empty result
-		Assert.True(result.IsSuccessful);
-		ctx.NativeManager.DidNotReceive().TryEnqueue(Arg.Any<DispatcherQueueHandler>());
-	}
-
-	[Theory, AutoSubstituteData<StoreCustomization>]
-	internal void IsNotMoving_IsHandledLocation(IContext ctx, MutableRootSector mutableRootSector, IWindow window)
-	{
-		// Given the window is not moving and the window is a handled restoring location window
-		mutableRootSector.WindowSector.IsMovingWindow = false;
-		mutableRootSector.WindowSector.HandledLocationRestoringWindows = ImmutableHashSet.Create(window.Handle);
-		ctx.WindowManager.LocationRestoringFilterManager.ShouldBeIgnored(window).Returns(true);
-
-		WindowMovedTransform sut = new(window);
-
-		// When we dispatch the transform
-		var result = AssertDoesNotRaise(ctx, mutableRootSector, sut);
-
-		// Then we get an empty result
-		Assert.True(result.IsSuccessful);
-		ctx.NativeManager.DidNotReceive().TryEnqueue(Arg.Any<DispatcherQueueHandler>());
-	}
-
-	[Theory, AutoSubstituteData<StoreCustomization>]
-	internal void IsNotMoving_IgnoredByLocationRestoringFilter(
-		IContext ctx,
-		MutableRootSector mutableRootSector,
-		IWindow window
-	)
-	{
-		// Given the window is not moving and the window is a handled restoring location window
-		mutableRootSector.WindowSector.IsMovingWindow = false;
-		ctx.WindowManager.LocationRestoringFilterManager.ShouldBeIgnored(window).Returns(false);
 
 		WindowMovedTransform sut = new(window);
 
@@ -117,19 +77,16 @@ IWindow window
 	{
 		// Given the window is not moving, we don't ignore the window moving event, but we can get the pos
 		mutableRootSector.WindowSector.IsMovingWindow = false;
-		mutableRootSector.WindowSector.WindowMovedDelay = 0;
 		mutableRootSector.WindowSector.IsLeftMouseButtonDown = true;
-		ctx.WindowManager.LocationRestoringFilterManager.ShouldBeIgnored(window).Returns(true);
 		Setup_GetCursorPos(internalCtx);
 
 		WindowMovedTransform sut = new(window);
 
 		// When we dispatch the transform
 		(Result<Unit>? result, Assert.RaisedEvent<WindowMovedEventArgs> ev) = AssertRaises(ctx, mutableRootSector, sut);
 
-		// Then we get a resulting window, a NativeManager.TryEnqueue to restore a window's position, and a second TryEnqueue
-		// to dispatch the events.
-		ctx.NativeManager.Received(2).TryEnqueue(Arg.Any<DispatcherQueueHandler>());
+		// Then we get a resulting window and a second TryEnqueue to dispatch the events.
+		ctx.NativeManager.Received(1).TryEnqueue(Arg.Any<DispatcherQueueHandler>());
 		Assert.True(result!.Value.IsSuccessful);
 		Assert.Equal(new Point<int>(1, 2), ev.Arguments.CursorDraggedPoint);
 		Assert.Equal(window, ev.Arguments.Window);

src/Whim.Tests/Store/WindowSector/WindowEventListenerTests.cs

@@ -302,7 +302,7 @@ public static IEnumerable<object[]> WinEventProcCasesData()
 
 	[MemberAutoSubstituteData(nameof(WinEventProcCasesData))]
 	[Theory]
-	internal void WindowFocusedTransform(
+	internal void WinEventProc_Dispatch(
 		uint ev,
 		Func<IWindow, Transform> createTransform,
 		IContext ctx,
@@ -323,6 +323,26 @@ IInternalContext internalCtx
 		ctx.Store.Received(1).Dispatch(createTransform(window));
 	}
 
+	[Theory, AutoSubstituteData]
+	internal void WinEventProc_Ignore(IContext ctx, IInternalContext internalCtx, IWindow window)
+	{
+		// Given the window is a Firefox window
+		window.Handle.Returns((HWND)1);
+		window.ProcessFileName.Returns("firefox.exe");
+		ctx.Store.Pick(Arg.Any<PurePicker<Result<IWindow>>>()).Returns(Result.FromValue(window));
+
+		CaptureWinEventProc capture = CaptureWinEventProc.Create(internalCtx);
+
+		WindowEventListener sut = new(ctx, internalCtx);
+		sut.Initialize();
+
+		// When we send through the event
+		capture.WinEventProc!.Invoke((HWINEVENTHOOK)0, PInvoke.EVENT_OBJECT_SHOW, window.Handle, 0, 0, 0, 0);
+
+		// Then nothing happens
+		AssertDispatches(ctx);
+	}
+
 	[Theory, AutoSubstituteData]
 	internal void Default(IContext ctx, IInternalContext internalCtx)
 	{

src/Whim/Filter/DefaultFilteredWindows.cs

@@ -13,12 +13,4 @@ public static void LoadWindowsIgnoredByWhim(IFilterManager filterManager)
 	{
 		DefaultFilteredWindowsKomorebi.LoadWindowsIgnoredByKomorebi(filterManager);
 	}
-
-	/// <summary>
-	/// Load the windows which try to set their own locations when the start up.
-	/// See <see cref="IWindowManager.LocationRestoringFilterManager"/>
-	/// </summary>
-	/// <param name="filterManager"></param>
-	public static void LoadLocationRestoringWindows(IFilterManager filterManager) =>
-		filterManager.AddProcessFileNameFilter("firefox.exe").AddProcessFileNameFilter("gateway64.exe");
 }

src/Whim/Native/HWND.cs

@@ -16,7 +16,10 @@ namespace Foundation
 
 			internal static HWND Null => default;
 
-			internal bool IsNull => Value == default;
+			/// <summary>
+			/// Whether the handle has a zero value.
+			/// </summary>
+			public bool IsNull => Value == default;
 
 			/// <inheritdoc />
 			public static implicit operator IntPtr(HWND value) => value.Value;

src/Whim/Store/Transform.cs

@@ -3,7 +3,6 @@ namespace Whim;
 /// <summary>
 /// Operation describing how to update the state of the <see cref="Store"/>.
 /// The implementing record should be populated with the payload.
-/// <see cref="Execute"/> will specify how to update the store.
 /// </summary>
 /// <typeparam name="TResult"></typeparam>
 public abstract record Transform<TResult>

src/Whim/Store/WindowSector/IWindowSector.cs

@@ -10,12 +10,6 @@ public interface IWindowSector
 	/// </summary>
 	ImmutableDictionary<HWND, IWindow> Windows { get; }
 
-	/// <summary>
-	/// The windows which had their first location change event handled - see <see cref="IWindowManager.LocationRestoringFilterManager"/>.
-	/// We maintain a set of the windows that have been handled so that we don't enter an infinite loop of location change events.
-	/// </summary>
-	ImmutableHashSet<HWND> HandledLocationRestoringWindows { get; }
-
 	/// <summary>
 	/// Whether a window is currently moving.
 	/// </summary>
@@ -26,9 +20,4 @@ public interface IWindowSector
 	/// Used for window movement.
 	/// </summary>
 	bool IsLeftMouseButtonDown { get; }
-
-	/// <summary>
-	/// The delay to wait when trying to restore windows from <see cref="IWindowManager.LocationRestoringFilterManager"/>.
-	/// </summary>
-	int WindowMovedDelay { get; }
 }