From 024f68a97cfef97883283dc67be2ca7e70d33a36 Mon Sep 17 00:00:00 2001 From: Lars Berger Date: Wed, 6 Sep 2023 04:26:29 +0800 Subject: [PATCH] fix: multi threading issue when running many keybinding commands at once --- GlazeWM.Domain/DependencyInjection.cs | 2 +- .../RegisterKeybindingsHandler.cs | 28 +-------- .../ReloadUserConfigHandler.cs | 7 ++- .../RunWithSubjectContainerHandler.cs | 62 +++++++++++++++++++ .../RunWithSubjectContainerCommand.cs | 23 +++++++ .../CommandHandlers/ManageWindowHandler.cs | 30 +++++---- .../CommandHandlers/RunWindowRulesHandler.cs | 57 ----------------- .../Windows/Commands/RunWindowRulesCommand.cs | 18 ------ 8 files changed, 112 insertions(+), 115 deletions(-) create mode 100644 GlazeWM.Domain/UserConfigs/CommandHandlers/RunWithSubjectContainerHandler.cs create mode 100644 GlazeWM.Domain/UserConfigs/Commands/RunWithSubjectContainerCommand.cs delete mode 100644 GlazeWM.Domain/Windows/CommandHandlers/RunWindowRulesHandler.cs delete mode 100644 GlazeWM.Domain/Windows/Commands/RunWindowRulesCommand.cs diff --git a/GlazeWM.Domain/DependencyInjection.cs b/GlazeWM.Domain/DependencyInjection.cs index f4e2a77a6..293d0c607 100644 --- a/GlazeWM.Domain/DependencyInjection.cs +++ b/GlazeWM.Domain/DependencyInjection.cs @@ -60,12 +60,12 @@ public static IServiceCollection AddDomainServices(this IServiceCollection servi services.AddSingleton, EvaluateUserConfigHandler>(); services.AddSingleton, RegisterKeybindingsHandler>(); services.AddSingleton, ReloadUserConfigHandler>(); + services.AddSingleton, RunWithSubjectContainerHandler>(); services.AddSingleton, CloseWindowHandler>(); services.AddSingleton, IgnoreWindowHandler>(); services.AddSingleton, ManageWindowHandler>(); services.AddSingleton, MoveWindowHandler>(); services.AddSingleton, ResizeWindowHandler>(); - services.AddSingleton, RunWindowRulesHandler>(); services.AddSingleton, ResizeWindowBordersHandler>(); services.AddSingleton, SetFloatingHandler>(); services.AddSingleton, SetMaximizedHandler>(); diff --git a/GlazeWM.Domain/UserConfigs/CommandHandlers/RegisterKeybindingsHandler.cs b/GlazeWM.Domain/UserConfigs/CommandHandlers/RegisterKeybindingsHandler.cs index 737ccc494..90f973191 100644 --- a/GlazeWM.Domain/UserConfigs/CommandHandlers/RegisterKeybindingsHandler.cs +++ b/GlazeWM.Domain/UserConfigs/CommandHandlers/RegisterKeybindingsHandler.cs @@ -40,8 +40,8 @@ public CommandResponse Handle(RegisterKeybindingsCommand command) foreach (var keybindingConfig in command.Keybindings) { // Format command strings defined in keybinding config. - var formattedCommandStrings = keybindingConfig.CommandList.Select( - commandString => CommandParsingService.FormatCommand(commandString) + var commandStrings = keybindingConfig.CommandList.Select( + CommandParsingService.FormatCommand ); // Register all keybindings for a command sequence. @@ -56,29 +56,7 @@ public CommandResponse Handle(RegisterKeybindingsCommand command) if (_windowService.IgnoredHandles.Contains(GetForegroundWindow())) return; - var subjectContainer = _containerService.FocusedContainer; - var subjectContainerId = subjectContainer.Id; - - // Invoke commands in sequence on keybinding press. - foreach (var commandString in formattedCommandStrings) - { - // Avoid calling command if container gets detached. This is to prevent crashes - // for edge cases like ["close", "move to workspace X"]. - if (subjectContainer.IsDetached()) - return; - - var parsedCommand = _commandParsingService.ParseCommand( - commandString, - subjectContainer - ); - - // Use `dynamic` to resolve the command type at runtime and allow multiple dispatch. - _bus.Invoke((dynamic)parsedCommand); - - // Update subject container in case the reference changes (eg. when going from a tiling to a - // floating window). - subjectContainer = _containerService.GetContainerById(subjectContainerId); - } + _bus.Invoke(new RunWithSubjectContainerCommand(commandStrings)); } catch (Exception e) { diff --git a/GlazeWM.Domain/UserConfigs/CommandHandlers/ReloadUserConfigHandler.cs b/GlazeWM.Domain/UserConfigs/CommandHandlers/ReloadUserConfigHandler.cs index 21330a764..dad2cbf42 100644 --- a/GlazeWM.Domain/UserConfigs/CommandHandlers/ReloadUserConfigHandler.cs +++ b/GlazeWM.Domain/UserConfigs/CommandHandlers/ReloadUserConfigHandler.cs @@ -38,7 +38,12 @@ public CommandResponse Handle(ReloadUserConfigCommand command) foreach (var window in _windowService.GetWindows()) { var windowRules = _userConfigService.GetMatchingWindowRules(window); - _bus.Invoke(new RunWindowRulesCommand(window, windowRules)); + var windowRuleCommands = windowRules + .SelectMany(rule => rule.CommandList) + .Select(CommandParsingService.FormatCommand); + + // Run matching window rules. + _bus.Invoke(new RunWithSubjectContainerCommand(windowRuleCommands, window)); } // Redraw full container tree. diff --git a/GlazeWM.Domain/UserConfigs/CommandHandlers/RunWithSubjectContainerHandler.cs b/GlazeWM.Domain/UserConfigs/CommandHandlers/RunWithSubjectContainerHandler.cs new file mode 100644 index 000000000..6667b2e9f --- /dev/null +++ b/GlazeWM.Domain/UserConfigs/CommandHandlers/RunWithSubjectContainerHandler.cs @@ -0,0 +1,62 @@ +using GlazeWM.Domain.Containers; +using GlazeWM.Domain.Containers.Commands; +using GlazeWM.Domain.UserConfigs.Commands; +using GlazeWM.Domain.UserConfigs.Events; +using GlazeWM.Domain.Windows; +using GlazeWM.Domain.Windows.Commands; +using GlazeWM.Domain.Workspaces.Commands; +using GlazeWM.Infrastructure.Bussing; + +namespace GlazeWM.Domain.UserConfigs.CommandHandlers +{ + internal sealed class RunWithSubjectContainerHandler + : ICommandHandler + { + private readonly Bus _bus; + private readonly CommandParsingService _commandParsingService; + private readonly ContainerService _containerService; + + public RegisterKeybindingsHandler( + Bus bus, + CommandParsingService commandParsingService, + ContainerService containerService) + { + _bus = bus; + _commandParsingService = commandParsingService; + _containerService = containerService; + } + + public CommandResponse Handle(RunWithSubjectContainerCommand command) + { + var commandStrings = command.CommandStrings; + var subjectContainer = + command.SubjectContainer ?? _containerService.FocusedContainer; + + var subjectContainerId = subjectContainer.Id; + + // Invoke commands in sequence. + foreach (var commandString in commandStrings) + { + // Avoid calling command if container gets detached. This is to prevent crashes + // for edge cases like ["close", "move to workspace X"]. + if (subjectContainer?.IsDetached() != false) + return; + + var parsedCommand = _commandParsingService.ParseCommand( + commandString, + subjectContainer + ); + + // Use `dynamic` to resolve the command type at runtime and allow multiple + // dispatch. + _bus.Invoke((dynamic)parsedCommand); + + // Update subject container in case the reference changes (eg. when going from a + // tiling to a floating window). + subjectContainer = _containerService.GetContainerById(subjectContainerId); + } + + return CommandResponse.Ok; + } + } +} diff --git a/GlazeWM.Domain/UserConfigs/Commands/RunWithSubjectContainerCommand.cs b/GlazeWM.Domain/UserConfigs/Commands/RunWithSubjectContainerCommand.cs new file mode 100644 index 000000000..c092ba523 --- /dev/null +++ b/GlazeWM.Domain/UserConfigs/Commands/RunWithSubjectContainerCommand.cs @@ -0,0 +1,23 @@ +using GlazeWM.Infrastructure.Bussing; + +namespace GlazeWM.Domain.UserConfigs.Commands +{ + public class RunWithSubjectContainerCommand : Command + { + public List CommandStrings { get; } + public Container SubjectContainer { get; } + + public RunWithSubjectContainerCommand( + List commandStrings, + Container subjectContainer) + { + CommandStrings = commandStrings; + SubjectContainer = subjectContainer; + } + + public RunWithSubjectContainerCommand(List commandStrings) + { + CommandStrings = commandStrings; + } + } +} diff --git a/GlazeWM.Domain/Windows/CommandHandlers/ManageWindowHandler.cs b/GlazeWM.Domain/Windows/CommandHandlers/ManageWindowHandler.cs index 24c085cfd..342312bf5 100644 --- a/GlazeWM.Domain/Windows/CommandHandlers/ManageWindowHandler.cs +++ b/GlazeWM.Domain/Windows/CommandHandlers/ManageWindowHandler.cs @@ -58,19 +58,15 @@ public CommandResponse Handle(ManageWindowCommand command) else _bus.Invoke(new AttachContainerCommand(window, targetParent, targetIndex)); - // The OS might spawn the window on a different monitor to the target parent, so adjustments - // might need to be made because of DPI. - var monitor = _monitorService.GetMonitorFromHandleLocation(windowHandle); - if (MonitorService.HasDpiDifference(monitor, window.Parent)) - window.HasPendingDpiAdjustment = true; + var windowRules = _userConfigService.GetMatchingWindowRules(window); + var windowRuleCommands = windowRules + .SelectMany(rule => rule.CommandList) + .Select(CommandParsingService.FormatCommand); // Set the newly added window as focus descendant. This means the window rules will be run as // if the window is focused. _bus.Invoke(new SetFocusedDescendantCommand(window)); - - // Run matching window rules. - var windowRules = _userConfigService.GetMatchingWindowRules(window); - _bus.Invoke(new RunWindowRulesCommand(window, windowRules)); + _bus.Invoke(new RunWithSubjectContainerCommand(windowRuleCommands, window)); // Update window in case the reference changes. window = _windowService.GetWindowByHandle(window.Handle); @@ -82,12 +78,12 @@ public CommandResponse Handle(ManageWindowCommand command) if (shouldRedraw) _bus.Invoke(new RedrawContainersCommand()); - // Set OS focus to the newly added window in case it's not already focused. - _bus.Invoke(new SetNativeFocusCommand(window)); - _logger.LogWindowEvent("New window managed", window); _bus.Emit(new WindowManagedEvent(window)); + // Set OS focus to the newly added window in case it's not already focused. + _bus.Invoke(new SetNativeFocusCommand(window)); + return CommandResponse.Ok; } @@ -106,7 +102,7 @@ private static Window CreateWindow(IntPtr windowHandle, Container targetParent) var isResizable = WindowService.HandleHasWindowStyle(windowHandle, WindowStyles.ThickFrame); // TODO: Handle initialization of maximized and fullscreen windows. - return windowType switch + var window = windowType switch { WindowType.Minimized => new MinimizedWindow( windowHandle, @@ -128,6 +124,14 @@ private static Window CreateWindow(IntPtr windowHandle, Container targetParent) WindowType.Fullscreen => throw new ArgumentException(null, nameof(windowHandle)), _ => throw new ArgumentException(null, nameof(windowHandle)), }; + + // The OS might spawn the window on a different monitor to the target parent, so adjustments + // might need to be made because of DPI. + var monitor = _monitorService.GetMonitorFromHandleLocation(windowHandle); + if (MonitorService.HasDpiDifference(monitor, window.Parent)) + window.HasPendingDpiAdjustment = true; + + return window; } private static WindowType GetWindowTypeToCreate(IntPtr windowHandle) diff --git a/GlazeWM.Domain/Windows/CommandHandlers/RunWindowRulesHandler.cs b/GlazeWM.Domain/Windows/CommandHandlers/RunWindowRulesHandler.cs deleted file mode 100644 index db91765a6..000000000 --- a/GlazeWM.Domain/Windows/CommandHandlers/RunWindowRulesHandler.cs +++ /dev/null @@ -1,57 +0,0 @@ -using System.Linq; -using GlazeWM.Domain.UserConfigs; -using GlazeWM.Domain.Windows.Commands; -using GlazeWM.Infrastructure.Bussing; - -namespace GlazeWM.Domain.Windows.CommandHandlers -{ - internal sealed class RunWindowRulesHandler : ICommandHandler - { - private readonly Bus _bus; - private readonly CommandParsingService _commandParsingService; - private readonly WindowService _windowService; - - public RunWindowRulesHandler( - Bus bus, - CommandParsingService commandParsingService, - WindowService windowService) - { - _bus = bus; - _commandParsingService = commandParsingService; - _windowService = windowService; - } - - public CommandResponse Handle(RunWindowRulesCommand command) - { - var window = command.Window; - var windowHandle = window.Handle; - var windowRules = command.WindowRules; - - var commandStrings = windowRules - .SelectMany(rule => rule.CommandList) - .Select(commandString => CommandParsingService.FormatCommand(commandString)); - - // Window to use as subject container when invoking commands. - var subjectWindow = window; - - foreach (var commandString in commandStrings) - { - // Subject window might be null if "ignore" rule has run. - if (subjectWindow?.IsDetached() != false) - return CommandResponse.Ok; - - var parsedCommand = _commandParsingService.ParseCommand(commandString, subjectWindow); - - // Invoke commands in the matching window rules. Use `dynamic` to resolve the command type - // at runtime and allow multiple dispatch. - _bus.Invoke((dynamic)parsedCommand); - - // Update subject window in case the reference changes (eg. when going from a tiling to a - // floating window). - subjectWindow = _windowService.GetWindowByHandle(windowHandle); - } - - return CommandResponse.Ok; - } - } -} diff --git a/GlazeWM.Domain/Windows/Commands/RunWindowRulesCommand.cs b/GlazeWM.Domain/Windows/Commands/RunWindowRulesCommand.cs deleted file mode 100644 index 24669240a..000000000 --- a/GlazeWM.Domain/Windows/Commands/RunWindowRulesCommand.cs +++ /dev/null @@ -1,18 +0,0 @@ -using System.Collections.Generic; -using GlazeWM.Domain.UserConfigs; -using GlazeWM.Infrastructure.Bussing; - -namespace GlazeWM.Domain.Windows.Commands -{ - public class RunWindowRulesCommand : Command - { - public Window Window { get; } - public List WindowRules { get; } - - public RunWindowRulesCommand(Window window, List windowRules) - { - Window = window; - WindowRules = windowRules; - } - } -}