🧹 Refactor displayName(for:) mapping to dictionary lookup#33
Conversation
Co-authored-by: NSEvent <44446865+NSEvent@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThis PR introduces left/right keyboard modifier key distinction (CommandSide, OptionSide, etc.) across input capture, display, and network protocols; refactors the InputSimulator API with a new ScrollEvent struct and side-aware modifier handling; and adds comprehensive accessibility support (help tooltips and labels) to icon-only UI buttons throughout the application. ChangesModifier Side Awareness System
Input Simulation API Refactoring
UI Accessibility Improvements
Supporting Infrastructure and Refactors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR spans multiple interconnected subsystems (modifier key handling, input simulation, universal control relay, UI accessibility) with substantial logic density in InputSimulator, UniversalControlMouseRelay, and KeyCaptureField. While individual accessor/property changes are straightforward, the cumulative scope of side-aware virtual-key selection, scroll event refactoring, KP2 protocol negotiation, and cross-file API updates across dozens of files demands careful review of interaction points and state management consistency. Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
NSEvent
left a comment
NSEvent
left a comment
There was a problem hiding this comment.
Reviewed. No blocker found. The dictionary preserves the prior display-name coverage and keeps the unknown-key fallback behavior.
|
@jules resolve merge conflicts |
|
@jules please resolve merge conflicts |
I have successfully resolved the merge conflicts and rebased the branch on top of |
Co-authored-by: NSEvent <44446865+NSEvent@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
XboxControllerMapper/XboxControllerMapper/Utilities/VariableExpander.swift (1)
85-107:⚠️ Potential issue | 🟠 MajorReusing one
DateFormattermakes variable expansion order-dependent (stale formatter state).
expand(_:)reuses a singleDateFormatterwhileresolveVariablemutates it per placeholder. Some cases (e.g.,date.long) only setdateStyleand don’t clear the priortimeStyle/dateFormat, so the output for a later placeholder can change based on placeholder order (e.g.,{datetime.long}|{date.long}vs{date.long}|{datetime.long}).Fix
resolveVariableby fully resetting formatter state at entry (at minimum:formatter.dateFormat = nil,formatter.dateStyle = .none,formatter.timeStyle = .none) or by creating a freshDateFormatterper variable resolution; keep/extend the order-independence test but ensure it’s not sensitive toDate()timing boundaries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@XboxControllerMapper/XboxControllerMapper/Utilities/VariableExpander.swift` around lines 85 - 107, The expand(_:) implementation reuses a single DateFormatter and mutate it inside resolveVariable(_:formatter:), causing order-dependent output; fix by resetting the formatter state at the start of resolveVariable(_:formatter:) (e.g., set formatter.dateFormat = nil, formatter.dateStyle = .none, formatter.timeStyle = .none) or by instantiating a new DateFormatter per call to resolveVariable, and keep/adjust the order-independence test so it no longer depends on DateFormatter state or Date() timing boundaries.
🧹 Nitpick comments (2)
XboxControllerMapper/XboxControllerMapper/Views/Components/OnScreenKeyboardView.swift (1)
573-573: ⚖️ Poor tradeoffConsider extending KeyLayout pattern to other key-rendering functions.
The
clickableKeyfunction now usesKeyLayoutfor a cleaner API, butmodifierKey,capsLockKey, andmediaKeystill accept individual layout parameters. Applying the same pattern would improve consistency, though the current implementation is fully functional.Also applies to: 864-864, 915-915
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@XboxControllerMapper/XboxControllerMapper/Views/Components/OnScreenKeyboardView.swift` at line 573, The mediaKey, modifierKey, and capsLockKey functions still take separate layout parameters while clickableKey uses the KeyLayout type; update these three functions to accept a KeyLayout (or optional KeyLayout with a sensible default) and adjust their internals to read width/height/row/column from that KeyLayout, then update all call sites (including where mediaKey, modifierKey and capsLockKey are invoked) to pass a KeyLayout instance to keep the API consistent with clickableKey and avoid duplicating layout parameters.test_performance.swift (1)
19-26: ⚡ Quick winConsider printing the match counts for verification.
The benchmark computes
arrayContainsCountandsetContainsCountbut never prints them. Printing these values would verify that both methods found the same number of matches, adding confidence that the benchmark is measuring equivalent operations.📊 Proposed enhancement to print match counts
let timeElapsedArray = CFAbsoluteTimeGetCurrent() - startTimeArray -print("Array contains time: \(timeElapsedArray) seconds") +print("Array contains time: \(timeElapsedArray) seconds (matches: \(arrayContainsCount))")let timeElapsedSet = CFAbsoluteTimeGetCurrent() - startTimeSet -print("Set contains time: \(timeElapsedSet) seconds") +print("Set contains time: \(timeElapsedSet) seconds (matches: \(setContainsCount))")Also applies to: 30-37
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test_performance.swift` around lines 19 - 26, The benchmark currently computes arrayContainsCount and setContainsCount but never outputs them; after the array search loop (variable arrayContainsCount) and after the set search loop (variable setContainsCount) add print statements to display each count (and optionally a comparison message asserting they match) so the benchmark verifies both methods found the same number of matches and confirms equivalent work is measured.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@XboxControllerMapper/XboxControllerMapper/Services/Macros/MacroExecutor.swift`:
- Around line 92-108: The mapping currently calls
inputSimulator.holdModifiers(mapping.modifiers) and then also treats the primary
keyCode as a modifier via KeyCodeMapping.isModifierKey(keyCode), causing the
same modifier to be pressed twice; fix by computing a
modifiersExcludingPrimaryModifier(mapping.modifiers, keyCode) (implement the
suggested modifiersExcludingPrimaryModifier function) and use that reduced set
when calling inputSimulator.holdModifiers and inputSimulator.releaseModifiers,
while leaving the existing branch that calls
inputSimulator.holdModifierKey(keyCode)/releaseModifierKey(keyCode) for the
primary modifier; ensure the same reduced modifiers are released after the keyUp
to keep holds symmetric.
In
`@XboxControllerMapper/XboxControllerMapper/Services/Profile/ProfileManager`+FaviconCache.swift:
- Around line 26-27: The pattern match against an optional enum is incorrect:
change the check that reads "if case .openLink(let url) = action.systemCommand,
action.iconData == nil" to either use an optional-case pattern (if case
.openLink(let url)? = action.systemCommand, action.iconData == nil) or unwrap
first (if let cmd = action.systemCommand, case .openLink(let url) = cmd,
action.iconData == nil) so that CommandWheelAction.systemCommand
(SystemCommand?) is handled correctly before calling
FaviconCache.shared.loadCachedFavicon(for: url).
In
`@XboxControllerMapper/XboxControllerMapper/Views/MainWindow/ButtonMappingSheet.swift`:
- Around line 388-402: The branch handling modifier keys
(KeyCodeMapping.isModifierKey in the setter) disables long hold, double tap and
repeat but forgets to clear enableHoldRepeat; update that modifier-key branch to
also set enableHoldRepeat = false (and ensure any related state cleared like
longHoldState and doubleTapState as already done) so previously-enabled
hold-repeat cannot persist when isHoldModifier is set via
userHasInteractedWithHold/isHoldModifier logic.
In
`@XboxControllerMapper/XboxControllerMapper/Views/MainWindow/SwipeTypingSection.swift`:
- Around line 141-145: The loaded custom words are not being normalized to
lowercase, so case-variant duplicates (e.g., "Apple" vs "apple") persist; update
the loading logic in SwipeTypingSection.swift (the code that builds the local
"words" array before assigning "customWords") to lowercase each trimmed
token—either by adding .lowercased() after trimming in the map chain or by
mapping to lowercased values before creating Set(words)—so customWords is
populated with consistently lowercased entries.
---
Outside diff comments:
In `@XboxControllerMapper/XboxControllerMapper/Utilities/VariableExpander.swift`:
- Around line 85-107: The expand(_:) implementation reuses a single
DateFormatter and mutate it inside resolveVariable(_:formatter:), causing
order-dependent output; fix by resetting the formatter state at the start of
resolveVariable(_:formatter:) (e.g., set formatter.dateFormat = nil,
formatter.dateStyle = .none, formatter.timeStyle = .none) or by instantiating a
new DateFormatter per call to resolveVariable, and keep/adjust the
order-independence test so it no longer depends on DateFormatter state or Date()
timing boundaries.
---
Nitpick comments:
In `@test_performance.swift`:
- Around line 19-26: The benchmark currently computes arrayContainsCount and
setContainsCount but never outputs them; after the array search loop (variable
arrayContainsCount) and after the set search loop (variable setContainsCount)
add print statements to display each count (and optionally a comparison message
asserting they match) so the benchmark verifies both methods found the same
number of matches and confirms equivalent work is measured.
In
`@XboxControllerMapper/XboxControllerMapper/Views/Components/OnScreenKeyboardView.swift`:
- Line 573: The mediaKey, modifierKey, and capsLockKey functions still take
separate layout parameters while clickableKey uses the KeyLayout type; update
these three functions to accept a KeyLayout (or optional KeyLayout with a
sensible default) and adjust their internals to read width/height/row/column
from that KeyLayout, then update all call sites (including where mediaKey,
modifierKey and capsLockKey are invoked) to pass a KeyLayout instance to keep
the API consistent with clickableKey and avoid duplicating layout parameters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: abee80d6-107e-40e8-bbd5-4670480fa5e6
📒 Files selected for processing (49)
.Jules/palette.mdLICENSEXboxControllerMapper/XboxControllerMapper/Models/CommandWheelAction.swiftXboxControllerMapper/XboxControllerMapper/Models/ControllerButton.swiftXboxControllerMapper/XboxControllerMapper/Models/KeyMapping.swiftXboxControllerMapper/XboxControllerMapper/Services/Input/InputSimulator.swiftXboxControllerMapper/XboxControllerMapper/Services/Input/UniversalControlMouseRelay.swiftXboxControllerMapper/XboxControllerMapper/Services/Macros/MacroExecutor.swiftXboxControllerMapper/XboxControllerMapper/Services/Mapping/ActionCommand.swiftXboxControllerMapper/XboxControllerMapper/Services/Mapping/ButtonInteractionFlowPolicy.swiftXboxControllerMapper/XboxControllerMapper/Services/Mapping/ButtonPressOrchestrationPolicy.swiftXboxControllerMapper/XboxControllerMapper/Services/Mapping/JoystickHandler.swiftXboxControllerMapper/XboxControllerMapper/Services/Mapping/MappingEngine.swiftXboxControllerMapper/XboxControllerMapper/Services/Mapping/TouchpadInputHandler.swiftXboxControllerMapper/XboxControllerMapper/Services/Profile/ProfileManager+FaviconCache.swiftXboxControllerMapper/XboxControllerMapper/Services/Profile/StreamDeckProfileParser.swiftXboxControllerMapper/XboxControllerMapper/Services/UI/FaviconCache.swiftXboxControllerMapper/XboxControllerMapper/Utilities/KeyCodeMapping.swiftXboxControllerMapper/XboxControllerMapper/Utilities/VariableExpander.swiftXboxControllerMapper/XboxControllerMapper/Views/Components/BookmarkPickerSheet.swiftXboxControllerMapper/XboxControllerMapper/Views/Components/KeyCaptureField.swiftXboxControllerMapper/XboxControllerMapper/Views/Components/KeyboardVisualView.swiftXboxControllerMapper/XboxControllerMapper/Views/Components/OnScreenKeyboardView.swiftXboxControllerMapper/XboxControllerMapper/Views/Macros/MacroEditorSheet.swiftXboxControllerMapper/XboxControllerMapper/Views/Macros/MacroListView.swiftXboxControllerMapper/XboxControllerMapper/Views/MainWindow/ButtonMappingSheet.swiftXboxControllerMapper/XboxControllerMapper/Views/MainWindow/CommandWheelSettingsView.swiftXboxControllerMapper/XboxControllerMapper/Views/MainWindow/ContentView.swiftXboxControllerMapper/XboxControllerMapper/Views/MainWindow/LinkedAppsSheet.swiftXboxControllerMapper/XboxControllerMapper/Views/MainWindow/LinkedControllersSheet.swiftXboxControllerMapper/XboxControllerMapper/Views/MainWindow/MappingEditorState.swiftXboxControllerMapper/XboxControllerMapper/Views/MainWindow/OnScreenKeyboardSettingsView.swiftXboxControllerMapper/XboxControllerMapper/Views/MainWindow/SwipeTypingSection.swiftXboxControllerMapper/XboxControllerMapper/Views/Scripts/ScriptEditorSheet.swiftXboxControllerMapper/XboxControllerMapper/Views/Scripts/ScriptListView.swiftXboxControllerMapper/XboxControllerMapperTests/ButtonInteractionFlowPolicyTests.swiftXboxControllerMapper/XboxControllerMapperTests/ButtonPressOrchestrationPolicyTests.swiftXboxControllerMapper/XboxControllerMapperTests/InputSimulatorBugTests.swiftXboxControllerMapper/XboxControllerMapperTests/KeyCodeMappingDisplayTests.swiftXboxControllerMapper/XboxControllerMapperTests/MappingEngineLayerAndLifecycleTests.swiftXboxControllerMapper/XboxControllerMapperTests/ModifierFlagsSideTests.swiftXboxControllerMapper/XboxControllerMapperTests/ScriptEngineSecurityTests.swiftXboxControllerMapper/XboxControllerMapperTests/ScriptExamplesTests.swiftXboxControllerMapper/XboxControllerMapperTests/ScriptTests.swiftXboxControllerMapper/XboxControllerMapperTests/UniversalControlRelaySecurityTests.swiftXboxControllerMapper/XboxControllerMapperTests/VariableExpanderTests.swiftXboxControllerMapper/XboxControllerMapperTests/XboxControllerMapperTests.swiftspecs/shannon-keybinding-analysis.mdtest_performance.swift
| if mapping.modifiers.hasAny { | ||
| inputSimulator.holdModifiers(mapping.modifiers) | ||
| } | ||
| if KeyCodeMapping.isModifierKey(keyCode) { | ||
| inputSimulator.holdModifierKey(keyCode) | ||
| } else { | ||
| inputSimulator.keyDown(keyCode, modifiers: mapping.modifiers.cgEventFlags) | ||
| } | ||
| usleep(useconds_t(clampedUs)) | ||
| inputSimulator.keyUp(keyCode) | ||
| if KeyCodeMapping.isModifierKey(keyCode) { | ||
| inputSimulator.releaseModifierKey(keyCode) | ||
| } else { | ||
| inputSimulator.keyUp(keyCode) | ||
| } | ||
| if mapping.modifiers.hasAny { | ||
| inputSimulator.releaseModifiers(mapping.modifiers) | ||
| } |
There was a problem hiding this comment.
Avoid double-holding the same modifier in hold mappings
Line 92 holds mapping.modifiers even when Line 95 treats keyCode as a modifier key. If the mapping includes that same modifier, you press/release it twice, which can produce wrong side behavior or unstable held-modifier state.
Proposed fix
- if mapping.modifiers.hasAny {
- inputSimulator.holdModifiers(mapping.modifiers)
- }
+ let extraModifiers = modifiersExcludingPrimaryModifier(
+ mapping.modifiers,
+ keyCode: keyCode
+ )
+ if extraModifiers.hasAny {
+ inputSimulator.holdModifiers(extraModifiers)
+ }
if KeyCodeMapping.isModifierKey(keyCode) {
inputSimulator.holdModifierKey(keyCode)
} else {
inputSimulator.keyDown(keyCode, modifiers: mapping.modifiers.cgEventFlags)
}
usleep(useconds_t(clampedUs))
if KeyCodeMapping.isModifierKey(keyCode) {
inputSimulator.releaseModifierKey(keyCode)
} else {
inputSimulator.keyUp(keyCode)
}
- if mapping.modifiers.hasAny {
- inputSimulator.releaseModifiers(mapping.modifiers)
+ if extraModifiers.hasAny {
+ inputSimulator.releaseModifiers(extraModifiers)
}private func modifiersExcludingPrimaryModifier(
_ modifiers: ModifierFlags,
keyCode: CGKeyCode
) -> ModifierFlags {
var result = modifiers
switch keyCode {
case CGKeyCode(kVK_Command), CGKeyCode(kVK_RightCommand):
result.command = false
result.commandSide = nil
case CGKeyCode(kVK_Option), CGKeyCode(kVK_RightOption):
result.option = false
result.optionSide = nil
case CGKeyCode(kVK_Shift), CGKeyCode(kVK_RightShift):
result.shift = false
result.shiftSide = nil
case CGKeyCode(kVK_Control), CGKeyCode(kVK_RightControl):
result.control = false
result.controlSide = nil
default:
break
}
return result
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@XboxControllerMapper/XboxControllerMapper/Services/Macros/MacroExecutor.swift`
around lines 92 - 108, The mapping currently calls
inputSimulator.holdModifiers(mapping.modifiers) and then also treats the primary
keyCode as a modifier via KeyCodeMapping.isModifierKey(keyCode), causing the
same modifier to be pressed twice; fix by computing a
modifiersExcludingPrimaryModifier(mapping.modifiers, keyCode) (implement the
suggested modifiersExcludingPrimaryModifier function) and use that reduced set
when calling inputSimulator.holdModifiers and inputSimulator.releaseModifiers,
while leaving the existing branch that calls
inputSimulator.holdModifierKey(keyCode)/releaseModifierKey(keyCode) for the
primary modifier; ensure the same reduced modifiers are released after the keyUp
to keep holds symmetric.
| if case .openLink(let url) = action.systemCommand, action.iconData == nil { | ||
| _ = await FaviconCache.shared.loadCachedFavicon(for: url) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Swift, when a value has type Optional<Enum>, is if case .someCase(let x) = valuevalid, or must it beif case .someCase(let x)? = value?
💡 Result:
In Swift, both if case .someCase(let x) = value and if case .someCase(let x)? = value are valid ways to pattern match against an Optional enum, but they have different meanings and requirements [1][2]. The syntax if case .someCase(let x) = value is valid. Since Swift 5.1, the compiler allows matching an enum case directly against an optional enum [3][4]. In this form, you are matching the optional enum against the case pattern directly. The syntax if case .someCase(let x)? = value uses the Optional pattern (the ? suffix) [1][2]. This is syntactic sugar that explicitly unwraps the optional before matching the case [1][5]. While both may work depending on the context, they function as follows: - if case .someCase(let x) = value matches the optional value against the specified case [3][4]. - if case let x? = value (the standard Optional pattern) or if case .someCase(let x)? = value explicitly treats the pattern as matching the unwrapped value [1][2]. In practice, because modern Swift handles optional enum matching quite flexibly, you can often omit the ? [3][4]. However, if you are working with older codebases or specific complex nesting, the optional pattern (?) provides a clear, explicit way to indicate that you are matching against the unwrapped content [1][2][4].
Citations:
- 1: https://web.archive.org/web/20220612012016/https:/docs.swift.org/swift-book/ReferenceManual/Patterns.html
- 2: https://www.bookstack.cn/read/TheSwiftProgrammingLanguageSwift-5.5-en/ReferenceManual-Patterns.xhtml
- 3: https://www.hackingwithswift.com/swift/5.1/matching-optional-enums
- 4: [Typechecker] Allow matching an enum case against an optional enum without '?' swiftlang/swift#22486
- 5: https://stackoverflow.com/questions/55879643/how-does-the-unwrap-an-optional-in-a-case-let-declaration
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the target file and show the relevant section around lines 26-27
FILE="XboxControllerMapper/XboxControllerMapper/Services/Profile/ProfileManager+FaviconCache.swift"
echo "== Target snippet =="
nl -ba "$FILE" | sed -n '1,120p' | sed -n '15,60p'
# Locate where `systemCommand` is declared (type of `action.systemCommand`)
echo "== Search for `systemCommand` declaration usages =="
rg -n "systemCommand" XboxControllerMapper/XboxControllerMapper/Services/Profile/ProfileManager+FaviconCache.swift XboxControllerMapper/XboxControllerMapper --hidden --glob '!**/.build/**'
# Specifically locate the type that defines `systemCommand` and the enum case `.openLink`
echo "== Search for `.openLink` case =="
rg -n "\.openLink" XboxControllerMapper/XboxControllerMapper --hidden --glob '!**/.build/**'Repository: NSEvent/xbox-controller-mapper
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="XboxControllerMapper/XboxControllerMapper/Services/Profile/ProfileManager+FaviconCache.swift"
echo "== Target snippet =="
# show a reasonable window around line 26
cat -n "$FILE" | sed -n '1,140p' | sed -n '15,70p'
echo "== Search for `systemCommand` declaration/usages in the codebase =="
rg -n "systemCommand" XboxControllerMapper/XboxControllerMapper --hidden --glob '!**/.build/**'
echo "== Search for `.openLink` case usages =="
rg -n "\.openLink" XboxControllerMapper/XboxControllerMapper --hidden --glob '!**/.build/**'Repository: NSEvent/xbox-controller-mapper
Length of output: 47496
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show CommandWheelAction model to confirm the type of `systemCommand`
FILE1="XboxControllerMapper/XboxControllerMapper/Models/CommandWheelAction.swift"
echo "== CommandWheelAction.swift (systemCommand type) =="
wc -l "$FILE1"
cat -n "$FILE1" | sed -n '1,220p'
# Show where Profile exposes commandWheelActions
FILE2="XboxControllerMapper/XboxControllerMapper/Models/Profile.swift"
echo "== Profile.swift (commandWheelActions property) =="
wc -l "$FILE2"
rg -n "commandWheelActions" "$FILE2"
# print a small region around the match
line=$(rg -n "commandWheelActions" "$FILE2" | head -n1 | cut -d: -f1)
start=$((line-40)); end=$((line+60))
cat -n "$FILE2" | sed -n "${start},${end}p"
# Inspect SystemCommand enum openLink case
FILE3="XboxControllerMapper/XboxControllerMapper/Models/SystemCommand.swift"
echo "== SystemCommand.swift (openLink case) =="
wc -l "$FILE3"
rg -n "case \\.openLink" "$FILE3" | head
cat -n "$FILE3" | sed -n '110,260p'
# Search for similar pattern-matching style in codebase
echo "== Similar pattern usages: `if case .openLink(let url) = ...` =="
rg -n "if case \\.openLink\\(let url\\) = " XboxControllerMapper/XboxControllerMapper --hidden --glob '!**/.build/**' || true
echo "== Similar pattern usages: `if case \\.openLink` (broader) =="
rg -n "if case \\.openLink" XboxControllerMapper/XboxControllerMapper --hidden --glob '!**/.build/**' || trueRepository: NSEvent/xbox-controller-mapper
Length of output: 22299
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate Package.swift and swift-tools-version =="
if [ -f "Package.swift" ]; then
cat -n Package.swift | head -n 80
rg -n "swift-tools-version" Package.swift || true
else
echo "No root Package.swift found."
fi
echo "== Search for SWIFT_VERSION / toolchain settings =="
rg -n "SWIFT_VERSION|swift-tools-version|PLATFORM_NAME|ENABLE_SWIFTUI_PREVIEWS" --hidden --glob '!**/.build/**' . || true
echo "== Show xcode project/podspec swift version hints (if present) =="
fd -e pbxproj -e xcconfig -e podspec . --max-results 20 2>/dev/null | head -n 50Repository: NSEvent/xbox-controller-mapper
Length of output: 934
🌐 Web query:
Swift 5.0 if case .someCase(let x) = optionalEnum enum pattern matching allowed without trailing ?
💡 Result:
Yes, pattern matching an enum case against an optional enum without a trailing '?' is allowed in Swift, but it was not supported in Swift 5.0 [1]. This capability was introduced in Swift 5.1 [1]. In versions of Swift prior to 5.1, when working with an optional enum, you were required to use a trailing question mark (e.g., case.someCase?) or explicitly match against.some(.someCase) to account for the optionality [2][3][1]. Starting with Swift 5.1, the compiler gained the ability to automatically look through optionals when matching against enumeration cases, allowing you to write cleaner code such as: if case.someCase = optionalEnum { // Handle case } This change was implemented as a bug fix/enhancement to the type checker to resolve inconsistencies where optional strings and integers could be matched this way, but optional enums could not [2][1]. Note that this applies to both switch statements and if/guard/for control flow statements using pattern matching [2][1].
Citations:
- 1: https://www.hackingwithswift.com/swift/5.1/matching-optional-enums
- 2: [Typechecker] Allow matching an enum case against an optional enum without '?' swiftlang/swift#22486
- 3: [SR-7799] Matching optional enums against non-optional values swiftlang/swift#50338
Fix optional enum case-pattern matching for Swift 5.0.
CommandWheelAction.systemCommand is SystemCommand?, but the code uses if case .openLink(let url) = action.systemCommand (no optional pattern). With SWIFT_VERSION = 5.0, matching an enum case directly against an optional enum isn’t supported—use an optional-case pattern (e.g. if case .openLink(let url)? = action.systemCommand, ...) or unwrap first (if let cmd = action.systemCommand, case .openLink(let url) = cmd, ...).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@XboxControllerMapper/XboxControllerMapper/Services/Profile/ProfileManager`+FaviconCache.swift
around lines 26 - 27, The pattern match against an optional enum is incorrect:
change the check that reads "if case .openLink(let url) = action.systemCommand,
action.iconData == nil" to either use an optional-case pattern (if case
.openLink(let url)? = action.systemCommand, action.iconData == nil) or unwrap
first (if let cmd = action.systemCommand, case .openLink(let url) = cmd,
action.iconData == nil) so that CommandWheelAction.systemCommand
(SystemCommand?) is handled correctly before calling
FaviconCache.shared.loadCachedFavicon(for: url).
| if let code = newValue, KeyCodeMapping.isModifierKey(code) { | ||
| // Modifier keys: auto-enable hold (so the modifier stays pressed while | ||
| // the controller button is held) and disable long hold / double tap / repeat | ||
| // which don't make sense for a sticky modifier. | ||
| if !userHasInteractedWithHold { | ||
| isHoldModifier = true | ||
| } | ||
| enableLongHold = false | ||
| enableDoubleTap = false | ||
| enableRepeat = false | ||
| longHoldState.keyCode = nil | ||
| longHoldState.modifiers = ModifierFlags() | ||
| doubleTapState.keyCode = nil | ||
| doubleTapState.modifiers = ModifierFlags() | ||
| } else if let code = newValue, KeyCodeMapping.isMouseButton(code) || KeyCodeMapping.isSpecialAction(code) { |
There was a problem hiding this comment.
Reset hold-repeat when switching primary action to a modifier key.
At Line 388, this branch disables repeat-style advanced behaviors, but enableHoldRepeat is not cleared. A previously enabled hold-repeat can carry into modifier mappings and produce unintended repeated modifier key-down posting.
Suggested fix
if let code = newValue, KeyCodeMapping.isModifierKey(code) {
// Modifier keys: auto-enable hold (so the modifier stays pressed while
// the controller button is held) and disable long hold / double tap / repeat
// which don't make sense for a sticky modifier.
if !userHasInteractedWithHold {
isHoldModifier = true
}
enableLongHold = false
enableDoubleTap = false
enableRepeat = false
+ enableHoldRepeat = false
longHoldState.keyCode = nil
longHoldState.modifiers = ModifierFlags()
doubleTapState.keyCode = nil
doubleTapState.modifiers = ModifierFlags()
} else if let code = newValue, KeyCodeMapping.isMouseButton(code) || KeyCodeMapping.isSpecialAction(code) {
@@
enableLongHold = false
enableDoubleTap = false
enableRepeat = false
+ enableHoldRepeat = false
longHoldState.keyCode = nil
longHoldState.modifiers = ModifierFlags()
doubleTapState.keyCode = nil
doubleTapState.modifiers = ModifierFlags()
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@XboxControllerMapper/XboxControllerMapper/Views/MainWindow/ButtonMappingSheet.swift`
around lines 388 - 402, The branch handling modifier keys
(KeyCodeMapping.isModifierKey in the setter) disables long hold, double tap and
repeat but forgets to clear enableHoldRepeat; update that modifier-key branch to
also set enableHoldRepeat = false (and ensure any related state cleared like
longHoldState and doubleTapState as already done) so previously-enabled
hold-repeat cannot persist when isHoldModifier is set via
userHasInteractedWithHold/isHoldModifier logic.
| let words = content.components(separatedBy: .newlines) | ||
| .map { $0.trimmingCharacters(in: .whitespaces) } | ||
| .filter { !$0.isEmpty } | ||
| customWords = Set(words) | ||
| } |
There was a problem hiding this comment.
Normalize loaded custom words to lowercase to keep dedupe behavior consistent.
At Line 148, user-entered words are lowercased before insertion, but loaded file words are not. This allows case-variant duplicates (for example, Apple and apple) after load.
Suggested fix
let words = content.components(separatedBy: .newlines)
.map { $0.trimmingCharacters(in: .whitespaces) }
+ .map { $0.lowercased() }
.filter { !$0.isEmpty }
customWords = Set(words)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@XboxControllerMapper/XboxControllerMapper/Views/MainWindow/SwipeTypingSection.swift`
around lines 141 - 145, The loaded custom words are not being normalized to
lowercase, so case-variant duplicates (e.g., "Apple" vs "apple") persist; update
the loading logic in SwipeTypingSection.swift (the code that builds the local
"words" array before assigning "customWords") to lowercase each trimmed
token—either by adding .lowercased() after trimming in the map chain or by
mapping to lowercased values before creating Set(words)—so customWords is
populated with consistently lowercased entries.
1 participant
🎯 What: Extracted the key code display names switch mapping in
XboxControllerMapper/XboxControllerMapper/Utilities/KeyCodeMapping.swiftinto aprivate static let displayNames: [Int: String]dictionary and updateddisplayName(for:)to use a single dictionary lookup.💡 Why: Reduces cyclic complexity and improves code readability and maintainability. A dictionary lookup provides O(1) performance and is cleaner than a massive switch statement for simple data mapping.
✅ Verification: Validated that the
git difflooks identical functionally with all key-value pairs accurately replicated from the previous switch cases. The default return statement is properly handled using a nil coalescing operator.✨ Result: A more readable and maintainable codebase structure that isolates mapping data from functional logic.
PR created automatically by Jules for task 4418355251378844985 started by @NSEvent
Summary by CodeRabbit
Release Notes
New Features
Improvements
Accessibility
Bug Fixes