Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to multithreading model #849

Closed
wants to merge 31 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
8cb2b9f
Migrate KeybindHook to Task
dalyIsaac Mar 16, 2024
2e666a9
Migrate entry points for core
dalyIsaac Mar 16, 2024
f5b9845
Remove DeferWindowPosManager and lock setting window position
dalyIsaac Mar 16, 2024
a1b5410
Remove DeferWindowPosManager tests, and fix existing tests
dalyIsaac Mar 16, 2024
739420d
Run commands on a separate thread
dalyIsaac Mar 16, 2024
822c17a
Lock CommandManager
dalyIsaac Mar 16, 2024
0ee86f5
Fix parallel tests
dalyIsaac Mar 16, 2024
a7ef8f8
Add Lock struct to use instead of `lock statement`
dalyIsaac Mar 16, 2024
c61be7f
Use `using Lock`
dalyIsaac Mar 16, 2024
14f7514
Lock ButlerPantry
dalyIsaac Mar 16, 2024
8796ca5
FilterManager locks
dalyIsaac Mar 18, 2024
3e7ea17
KeybindManager locks
dalyIsaac Mar 18, 2024
5c427d5
PluginManager locks
dalyIsaac Mar 18, 2024
0bc76be
RouterManager locks
dalyIsaac Mar 18, 2024
fe50411
WorkspaceManager locks
dalyIsaac Mar 18, 2024
5e2e1c3
Add missing lock objects
dalyIsaac Mar 19, 2024
2dde577
WindowManager locks
dalyIsaac Mar 20, 2024
e53f315
MonitorManager locks
dalyIsaac Mar 21, 2024
3d61a57
Remove event call from WindowManager lock
dalyIsaac Mar 21, 2024
f835b48
Added new method to await a callback on the STA thread
dalyIsaac Mar 23, 2024
0cf8735
Added `ISubscriber`
dalyIsaac Mar 28, 2024
9af38dc
Implement `ISubscriber` for all relevant `Manager` classes
dalyIsaac Mar 30, 2024
217012e
Fix tests which refer to Subscribe
dalyIsaac Mar 30, 2024
e1f98d6
Added `ThreadSafeEvent`, and used it in `Butler`
dalyIsaac Mar 30, 2024
13ca7fc
Added `ThreadSafeEvent` to the other core events
dalyIsaac Mar 30, 2024
264ab2e
Switch to Task.Run from NativeManager.TryEnqueue
dalyIsaac Mar 30, 2024
bea6cea
Fix ThreadSafeEvent calls
dalyIsaac Mar 30, 2024
f6edf16
Fix tests
dalyIsaac Mar 30, 2024
2a940cf
Add a delay for a test
dalyIsaac Mar 30, 2024
2669562
Formatting
dalyIsaac Mar 30, 2024
185fea7
Move event subscription to STA thread
dalyIsaac Mar 31, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Whim.Bar.Tests/BarPluginTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ public void MonitorManager_MonitorsChanged_RemovedMonitors(IContext context, IMo
NativeManagerUtils.SetupTryEnqueue(context);

// When MonitorManager_MonitorsChanged is called with a removed monitor which is not in the map
barPlugin.PreInitialize();
barPlugin.Subscribe();
context.MonitorManager.MonitorsChanged += Raise.EventWith(
new MonitorsChangedEventArgs()
{
Expand All @@ -28,6 +28,6 @@ public void MonitorManager_MonitorsChanged_RemovedMonitors(IContext context, IMo

// Then an exception is not thrown.
barPlugin.Dispose();
context.NativeManager.Received(1).TryEnqueue(Arg.Any<DispatcherQueueHandler>());
context.NativeManager.Received(1).InvokeOnUIThread(Arg.Any<DispatcherQueueHandler>());
}
}
9 changes: 7 additions & 2 deletions src/Whim.Bar/BarPlugin.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ public BarPlugin(IContext context, BarConfig barConfig)
/// <inheritdoc />
public void PreInitialize()
{
_context.MonitorManager.MonitorsChanged += MonitorManager_MonitorsChanged;
_context.FilterManager.AddTitleMatchFilter("Whim Bar");
_context.WorkspaceManager.AddProxyLayoutEngine(layout => new BarLayoutEngine(_barConfig, layout));
}
Expand All @@ -56,7 +55,7 @@ private void MonitorManager_MonitorsChanged(object? sender, MonitorsChangedEvent
foreach (IMonitor monitor in e.RemovedMonitors)
{
_monitorBarMap.TryGetValue(monitor, out BarWindow? value);
_context.NativeManager.TryEnqueue(() => value?.Close());
_context.NativeManager.InvokeOnUIThread(() => value?.Close());
_monitorBarMap.Remove(monitor);
}

Expand Down Expand Up @@ -87,6 +86,12 @@ private void ShowAll()
}
}

/// <inheritdoc />
public void Subscribe()
{
_context.MonitorManager.MonitorsChanged += MonitorManager_MonitorsChanged;
}

/// <inheritdoc />
public void LoadState(JsonElement pluginSavedState) { }

Expand Down
3 changes: 3 additions & 0 deletions src/Whim.CommandPalette/CommandPalettePlugin.cs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@
_commandPaletteWindow = new CommandPaletteWindow(_context, this);
}

