Conversation
…d (I like the code this way better though)
…ly get us non-janky mouse move input. It appears to..
|
Since The other issues (including the jitteriness introduced by this PR) can be resolved later in separate PRs. I'll rename this PR to get it ready for review :) |
|
I can't request a review from you for some reason, so here's a ping: @luttje :P |
There was a problem hiding this comment.
Pull request overview
This pull request addresses issue #62 by fixing how old mappings are unloaded when new mappings are armed. The core fix involves creating new instances of trigger listeners each time mappings are armed, rather than reusing singleton instances that could retain state from previous mappings. Additionally, the PR implements WndProc-based raw input handling for mouse movement and updates several dependencies.
Changes:
- Refactored trigger listener lifecycle management to create fresh instances on each arming, preventing state carryover
- Implemented WndProc message routing from MainForm to listeners that need raw Windows messages
- Updated Jint library from 3.0.0 to 3.1.4 and adapted code to API changes
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| Key2Joy.Core/Key2JoyManager.cs | Refactored ArmMappings to create new listener instances instead of reusing singletons, added WndProc routing |
| Key2Joy.Core/IKey2JoyManager.cs | Updated interface signature for ArmMappings with optional parameter |
| Key2Joy.Core/Mapping/Triggers/*.cs | Added NewInstance() methods to all trigger listeners for proper reset |
| Key2Joy.Core/Mapping/Triggers/Mouse/MouseMoveTriggerListener.cs | Implemented WndProc handling for raw mouse input |
| Key2Joy.Contracts/Mapping/Triggers/AbstractTriggerListener.cs | Added virtual WndProc method and HasWndProcHandle property |
| Key2Joy.Gui/MainForm.cs | Added WndProc override and RawInputDevice registration/unregistration |
| Key2Joy.Gui/DeviceControl.cs | Added gamepad activity visualization with fade animation |
| Key2Joy.Core/LowLevelInput/SimulatedGamePad/*.cs | Refactored state access pattern with AccessState callback |
| Key2Joy.Core/Mapping/Actions/Input/GamePad*.cs | Updated to use new AccessState pattern |
| Key2Joy.Core/Mapping/Actions/Scripting/JavascriptScriptAction.cs | Updated delegate registration for Jint 3.1.4 compatibility |
| Key2Joy.Core/Util/TypeConverter.cs | Added error handling for invalid enum parsing |
| Key2Joy.Tests/*.cs | Updated test calls to match new ArmMappings signature |
| Documentation files | Fixed command capitalization consistency ("abort" → "Abort") |
Comments suppressed due to low confidence (1)
Core/Key2Joy.Core/Mapping/Triggers/Mouse/MouseMoveTriggerListener.cs:221
- The condition at line 217 is commented out, causing the block at lines 218-221 to execute unconditionally. This means that
lastAllowedXandlastAllowedYare now always updated regardless of whether the mouse should be overridden. This appears to be intentional based on the TODO comment at line 141, but it changes the behavior in a way that may affect mouse cursor centering functionality. Please verify this is the intended behavior.
//if (!shouldOverride || (!this.lastAllowedX.HasValue && !this.lastAllowedY.HasValue))
{
this.lastAllowedX = e.RawData.Position.X;
this.lastAllowedY = e.RawData.Position.Y;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| using Key2Joy.Config; | ||
| using Key2Joy.Contracts.Mapping.Triggers; | ||
| using Key2Joy.LowLevelInput; | ||
| using Key2Joy.Mapping.Triggers.Keyboard; |
There was a problem hiding this comment.
The using statement for Key2Joy.Mapping.Triggers.Keyboard is not used anywhere in this file and should be removed.
| using Key2Joy.Mapping.Triggers.Keyboard; |
| using Key2Joy.Contracts.Mapping; | ||
| using Key2Joy.Contracts.Mapping.Triggers; | ||
| using Key2Joy.LowLevelInput.XInput; | ||
| using Key2Joy.Mapping.Triggers.Mouse; |
There was a problem hiding this comment.
The using statement for Key2Joy.Mapping.Triggers.Mouse is not used anywhere in this file and should be removed.
| using Key2Joy.Mapping.Triggers.Mouse; |
| return; | ||
| } | ||
|
|
||
| /// Mouse buttons are handled through <see cref="MouseButtonTriggerListener"/> |
There was a problem hiding this comment.
The comment uses triple slashes (///) which is typically reserved for XML documentation comments in C#. Since this is a regular inline comment describing logic, it should use a double slash (//) instead.
| /// Mouse buttons are handled through <see cref="MouseButtonTriggerListener"/> | |
| // Mouse buttons are handled through MouseButtonTriggerListener |
| device.ActivityOccurred += this.Device_ActivityOccurred; | ||
| } | ||
|
|
||
| private void DeviceControl_Layout(object sender, LayoutEventArgs e) | ||
| { | ||
| if (this.Height == this.Width) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| this.Height = this.Width; | ||
| } | ||
|
|
||
| private void Device_ActivityOccurred(object sender, GamePadActivityOccurredEventArgs e) | ||
| { | ||
| this.lastActivityOccurred = DateTime.Now; | ||
|
|
||
| this.Invoke((MethodInvoker)delegate | ||
| { | ||
| try | ||
| { | ||
| this.fadeTimer.Stop(); | ||
| this.fadeTimer.Start(); | ||
| this.Invalidate(); | ||
| } | ||
| catch (System.ComponentModel.Win32Exception) | ||
| { | ||
| // Ignore (happens if we spam the chkArmed in MainForm) | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| private void FadeTimer_Tick(object sender, EventArgs e) | ||
| { | ||
| if ((DateTime.Now - this.lastActivityOccurred).TotalSeconds <= 2) | ||
| { | ||
| this.Invalidate(); | ||
| } | ||
| else | ||
| { | ||
| this.fadeTimer.Stop(); | ||
| } | ||
| } | ||
|
|
||
| private void DeviceControl_Paint(object sender, PaintEventArgs e) | ||
| { | ||
| var owner = (Control)sender; | ||
| var g = e.Graphics; | ||
|
|
||
| var elapsed = (DateTime.Now - this.lastActivityOccurred).TotalSeconds; | ||
|
|
||
| var currentColor = this.InterpolateColors(Color.White, Color.Gold, elapsed / FADE_DURATION); | ||
| using var brush = new SolidBrush(currentColor); | ||
| g.FillRectangle(brush, BORDER_WIDTH, BORDER_WIDTH, owner.Width - (BORDER_WIDTH * 2), owner.Height - (BORDER_WIDTH * 2)); | ||
| } | ||
|
|
||
| private Color InterpolateColors(Color start, Color end, double ratio) | ||
| { | ||
| var r = (int)((start.R * (1 - ratio)) + (end.R * ratio)); | ||
| var g = (int)((start.G * (1 - ratio)) + (end.G * ratio)); | ||
| var b = (int)((start.B * (1 - ratio)) + (end.B * ratio)); | ||
|
|
||
| return Color.FromArgb( | ||
| Math.Max(r, 0), | ||
| Math.Max(g, 0), | ||
| Math.Max(b, 0) | ||
| ); | ||
| } |
There was a problem hiding this comment.
The code references lastActivityOccurred, fadeTimer, FADE_DURATION, and BORDER_WIDTH fields that are not defined in this file. These fields need to be declared as class members for the code to compile. For example, you need to add field declarations like:
private DateTime lastActivityOccurred;private System.Timers.Timer fadeTimer;(or the appropriate timer type)private const double FADE_DURATION = ...;private const int BORDER_WIDTH = ...;
| device.ActivityOccurred += this.Device_ActivityOccurred; | ||
| } | ||
|
|
||
| private void DeviceControl_Layout(object sender, LayoutEventArgs e) | ||
| { | ||
| if (this.Height == this.Width) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| this.Height = this.Width; | ||
| } | ||
|
|
||
| private void Device_ActivityOccurred(object sender, GamePadActivityOccurredEventArgs e) | ||
| { | ||
| this.lastActivityOccurred = DateTime.Now; | ||
|
|
||
| this.Invoke((MethodInvoker)delegate | ||
| { | ||
| try | ||
| { | ||
| this.fadeTimer.Stop(); | ||
| this.fadeTimer.Start(); | ||
| this.Invalidate(); | ||
| } | ||
| catch (System.ComponentModel.Win32Exception) | ||
| { | ||
| // Ignore (happens if we spam the chkArmed in MainForm) | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| private void FadeTimer_Tick(object sender, EventArgs e) | ||
| { | ||
| if ((DateTime.Now - this.lastActivityOccurred).TotalSeconds <= 2) | ||
| { | ||
| this.Invalidate(); | ||
| } | ||
| else | ||
| { | ||
| this.fadeTimer.Stop(); | ||
| } | ||
| } | ||
|
|
||
| private void DeviceControl_Paint(object sender, PaintEventArgs e) | ||
| { | ||
| var owner = (Control)sender; | ||
| var g = e.Graphics; | ||
|
|
||
| var elapsed = (DateTime.Now - this.lastActivityOccurred).TotalSeconds; | ||
|
|
||
| var currentColor = this.InterpolateColors(Color.White, Color.Gold, elapsed / FADE_DURATION); | ||
| using var brush = new SolidBrush(currentColor); | ||
| g.FillRectangle(brush, BORDER_WIDTH, BORDER_WIDTH, owner.Width - (BORDER_WIDTH * 2), owner.Height - (BORDER_WIDTH * 2)); | ||
| } | ||
|
|
||
| private Color InterpolateColors(Color start, Color end, double ratio) | ||
| { | ||
| var r = (int)((start.R * (1 - ratio)) + (end.R * ratio)); | ||
| var g = (int)((start.G * (1 - ratio)) + (end.G * ratio)); | ||
| var b = (int)((start.B * (1 - ratio)) + (end.B * ratio)); | ||
|
|
||
| return Color.FromArgb( | ||
| Math.Max(r, 0), | ||
| Math.Max(g, 0), | ||
| Math.Max(b, 0) | ||
| ); | ||
| } |
There was a problem hiding this comment.
The code uses DateTime, Color, SolidBrush, Math, and GamePadActivityOccurredEventArgs types without the necessary using statements. You need to add:
using System;(for DateTime and Math)using System.Drawing;(for Color, SolidBrush)
Please add these using directives at the top of the file.
| // Commented, because as expected this freezes the app up when | ||
| // everyone has to wait for the lock to be released. And it didn't | ||
| // fix #61 like I hoped it would. | ||
| //lock (this.stateLock) | ||
| { | ||
| var state = SimGamePad.Instance.State[this.Index]; | ||
|
|
||
| if (stateAccessor(state) == StateAccessorResult.Unchanged) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| SimGamePad.Instance.Update(this.Index); | ||
| this.gamePadInfo.OnActivityOccurred(); | ||
| } |
There was a problem hiding this comment.
There is a commented-out lock statement with an explanation that it freezes the app. However, removing thread synchronization in a multi-threaded context (where state is accessed concurrently) could lead to race conditions. The comment mentions it didn't fix issue #61, but you should ensure that the absence of locking doesn't introduce new concurrency issues. Consider if there are alternative synchronization mechanisms or if the architecture can be adjusted to avoid concurrent state access.
| RawInputDevice.RegisterDevice(HidUsageAndPage.Mouse, RawInputDeviceFlags.InputSink, this.Handle); | ||
| Key2JoyManager.Instance.ArmMappings(this.selectedProfile); | ||
| } |
There was a problem hiding this comment.
The RawInputDevice registration and unregistration should ideally be wrapped in try-catch blocks to handle potential failures gracefully. If registration fails (e.g., due to invalid window handle or permissions), the application may behave unexpectedly. Consider adding error handling and potentially rolling back the ArmMappings call if registration fails.
| private List<AbstractTriggerListener> armedListeners; | ||
| private IHaveHandleAndInvoke handleAndInvoker; | ||
|
|
||
| private List<AbstractTriggerListener> wndProcListeners = new List<AbstractTriggerListener>(); |
There was a problem hiding this comment.
Field 'wndProcListeners' can be 'readonly'.
| private List<AbstractTriggerListener> wndProcListeners = new List<AbstractTriggerListener>(); | |
| private readonly List<AbstractTriggerListener> wndProcListeners = new List<AbstractTriggerListener>(); |
| private bool isPluggedIn = false; | ||
| private readonly IGamePadInfo gamePadInfo; | ||
|
|
||
| private object stateLock = new(); |
There was a problem hiding this comment.
Field 'stateLock' can be 'readonly'.
| private object stateLock = new(); | |
| private readonly object stateLock = new(); |
| if (this.Side == GamePadSide.Left) | ||
| { | ||
| deltaX = Scale(triggerInputBag.LeftTriggerDelta, this.InputScale); | ||
| } | ||
| else | ||
| { | ||
| deltaX = Scale(triggerInputBag.RightTriggerDelta, this.InputScale); | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
| if (this.Side == GamePadSide.Left) | |
| { | |
| deltaX = Scale(triggerInputBag.LeftTriggerDelta, this.InputScale); | |
| } | |
| else | |
| { | |
| deltaX = Scale(triggerInputBag.RightTriggerDelta, this.InputScale); | |
| } | |
| var triggerDelta = this.Side == GamePadSide.Left | |
| ? triggerInputBag.LeftTriggerDelta | |
| : triggerInputBag.RightTriggerDelta; | |
| deltaX = Scale(triggerDelta, this.InputScale); |
I'm a bit lost at what I was doing in this PR. It seems all over the place (sorry for that!). You seem a lot more in-the-know now than me. (@Aurumaker72) What do you want me to do? Just merge this? Or do I have to test and clean it up still?
Maybe because I made this PR? I unassigned myself, but that didn't show me as a possible reviewer either. |
|
The PR includes a fix for #62 as well as an attempted fix for #61. I can't verify whether this fixes #61 because I can't repro the issue on the main branch either, but the behavior seems sane otherwise. It seems like this PR contains two regression: jitteriness in joystick movement when the mouse is mapped to it, as well as the mouse freezing while the device armed even when mouse override is disabled in the settings. I've only noticed the latter regression now, and it's a dealbreaker which should be resolved before merging this. How do you think we should proceed? I don't feel too confident debugging it myself since I'm not too familiar with the core's input internals :/ |
|
If you're comfortable with rebasing it for the #62 fix that would be awesome. I think the rest of this PR can just be thrown away as a failed experiment in fixing input issues. Imo I can just close this and we could manually copy the fix onto a new branch. I see no reason to keep this in the timeline tbh |
|
Sounds good! |
Closes #62.