From 81e80b8cf5913ea387695e82ec7d72bef9350c86 Mon Sep 17 00:00:00 2001 From: sudacode Date: Sat, 14 Mar 2026 21:10:27 -0700 Subject: [PATCH] fix: address latest controller review feedback --- .../handlers/gamepad-controller.test.ts | 40 +++++++ src/renderer/handlers/gamepad-controller.ts | 22 +++- src/renderer/modals/controller-select.test.ts | 104 ++++++++++++++++++ src/renderer/modals/controller-select.ts | 27 +++-- 4 files changed, 183 insertions(+), 10 deletions(-) diff --git a/src/renderer/handlers/gamepad-controller.test.ts b/src/renderer/handlers/gamepad-controller.test.ts index abb6f1a2..120b38ed 100644 --- a/src/renderer/handlers/gamepad-controller.test.ts +++ b/src/renderer/handlers/gamepad-controller.test.ts @@ -771,6 +771,46 @@ test('gamepad controller trigger digital mode uses pressed state only', () => { assert.deepEqual(calls, ['play-audio', 'toggle-mpv-pause']); }); +test('gamepad controller digital trigger bindings ignore analog-only trigger values', () => { + const calls: string[] = []; + const buttons = Array.from({ length: 16 }, () => ({ value: 0, pressed: false, touched: false })); + buttons[6] = { value: 0.9, pressed: false, touched: true }; + buttons[7] = { value: 0.9, pressed: false, touched: true }; + + const controller = createGamepadController({ + getGamepads: () => [createGamepad('pad-1', { buttons })], + getConfig: () => + createControllerConfig({ + triggerInputMode: 'digital', + triggerDeadzone: 0.6, + bindings: { + playCurrentAudio: 'rightTrigger', + toggleMpvPause: 'leftTrigger', + }, + }), + getKeyboardModeEnabled: () => true, + getLookupWindowOpen: () => true, + getInteractionBlocked: () => false, + toggleKeyboardMode: () => {}, + toggleLookup: () => {}, + closeLookup: () => {}, + moveSelection: () => {}, + mineCard: () => {}, + quitMpv: () => {}, + previousAudio: () => {}, + nextAudio: () => {}, + playCurrentAudio: () => calls.push('play-audio'), + toggleMpvPause: () => calls.push('toggle-mpv-pause'), + scrollPopup: () => {}, + jumpPopup: () => {}, + onState: () => {}, + }); + + controller.poll(0); + + assert.deepEqual(calls, []); +}); + test('gamepad controller maps L3 to mpv pause and keeps unbound audio action inactive', () => { const calls: string[] = []; const buttons = Array.from({ length: 16 }, () => ({ value: 0, pressed: false, touched: false })); diff --git a/src/renderer/handlers/gamepad-controller.ts b/src/renderer/handlers/gamepad-controller.ts index 39a3ccfe..3af32a8e 100644 --- a/src/renderer/handlers/gamepad-controller.ts +++ b/src/renderer/handlers/gamepad-controller.ts @@ -69,6 +69,20 @@ function normalizeRawButtonState( return Boolean(button.pressed) || button.value >= triggerDeadzone; } +function resolveTriggerBindingPressed( + button: ControllerButtonState | undefined, + config: ResolvedControllerConfig, +): boolean { + if (!button) return false; + if (config.triggerInputMode === 'digital') { + return Boolean(button.pressed); + } + if (config.triggerInputMode === 'analog') { + return button.value >= config.triggerDeadzone; + } + return normalizeRawButtonState(button, config.triggerDeadzone); +} + function resolveGamepadAxis(gamepad: GamepadLike, axisIndex: number): number { const value = gamepad.axes[axisIndex]; return typeof value === 'number' && Number.isFinite(value) ? value : 0; @@ -190,7 +204,13 @@ function resolveDiscreteBindingPressed( } if (binding.kind === 'button') { - return normalizeRawButtonState(gamepad.buttons[binding.buttonIndex], config.triggerDeadzone); + const button = gamepad.buttons[binding.buttonIndex]; + const isTriggerBinding = + binding.buttonIndex === config.buttonIndices.leftTrigger || + binding.buttonIndex === config.buttonIndices.rightTrigger; + return isTriggerBinding + ? resolveTriggerBindingPressed(button, config) + : normalizeRawButtonState(button, config.triggerDeadzone); } const activationThreshold = Math.max(config.stickDeadzone, 0.55); diff --git a/src/renderer/modals/controller-select.test.ts b/src/renderer/modals/controller-select.test.ts index cf2e54dc..7f1a6fc8 100644 --- a/src/renderer/modals/controller-select.test.ts +++ b/src/renderer/modals/controller-select.test.ts @@ -142,6 +142,10 @@ function buildContext() { return { state, dom }; } +function findActionRow(container: ReturnType, labelText: string) { + return container.children.find((child) => child.children?.[0]?.textContent === labelText) ?? null; +} + test('controller select modal saves preferred controller from dropdown selection', async () => { const domHandle = installFakeDom(); const saved: unknown[] = []; @@ -166,6 +170,7 @@ test('controller select modal saves preferred controller from dropdown selection syncSettingsModalSubtitleSuppression: () => {}, }); + modal.wireDomEvents(); modal.openControllerSelectModal(); state.controllerDeviceSelectedIndex = 1; @@ -210,6 +215,7 @@ test('controller select modal learn mode captures fresh button input and persist syncSettingsModalSubtitleSuppression: () => {}, }); + modal.wireDomEvents(); modal.openControllerSelectModal(); const firstRow = dom.controllerConfigList.children[1]; @@ -239,3 +245,101 @@ test('controller select modal learn mode captures fresh button input and persist domHandle.restore(); } }); + +test('controller select modal preserves saved axis dpad fallback while relearning', async () => { + const domHandle = installFakeDom(); + const saved: unknown[] = []; + + Object.defineProperty(globalThis, 'window', { + configurable: true, + value: { + focus: () => {}, + electronAPI: { + saveControllerConfig: async (update: unknown) => { + saved.push(update); + }, + notifyOverlayModalClosed: () => {}, + }, + }, + }); + + try { + const { state, dom } = buildContext(); + state.controllerConfig!.bindings.leftStickHorizontal = { + kind: 'axis', + axisIndex: 0, + dpadFallback: 'none', + }; + + const modal = createControllerSelectModal({ state, dom } as never, { + modalStateReader: { isAnyModalOpen: () => false }, + syncSettingsModalSubtitleSuppression: () => {}, + }); + + modal.openControllerSelectModal(); + + const tokenMoveRow = findActionRow(dom.controllerConfigList, 'Token Move'); + assert.ok(tokenMoveRow); + const learnButton = tokenMoveRow.children[2].children[0]; + learnButton.dispatch('click'); + + state.controllerRawAxes = [0, 0, 0.85]; + modal.updateDevices(); + + await Promise.resolve(); + + assert.deepEqual(saved.at(-1), { + bindings: { + leftStickHorizontal: { + kind: 'axis', + axisIndex: 2, + dpadFallback: 'none', + }, + }, + }); + } finally { + domHandle.restore(); + } +}); + +test('controller select modal uses unique picker values for duplicate controller ids', async () => { + const domHandle = installFakeDom(); + + Object.defineProperty(globalThis, 'window', { + configurable: true, + value: { + focus: () => {}, + electronAPI: { + saveControllerConfig: async () => {}, + notifyOverlayModalClosed: () => {}, + }, + }, + }); + + try { + const { state, dom } = buildContext(); + state.connectedGamepads = [ + { id: 'same-pad', index: 0, mapping: 'standard', connected: true }, + { id: 'same-pad', index: 1, mapping: 'standard', connected: true }, + ]; + state.activeGamepadId = 'same-pad'; + + const modal = createControllerSelectModal({ state, dom } as never, { + modalStateReader: { isAnyModalOpen: () => false }, + syncSettingsModalSubtitleSuppression: () => {}, + }); + + modal.wireDomEvents(); + modal.openControllerSelectModal(); + + const [firstOption, secondOption] = dom.controllerSelectPicker.children; + assert.notEqual(firstOption.value, secondOption.value); + + dom.controllerSelectPicker.value = secondOption.value; + dom.controllerSelectPicker.dispatch('change'); + + assert.equal(state.controllerDeviceSelectedIndex, 1); + } finally { + domHandle.restore(); + } +}); diff --git a/src/renderer/modals/controller-select.ts b/src/renderer/modals/controller-select.ts index 5f43b143..d0a1f299 100644 --- a/src/renderer/modals/controller-select.ts +++ b/src/renderer/modals/controller-select.ts @@ -25,7 +25,7 @@ export function createControllerSelectModal( syncSettingsModalSubtitleSuppression: () => void; }, ) { - let selectedControllerId: string | null = null; + let selectedControllerKey: string | null = null; let lastRenderedDevicesKey = ''; let lastRenderedActiveGamepadId: string | null = null; let lastRenderedPreferredId = ''; @@ -58,6 +58,7 @@ export function createControllerSelectModal( const definition = getControllerBindingDefinition(actionId); if (!definition) return; const config = ctx.state.controllerConfig; + const currentBinding = config?.bindings[actionId]; bindingCapture = createControllerBindingCapture({ triggerDeadzone: config?.triggerDeadzone ?? 0.5, stickDeadzone: config?.stickDeadzone ?? 0.2, @@ -68,8 +69,10 @@ export function createControllerSelectModal( actionId, bindingType: 'axis', dpadFallback: - definition.defaultBinding.kind === 'axis' && - 'dpadFallback' in definition.defaultBinding + currentBinding?.kind === 'axis' && 'dpadFallback' in currentBinding + ? currentBinding.dpadFallback + : definition.defaultBinding.kind === 'axis' && + 'dpadFallback' in definition.defaultBinding ? definition.defaultBinding.dpadFallback : 'none', } @@ -100,9 +103,13 @@ export function createControllerSelectModal( .join('||'); } + function getDeviceSelectionKey(device: { id: string; index: number }): string { + return `${device.id}:${device.index}`; + } + function syncSelectedControllerId(): void { const selected = ctx.state.connectedGamepads[ctx.state.controllerDeviceSelectedIndex]; - selectedControllerId = selected?.id ?? null; + selectedControllerKey = selected ? getDeviceSelectionKey(selected) : null; } function syncSelectedIndexToCurrentController(): void { @@ -139,7 +146,7 @@ export function createControllerSelectModal( const preferredId = ctx.state.controllerConfig?.preferredGamepadId ?? ''; ctx.state.connectedGamepads.forEach((device, index) => { const option = document.createElement('option'); - option.value = device.id; + option.value = getDeviceSelectionKey(device); option.selected = index === ctx.state.controllerDeviceSelectedIndex; option.textContent = `${device.id || `Gamepad ${device.index}`} (${[ `#${device.index}`, @@ -226,9 +233,9 @@ export function createControllerSelectModal( function updateDevices(): void { if (!ctx.state.controllerSelectModalOpen) return; - if (selectedControllerId) { + if (selectedControllerKey) { const preservedIndex = ctx.state.connectedGamepads.findIndex( - (device) => device.id === selectedControllerId, + (device) => getDeviceSelectionKey(device) === selectedControllerKey, ); if (preservedIndex >= 0) { ctx.state.controllerDeviceSelectedIndex = preservedIndex; @@ -353,8 +360,10 @@ export function createControllerSelectModal( void saveSelectedController(); }); ctx.dom.controllerSelectPicker.addEventListener('change', () => { - const selectedId = ctx.dom.controllerSelectPicker.value; - const selectedIndex = ctx.state.connectedGamepads.findIndex((device) => device.id === selectedId); + const selectedKey = ctx.dom.controllerSelectPicker.value; + const selectedIndex = ctx.state.connectedGamepads.findIndex( + (device) => getDeviceSelectionKey(device) === selectedKey, + ); if (selectedIndex >= 0) { ctx.state.controllerDeviceSelectedIndex = selectedIndex; syncSelectedControllerId();