/// <inheritdoc />
public void Subscribe() { }

Check warning on line 54 in src/Whim.CommandPalette/CommandPalettePlugin.cs

View check run for this annotation

Codecov / codecov/patch

src/Whim.CommandPalette/CommandPalettePlugin.cs#L54

Added line #L54 was not covered by tests

/// <summary>
/// Activate the command palette.
/// </summary>
Expand Down
1 change: 1 addition & 0 deletions src/Whim.FloatingLayout.Tests/FloatingLayoutPluginTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ IWorkspace activeWorkspace

// When
plugin.PreInitialize();
plugin.PostInitialize();
plugin.MarkWindowAsFloating(window);
context.WindowManager.WindowRemoved += Raise.EventWith(new WindowEventArgs() { Window = window });

Expand Down
10 changes: 9 additions & 1 deletion src/Whim.FloatingLayout/FloatingLayoutPlugin.cs
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,19 @@
public void PreInitialize()
{
_context.WorkspaceManager.AddProxyLayoutEngine(layout => new FloatingLayoutEngine(_context, this, layout));
}

/// <inheritdoc />
public void PostInitialize()
{
_context.WindowManager.WindowRemoved += WindowManager_WindowRemoved;
}

/// <inheritdoc />
public void PostInitialize() { }
public void Subscribe()
{
_context.WindowManager.WindowRemoved += WindowManager_WindowRemoved;
}

Check warning on line 46 in src/Whim.FloatingLayout/FloatingLayoutPlugin.cs

View check run for this annotation

Codecov / codecov/patch

src/Whim.FloatingLayout/FloatingLayoutPlugin.cs#L44-L46

Added lines #L44 - L46 were not covered by tests

/// <inheritdoc />
public IPluginCommands PluginCommands => new FloatingLayoutCommands(this);
Expand Down
10 changes: 7 additions & 3 deletions src/Whim.FocusIndicator/FocusIndicatorPlugin.cs
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,6 @@ public FocusIndicatorPlugin(IContext context, FocusIndicatorConfig focusIndicato
public void PreInitialize()
{
_context.FilterManager.AddTitleMatchFilter(FocusIndicatorConfig.Title);

_context.WindowManager.WindowMoveStart += WindowManager_WindowMoveStart;
_context.WindowManager.WindowFocused += WindowManager_WindowFocused;
}

/// <inheritdoc/>
Expand All @@ -51,6 +48,13 @@ public void PostInitialize()
// Activate the window so it renders.
_focusIndicatorWindow.Activate();
_focusIndicatorWindow.Hide(_context);
}

