From a03388a38fbf7c5ba154eb97e85b04d79b118b4f Mon Sep 17 00:00:00 2001 From: sudacode Date: Thu, 26 Feb 2026 18:47:51 -0800 Subject: [PATCH] fix: address claude review feedback on overlay refactor --- src/config/definitions/options-core.ts | 2 +- src/core/services/ipc.ts | 4 -- src/core/services/mpv-protocol.test.ts | 18 +---- src/core/services/mpv-protocol.ts | 10 +-- src/core/services/mpv.ts | 2 +- src/core/services/overlay-visibility.ts | 6 +- src/main.ts | 29 ++------ src/main/overlay-runtime.test.ts | 25 ++++++- src/main/overlay-runtime.ts | 15 ++--- .../runtime/app-lifecycle-actions.test.ts | 2 +- src/main/runtime/app-lifecycle-actions.ts | 4 +- .../app-lifecycle-main-cleanup.test.ts | 4 +- .../runtime/app-lifecycle-main-cleanup.ts | 6 +- .../composers/mpv-runtime-composer.test.ts | 2 +- .../startup-lifecycle-composer.test.ts | 2 +- .../runtime/mpv-main-event-actions.test.ts | 2 +- src/main/runtime/mpv-main-event-actions.ts | 4 +- .../runtime/mpv-main-event-bindings.test.ts | 2 +- src/main/runtime/mpv-main-event-bindings.ts | 6 +- .../runtime/mpv-main-event-main-deps.test.ts | 4 +- src/main/runtime/mpv-main-event-main-deps.ts | 6 +- .../overlay-mpv-sub-visibility.test.ts | 42 +----------- .../runtime/overlay-mpv-sub-visibility.ts | 66 ++++--------------- src/main/state.ts | 2 - src/preload.ts | 2 +- src/renderer/subtitle-render.test.ts | 10 +-- src/renderer/subtitle-render.ts | 14 ++-- src/shared/ipc/contracts.ts | 1 - 28 files changed, 95 insertions(+), 197 deletions(-) diff --git a/src/config/definitions/options-core.ts b/src/config/definitions/options-core.ts index a2e44a1..ce97521 100644 --- a/src/config/definitions/options-core.ts +++ b/src/config/definitions/options-core.ts @@ -43,7 +43,7 @@ export function buildCoreConfigOptionRegistry( kind: 'boolean', defaultValue: defaultConfig.bind_visible_overlay_to_mpv_sub_visibility, description: - 'Link visible overlay toggles to MPV subtitle visibility (primary and secondary).', + 'Link visible overlay toggles to MPV primary subtitle visibility.', }, ]; } diff --git a/src/core/services/ipc.ts b/src/core/services/ipc.ts index 76a8b89..81f7e07 100644 --- a/src/core/services/ipc.ts +++ b/src/core/services/ipc.ts @@ -212,10 +212,6 @@ export function registerIpcHandlers(deps: IpcServiceDeps, ipc: IpcMainRegistrar deps.toggleDevTools(); }); - ipc.handle(IPC_CHANNELS.request.getOverlayVisibility, () => { - return deps.getVisibleOverlayVisibility(); - }); - ipc.on(IPC_CHANNELS.command.toggleOverlay, () => { deps.toggleVisibleOverlay(); }); diff --git a/src/core/services/mpv-protocol.test.ts b/src/core/services/mpv-protocol.test.ts index 38591e3..787f19e 100644 --- a/src/core/services/mpv-protocol.test.ts +++ b/src/core/services/mpv-protocol.test.ts @@ -131,23 +131,7 @@ test('dispatchMpvProtocolMessage enforces sub-visibility hidden when overlay sup ); assert.deepEqual(state.commands.pop(), { - command: ['set_property', 'sub-visibility', 'no'], - }); -}); - -test('dispatchMpvProtocolMessage enforces secondary sub-visibility hidden when overlay suppression is enabled', async () => { - const { deps, state } = createDeps({ - shouldBindVisibleOverlayToMpvSubVisibility: () => true, - isVisibleOverlayVisible: () => true, - }); - - await dispatchMpvProtocolMessage( - { event: 'property-change', name: 'secondary-sub-visibility', data: 'yes' }, - deps, - ); - - assert.deepEqual(state.commands.pop(), { - command: ['set_property', 'secondary-sub-visibility', 'no'], + command: ['set_property', 'sub-visibility', false], }); }); diff --git a/src/core/services/mpv-protocol.ts b/src/core/services/mpv-protocol.ts index 43c08c2..919b2ee 100644 --- a/src/core/services/mpv-protocol.ts +++ b/src/core/services/mpv-protocol.ts @@ -223,15 +223,7 @@ export async function dispatchMpvProtocolMessage( deps.isVisibleOverlayVisible() && asBoolean(msg.data, false) ) { - deps.sendCommand({ command: ['set_property', 'sub-visibility', 'no'] }); - } - } else if (msg.name === 'secondary-sub-visibility') { - if ( - deps.shouldBindVisibleOverlayToMpvSubVisibility?.() && - deps.isVisibleOverlayVisible() && - asBoolean(msg.data, false) - ) { - deps.sendCommand({ command: ['set_property', 'secondary-sub-visibility', 'no'] }); + deps.sendCommand({ command: ['set_property', 'sub-visibility', false] }); } } else if (msg.name === 'sub-use-margins') { deps.emitSubtitleMetricsChange({ diff --git a/src/core/services/mpv.ts b/src/core/services/mpv.ts index f48a3d8..ec6ebc0 100644 --- a/src/core/services/mpv.ts +++ b/src/core/services/mpv.ts @@ -474,7 +474,7 @@ export class MpvIpcClient implements MpvClient { setSubVisibility(visible: boolean): void { this.send({ - command: ['set_property', 'sub-visibility', visible ? 'yes' : 'no'], + command: ['set_property', 'sub-visibility', visible], }); } diff --git a/src/core/services/overlay-visibility.ts b/src/core/services/overlay-visibility.ts index 96cea34..9bd2e8b 100644 --- a/src/core/services/overlay-visibility.ts +++ b/src/core/services/overlay-visibility.ts @@ -15,7 +15,7 @@ export function updateVisibleOverlayVisibility(args: { syncOverlayShortcuts: () => void; isMacOSPlatform?: boolean; showOverlayLoadingOsd?: (message: string) => void; - resolveFallbackBounds: () => WindowGeometry; + resolveFallbackBounds?: () => WindowGeometry; }): void { if (!args.mainWindow || args.mainWindow.isDestroyed()) { return; @@ -78,7 +78,9 @@ export function updateVisibleOverlayVisibility(args: { return; } - const fallbackBounds = args.resolveFallbackBounds(); + const fallbackBounds = args.resolveFallbackBounds?.(); + if (!fallbackBounds) return; + args.updateVisibleOverlayBounds(fallbackBounds); args.syncPrimaryOverlayWindowLayer('visible'); args.mainWindow.setIgnoreMouseEvents(false); diff --git a/src/main.ts b/src/main.ts index 05f703f..6cdc734 100644 --- a/src/main.ts +++ b/src/main.ts @@ -354,7 +354,6 @@ import { runStartupBootstrapRuntime, saveSubtitlePosition as saveSubtitlePositionCore, sendMpvCommandRuntime, - setMpvSecondarySubVisibilityRuntime, setMpvSubVisibilityRuntime, setOverlayDebugVisualizationEnabledRuntime, syncOverlayWindowLayer, @@ -731,10 +730,6 @@ const ensureOverlayMpvSubtitlesHidden = createEnsureOverlayMpvSubtitlesHiddenHan setSavedSubVisibility: (visible) => { appState.overlaySavedMpvSubVisibility = visible; }, - getSavedSecondarySubVisibility: () => appState.overlaySavedSecondaryMpvSubVisibility, - setSavedSecondarySubVisibility: (visible) => { - appState.overlaySavedSecondaryMpvSubVisibility = visible; - }, getRevision: () => appState.overlayMpvSubVisibilityRevision, setRevision: (revision) => { appState.overlayMpvSubVisibilityRevision = revision; @@ -742,9 +737,6 @@ const ensureOverlayMpvSubtitlesHidden = createEnsureOverlayMpvSubtitlesHiddenHan setMpvSubVisibility: (visible) => { setMpvSubVisibilityRuntime(appState.mpvClient, visible); }, - setMpvSecondarySubVisibility: (visible) => { - setMpvSecondarySubVisibilityRuntime(appState.mpvClient, visible); - }, logWarn: (message, error) => { logger.warn(message, error); }, @@ -754,10 +746,6 @@ const restoreOverlayMpvSubtitles = createRestoreOverlayMpvSubtitlesHandler({ setSavedSubVisibility: (visible) => { appState.overlaySavedMpvSubVisibility = visible; }, - getSavedSecondarySubVisibility: () => appState.overlaySavedSecondaryMpvSubVisibility, - setSavedSecondarySubVisibility: (visible) => { - appState.overlaySavedSecondaryMpvSubVisibility = visible; - }, getRevision: () => appState.overlayMpvSubVisibilityRevision, setRevision: (revision) => { appState.overlayMpvSubVisibilityRevision = revision; @@ -767,16 +755,12 @@ const restoreOverlayMpvSubtitles = createRestoreOverlayMpvSubtitlesHandler({ setMpvSubVisibility: (visible) => { setMpvSubVisibilityRuntime(appState.mpvClient, visible); }, - setMpvSecondarySubVisibility: (visible) => { - setMpvSecondarySubVisibilityRuntime(appState.mpvClient, visible); - }, }); function shouldSuppressMpvSubtitlesForOverlay(): boolean { return ( - appState.secondarySubMode === 'visible' || - (overlayManager.getVisibleOverlayVisible() && - configDerivedRuntime.shouldBindVisibleOverlayToMpvSubVisibility()) + overlayManager.getVisibleOverlayVisible() && + configDerivedRuntime.shouldBindVisibleOverlayToMpvSubVisibility() ); } @@ -1884,8 +1868,8 @@ const { destroyTray: () => destroyTray(), stopConfigHotReload: () => configHotReloadRuntime.stop(), restorePreviousSecondarySubVisibility: () => restorePreviousSecondarySubVisibility(), - restoreMpvSubVisibilityForInvisibleOverlay: () => { - restoreOverlayMpvSubtitles({ respectVisibleOverlayBinding: false }); + restoreMpvSubVisibility: () => { + restoreOverlayMpvSubtitles(); }, unregisterAllGlobalShortcuts: () => globalShortcut.unregisterAll(), stopSubtitleWebsocket: () => subtitleWsService.stop(), @@ -2181,8 +2165,8 @@ const { updateCurrentMediaPath: (path) => { mediaRuntime.updateCurrentMediaPath(path); }, - restoreMpvSubVisibilityForInvisibleOverlay: () => { - restoreOverlayMpvSubtitles({ respectVisibleOverlayBinding: false }); + restoreMpvSubVisibility: () => { + restoreOverlayMpvSubtitles(); }, getCurrentAnilistMediaKey: () => getCurrentAnilistMediaKey(), resetAnilistMediaTracking: (mediaKey) => { @@ -2529,7 +2513,6 @@ const cycleSecondarySubMode = createCycleSecondarySubModeRuntimeHandler({ function setSecondarySubMode(mode: SecondarySubMode): void { appState.secondarySubMode = mode; - syncOverlayMpvSubtitleSuppression(); } function handleCycleSecondarySubMode(): void { diff --git a/src/main/overlay-runtime.test.ts b/src/main/overlay-runtime.test.ts index 43b775f..a65a51e 100644 --- a/src/main/overlay-runtime.test.ts +++ b/src/main/overlay-runtime.test.ts @@ -50,7 +50,7 @@ function createMockWindow(): MockWindow & { url: 'file:///overlay/index.html?layer=modal', loadCallbacks: [], }; - return { + const window = { ...state, isDestroyed: () => state.destroyed, isVisible: () => state.visible, @@ -92,6 +92,29 @@ function createMockWindow(): MockWindow & { }, }, }; + + Object.defineProperty(window, 'loading', { + get: () => state.loading, + set: (value: boolean) => { + state.loading = value; + }, + }); + + Object.defineProperty(window, 'url', { + get: () => state.url, + set: (value: string) => { + state.url = value; + }, + }); + + Object.defineProperty(window, 'ignoreMouseEvents', { + get: () => state.ignoreMouseEvents, + set: (value: boolean) => { + state.ignoreMouseEvents = value; + }, + }); + + return window; } test('sendToActiveOverlayWindow targets modal window with full geometry and tracks close restore', () => { diff --git a/src/main/overlay-runtime.ts b/src/main/overlay-runtime.ts index f788034..e33e75e 100644 --- a/src/main/overlay-runtime.ts +++ b/src/main/overlay-runtime.ts @@ -1,6 +1,8 @@ import type { BrowserWindow } from 'electron'; import type { WindowGeometry } from '../types'; +const MODAL_REVEAL_FALLBACK_DELAY_MS = 250; + type OverlayHostedModal = 'runtime-options' | 'subsync' | 'jimaku' | 'kiku'; export interface OverlayWindowResolver { @@ -67,14 +69,7 @@ export function createOverlayModalRuntimeService( if (window.webContents.isLoading()) { return false; } - - const getURL = window.webContents.getURL; - if (typeof getURL !== 'function') { - return true; - } - - const currentURL = getURL.call(window.webContents); - return currentURL !== '' && currentURL !== 'about:blank'; + return window.webContents.getURL() !== '' && window.webContents.getURL() !== 'about:blank'; }; const sendOrQueueForWindow = ( @@ -142,7 +137,7 @@ export function createOverlayModalRuntimeService( return; } showModalWindow(targetWindow, { passThroughMouseEvents: true }); - }, 250); + }, MODAL_REVEAL_FALLBACK_DELAY_MS); }; const sendToActiveOverlayWindow = ( @@ -208,8 +203,6 @@ export function createOverlayModalRuntimeService( if (restoreVisibleOverlayOnModalClose.size === 0) { clearPendingModalWindowReveal(); notifyModalStateChange(false); - } - if (restoreVisibleOverlayOnModalClose.size === 0) { modalWindow.hide(); } }; diff --git a/src/main/runtime/app-lifecycle-actions.test.ts b/src/main/runtime/app-lifecycle-actions.test.ts index a9da703..4c92450 100644 --- a/src/main/runtime/app-lifecycle-actions.test.ts +++ b/src/main/runtime/app-lifecycle-actions.test.ts @@ -12,7 +12,7 @@ test('on will quit cleanup handler runs all cleanup steps', () => { destroyTray: () => calls.push('destroy-tray'), stopConfigHotReload: () => calls.push('stop-config'), restorePreviousSecondarySubVisibility: () => calls.push('restore-sub'), - restoreMpvSubVisibilityForInvisibleOverlay: () => calls.push('restore-mpv-sub'), + restoreMpvSubVisibility: () => calls.push('restore-mpv-sub'), unregisterAllGlobalShortcuts: () => calls.push('unregister-shortcuts'), stopSubtitleWebsocket: () => calls.push('stop-ws'), stopTexthookerService: () => calls.push('stop-texthooker'), diff --git a/src/main/runtime/app-lifecycle-actions.ts b/src/main/runtime/app-lifecycle-actions.ts index aca18e0..28fbe8e 100644 --- a/src/main/runtime/app-lifecycle-actions.ts +++ b/src/main/runtime/app-lifecycle-actions.ts @@ -2,7 +2,7 @@ export function createOnWillQuitCleanupHandler(deps: { destroyTray: () => void; stopConfigHotReload: () => void; restorePreviousSecondarySubVisibility: () => void; - restoreMpvSubVisibilityForInvisibleOverlay: () => void; + restoreMpvSubVisibility: () => void; unregisterAllGlobalShortcuts: () => void; stopSubtitleWebsocket: () => void; stopTexthookerService: () => void; @@ -26,7 +26,7 @@ export function createOnWillQuitCleanupHandler(deps: { deps.destroyTray(); deps.stopConfigHotReload(); deps.restorePreviousSecondarySubVisibility(); - deps.restoreMpvSubVisibilityForInvisibleOverlay(); + deps.restoreMpvSubVisibility(); deps.unregisterAllGlobalShortcuts(); deps.stopSubtitleWebsocket(); deps.stopTexthookerService(); diff --git a/src/main/runtime/app-lifecycle-main-cleanup.test.ts b/src/main/runtime/app-lifecycle-main-cleanup.test.ts index ccfb6a8..bc3702d 100644 --- a/src/main/runtime/app-lifecycle-main-cleanup.test.ts +++ b/src/main/runtime/app-lifecycle-main-cleanup.test.ts @@ -14,7 +14,7 @@ test('cleanup deps builder returns handlers that guard optional runtime objects' destroyTray: () => calls.push('destroy-tray'), stopConfigHotReload: () => calls.push('stop-config'), restorePreviousSecondarySubVisibility: () => calls.push('restore-sub'), - restoreMpvSubVisibilityForInvisibleOverlay: () => calls.push('restore-mpv-sub'), + restoreMpvSubVisibility: () => calls.push('restore-mpv-sub'), unregisterAllGlobalShortcuts: () => calls.push('unregister-shortcuts'), stopSubtitleWebsocket: () => calls.push('stop-ws'), stopTexthookerService: () => calls.push('stop-texthooker'), @@ -73,7 +73,7 @@ test('cleanup deps builder skips destroyed yomitan window', () => { destroyTray: () => {}, stopConfigHotReload: () => {}, restorePreviousSecondarySubVisibility: () => {}, - restoreMpvSubVisibilityForInvisibleOverlay: () => {}, + restoreMpvSubVisibility: () => {}, unregisterAllGlobalShortcuts: () => {}, stopSubtitleWebsocket: () => {}, stopTexthookerService: () => {}, diff --git a/src/main/runtime/app-lifecycle-main-cleanup.ts b/src/main/runtime/app-lifecycle-main-cleanup.ts index 6c661b6..21ba6d8 100644 --- a/src/main/runtime/app-lifecycle-main-cleanup.ts +++ b/src/main/runtime/app-lifecycle-main-cleanup.ts @@ -21,7 +21,7 @@ export function createBuildOnWillQuitCleanupDepsHandler(deps: { destroyTray: () => void; stopConfigHotReload: () => void; restorePreviousSecondarySubVisibility: () => void; - restoreMpvSubVisibilityForInvisibleOverlay: () => void; + restoreMpvSubVisibility: () => void; unregisterAllGlobalShortcuts: () => void; stopSubtitleWebsocket: () => void; stopTexthookerService: () => void; @@ -52,8 +52,8 @@ export function createBuildOnWillQuitCleanupDepsHandler(deps: { destroyTray: () => deps.destroyTray(), stopConfigHotReload: () => deps.stopConfigHotReload(), restorePreviousSecondarySubVisibility: () => deps.restorePreviousSecondarySubVisibility(), - restoreMpvSubVisibilityForInvisibleOverlay: () => - deps.restoreMpvSubVisibilityForInvisibleOverlay(), + restoreMpvSubVisibility: () => + deps.restoreMpvSubVisibility(), unregisterAllGlobalShortcuts: () => deps.unregisterAllGlobalShortcuts(), stopSubtitleWebsocket: () => deps.stopSubtitleWebsocket(), stopTexthookerService: () => deps.stopTexthookerService(), diff --git a/src/main/runtime/composers/mpv-runtime-composer.test.ts b/src/main/runtime/composers/mpv-runtime-composer.test.ts index 440766c..0043a4b 100644 --- a/src/main/runtime/composers/mpv-runtime-composer.test.ts +++ b/src/main/runtime/composers/mpv-runtime-composer.test.ts @@ -75,7 +75,7 @@ test('composeMpvRuntimeHandlers returns callable handlers and forwards to inject onSubtitleChange: () => {}, refreshDiscordPresence: () => {}, updateCurrentMediaPath: () => {}, - restoreMpvSubVisibilityForInvisibleOverlay: () => {}, + restoreMpvSubVisibility: () => {}, getCurrentAnilistMediaKey: () => null, resetAnilistMediaTracking: () => {}, maybeProbeAnilistDuration: () => {}, diff --git a/src/main/runtime/composers/startup-lifecycle-composer.test.ts b/src/main/runtime/composers/startup-lifecycle-composer.test.ts index eba097e..92f95b1 100644 --- a/src/main/runtime/composers/startup-lifecycle-composer.test.ts +++ b/src/main/runtime/composers/startup-lifecycle-composer.test.ts @@ -16,7 +16,7 @@ test('composeStartupLifecycleHandlers returns callable startup lifecycle handler destroyTray: () => {}, stopConfigHotReload: () => {}, restorePreviousSecondarySubVisibility: () => {}, - restoreMpvSubVisibilityForInvisibleOverlay: () => {}, + restoreMpvSubVisibility: () => {}, unregisterAllGlobalShortcuts: () => {}, stopSubtitleWebsocket: () => {}, stopTexthookerService: () => {}, diff --git a/src/main/runtime/mpv-main-event-actions.test.ts b/src/main/runtime/mpv-main-event-actions.test.ts index 9a923af..49d3ed3 100644 --- a/src/main/runtime/mpv-main-event-actions.test.ts +++ b/src/main/runtime/mpv-main-event-actions.test.ts @@ -51,7 +51,7 @@ test('media path change handler reports stop for empty path and probes media key const handler = createHandleMpvMediaPathChangeHandler({ updateCurrentMediaPath: (path) => calls.push(`path:${path}`), reportJellyfinRemoteStopped: () => calls.push('stopped'), - restoreMpvSubVisibilityForInvisibleOverlay: () => calls.push('restore-mpv-sub'), + restoreMpvSubVisibility: () => calls.push('restore-mpv-sub'), getCurrentAnilistMediaKey: () => 'show:1', resetAnilistMediaTracking: (mediaKey) => calls.push(`reset:${String(mediaKey)}`), maybeProbeAnilistDuration: (mediaKey) => calls.push(`probe:${mediaKey}`), diff --git a/src/main/runtime/mpv-main-event-actions.ts b/src/main/runtime/mpv-main-event-actions.ts index a6cceae..40143d9 100644 --- a/src/main/runtime/mpv-main-event-actions.ts +++ b/src/main/runtime/mpv-main-event-actions.ts @@ -33,7 +33,7 @@ export function createHandleMpvSecondarySubtitleChangeHandler(deps: { export function createHandleMpvMediaPathChangeHandler(deps: { updateCurrentMediaPath: (path: string) => void; reportJellyfinRemoteStopped: () => void; - restoreMpvSubVisibilityForInvisibleOverlay: () => void; + restoreMpvSubVisibility: () => void; getCurrentAnilistMediaKey: () => string | null; resetAnilistMediaTracking: (mediaKey: string | null) => void; maybeProbeAnilistDuration: (mediaKey: string) => void; @@ -45,7 +45,7 @@ export function createHandleMpvMediaPathChangeHandler(deps: { deps.updateCurrentMediaPath(path); if (!path) { deps.reportJellyfinRemoteStopped(); - deps.restoreMpvSubVisibilityForInvisibleOverlay(); + deps.restoreMpvSubVisibility(); } const mediaKey = deps.getCurrentAnilistMediaKey(); deps.resetAnilistMediaTracking(mediaKey); diff --git a/src/main/runtime/mpv-main-event-bindings.test.ts b/src/main/runtime/mpv-main-event-bindings.test.ts index 28d7685..79c6ca8 100644 --- a/src/main/runtime/mpv-main-event-bindings.test.ts +++ b/src/main/runtime/mpv-main-event-bindings.test.ts @@ -36,7 +36,7 @@ test('main mpv event binder wires callbacks through to runtime deps', () => { broadcastSecondarySubtitle: (text) => calls.push(`broadcast-secondary:${text}`), updateCurrentMediaPath: (path) => calls.push(`media-path:${path}`), - restoreMpvSubVisibilityForInvisibleOverlay: () => calls.push('restore-mpv-sub'), + restoreMpvSubVisibility: () => calls.push('restore-mpv-sub'), getCurrentAnilistMediaKey: () => 'media-key', resetAnilistMediaTracking: (key) => calls.push(`reset-media:${String(key)}`), maybeProbeAnilistDuration: (mediaKey) => calls.push(`probe:${mediaKey}`), diff --git a/src/main/runtime/mpv-main-event-bindings.ts b/src/main/runtime/mpv-main-event-bindings.ts index d1a33f3..9e50601 100644 --- a/src/main/runtime/mpv-main-event-bindings.ts +++ b/src/main/runtime/mpv-main-event-bindings.ts @@ -43,7 +43,7 @@ export function createBindMpvMainEventHandlersHandler(deps: { broadcastSecondarySubtitle: (text: string) => void; updateCurrentMediaPath: (path: string) => void; - restoreMpvSubVisibilityForInvisibleOverlay: () => void; + restoreMpvSubVisibility: () => void; getCurrentAnilistMediaKey: () => string | null; resetAnilistMediaTracking: (mediaKey: string | null) => void; maybeProbeAnilistDuration: (mediaKey: string) => void; @@ -97,8 +97,8 @@ export function createBindMpvMainEventHandlersHandler(deps: { const handleMpvMediaPathChange = createHandleMpvMediaPathChangeHandler({ updateCurrentMediaPath: (path) => deps.updateCurrentMediaPath(path), reportJellyfinRemoteStopped: () => deps.reportJellyfinRemoteStopped(), - restoreMpvSubVisibilityForInvisibleOverlay: () => - deps.restoreMpvSubVisibilityForInvisibleOverlay(), + restoreMpvSubVisibility: () => + deps.restoreMpvSubVisibility(), getCurrentAnilistMediaKey: () => deps.getCurrentAnilistMediaKey(), resetAnilistMediaTracking: (mediaKey) => deps.resetAnilistMediaTracking(mediaKey), maybeProbeAnilistDuration: (mediaKey) => deps.maybeProbeAnilistDuration(mediaKey), diff --git a/src/main/runtime/mpv-main-event-main-deps.test.ts b/src/main/runtime/mpv-main-event-main-deps.test.ts index 54e2844..a9e0dc5 100644 --- a/src/main/runtime/mpv-main-event-main-deps.test.ts +++ b/src/main/runtime/mpv-main-event-main-deps.test.ts @@ -41,7 +41,7 @@ test('mpv main event main deps map app state updates and delegate callbacks', as calls.push(`broadcast:${channel}:${String(payload)}`), onSubtitleChange: (text) => calls.push(`subtitle-change:${text}`), updateCurrentMediaPath: (path) => calls.push(`path:${path}`), - restoreMpvSubVisibilityForInvisibleOverlay: () => calls.push('restore-mpv-sub'), + restoreMpvSubVisibility: () => calls.push('restore-mpv-sub'), getCurrentAnilistMediaKey: () => 'media-key', resetAnilistMediaTracking: (mediaKey) => calls.push(`reset:${mediaKey}`), maybeProbeAnilistDuration: (mediaKey) => calls.push(`probe:${mediaKey}`), @@ -75,7 +75,7 @@ test('mpv main event main deps map app state updates and delegate callbacks', as deps.broadcastSubtitleAss('ass'); deps.broadcastSecondarySubtitle('sec'); deps.updateCurrentMediaPath('/tmp/video'); - deps.restoreMpvSubVisibilityForInvisibleOverlay(); + deps.restoreMpvSubVisibility(); assert.equal(deps.getCurrentAnilistMediaKey(), 'media-key'); deps.resetAnilistMediaTracking('media-key'); deps.maybeProbeAnilistDuration('media-key'); diff --git a/src/main/runtime/mpv-main-event-main-deps.ts b/src/main/runtime/mpv-main-event-main-deps.ts index 802ed00..4129000 100644 --- a/src/main/runtime/mpv-main-event-main-deps.ts +++ b/src/main/runtime/mpv-main-event-main-deps.ts @@ -27,7 +27,7 @@ export function createBuildBindMpvMainEventHandlersMainDepsHandler(deps: { broadcastToOverlayWindows: (channel: string, payload: unknown) => void; onSubtitleChange: (text: string) => void; updateCurrentMediaPath: (path: string) => void; - restoreMpvSubVisibilityForInvisibleOverlay: () => void; + restoreMpvSubVisibility: () => void; getCurrentAnilistMediaKey: () => string | null; resetAnilistMediaTracking: (mediaKey: string | null) => void; maybeProbeAnilistDuration: (mediaKey: string) => void; @@ -71,8 +71,8 @@ export function createBuildBindMpvMainEventHandlersMainDepsHandler(deps: { broadcastSecondarySubtitle: (text: string) => deps.broadcastToOverlayWindows('secondary-subtitle:set', text), updateCurrentMediaPath: (path: string) => deps.updateCurrentMediaPath(path), - restoreMpvSubVisibilityForInvisibleOverlay: () => - deps.restoreMpvSubVisibilityForInvisibleOverlay(), + restoreMpvSubVisibility: () => + deps.restoreMpvSubVisibility(), getCurrentAnilistMediaKey: () => deps.getCurrentAnilistMediaKey(), resetAnilistMediaTracking: (mediaKey: string | null) => deps.resetAnilistMediaTracking(mediaKey), diff --git a/src/main/runtime/overlay-mpv-sub-visibility.test.ts b/src/main/runtime/overlay-mpv-sub-visibility.test.ts index 59b9a11..b7d4c62 100644 --- a/src/main/runtime/overlay-mpv-sub-visibility.test.ts +++ b/src/main/runtime/overlay-mpv-sub-visibility.test.ts @@ -8,14 +8,12 @@ import { type VisibilityState = { savedSubVisibility: boolean | null; - savedSecondarySubVisibility: boolean | null; revision: number; }; test('ensure overlay mpv subtitle suppression captures previous visibility then hides subtitles', async () => { const state: VisibilityState = { savedSubVisibility: null, - savedSecondarySubVisibility: null, revision: 0, }; const calls: boolean[] = []; @@ -29,10 +27,6 @@ test('ensure overlay mpv subtitle suppression captures previous visibility then setSavedSubVisibility: (visible) => { state.savedSubVisibility = visible; }, - getSavedSecondarySubVisibility: () => state.savedSecondarySubVisibility, - setSavedSecondarySubVisibility: (visible) => { - state.savedSecondarySubVisibility = visible; - }, getRevision: () => state.revision, setRevision: (revision) => { state.revision = revision; @@ -40,24 +34,19 @@ test('ensure overlay mpv subtitle suppression captures previous visibility then setMpvSubVisibility: (visible) => { calls.push(visible); }, - setMpvSecondarySubVisibility: (visible) => { - calls.push(visible); - }, logWarn: () => {}, }); await ensureHidden(); assert.equal(state.savedSubVisibility, false); - assert.equal(state.savedSecondarySubVisibility, false); assert.equal(state.revision, 1); - assert.deepEqual(calls, [false, false]); + assert.deepEqual(calls, [false]); }); test('restore overlay mpv subtitle suppression restores saved visibility', () => { const state: VisibilityState = { savedSubVisibility: false, - savedSecondarySubVisibility: true, revision: 4, }; const calls: boolean[] = []; @@ -67,10 +56,6 @@ test('restore overlay mpv subtitle suppression restores saved visibility', () => setSavedSubVisibility: (visible) => { state.savedSubVisibility = visible; }, - getSavedSecondarySubVisibility: () => state.savedSecondarySubVisibility, - setSavedSecondarySubVisibility: (visible) => { - state.savedSecondarySubVisibility = visible; - }, getRevision: () => state.revision, setRevision: (revision) => { state.revision = revision; @@ -80,23 +65,18 @@ test('restore overlay mpv subtitle suppression restores saved visibility', () => setMpvSubVisibility: (visible) => { calls.push(visible); }, - setMpvSecondarySubVisibility: (visible) => { - calls.push(visible); - }, }); restore(); assert.equal(state.savedSubVisibility, null); - assert.equal(state.savedSecondarySubVisibility, null); assert.equal(state.revision, 5); - assert.deepEqual(calls, [false, true]); + assert.deepEqual(calls, [false]); }); test('restore keeps mpv subtitles hidden when visible-overlay binding still requires suppression', () => { const state: VisibilityState = { savedSubVisibility: true, - savedSecondarySubVisibility: true, revision: 9, }; const calls: boolean[] = []; @@ -106,10 +86,6 @@ test('restore keeps mpv subtitles hidden when visible-overlay binding still requ setSavedSubVisibility: (visible) => { state.savedSubVisibility = visible; }, - getSavedSecondarySubVisibility: () => state.savedSecondarySubVisibility, - setSavedSecondarySubVisibility: (visible) => { - state.savedSecondarySubVisibility = visible; - }, getRevision: () => state.revision, setRevision: (revision) => { state.revision = revision; @@ -119,23 +95,18 @@ test('restore keeps mpv subtitles hidden when visible-overlay binding still requ setMpvSubVisibility: (visible) => { calls.push(visible); }, - setMpvSecondarySubVisibility: (visible) => { - calls.push(visible); - }, }); restore(); assert.equal(state.savedSubVisibility, true); - assert.equal(state.savedSecondarySubVisibility, true); assert.equal(state.revision, 10); - assert.deepEqual(calls, [false, false]); + assert.deepEqual(calls, [false]); }); test('restore defers mpv subtitle restore while mpv is disconnected', () => { const state: VisibilityState = { savedSubVisibility: true, - savedSecondarySubVisibility: false, revision: 2, }; const calls: boolean[] = []; @@ -145,10 +116,6 @@ test('restore defers mpv subtitle restore while mpv is disconnected', () => { setSavedSubVisibility: (visible) => { state.savedSubVisibility = visible; }, - getSavedSecondarySubVisibility: () => state.savedSecondarySubVisibility, - setSavedSecondarySubVisibility: (visible) => { - state.savedSecondarySubVisibility = visible; - }, getRevision: () => state.revision, setRevision: (revision) => { state.revision = revision; @@ -158,9 +125,6 @@ test('restore defers mpv subtitle restore while mpv is disconnected', () => { setMpvSubVisibility: (visible) => { calls.push(visible); }, - setMpvSecondarySubVisibility: (visible) => { - calls.push(visible); - }, }); restore(); diff --git a/src/main/runtime/overlay-mpv-sub-visibility.ts b/src/main/runtime/overlay-mpv-sub-visibility.ts index 8a85468..1640834 100644 --- a/src/main/runtime/overlay-mpv-sub-visibility.ts +++ b/src/main/runtime/overlay-mpv-sub-visibility.ts @@ -3,10 +3,6 @@ type MpvVisibilityClient = { requestProperty: (name: string) => Promise; }; -type RestoreOptions = { - respectVisibleOverlayBinding?: boolean; -}; - function parseSubVisibility(value: unknown): boolean { if (typeof value === 'string') { const normalized = value.trim().toLowerCase(); @@ -33,12 +29,9 @@ export function createEnsureOverlayMpvSubtitlesHiddenHandler(deps: { getMpvClient: () => MpvVisibilityClient | null; getSavedSubVisibility: () => boolean | null; setSavedSubVisibility: (visible: boolean | null) => void; - getSavedSecondarySubVisibility: () => boolean | null; - setSavedSecondarySubVisibility: (visible: boolean | null) => void; getRevision: () => number; setRevision: (revision: number) => void; setMpvSubVisibility: (visible: boolean) => void; - setMpvSecondarySubVisibility: (visible: boolean) => void; logWarn: (message: string, error: unknown) => void; }) { return async (): Promise => { @@ -50,9 +43,17 @@ export function createEnsureOverlayMpvSubtitlesHiddenHandler(deps: { return; } - if (deps.getSavedSubVisibility() === null) { + const shouldCaptureSavedVisibility = deps.getSavedSubVisibility() === null; + const savedVisibilityPromise = shouldCaptureSavedVisibility + ? mpvClient.requestProperty('sub-visibility') + : null; + + // Hide immediately on overlay toggle; capture/restore logic is handled separately. + deps.setMpvSubVisibility(false); + + if (shouldCaptureSavedVisibility && savedVisibilityPromise) { try { - const currentSubVisibility = await mpvClient.requestProperty('sub-visibility'); + const currentSubVisibility = await savedVisibilityPromise; if (revision !== deps.getRevision()) { return; } @@ -68,64 +69,28 @@ export function createEnsureOverlayMpvSubtitlesHiddenHandler(deps: { deps.setSavedSubVisibility(true); } } - - if (deps.getSavedSecondarySubVisibility() === null) { - try { - const currentSecondarySubVisibility = await mpvClient.requestProperty('secondary-sub-visibility'); - if (revision !== deps.getRevision()) { - return; - } - deps.setSavedSecondarySubVisibility(parseSubVisibility(currentSecondarySubVisibility)); - } catch (error) { - if (revision !== deps.getRevision()) { - return; - } - deps.logWarn( - '[overlay] Failed to capture secondary mpv sub-visibility; falling back to visible restore', - error, - ); - deps.setSavedSecondarySubVisibility(true); - } - } - - if (revision !== deps.getRevision()) { - return; - } - - deps.setMpvSubVisibility(false); - deps.setMpvSecondarySubVisibility(false); }; } export function createRestoreOverlayMpvSubtitlesHandler(deps: { getSavedSubVisibility: () => boolean | null; setSavedSubVisibility: (visible: boolean | null) => void; - getSavedSecondarySubVisibility: () => boolean | null; - setSavedSecondarySubVisibility: (visible: boolean | null) => void; getRevision: () => number; setRevision: (revision: number) => void; isMpvConnected: () => boolean; shouldKeepSuppressedFromVisibleOverlayBinding: () => boolean; setMpvSubVisibility: (visible: boolean) => void; - setMpvSecondarySubVisibility: (visible: boolean) => void; }) { - return (options?: RestoreOptions): void => { + return (): void => { deps.setRevision(deps.getRevision() + 1); const savedVisibility = deps.getSavedSubVisibility(); - const respectVisibleOverlayBinding = options?.respectVisibleOverlayBinding ?? true; - if ( - respectVisibleOverlayBinding && - deps.shouldKeepSuppressedFromVisibleOverlayBinding() - ) { + if (deps.shouldKeepSuppressedFromVisibleOverlayBinding()) { deps.setMpvSubVisibility(false); - deps.setMpvSecondarySubVisibility(false); return; } - const hasSecondarySavedVisibility = deps.getSavedSecondarySubVisibility() !== null; - - if (savedVisibility === null && !hasSecondarySavedVisibility) { + if (savedVisibility === null) { return; } @@ -136,12 +101,7 @@ export function createRestoreOverlayMpvSubtitlesHandler(deps: { if (savedVisibility !== null) { deps.setMpvSubVisibility(savedVisibility); } - const savedSecondaryVisibility = deps.getSavedSecondarySubVisibility(); - if (savedSecondaryVisibility !== null) { - deps.setMpvSecondarySubVisibility(savedSecondaryVisibility); - } deps.setSavedSubVisibility(null); - deps.setSavedSecondarySubVisibility(null); }; } diff --git a/src/main/state.ts b/src/main/state.ts index cff85d8..0e89628 100644 --- a/src/main/state.ts +++ b/src/main/state.ts @@ -172,7 +172,6 @@ export interface AppState { lastSecondarySubToggleAtMs: number; previousSecondarySubVisibility: boolean | null; overlaySavedMpvSubVisibility: boolean | null; - overlaySavedSecondaryMpvSubVisibility: boolean | null; overlayMpvSubVisibilityRevision: number; mpvSubtitleRenderMetrics: MpvSubtitleRenderMetrics; shortcutsRegistered: boolean; @@ -247,7 +246,6 @@ export function createAppState(values: AppStateInitialValues): AppState { lastSecondarySubToggleAtMs: 0, previousSecondarySubVisibility: null, overlaySavedMpvSubVisibility: null, - overlaySavedSecondaryMpvSubVisibility: null, overlayMpvSubVisibilityRevision: 0, mpvSubtitleRenderMetrics: { ...DEFAULT_MPV_SUBTITLE_RENDER_METRICS, diff --git a/src/preload.ts b/src/preload.ts index e0d89c4..28c27eb 100644 --- a/src/preload.ts +++ b/src/preload.ts @@ -154,7 +154,7 @@ const electronAPI: ElectronAPI = { }, getOverlayVisibility: (): Promise => - ipcRenderer.invoke(IPC_CHANNELS.request.getOverlayVisibility), + ipcRenderer.invoke(IPC_CHANNELS.request.getVisibleOverlayVisibility), getCurrentSubtitle: (): Promise => ipcRenderer.invoke(IPC_CHANNELS.request.getCurrentSubtitle), getCurrentSubtitleRaw: (): Promise => diff --git a/src/renderer/subtitle-render.test.ts b/src/renderer/subtitle-render.test.ts index 8015420..959a357 100644 --- a/src/renderer/subtitle-render.test.ts +++ b/src/renderer/subtitle-render.test.ts @@ -7,7 +7,7 @@ import type { MergedToken } from '../types'; import { PartOfSpeech } from '../types.js'; import { alignTokensToSourceText, - buildInvisibleTokenHoverRanges, + buildSubtitleTokenHoverRanges, computeWordClass, normalizeSubtitle, sanitizeSubtitleHoverTokenColor, @@ -266,26 +266,26 @@ test('alignTokensToSourceText avoids duplicate tail when later token surface doe ); }); -test('buildInvisibleTokenHoverRanges tracks token offsets across text separators', () => { +test('buildSubtitleTokenHoverRanges tracks token offsets across text separators', () => { const tokens = [ createToken({ surface: 'キリキリと' }), createToken({ surface: 'かかってこい' }), ]; - const ranges = buildInvisibleTokenHoverRanges(tokens, 'キリキリと\nかかってこい'); + const ranges = buildSubtitleTokenHoverRanges(tokens, 'キリキリと\nかかってこい'); assert.deepEqual(ranges, [ { start: 0, end: 5, tokenIndex: 0 }, { start: 6, end: 12, tokenIndex: 1 }, ]); }); -test('buildInvisibleTokenHoverRanges ignores unmatched token surfaces', () => { +test('buildSubtitleTokenHoverRanges ignores unmatched token surfaces', () => { const tokens = [ createToken({ surface: '君たちが潰した拠点に' }), createToken({ surface: '教団の主力は1人もいない' }), ]; - const ranges = buildInvisibleTokenHoverRanges(tokens, '君たちが潰した拠点に\n教団の主力は１人もいない'); + const ranges = buildSubtitleTokenHoverRanges(tokens, '君たちが潰した拠点に\n教団の主力は１人もいない'); assert.deepEqual(ranges, [{ start: 0, end: 10, tokenIndex: 0 }]); }); diff --git a/src/renderer/subtitle-render.ts b/src/renderer/subtitle-render.ts index e1b7768..e337e84 100644 --- a/src/renderer/subtitle-render.ts +++ b/src/renderer/subtitle-render.ts @@ -9,7 +9,7 @@ type FrequencyRenderSettings = { bandedColors: [string, string, string, string, string]; }; -export type InvisibleTokenHoverRange = { +export type SubtitleTokenHoverRange = { start: number; end: number; tokenIndex: number; @@ -37,6 +37,8 @@ export function normalizeSubtitle(text: string, trim = true, collapseLineBreaks } const HEX_COLOR_PATTERN = /^#(?:[0-9a-fA-F]{3}|[0-9a-fA-F]{4}|[0-9a-fA-F]{6}|[0-9a-fA-F]{8})$/; +const SAFE_CSS_COLOR_PATTERN = + /^(?:#(?:[0-9a-fA-F]{3}|[0-9a-fA-F]{4}|[0-9a-fA-F]{6}|[0-9a-fA-F]{8})|(?:rgba?|hsla?)\([^)]*\)|var\([^)]*\)|[a-zA-Z]+)$/; function sanitizeHexColor(value: unknown, fallback: string): string { return typeof value === 'string' && HEX_COLOR_PATTERN.test(value.trim()) @@ -58,7 +60,9 @@ function sanitizeSubtitleHoverTokenBackgroundColor(value: unknown): string { return 'rgba(54, 58, 79, 0.84)'; } const trimmed = value.trim(); - return trimmed.length > 0 ? trimmed : 'rgba(54, 58, 79, 0.84)'; + return trimmed.length > 0 && SAFE_CSS_COLOR_PATTERN.test(trimmed) + ? trimmed + : 'rgba(54, 58, 79, 0.84)'; } const DEFAULT_FREQUENCY_RENDER_SETTINGS: FrequencyRenderSettings = { @@ -293,16 +297,16 @@ export function alignTokensToSourceText( return segments; } -export function buildInvisibleTokenHoverRanges( +export function buildSubtitleTokenHoverRanges( tokens: MergedToken[], sourceText: string, -): InvisibleTokenHoverRange[] { +): SubtitleTokenHoverRange[] { if (tokens.length === 0 || sourceText.length === 0) { return []; } const segments = alignTokensToSourceText(tokens, sourceText); - const ranges: InvisibleTokenHoverRange[] = []; + const ranges: SubtitleTokenHoverRange[] = []; let cursor = 0; for (const segment of segments) { diff --git a/src/shared/ipc/contracts.ts b/src/shared/ipc/contracts.ts index 24d346b..7c4d392 100644 --- a/src/shared/ipc/contracts.ts +++ b/src/shared/ipc/contracts.ts @@ -22,7 +22,6 @@ export const IPC_CHANNELS = { overlayModalOpened: 'overlay:modal-opened', }, request: { - getOverlayVisibility: 'get-overlay-visibility', getVisibleOverlayVisibility: 'get-visible-overlay-visibility', getCurrentSubtitle: 'get-current-subtitle', getCurrentSubtitleRaw: 'get-current-subtitle-raw',