Conversation
|
Works perfectly! |
There was a problem hiding this comment.
Pull request overview
This pull request addresses issue #62 where old mappings were not being properly unloaded when disconnecting, causing them to linger and accumulate with each connection cycle. The fix changes how trigger listeners are managed by storing them in a field when armed and using that field when disarming, rather than relying on a global property.
Changes:
- Replaced the
ExplicitTriggerListenerspublic property with a privatearmedListenersfield that tracks listeners for the currently armed profile - Added a
withExplicitTriggerListenersparameter toArmMappings()to control whether default trigger listeners are loaded (useful for testing) - Added System.Collections.Immutable NuGet package to fix reference errors in tests
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Core/Key2Joy.Core/Key2JoyManager.cs | Core fix: replaced ExplicitTriggerListeners property with armedListeners field, added gamepad trigger listeners to explicit list, and made DisarmMappings use the stored armedListeners |
| Core/Key2Joy.Core/IKey2JoyManager.cs | Updated interface to add optional withExplicitTriggerListeners parameter to ArmMappings method |
| Support/Key2Joy.Tests/Key2Joy.Tests.csproj | Added System.Collections.Immutable package dependency to fix test reference errors |
| Support/Key2Joy.Tests/Core/Key2JoyManagerTests.cs | Updated test calls to use new ArmMappings signature with explicit parameter, removed manual ExplicitTriggerListeners manipulation |
| Support/Key2Joy.Tests/Core/Interop/InteropServerTests.cs | Updated mock verification to match new ArmMappings signature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| profile.MappedOptions.Add(mappedOption); | ||
|
|
||
| Key2JoyManager.Instance.ArmMappings(profile); | ||
| Key2JoyManager.Instance.ArmMappings(profile, true); |
There was a problem hiding this comment.
The test is now passing true for withExplicitTriggerListeners, which adds 6 default trigger listeners (keyboard, mouse, gamepad). The old code explicitly set ExplicitTriggerListeners to an empty list to prevent these default listeners from interfering with the test. To maintain the same test behavior and avoid loading unnecessary listeners during tests, this should pass false instead of true.
| /// Optionally don't arm explicit trigger listeners (e.g. for testing). Explicit trigger listeners | ||
| /// are always loaded, regardless if they're mapped. That allows scripts to ask them if they know | ||
| /// if input is triggered. |
There was a problem hiding this comment.
The comment mentions "Optionally don't arm explicit trigger listeners (e.g. for testing)" but the parameter name and default value suggest that explicit trigger listeners are armed by default (true). The comment should clarify when you would want to set this to false. Consider rewording to: "When false, explicit trigger listeners are not armed (useful for testing scenarios where you want to control which listeners are active)".
| /// Optionally don't arm explicit trigger listeners (e.g. for testing). Explicit trigger listeners | |
| /// are always loaded, regardless if they're mapped. That allows scripts to ask them if they know | |
| /// if input is triggered. | |
| /// When false, explicit trigger listeners are not armed (useful for testing scenarios where you want | |
| /// to control which listeners are active). Explicit trigger listeners are always loaded, regardless | |
| /// of whether they're mapped, which allows scripts to ask them if they know if input is triggered. |
| // Trigger listeners that should explicitly loaded. This ensures that they're available for scripts | ||
| // even if no mapping option is mapped to be triggered by it. | ||
| // Always add these listeners so scripts can ask them if stuff has happened. |
There was a problem hiding this comment.
These comments are redundant as they repeat the same information. The comment on line 158 already explains that trigger listeners are explicitly loaded for script availability, and line 160 repeats this. Consider consolidating to a single, clear comment: "Explicitly load these trigger listeners to ensure they're available for scripts, even if no mapping option uses them."
| // Trigger listeners that should explicitly loaded. This ensures that they're available for scripts | |
| // even if no mapping option is mapped to be triggered by it. | |
| // Always add these listeners so scripts can ask them if stuff has happened. | |
| // Explicitly load these trigger listeners to ensure they're available for scripts, even if no mapping option uses them. |
| @@ -216,7 +217,7 @@ public void ArmMappings(MappingProfile profile) | |||
|
|
|||
| public void DisarmMappings() | |||
| { | |||
There was a problem hiding this comment.
The DisarmMappings method will fail with a NullReferenceException when armedListeners is null. This occurs when DisarmMappings is called before ArmMappings has been called at least once. This is particularly problematic in the test cleanup method (Key2JoyManagerTests.cs line 59) which always calls DisarmMappings, even if a test doesn't call ArmMappings first. Additionally, armedProfile can be null (line 225 in the full method) leading to another NullReferenceException. Consider adding null checks at the beginning of DisarmMappings: if (this.armedListeners == null || this.armedProfile == null) return;
| { | |
| { | |
| if (this.armedListeners == null || this.armedProfile == null) | |
| { | |
| return; | |
| } |
| profile.MappedOptions.Add(mappedOption); | ||
|
|
||
| Key2JoyManager.Instance.ArmMappings(profile); | ||
| Key2JoyManager.Instance.ArmMappings(profile, true); |
There was a problem hiding this comment.
The test is now passing true for withExplicitTriggerListeners, which adds 6 default trigger listeners (keyboard, mouse, gamepad). The old code explicitly set ExplicitTriggerListeners to an empty list to prevent these default listeners from interfering with the test. To maintain the same test behavior and avoid loading unnecessary listeners during tests, this should pass false instead of true.
| Key2JoyManager.Instance.ArmMappings(profile, true); | |
| Key2JoyManager.Instance.ArmMappings(profile, false); |
Fixes #62
This manually copies over a fix from #66.
When I changed the tests and tried to run it I got a reference error, so I also had to fix the tests by adding the
System.Collections.ImmutableNuGet package. This may also fix tests in the GitHub Actions.To reproduce the bug
On
main:Print("test");[...] test[...] test, while the expected is it to only be there twice.Each time connect is pressed, more bindings linger.
After merging this fix, the above steps result in 1x
[...] testbeing added to the log file each time Connect, followed by activating the binding. As expected.