From cb8c2893104d206c080fd87519daf5617ab5ddf8 Mon Sep 17 00:00:00 2001 From: 4gray Date: Sun, 4 Oct 2026 09:17:29 +0200 Subject: [PATCH] fix(settings): follow Embedded MPV support for search only while the settings page is open The settings search service is provided in the root injector, so the watch it started on its first use had no owner: with a login shell that never answers it kept asking every 30 s until the app quit, long after the settings page or the command palette that needed the answer was closed. A failed recheck also ended the watch as if it were a final answer, hiding the Embedded MPV rows for the rest of the session. The service now separates the two uses. `ensureEmbeddedMpvSupportLoaded()` is a single request again, for the command palette. The settings page calls `followEmbeddedMpvSupport()` and ends it when the page is destroyed. Only a final answer is kept: after an inconclusive one or a failed request the next use asks again, for the palette's player commands too. Co-Authored-By: Claude Fable 5.1 --- .../app/settings/settings-search.facade.ts | 4 +- .../settings.component.search.spec.ts | 15 +++ docs/architecture/embedded-mpv-native.md | 17 ++-- ...kspace-player-commands.contributor.spec.ts | 27 ++++++ .../workspace-player-commands.contributor.ts | 6 +- .../settings-search.service.spec.ts | 92 +++++++++++++++---- .../settings-search.service.ts | 82 ++++++++++------- 7 files changed, 178 insertions(+), 65 deletions(-) diff --git a/apps/web/src/app/settings/settings-search.facade.ts b/apps/web/src/app/settings/settings-search.facade.ts index 036c80b73..5893c85e0 100644 --- a/apps/web/src/app/settings/settings-search.facade.ts +++ b/apps/web/src/app/settings/settings-search.facade.ts @@ -66,7 +66,9 @@ export class SettingsSearchFacade { }); constructor() { - void this.settingsSearch.ensureEmbeddedMpvSupportLoaded(); + inject(DestroyRef).onDestroy( + this.settingsSearch.followEmbeddedMpvSupport() + ); effect(() => { if (!this.isSearching()) { diff --git a/apps/web/src/app/settings/settings.component.search.spec.ts b/apps/web/src/app/settings/settings.component.search.spec.ts index 075357c25..540761a1d 100644 --- a/apps/web/src/app/settings/settings.component.search.spec.ts +++ b/apps/web/src/app/settings/settings.component.search.spec.ts @@ -95,6 +95,21 @@ describe('SettingsComponent search', () => { expect(query('app-settings-general-section')).not.toBeNull(); }); + it('follows Embedded MPV support only while the page is open', () => { + const stopFollowing = jest.fn(); + const follow = jest + .spyOn(settingsSearch, 'followEmbeddedMpvSupport') + .mockReturnValue(stopFollowing); + + const page = TestBed.createComponent(SettingsComponent); + expect(follow).toHaveBeenCalledTimes(1); + expect(stopFollowing).not.toHaveBeenCalled(); + + // Closing the page ends it: nothing shows these rows any more. + page.destroy(); + expect(stopFollowing).toHaveBeenCalledTimes(1); + }); + it('shows an empty state when nothing matches', () => { setSettingsSearchQuery('zzzz-no-such-setting'); fixture.detectChanges(); diff --git a/docs/architecture/embedded-mpv-native.md b/docs/architecture/embedded-mpv-native.md index f34f5d1f4..d3ac24548 100644 --- a/docs/architecture/embedded-mpv-native.md +++ b/docs/architecture/embedded-mpv-native.md @@ -206,14 +206,17 @@ Whatever holds on to one answer follows it through `watchEmbeddedMpvSupport()` mounted on one answer: a player mounted in that window starts playback by itself once `mpv` is found, and the option appears without reopening the page. -- The settings search (`SettingsSearchService`) serves both the open settings - page and the command palette, so it follows the answer as well: the Embedded - MPV rows become searchable while the page stays open, and a call made while - the answer is still inconclusive asks again at once. +- The settings page also follows the answer for its search + (`SettingsSearchService.followEmbeddedMpvSupport()`) and ends that when it + closes: the Embedded MPV rows become searchable while the page stays open, + and nothing keeps asking once no surface shows them. -The command palette's player commands ask on demand, on every palette open; -they keep a final answer for the session, but probe again on the next open -after an inconclusive one. +The command palette asks on demand, on every open, for its player commands and +its settings rows (`ensureEmbeddedMpvSupportLoaded()`). Only a final answer is +kept for the session; after an inconclusive one, or a failed request, the next +open asks again. An open palette is a snapshot of that moment: it does not wait +for a final answer, because a login shell that never answers would then keep +it from opening. When `embedded-mpv` is the saved player, the settings store schedules an idle `prepareEmbeddedMpv()` call. This intentionally moves the first native addon load away from the click-to-play path. It can still block the Electron main process briefly because Node native addon loading is synchronous, but doing it during idle is less visible than doing it when the user clicks a video. Actual MPV session creation still happens on playback because it needs the current Electron window handle and viewport bounds. diff --git a/libs/workspace/shell/feature/src/lib/workspace-player-commands/workspace-player-commands.contributor.spec.ts b/libs/workspace/shell/feature/src/lib/workspace-player-commands/workspace-player-commands.contributor.spec.ts index a52118bdc..f3c93b8a0 100644 --- a/libs/workspace/shell/feature/src/lib/workspace-player-commands/workspace-player-commands.contributor.spec.ts +++ b/libs/workspace/shell/feature/src/lib/workspace-player-commands/workspace-player-commands.contributor.spec.ts @@ -247,6 +247,33 @@ describe('WorkspacePlayerCommandsContributor', () => { expect(contributor.ensureEmbeddedMpvSupportLoaded()).toBeUndefined(); }); + it('asks again after a failed request instead of giving up for the session', async () => { + const warn = jest.spyOn(console, 'warn').mockImplementation(); + const contributor = bootstrap({ + supportsManagedExternalPlayers: true, + supportsEmbeddedMpv: true, + }); + const embedded = getRegistered(viewCommands).find( + (c) => c.id === 'switch-player-embedded-mpv' + ); + electronStub?.getEmbeddedMpvSupport.mockRejectedValueOnce( + new Error('bridge failed') + ); + + try { + await contributor.ensureEmbeddedMpvSupportLoaded(); + expect(resolveBoolean(embedded?.visible)).toBe(false); + + await contributor.ensureEmbeddedMpvSupportLoaded(); + expect(resolveBoolean(embedded?.visible)).toBe(true); + expect(electronStub?.getEmbeddedMpvSupport).toHaveBeenCalledTimes( + 2 + ); + } finally { + warn.mockRestore(); + } + }); + it('switches to embedded MPV on run', () => { bootstrap({ supportsManagedExternalPlayers: true, diff --git a/libs/workspace/shell/feature/src/lib/workspace-player-commands/workspace-player-commands.contributor.ts b/libs/workspace/shell/feature/src/lib/workspace-player-commands/workspace-player-commands.contributor.ts index 15e80942c..fc9f58dd3 100644 --- a/libs/workspace/shell/feature/src/lib/workspace-player-commands/workspace-player-commands.contributor.ts +++ b/libs/workspace/shell/feature/src/lib/workspace-player-commands/workspace-player-commands.contributor.ts @@ -120,9 +120,9 @@ export class WorkspacePlayerCommandsContributor { } private async loadEmbeddedMpvSupport(): Promise { - // An inconclusive answer hides the command for now, but is not kept: - // the next palette open asks again. - let final = true; + // Only a final answer is kept. An inconclusive one, or a failed + // request, hides the command for now: the next palette open asks again. + let final = false; try { const support = await window.electron?.getEmbeddedMpvSupport?.(); this.embeddedMpvSupported.set(!!support?.supported); diff --git a/libs/workspace/shell/util/src/lib/settings-search/settings-search.service.spec.ts b/libs/workspace/shell/util/src/lib/settings-search/settings-search.service.spec.ts index 62850b7dc..260e83686 100644 --- a/libs/workspace/shell/util/src/lib/settings-search/settings-search.service.spec.ts +++ b/libs/workspace/shell/util/src/lib/settings-search/settings-search.service.spec.ts @@ -214,24 +214,42 @@ describe('SettingsSearchService', () => { expect(getEmbeddedMpvSupport).toHaveBeenCalledTimes(2); }); + it('asks again after a failed request instead of giving up for the session', async () => { + const getEmbeddedMpvSupport = probeWith({ supported: true }); + getEmbeddedMpvSupport.mockRejectedValueOnce( + new Error('bridge failed') + ); + const { service } = setup({ + ...DESKTOP, + supportsEmbeddedMpv: true, + }); + const entries = () => service.visibleEntries().map(({ id }) => id); + + await service.ensureEmbeddedMpvSupportLoaded(); + expect(entries()).not.toContain('embedded-mpv-extra-options'); + + await service.ensureEmbeddedMpvSupportLoaded(); + expect(entries()).toContain('embedded-mpv-extra-options'); + expect(getEmbeddedMpvSupport).toHaveBeenCalledTimes(2); + }); + describe('while the settings page stays open', () => { + const inconclusive = { supported: false, inconclusive: true }; + const mountedSetup = () => + setup({ ...DESKTOP, supportsEmbeddedMpv: true }); + beforeEach(() => jest.useFakeTimers()); afterEach(() => jest.useRealTimers()); it('follows an inconclusive answer without being asked again', async () => { - const getEmbeddedMpvSupport = probeWith({ - supported: false, - inconclusive: true, - }); - const { service } = setup({ - ...DESKTOP, - supportsEmbeddedMpv: true, - }); + const getEmbeddedMpvSupport = probeWith(inconclusive); + const { service } = mountedSetup(); const entries = () => service.visibleEntries().map(({ id }) => id); - // The page asks once, when it is created. - await service.ensureEmbeddedMpvSupportLoaded(); + // The page starts following once, when it is created. + service.followEmbeddedMpvSupport(); + await jest.advanceTimersByTimeAsync(0); expect(entries()).not.toContain('embedded-mpv-extra-options'); getEmbeddedMpvSupport.mockResolvedValue({ @@ -252,18 +270,54 @@ describe('SettingsSearchService', () => { expect(getEmbeddedMpvSupport).toHaveBeenCalledTimes(2); }); - it('stops following once the service is destroyed', async () => { - const getEmbeddedMpvSupport = probeWith({ - supported: false, - inconclusive: true, - }); - const { service } = setup({ - ...DESKTOP, - supportsEmbeddedMpv: true, + it('stops asking once the page is closed, and asks again on the next use', async () => { + const getEmbeddedMpvSupport = probeWith(inconclusive); + const { service } = mountedSetup(); + const stopFollowing = service.followEmbeddedMpvSupport(); + await jest.advanceTimersByTimeAsync(0); + + // Nothing shows these rows any more: no polling is left. + stopFollowing(); + await jest.advanceTimersByTimeAsync( + EMBEDDED_MPV_SUPPORT_RECHECK_MS * 20 + ); + expect(getEmbeddedMpvSupport).toHaveBeenCalledTimes(1); + + await service.ensureEmbeddedMpvSupportLoaded(); + expect(getEmbeddedMpvSupport).toHaveBeenCalledTimes(2); + }); + + it('ends on a failed recheck, and the next use asks again', async () => { + const getEmbeddedMpvSupport = probeWith(inconclusive); + const { service } = mountedSetup(); + service.followEmbeddedMpvSupport(); + await jest.advanceTimersByTimeAsync(0); + + getEmbeddedMpvSupport.mockRejectedValueOnce( + new Error('bridge failed') + ); + await jest.advanceTimersByTimeAsync( + EMBEDDED_MPV_SUPPORT_RECHECK_MS * 20 + ); + expect(getEmbeddedMpvSupport).toHaveBeenCalledTimes(2); + + getEmbeddedMpvSupport.mockResolvedValue({ + platform: 'linux', + supported: true, }); await service.ensureEmbeddedMpvSupportLoaded(); + expect(service.visibleEntries().map(({ id }) => id)).toContain( + 'embedded-mpv-extra-options' + ); + expect(getEmbeddedMpvSupport).toHaveBeenCalledTimes(3); + }); - TestBed.resetTestingModule(); + it('has nothing to follow once a final answer is in hand', async () => { + const getEmbeddedMpvSupport = probeWith({ supported: true }); + const { service } = mountedSetup(); + await service.ensureEmbeddedMpvSupportLoaded(); + + service.followEmbeddedMpvSupport(); await jest.advanceTimersByTimeAsync( EMBEDDED_MPV_SUPPORT_RECHECK_MS * 20 ); diff --git a/libs/workspace/shell/util/src/lib/settings-search/settings-search.service.ts b/libs/workspace/shell/util/src/lib/settings-search/settings-search.service.ts index eed3bb847..1a17827c1 100644 --- a/libs/workspace/shell/util/src/lib/settings-search/settings-search.service.ts +++ b/libs/workspace/shell/util/src/lib/settings-search/settings-search.service.ts @@ -1,4 +1,4 @@ -import { DestroyRef, inject, Injectable, signal } from '@angular/core'; +import { inject, Injectable, signal } from '@angular/core'; import { Router } from '@angular/router'; import { TranslateService } from '@ngx-translate/core'; import { VodSourceDiscoveryService } from '@iptvnator/portal/shared/data-access'; @@ -42,13 +42,6 @@ export class SettingsSearchService { ); private embeddedMpvSupportChecked = false; private embeddedMpvSupportLoad: Promise | undefined; - private stopEmbeddedMpvSupportWatch: (() => void) | undefined; - - constructor() { - inject(DestroyRef).onDestroy(() => - this.stopEmbeddedMpvSupportWatch?.() - ); - } /** Row the settings page should scroll to and highlight next. */ readonly pendingReveal = this.revealRequest.asReadonly(); @@ -74,13 +67,49 @@ export class SettingsSearchService { /** * Probes embedded MPV support so rows that need it become searchable. * Returns the pending probe, or `undefined` when there is nothing to - * wait for. Call it lazily (palette open, settings page), never from - * shell bootstrap: supported desktop builds may load the native addon - * while answering. A final answer is kept. An inconclusive one keeps - * being followed, so an open settings page updates by itself, and the - * next call asks again at once. + * wait for. Call it lazily (palette open), never from shell bootstrap: + * supported desktop builds may load the native addon while answering. + * Only a final answer is kept; after an inconclusive one, or a failed + * request, the next call asks again. */ ensureEmbeddedMpvSupportLoaded(): Promise | undefined { + const getSupport = this.embeddedMpvSupportProbe(); + if (!getSupport) { + return undefined; + } + + this.embeddedMpvSupportLoad ??= getSupport() + .then((support) => this.takeEmbeddedMpvSupport(support)) + .catch(() => this.takeEmbeddedMpvSupport(null)) + .finally(() => { + this.embeddedMpvSupportLoad = undefined; + }); + return this.embeddedMpvSupportLoad; + } + + /** + * For a surface that keeps showing these rows (the settings page): probes + * like `ensureEmbeddedMpvSupportLoaded()` and follows an inconclusive + * answer until it is final, so the rows appear by themselves. Returns + * the function that ends it; call it when the surface goes away, so + * nothing keeps asking for an answer no one shows. + */ + followEmbeddedMpvSupport(): () => void { + const getSupport = this.embeddedMpvSupportProbe(); + if (!getSupport) { + return () => undefined; + } + + return watchEmbeddedMpvSupport( + getSupport, + (support) => this.takeEmbeddedMpvSupport(support), + () => this.takeEmbeddedMpvSupport(null) + ); + } + + /** The support request, or `undefined` when there is nothing to ask. */ + private embeddedMpvSupportProbe(): + (() => Promise) | undefined { if (this.embeddedMpvSupportChecked) { return undefined; } @@ -95,30 +124,13 @@ export class SettingsSearchService { return undefined; } - this.embeddedMpvSupportLoad ??= this.followEmbeddedMpvSupport(() => - electron.getEmbeddedMpvSupport() - ); - return this.embeddedMpvSupportLoad; + return () => electron.getEmbeddedMpvSupport(); } - /** Follows the answer afresh; resolves with its first one. */ - private followEmbeddedMpvSupport( - getSupport: () => Promise - ): Promise { - this.stopEmbeddedMpvSupportWatch?.(); - return new Promise((answered) => { - const take = (support: EmbeddedMpvSupport | null) => { - this.embeddedMpvSupport.set(support); - this.embeddedMpvSupportChecked = !support?.inconclusive; - this.embeddedMpvSupportLoad = undefined; - answered(); - }; - this.stopEmbeddedMpvSupportWatch = watchEmbeddedMpvSupport( - getSupport, - take, - () => take(null) - ); - }); + private takeEmbeddedMpvSupport(support: EmbeddedMpvSupport | null): void { + this.embeddedMpvSupport.set(support); + this.embeddedMpvSupportChecked = + support !== null && !support.inconclusive; } /**