/// <inheritdoc/>
public void Subscribe()
{
_context.WindowManager.WindowMoveStart += WindowManager_WindowMoveStart;
_context.WindowManager.WindowFocused += WindowManager_WindowFocused;

// Only subscribe to workspace changes once the indicator window has been created - we shouldn't
// show a window which doesn't yet exist (it'll just crash Whim).
Expand Down
3 changes: 3 additions & 0 deletions src/Whim.Gaps/GapsPlugin.cs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@
/// <inheritdoc />
public void PostInitialize() { }

/// <inheritdoc />
public void Subscribe() { }

Check warning on line 41 in src/Whim.Gaps/GapsPlugin.cs

View check run for this annotation

Codecov / codecov/patch

src/Whim.Gaps/GapsPlugin.cs#L41

Added line #L41 was not covered by tests

/// <summary>
/// Update the gap between the parent layout engine and the area where windows are placed by
/// the <paramref name="delta"/>.
Expand Down
21 changes: 10 additions & 11 deletions src/Whim.LayoutPreview.Tests/LayoutPreviewPluginTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ public void PluginCommands(IContext ctx)
}

[Theory, AutoSubstituteData<LayoutPreviewPluginCustomization>]
[SuppressMessage("Usage", "NS5000:Received check.")]
public void PreInitialize(IContext ctx)
{
// Given
Expand All @@ -70,24 +69,24 @@ public void PreInitialize(IContext ctx)
plugin.PreInitialize();

// Then
ctx.WindowManager.Received(1).WindowMoveStart += Arg.Any<EventHandler<WindowMovedEventArgs>>();
ctx.WindowManager.Received(1).WindowMoved += Arg.Any<EventHandler<WindowMovedEventArgs>>();
ctx.WindowManager.Received(1).WindowMoveEnd += Arg.Any<EventHandler<WindowMovedEventArgs>>();
ctx.WindowManager.Received(1).WindowRemoved += Arg.Any<EventHandler<WindowEventArgs>>();
ctx.FilterManager.Received(1).AddTitleMatchFilter(LayoutPreviewWindow.WindowTitle);
}

[Theory, AutoSubstituteData<LayoutPreviewPluginCustomization>]
public void PostInitialize(IContext ctx)
[SuppressMessage("Usage", "NS5000:Received check.")]
public void Subscribe(IContext ctx)
{
// Given
using LayoutPreviewPlugin plugin = new(ctx);

// When
plugin.PostInitialize();
plugin.Subscribe();

// Then
Assert.True(true);
ctx.WindowManager.Received(1).WindowMoveStart += Arg.Any<EventHandler<WindowMovedEventArgs>>();
ctx.WindowManager.Received(1).WindowMoved += Arg.Any<EventHandler<WindowMovedEventArgs>>();
ctx.WindowManager.Received(1).WindowMoveEnd += Arg.Any<EventHandler<WindowMovedEventArgs>>();
ctx.WindowManager.Received(1).WindowRemoved += Arg.Any<EventHandler<WindowEventArgs>>();
}

