diff --git a/backlog/tasks/task-340 - Restore-default-replay-and-next-subtitle-overlay-keybindings.md b/backlog/tasks/task-340 - Restore-default-replay-and-next-subtitle-overlay-keybindings.md new file mode 100644 index 00000000..de6ab702 --- /dev/null +++ b/backlog/tasks/task-340 - Restore-default-replay-and-next-subtitle-overlay-keybindings.md @@ -0,0 +1,65 @@ +--- +id: TASK-340 +title: Restore default replay and next subtitle overlay keybindings +status: Done +assignee: + - Codex +created_date: '2026-05-04 06:25' +updated_date: '2026-05-04 06:49' +labels: + - bug + - keybindings + - overlay + - mpv +dependencies: [] +priority: high +--- + +## Description + + +Default overlay/mpv keybindings for replaying the current subtitle line and playing the next subtitle line are not firing. Shift+H and Shift+L subtitle jumps still work, but Ctrl+Shift+H should replay the current subtitle and pause at subtitle end, and Ctrl+Shift+L should play the next subtitle and pause at subtitle end. Keep the other built-in defaults working. + + +## Acceptance Criteria + +- [x] #1 Default keybindings include working replay-current-subtitle and play-next-subtitle bindings on Ctrl+Shift+H and Ctrl+Shift+L. +- [x] #2 Replay-current-subtitle dispatch reaches the existing runtime path that pauses at the subtitle end. +- [x] #3 Play-next-subtitle dispatch reaches the existing runtime path that pauses at the subtitle end. +- [x] #4 Existing default keybindings continue to compile/register without regressions. +- [x] #5 Focused regression tests cover the broken default bindings. + + +## Implementation Plan + + +1. Add focused regression coverage that the resolved defaults compile on Linux without dropping Ctrl+Shift+H/L, and that those keys map to replayCurrentSubtitle/playNextSubtitle session actions. +2. Move the default session-help shortcut off Ctrl/Cmd+Shift+H to a non-conflicting shortcut, then update generated/default config docs so shipped defaults match documentation. +3. Add/adjust coverage for default replay/next bindings and run targeted Bun tests plus plugin session-binding smoke. + +4. Follow-up after live test: fix the mpv plugin shifted-letter key-name conversion so `Ctrl+Shift+KeyL` registers using mpv's uppercase letter form and add Lua regression coverage for both `Ctrl+Shift+L` and `Shift+L`. + + +## Implementation Notes + + +Root cause: default `shortcuts.openSessionHelp = CommandOrControl+Shift+H` canonicalized to `ctrl+shift+KeyH` on Linux/Windows, conflicting with the built-in replay-current-subtitle keybinding. The session-binding compiler drops conflicted bindings, so replay did not register. Moved default session help to `CommandOrControl+Slash` and added regression coverage that defaults compile without a conflict and keep replay/next actions on `Ctrl+Shift+H/L`. + +Follow-up from live test: `Ctrl+Shift+H` works after resolving the help shortcut conflict, but `Ctrl+Shift+L` still behaves like native/other `Ctrl+L`. Investigating mpv/plugin key-name generation for shifted letter chords. + +Follow-up fix: mpv normalizes shifted letter chords to uppercase letter key names (for example `Ctrl+Shift+l` becomes `Ctrl+L`). The plugin previously emitted `Ctrl+Shift+l`, which let live `Ctrl+Shift+L` fall through as the `Ctrl+L` key path. `plugin/subminer/session_bindings.lua` now emits uppercase letters and omits the Shift modifier for shifted `Key[A-Z]` bindings. Lua regression coverage now checks `Ctrl+Shift+KeyL -> Ctrl+L`, `Shift+KeyL -> L`, and the play-next CLI dispatch. + +Second live follow-up: `Ctrl+Shift+L` routed to play-next but still behaved like `Shift+L` when playback was already paused because `MpvIpcClient.playNextSubtitle()` explicitly cleared `pendingPauseAtSubEnd` and only sent `sub-seek 1` in paused state. Changed play-next to always arm pause-at-sub-end, clear stale pause target, seek to next subtitle, and unpause when currently paused. Existing sub-end/time-pos handling then pauses at the next subtitle end. + + +## Final Summary + + +Changed the default session-help shortcut from `CommandOrControl+Shift+H` to `CommandOrControl+Slash` so `Ctrl+Shift+H` remains available for replay-current-subtitle and `Ctrl+Shift+L` remains available for play-next-subtitle. Updated config examples, docs-site shortcut/config/usage docs, and added changelog fragment `changes/340-default-subtitle-keybindings.md`. + +Fixed both follow-up issues from live testing. First, the mpv plugin key-name converter now uses mpv's uppercase key form for shifted letter bindings (`Ctrl+Shift+KeyL` registers as `Ctrl+L`, `Shift+KeyL` as `L`). Second, `MpvIpcClient.playNextSubtitle()` now starts playback even when mpv is paused, keeps the pause-at-sub-end path armed, and lets existing subtitle-end timing pause again at the next subtitle end. + +Regression coverage now includes compiled default bindings, Lua plugin shifted-letter registration/CLI dispatch, and paused-state play-next behavior. + +Verification passed: targeted Bun session/mpv/protocol tests, `bun run test:plugin:src`, `bun run changelog:lint`, `bun run build`, and `bun run test:smoke:dist`. Earlier full gate also passed before the follow-ups: `bun run typecheck`, `bun run test:fast`, `bun run test:env`, docs/config checks, and dist smoke. + diff --git a/changes/340-default-subtitle-keybindings.md b/changes/340-default-subtitle-keybindings.md new file mode 100644 index 00000000..c5d71b55 --- /dev/null +++ b/changes/340-default-subtitle-keybindings.md @@ -0,0 +1,4 @@ +type: fixed +area: overlay + +- Overlay: Fixed the default replay/next subtitle keybindings by moving the session-help shortcut to `Ctrl/Cmd+/`, leaving `Ctrl+Shift+H` and `Ctrl+Shift+L` free for subtitle playback controls. The mpv plugin now registers shifted letter chords with mpv's uppercase key form so `Ctrl+Shift+L` reaches the play-next-subtitle action instead of falling through as `Ctrl+L`, and play-next now starts playback from a paused state before pausing again at the subtitle end. diff --git a/config.example.jsonc b/config.example.jsonc index 1b3470ed..d734c08a 100644 --- a/config.example.jsonc +++ b/config.example.jsonc @@ -175,7 +175,7 @@ "openCharacterDictionary": "CommandOrControl+Alt+A", // Open character dictionary setting. "openRuntimeOptions": "CommandOrControl+Shift+O", // Open runtime options setting. "openJimaku": "Ctrl+Shift+J", // Open jimaku setting. - "openSessionHelp": "CommandOrControl+Shift+H", // Open session help setting. + "openSessionHelp": "CommandOrControl+Slash", // Open session help setting. "openControllerSelect": "Alt+C", // Open controller select setting. "openControllerDebug": "Alt+Shift+C", // Open controller debug setting. "toggleSubtitleSidebar": "Backslash" // Toggle subtitle sidebar setting. diff --git a/docs-site/configuration.md b/docs-site/configuration.md index 8acab511..5654bfbe 100644 --- a/docs-site/configuration.md +++ b/docs-site/configuration.md @@ -537,7 +537,7 @@ See `config.example.jsonc` for detailed configuration options. "markAudioCard": "CommandOrControl+Shift+A", "openCharacterDictionary": "CommandOrControl+Alt+A", "openRuntimeOptions": "CommandOrControl+Shift+O", - "openSessionHelp": "CommandOrControl+Shift+H", + "openSessionHelp": "CommandOrControl+Slash", "openControllerSelect": "Alt+C", "openControllerDebug": "Alt+Shift+C", "openJimaku": "Ctrl+Shift+J", @@ -562,7 +562,7 @@ See `config.example.jsonc` for detailed configuration options. | `markAudioCard` | string \| `null` | Accelerator for marking last card as audio card (default: `"CommandOrControl+Shift+A"`) | | `openCharacterDictionary` | string \| `null` | Opens the character dictionary AniList selector (default: `"CommandOrControl+Alt+A"`) | | `openRuntimeOptions` | string \| `null` | Opens runtime options palette for live session-only toggles (default: `"CommandOrControl+Shift+O"`) | -| `openSessionHelp` | string \| `null` | Opens the in-overlay session help modal (default: `"CommandOrControl+Shift+H"`) | +| `openSessionHelp` | string \| `null` | Opens the in-overlay session help modal (default: `"CommandOrControl+Slash"`) | | `openControllerSelect` | string \| `null` | Opens the controller config/remap modal (default: `"Alt+C"`) | | `openControllerDebug` | string \| `null` | Opens the controller debug modal (default: `"Alt+Shift+C"`) | | `openJimaku` | string \| `null` | Opens the Jimaku search modal (default: `"Ctrl+Shift+J"`) | @@ -706,7 +706,7 @@ These shortcuts are only active when the overlay window is visible and automatic ### Session Help Modal -The session help modal opens from the overlay with `Ctrl/Cmd+Shift+H` by default. The mpv plugin also exposes it through the `Y-H` chord (falling back to `Y-K` if needed). It shows the current session keybindings and color legend. +The session help modal opens from the overlay with `Ctrl/Cmd+/` by default. The mpv plugin also exposes it through the `Y-H` chord (falling back to `Y-K` if needed). It shows the current session keybindings and color legend. You can filter the modal quickly with `/`: diff --git a/docs-site/public/config.example.jsonc b/docs-site/public/config.example.jsonc index 1b3470ed..d734c08a 100644 --- a/docs-site/public/config.example.jsonc +++ b/docs-site/public/config.example.jsonc @@ -175,7 +175,7 @@ "openCharacterDictionary": "CommandOrControl+Alt+A", // Open character dictionary setting. "openRuntimeOptions": "CommandOrControl+Shift+O", // Open runtime options setting. "openJimaku": "Ctrl+Shift+J", // Open jimaku setting. - "openSessionHelp": "CommandOrControl+Shift+H", // Open session help setting. + "openSessionHelp": "CommandOrControl+Slash", // Open session help setting. "openControllerSelect": "Alt+C", // Open controller select setting. "openControllerDebug": "Alt+Shift+C", // Open controller debug setting. "toggleSubtitleSidebar": "Backslash" // Toggle subtitle sidebar setting. diff --git a/docs-site/shortcuts.md b/docs-site/shortcuts.md index b37a0e8c..8b4e263d 100644 --- a/docs-site/shortcuts.md +++ b/docs-site/shortcuts.md @@ -69,7 +69,7 @@ Mouse-hover playback behavior is configured separately from shortcuts: `subtitle | `Ctrl/Cmd+Shift+V` | Cycle secondary subtitle mode (hidden → visible → hover) | `shortcuts.toggleSecondarySub` | | `Ctrl/Cmd+Alt+A` | Open character dictionary AniList selector | `shortcuts.openCharacterDictionary` | | `Ctrl/Cmd+Shift+O` | Open runtime options palette | `shortcuts.openRuntimeOptions` | -| `Ctrl/Cmd+Shift+H` | Open session help modal | `shortcuts.openSessionHelp` | +| `Ctrl/Cmd+/` | Open session help modal | `shortcuts.openSessionHelp` | | `Ctrl+Shift+J` | Open Jimaku subtitle search modal | `shortcuts.openJimaku` | | `Ctrl+Alt+C` | Open the manual YouTube subtitle picker | `keybindings` | | `Ctrl+Alt+S` | Open subtitle sync (subsync) modal | `shortcuts.triggerSubsync` | diff --git a/docs-site/usage.md b/docs-site/usage.md index 8d69c74a..947ab1e8 100644 --- a/docs-site/usage.md +++ b/docs-site/usage.md @@ -334,7 +334,7 @@ Useful overlay-local default keybinding: `Ctrl+Alt+P` opens the playlist browser Press `V` to hide or restore the primary SubMiner subtitle bar. The mpv plugin also binds bare `v` to the same action, overriding mpv's native primary subtitle visibility toggle. -`Ctrl/Cmd+Shift+H` opens the session help modal with the current overlay and mpv keybindings. If you use the mpv plugin, the same help view is also available through the `y-h` chord. +`Ctrl/Cmd+/` opens the session help modal with the current overlay and mpv keybindings. If you use the mpv plugin, the same help view is also available through the `y-h` chord. Hovering over subtitle text pauses mpv by default; leaving resumes it. Yomitan popups also pause playback by default. Set `subtitleStyle.autoPauseVideoOnHover: false` or `subtitleStyle.autoPauseVideoOnYomitanPopup: false` to disable either behavior. diff --git a/plugin/subminer/session_bindings.lua b/plugin/subminer/session_bindings.lua index ef30528f..523d93e9 100644 --- a/plugin/subminer/session_bindings.lua +++ b/plugin/subminer/session_bindings.lua @@ -96,16 +96,30 @@ function M.create(ctx) return nil end + local shifted_letter = key.code:match("^Key([A-Z])$") + local has_shift = false + for _, modifier in ipairs(key.modifiers) do + if modifier == "shift" then + has_shift = true + break + end + end + local key_name = key_code_to_mpv_name(key.code) + if shifted_letter and has_shift then + key_name = shifted_letter + end if not key_name then return nil end local parts = {} for _, modifier in ipairs(key.modifiers) do - local mapped = MODIFIER_MAP[modifier] - if mapped then - parts[#parts + 1] = mapped + if not (modifier == "shift" and shifted_letter) then + local mapped = MODIFIER_MAP[modifier] + if mapped then + parts[#parts + 1] = mapped + end end end parts[#parts + 1] = key_name diff --git a/scripts/test-plugin-session-bindings.lua b/scripts/test-plugin-session-bindings.lua index c2f8ea5d..5ca776e3 100644 --- a/scripts/test-plugin-session-bindings.lua +++ b/scripts/test-plugin-session-bindings.lua @@ -71,6 +71,22 @@ local ctx = { actionType = "session-action", actionId = "mineSentenceMultiple", }, + { + key = { + code = "KeyL", + modifiers = { "ctrl", "shift" }, + }, + actionType = "session-action", + actionId = "playNextSubtitle", + }, + { + key = { + code = "KeyL", + modifiers = { "shift" }, + }, + actionType = "mpv-command", + command = { "sub-seek", 1 }, + }, }, }, nil end, @@ -107,13 +123,36 @@ assert_true(bindings.register_bindings(), "session bindings should register") local starter = nil for _, binding in ipairs(recorded.bindings) do - if binding.keys == "Ctrl+Shift+s" then + if binding.keys == "Ctrl+S" then starter = binding break end end assert_true(starter ~= nil, "multi-mine starter binding should be registered") +local play_next = nil +for _, binding in ipairs(recorded.bindings) do + if binding.keys == "Ctrl+L" then + play_next = binding + break + end +end +assert_true(play_next ~= nil, "play-next subtitle binding should use mpv shifted-letter form") + +local subtitle_jump = nil +for _, binding in ipairs(recorded.bindings) do + if binding.keys == "L" then + subtitle_jump = binding + break + end +end +assert_true(subtitle_jump ~= nil, "shifted subtitle jump binding should use mpv uppercase letter form") + +play_next.fn() +local play_next_call = recorded.async_calls[#recorded.async_calls] +assert_true(play_next_call ~= nil, "play-next binding should invoke CLI action") +assert_true(play_next_call[2] == "--play-next-subtitle", "play-next binding should pass CLI flag") + starter.fn() local modified_digit = nil diff --git a/src/config/definitions/defaults-core.ts b/src/config/definitions/defaults-core.ts index b590cbd7..bcff4c9d 100644 --- a/src/config/definitions/defaults-core.ts +++ b/src/config/definitions/defaults-core.ts @@ -89,7 +89,7 @@ export const CORE_DEFAULT_CONFIG: Pick< openCharacterDictionary: 'CommandOrControl+Alt+A', openRuntimeOptions: 'CommandOrControl+Shift+O', openJimaku: 'Ctrl+Shift+J', - openSessionHelp: 'CommandOrControl+Shift+H', + openSessionHelp: 'CommandOrControl+Slash', openControllerSelect: 'Alt+C', openControllerDebug: 'Alt+Shift+C', toggleSubtitleSidebar: 'Backslash', diff --git a/src/config/definitions/domain-registry.test.ts b/src/config/definitions/domain-registry.test.ts index 8c85b57e..8a0f296f 100644 --- a/src/config/definitions/domain-registry.test.ts +++ b/src/config/definitions/domain-registry.test.ts @@ -92,3 +92,11 @@ test('default keybindings include fullscreen on F', () => { ); assert.deepEqual(keybindingMap.get('KeyF'), ['cycle', 'fullscreen']); }); + +test('default keybindings include replay and next subtitle controls', () => { + const keybindingMap = new Map( + DEFAULT_KEYBINDINGS.map((binding) => [binding.key, binding.command]), + ); + assert.deepEqual(keybindingMap.get('Ctrl+Shift+KeyH'), ['__replay-subtitle']); + assert.deepEqual(keybindingMap.get('Ctrl+Shift+KeyL'), ['__play-next-subtitle']); +}); diff --git a/src/core/services/mpv.test.ts b/src/core/services/mpv.test.ts index 1228f9fa..f318f5f9 100644 --- a/src/core/services/mpv.test.ts +++ b/src/core/services/mpv.test.ts @@ -489,7 +489,7 @@ test('MpvIpcClient updates current audio stream index from track list', async () assert.equal(client.currentAudioStreamIndex, 11); }); -test('MpvIpcClient playNextSubtitle preserves a manual paused state', async () => { +test('MpvIpcClient playNextSubtitle starts playback from paused state and auto-pauses at end', async () => { const commands: unknown[] = []; const client = new MpvIpcClient('/tmp/mpv.sock', makeDeps()); (client as any).send = (payload: unknown) => { @@ -507,9 +507,12 @@ test('MpvIpcClient playNextSubtitle preserves a manual paused state', async () = client.playNextSubtitle(); - assert.equal((client as any).pendingPauseAtSubEnd, false); + assert.equal((client as any).pendingPauseAtSubEnd, true); assert.equal((client as any).pauseAtTime, null); - assert.deepEqual(commands, [{ command: ['sub-seek', 1] }]); + assert.deepEqual(commands, [ + { command: ['sub-seek', 1] }, + { command: ['set_property', 'pause', false] }, + ]); }); test('MpvIpcClient playNextSubtitle still auto-pauses at end while already playing', async () => { diff --git a/src/core/services/mpv.ts b/src/core/services/mpv.ts index 99f023b4..25c1e73c 100644 --- a/src/core/services/mpv.ts +++ b/src/core/services/mpv.ts @@ -522,14 +522,12 @@ export class MpvIpcClient implements MpvClient { } playNextSubtitle(): void { - if (this.playbackPaused === true) { - this.pendingPauseAtSubEnd = false; - this.pauseAtTime = null; - this.send({ command: ['sub-seek', 1] }); - return; - } this.pendingPauseAtSubEnd = true; + this.pauseAtTime = null; this.send({ command: ['sub-seek', 1] }); + if (this.playbackPaused === true) { + this.send({ command: ['set_property', 'pause', false] }); + } } restorePreviousSecondarySubVisibility(): void { diff --git a/src/core/services/session-bindings.test.ts b/src/core/services/session-bindings.test.ts index 5cf95501..932e4843 100644 --- a/src/core/services/session-bindings.test.ts +++ b/src/core/services/session-bindings.test.ts @@ -2,7 +2,8 @@ import assert from 'node:assert/strict'; import test from 'node:test'; import type { Keybinding } from '../../types'; import type { ConfiguredShortcuts } from '../utils/shortcut-config'; -import { SPECIAL_COMMANDS } from '../../config/definitions'; +import { DEFAULT_CONFIG, DEFAULT_KEYBINDINGS, SPECIAL_COMMANDS } from '../../config/definitions'; +import { resolveConfiguredShortcuts } from '../utils/shortcut-config'; import { compileSessionBindings } from './session-bindings'; function createShortcuts(overrides: Partial = {}): ConfiguredShortcuts { @@ -179,6 +180,35 @@ test('compileSessionBindings drops conflicting bindings that canonicalize to the ]); }); +test('compileSessionBindings keeps default replay and next subtitle session actions on Linux', () => { + const result = compileSessionBindings({ + shortcuts: resolveConfiguredShortcuts(DEFAULT_CONFIG, DEFAULT_CONFIG), + keybindings: DEFAULT_KEYBINDINGS, + statsToggleKey: DEFAULT_CONFIG.stats.toggleKey, + platform: 'linux', + rawConfig: DEFAULT_CONFIG, + }); + + assert.deepEqual( + result.warnings.filter((warning) => warning.kind === 'conflict'), + [], + ); + const bySignature = new Map( + result.bindings.map((binding) => [ + `${binding.key.modifiers.join('+')}+${binding.key.code}`, + binding, + ]), + ); + + const replay = bySignature.get('ctrl+shift+KeyH'); + assert.equal(replay?.actionType, 'session-action'); + assert.equal(replay?.actionId, 'replayCurrentSubtitle'); + + const next = bySignature.get('ctrl+shift+KeyL'); + assert.equal(next?.actionType, 'session-action'); + assert.equal(next?.actionId, 'playNextSubtitle'); +}); + test('compileSessionBindings omits disabled bindings', () => { const result = compileSessionBindings({ shortcuts: createShortcuts({