[Theory, AutoSubstituteData<LayoutPreviewPluginCustomization>]
Expand Down Expand Up @@ -199,7 +198,7 @@ public void WindowMoved_Dragged_CannotFindWorkspace(IContext ctx, IWorkspace wor
workspace.ActiveLayoutEngine.ClearReceivedCalls();

// When
plugin.PreInitialize();
plugin.Subscribe();
ctx.WindowManager.WindowMoved += Raise.Event<EventHandler<WindowMovedEventArgs>>(ctx.WindowManager, e);

// Then
Expand All @@ -225,7 +224,7 @@ public void WindowMoved_Dragged_Success(IContext ctx, IWindow window, IWorkspace
workspace.ActiveLayoutEngine.ClearReceivedCalls();

// When
plugin.PreInitialize();
plugin.Subscribe();
ctx.WindowManager.WindowMoved += Raise.Event<EventHandler<WindowMovedEventArgs>>(ctx.WindowManager, e);

// Then
Expand Down Expand Up @@ -274,7 +273,7 @@ public void WindowManager_WindowRemoved_NotHidden(IContext ctx, IWindow movedWin
WindowEventArgs removeArgs = new() { Window = removedWindow };

// When
plugin.PreInitialize();
plugin.Subscribe();
ctx.WindowManager.WindowMoved += Raise.Event<EventHandler<WindowMovedEventArgs>>(ctx.WindowManager, moveArgs);
ctx.WindowManager.WindowRemoved += Raise.Event<EventHandler<WindowEventArgs>>(ctx.WindowManager, removeArgs);

Expand Down
2 changes: 0 additions & 2 deletions src/Whim.LayoutPreview.Tests/LayoutPreviewWindowTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -181,8 +181,6 @@ IMonitor monitor
{
// Given
ctx.NativeManager.DeferWindowPos().Returns(new DeferWindowPosHandle(ctx, internalCtx));
internalCtx.DeferWindowPosManager.CanDoLayout().Returns(true);
internalCtx.DeferWindowPosManager.ParallelOptions.Returns(new ParallelOptions { MaxDegreeOfParallelism = 1 });

// When
LayoutPreviewWindow.Activate(ctx, layoutWindow, movingWindow, monitor);
Expand Down
13 changes: 9 additions & 4 deletions src/Whim.LayoutPreview/LayoutPreviewPlugin.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,23 @@

/// <inheritdoc />
public void PreInitialize()
{
_context.FilterManager.AddTitleMatchFilter(LayoutPreviewWindow.WindowTitle);
}

/// <inheritdoc />
public void PostInitialize() { }

Check warning on line 44 in src/Whim.LayoutPreview/LayoutPreviewPlugin.cs

View check run for this annotation

Codecov / codecov/patch

src/Whim.LayoutPreview/LayoutPreviewPlugin.cs#L44

Added line #L44 was not covered by tests

/// <inheritdoc />
public void Subscribe()
{
_context.WindowManager.WindowMoveStart += WindowManager_WindowMoveStart;
_context.WindowManager.WindowMoved += WindowMoved;
_context.WindowManager.WindowMoveEnd += WindowManager_WindowMoveEnd;
_context.WindowManager.WindowRemoved += WindowManager_WindowRemoved;
_context.WindowManager.WindowFocused += WindowManager_WindowFocused;
_context.FilterManager.AddTitleMatchFilter(LayoutPreviewWindow.WindowTitle);
}

/// <inheritdoc />
public void PostInitialize() { }

/// <inheritdoc />
public void LoadState(JsonElement state) { }

Expand Down
3 changes: 3 additions & 0 deletions src/Whim.SliceLayout/SliceLayoutPlugin.cs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@
/// <inheritdoc />
public void PostInitialize() { }

/// <inheritdoc />
public void Subscribe() { }

Check warning on line 49 in src/Whim.SliceLayout/SliceLayoutPlugin.cs

View check run for this annotation

Codecov / codecov/patch

src/Whim.SliceLayout/SliceLayoutPlugin.cs#L49

Added line #L49 was not covered by tests

/// <inheritdoc />
public void LoadState(JsonElement state) { }

Expand Down
59 changes: 49 additions & 10 deletions src/Whim.TestUtils/Assert.cs
Original file line number Diff line number Diff line change
@@ -1,24 +1,26 @@
using System;
using System.ComponentModel;
using System.Threading.Tasks;
using NSubstitute;
using Xunit;
using Xunit.Sdk;

namespace Whim.TestUtils;

/// <summary>
/// Exception thrown when an event is raised when it should not have been.
/// Exception thrown when an event is not raises when it should have been (or vice versa).
/// </summary>
#pragma warning disable CA1032 // Implement standard exception constructors
public class DoesNotRaiseException : XunitException
public class ShouldRaiseException : XunitException
#pragma warning restore CA1032 // Implement standard exception constructors
{
/// <summary>
/// Creates a new instance of the <see cref="DoesNotRaiseException"/> class.
/// Creates a new instance of the <see cref="ShouldRaiseException"/> class.
/// </summary>
/// <param name="type"></param>
public DoesNotRaiseException(Type type)
: base($"Expected event of type {type} to not be raised.") { }
/// <param name="expected">Whether it was expected that the exception was raised.</param>
public ShouldRaiseException(Type type, bool expected)
: base($"Expected event of type {type} to {(expected ? "not" : "")} be raised.") { }
}

/// <summary>
Expand All @@ -33,7 +35,7 @@
/// <param name="attach">The method to attach the event handler.</param>
/// <param name="detach">The method to detach the event handler.</param>
/// <param name="action">The action to perform.</param>
/// <exception cref="DoesNotRaiseException">Thrown when the event is raised.</exception>
/// <exception cref="ShouldRaiseException">Thrown when the event is raised.</exception>
public static void DoesNotRaise<T>(Action<EventHandler<T>> attach, Action<EventHandler<T>> detach, Action action)
{
bool raised = false;
Expand All @@ -53,7 +55,45 @@

if (raised)
{
throw new DoesNotRaiseException(typeof(T));
throw new ShouldRaiseException(typeof(T), expected: false);

Check warning on line 58 in src/Whim.TestUtils/Assert.cs

View check run for this annotation

Codecov / codecov/patch

src/Whim.TestUtils/Assert.cs#L58

Added line #L58 was not covered by tests
}
}

/// <summary>
/// Asserts that an event is not raised.
/// </summary>
/// <typeparam name="T">The type of event to check.</typeparam>
/// <param name="attach">The method to attach the event handler.</param>
/// <param name="detach">The method to detach the event handler.</param>
/// <param name="action">The action to perform.</param>
/// <param name="delayMs">The delay in milliseconds to wait.</param>
/// <exception cref="ShouldRaiseException">Thrown when the event is raised.</exception>
public static async Task RaisesAsync<T>(
Action<EventHandler<T>> attach,
Action<EventHandler<T>> detach,
Action action,
int delayMs
)
{
bool raised = false;
void handler(object? sender, T e)
{
raised = true;
}
attach(handler);
try
{
action();
await Task.Delay(delayMs).ConfigureAwait(true);
}
finally
{
detach(handler);
}

if (!raised)
{
throw new ShouldRaiseException(typeof(T), expected: true);

Check warning on line 96 in src/Whim.TestUtils/Assert.cs

View check run for this annotation

Codecov / codecov/patch

src/Whim.TestUtils/Assert.cs#L95-L96

Added lines #L95 - L96 were not covered by tests
}
}

Expand All @@ -63,7 +103,7 @@
/// <param name="attach">The method to attach the event handler.</param>
/// <param name="detach">The method to detach the event handler.</param>
/// <param name="action">The action to perform.</param>
/// <exception cref="DoesNotRaiseException">Thrown when the event is raised.</exception>
/// <exception cref="ShouldRaiseException">Thrown when the event is raised.</exception>
public static void DoesNotPropertyChange(
Action<PropertyChangedEventHandler> attach,
Action<PropertyChangedEventHandler> detach,
Expand All @@ -87,7 +127,7 @@

if (raised)
{
throw new DoesNotRaiseException(typeof(PropertyChangedEventArgs));
throw new ShouldRaiseException(typeof(PropertyChangedEventArgs), expected: false);

Check warning on line 130 in src/Whim.TestUtils/Assert.cs

View check run for this annotation

Codecov / codecov/patch

src/Whim.TestUtils/Assert.cs#L130

Added line #L130 was not covered by tests
}
}

Expand Down Expand Up @@ -116,6 +156,5 @@
Assert.Empty(internalCtx.WindowManager.ReceivedCalls());
Assert.Empty(internalCtx.KeybindHook.ReceivedCalls());
Assert.Empty(internalCtx.MouseHook.ReceivedCalls());
Assert.Empty(internalCtx.DeferWindowPosManager.ReceivedCalls());
}
}
Loading
Loading