diff --git a/changes/settings-live-save-feedback-docs.md b/changes/settings-live-save-feedback-docs.md new file mode 100644 index 00000000..827a3e99 --- /dev/null +++ b/changes/settings-live-save-feedback-docs.md @@ -0,0 +1,4 @@ +type: docs +area: config + +- Clarified live-setting save feedback, mixed restart warnings, and subtitle-generation reload behavior. diff --git a/changes/settings-live-save-feedback.md b/changes/settings-live-save-feedback.md new file mode 100644 index 00000000..e51b0365 --- /dev/null +++ b/changes/settings-live-save-feedback.md @@ -0,0 +1,5 @@ +type: fixed +area: config + +- Settings marked LIVE now use the same reload policy as save results, fixing false restart warnings for notifications and subtitle generation. +- Mixed saves apply live changes and list only sections with changed fields that require a restart. diff --git a/docs-site/configuration.md b/docs-site/configuration.md index 2698db50..879d3f77 100644 --- a/docs-site/configuration.md +++ b/docs-site/configuration.md @@ -61,7 +61,7 @@ The Settings window preserves existing JSONC comments, trailing commas, and unre Secret fields do not display stored values. They show whether a value is configured; entering a new value writes it, and reset clears the explicit path. Prefer command-based secret options such as `jimaku.apiKeyCommand` when available. -Saving validates the candidate config before writing. Live-reloadable changes are applied immediately; other changes return a restart-required banner in the window. +Saving validates the candidate config before writing. Saving only fields marked **LIVE** shows "Saved. Live settings applied." If a save also changes fields that need a restart, the banner lists only the sections containing those changed fields. Live changes still apply in the same save. ## Configuration file @@ -103,7 +103,7 @@ SubMiner watches the active config file (`config.jsonc` or `config.json`) while Hot-reloadable settings include subtitle appearance, sidebar controls, keybindings, shortcuts, notifications, logging level, selected source-language preferences, -Jimaku/Subsync settings, AniSkip settings (`mpv.aniskipEnabled`, `mpv.aniskipButtonKey`), +Jimaku/Subsync and subtitle-generation settings, AniSkip settings (`mpv.aniskipEnabled`, `mpv.aniskipButtonKey`), stats keys (`stats.toggleKey`, `stats.markWatchedKey`), the secondary-subtitle default mode, and the Anki deck, known-word, N+1, field, sentence-card, and Kiku options listed in the reference tables below. @@ -975,7 +975,7 @@ This example is intentionally compact. The option table below documents availabl | `tags` | array of strings | Tags automatically added to cards mined/updated by SubMiner (default: `['SubMiner']`; set `[]` to disable automatic tagging). | | `ankiConnect.deck` | string | Restrict duplicate detection and card enrichment to this Anki deck. Leave empty to use the Yomitan mining deck when available. In Settings, this dropdown auto-fills and persists Yomitan's current mining deck when available. | | `fields.word` | string | Card field for mined word / expression text (default: `Expression`) | -| `fields.audio` | string | Card field for the generated sentence audio clip (default: `ExpressionAudio`). Set this to a dedicated field such as `SentenceAudio` so it does not collide with the word audio Yomitan writes. | +| `fields.audio` | string | Card field for the generated sentence audio clip (default: `ExpressionAudio`). Set this to a dedicated field such as `SentenceAudio` so it does not collide with the word audio Yomitan writes. | | `fields.image` | string | Card field for images (default: `Picture`) | | `fields.sentence` | string | Card field for sentences (default: `Sentence`) | | `fields.miscInfo` | string | Card field for metadata (default: `"MiscInfo"`, set to `null` to disable) | diff --git a/src/config/hot-reload.ts b/src/config/hot-reload.ts new file mode 100644 index 00000000..f5d54a09 --- /dev/null +++ b/src/config/hot-reload.ts @@ -0,0 +1,64 @@ +function pathStartsWith(path: string, prefix: string): boolean { + return path === prefix || path.startsWith(`${prefix}.`); +} + +const HOT_RELOAD_ROOTS = ['subtitleStyle', 'keybindings', 'shortcuts', 'subtitleSidebar'] as const; + +const HOT_RELOAD_EXACT_OR_PREFIX_PATHS = [ + 'secondarySub.defaultMode', + 'mpv.aniskipEnabled', + 'mpv.aniskipButtonKey', + 'ankiConnect.ai.enabled', + 'stats.toggleKey', + 'stats.markWatchedKey', + 'logging.level', + 'logging.rotation', + 'logging.files', + 'youtube.primarySubLanguages', + 'ankiConnect.deck', + 'ankiConnect.media.normalizeAudio', + 'ankiConnect.media.mirrorMpvVolume', + 'ankiConnect.media.reviewTiming', + 'ankiConnect.behavior.autoUpdateNewCards', + 'ankiConnect.knownWords.highlightEnabled', + 'ankiConnect.knownWords.refreshMinutes', + 'ankiConnect.knownWords.addMinedWordsImmediately', + 'ankiConnect.knownWords.matchMode', + 'ankiConnect.knownWords.decks', + 'ankiConnect.nPlusOne.enabled', + 'ankiConnect.nPlusOne.minSentenceWords', + 'ankiConnect.fields.word', + 'ankiConnect.fields.audio', + 'ankiConnect.fields.image', + 'ankiConnect.fields.sentence', + 'ankiConnect.fields.miscInfo', + 'ankiConnect.isLapis.sentenceCardModel', + 'ankiConnect.isKiku.fieldGrouping', + 'ankiConnect.isSenren.fieldGrouping', + 'ankiConnect.lapisKiku.wordCardKind', +] as const; + +export function getConfigHotReloadField(path: string): string | null { + for (const root of HOT_RELOAD_ROOTS) { + if (pathStartsWith(path, root)) { + return root; + } + } + + for (const hotPath of HOT_RELOAD_EXACT_OR_PREFIX_PATHS) { + if (pathStartsWith(path, hotPath)) { + return hotPath; + } + } + + // These consumers read the current config when the next operation starts. + if ( + ['jimaku', 'subsync', 'notifications', 'subtitleGeneration'].some((root) => + pathStartsWith(path, root), + ) + ) { + return path; + } + + return null; +} diff --git a/src/config/settings/registry.ts b/src/config/settings/registry.ts index 27953dcc..ccb90183 100644 --- a/src/config/settings/registry.ts +++ b/src/config/settings/registry.ts @@ -1,9 +1,9 @@ +import { getConfigHotReloadField } from '../hot-reload'; import type { ResolvedConfig } from '../../types/config'; import type { ConfigSettingsCategory, ConfigSettingsControl, ConfigSettingsField, - ConfigSettingsRestartBehavior, } from '../../types/settings'; import { CONFIG_OPTION_REGISTRY, DEFAULT_CONFIG } from '../definitions'; import { @@ -693,53 +693,6 @@ function compareFields(a: ConfigSettingsField, b: ConfigSettingsField): number { return a.configPath.localeCompare(b.configPath); } -function restartBehaviorForPath(path: string): ConfigSettingsRestartBehavior { - if ( - path === 'keybindings' || - pathStartsWith(path, 'shortcuts') || - pathStartsWith(path, 'subtitleStyle') || - pathStartsWith(path, 'subtitleSidebar') || - path === 'secondarySub.defaultMode' || - path === 'ankiConnect.deck' || - path === 'ankiConnect.ai.enabled' || - path === 'ankiConnect.media.normalizeAudio' || - path === 'ankiConnect.media.mirrorMpvVolume' || - path === 'ankiConnect.media.reviewTiming' || - path === 'ankiConnect.behavior.autoUpdateNewCards' || - path === 'ankiConnect.knownWords.highlightEnabled' || - path === 'ankiConnect.knownWords.refreshMinutes' || - path === 'ankiConnect.knownWords.addMinedWordsImmediately' || - path === 'ankiConnect.knownWords.matchMode' || - path === 'ankiConnect.knownWords.decks' || - path === 'ankiConnect.nPlusOne.enabled' || - path === 'ankiConnect.nPlusOne.minSentenceWords' || - path === 'ankiConnect.fields.word' || - path === 'ankiConnect.fields.audio' || - path === 'ankiConnect.fields.image' || - path === 'ankiConnect.fields.sentence' || - path === 'ankiConnect.fields.miscInfo' || - path === 'ankiConnect.isLapis.sentenceCardModel' || - path === 'ankiConnect.isKiku.fieldGrouping' || - path === 'ankiConnect.isSenren.fieldGrouping' || - path === 'ankiConnect.lapisKiku.wordCardKind' || - path === 'mpv.aniskipEnabled' || - path === 'mpv.aniskipButtonKey' || - path === 'stats.toggleKey' || - path === 'stats.markWatchedKey' || - path === 'logging.level' || - path === 'logging.rotation' || - pathStartsWith(path, 'logging.files') || - pathStartsWith(path, 'notifications') || - path === 'youtube.primarySubLanguages' || - pathStartsWith(path, 'jimaku') || - pathStartsWith(path, 'subsync') || - pathStartsWith(path, 'subtitleGeneration') - ) { - return 'hot-reload'; - } - return 'restart'; -} - function fieldForLeaf(leaf: Leaf): ConfigSettingsField { const option = OPTION_BY_PATH.get(leaf.path); const { category, section } = categoryAndSection(leaf.path); @@ -758,7 +711,7 @@ function fieldForLeaf(leaf: Leaf): ConfigSettingsField { ? { enumValues: option.settingsEnumValues ?? option.enumValues } : {}), ...(option?.enumLabels ? { enumLabels: option.enumLabels } : {}), - restartBehavior: restartBehaviorForPath(leaf.path), + restartBehavior: getConfigHotReloadField(leaf.path) ? 'hot-reload' : 'restart', advanced: leaf.path.startsWith('controller.') || leaf.path.startsWith('immersionTracking.retention.') || diff --git a/src/core/services/config-hot-reload.test.ts b/src/core/services/config-hot-reload.test.ts index e2b3391d..65f52844 100644 --- a/src/core/services/config-hot-reload.test.ts +++ b/src/core/services/config-hot-reload.test.ts @@ -1,12 +1,47 @@ import test from 'node:test'; import assert from 'node:assert/strict'; import { DEFAULT_CONFIG, deepCloneConfig } from '../../config'; +import { buildConfigSettingsRegistry, getConfigValueAtPath } from '../../config/settings/registry'; import { classifyConfigHotReloadDiff, createConfigHotReloadRuntime, type ConfigHotReloadRuntimeDeps, } from './config-hot-reload'; +test('every LIVE settings field is classified without a restart warning', () => { + for (const field of buildConfigSettingsRegistry(DEFAULT_CONFIG)) { + if (field.restartBehavior !== 'hot-reload') continue; + const next = deepCloneConfig(DEFAULT_CONFIG); + const segments = field.configPath.split('.'); + const leaf = segments.pop(); + assert.ok(leaf); + const parent = segments.length ? getConfigValueAtPath(next, segments.join('.')) : next; + assert.ok(parent && typeof parent === 'object', field.configPath); + // The classifier compares structure; validation of field values is tested separately. + Object.defineProperty(parent, leaf, { + value: getConfigValueAtPath(next, field.configPath) === null ? 'changed' : null, + enumerable: true, + }); + const diff = classifyConfigHotReloadDiff(DEFAULT_CONFIG, next); + assert.deepEqual(diff.restartRequiredFields, [], field.configPath); + assert.ok(diff.hotReloadFields.length > 0, field.configPath); + } +}); + +test('live notifications and subtitle generation changes preserve unrelated restart warnings', () => { + const next = deepCloneConfig(DEFAULT_CONFIG); + next.notifications.overlayPosition = 'top'; + next.subtitleGeneration.threads += 1; + next.websocket.port += 1; + + const diff = classifyConfigHotReloadDiff(DEFAULT_CONFIG, next); + assert.deepEqual( + new Set(diff.hotReloadFields), + new Set(['notifications.overlayPosition', 'subtitleGeneration.threads']), + ); + assert.deepEqual(diff.restartRequiredFields, ['websocket.port']); +}); + test('classifyConfigHotReloadDiff separates hot and restart-required fields', () => { const prev = deepCloneConfig(DEFAULT_CONFIG); const next = deepCloneConfig(DEFAULT_CONFIG); @@ -15,7 +50,7 @@ test('classifyConfigHotReloadDiff separates hot and restart-required fields', () const diff = classifyConfigHotReloadDiff(prev, next); assert.deepEqual(diff.hotReloadFields, ['subtitleStyle']); - assert.deepEqual(diff.restartRequiredFields, ['websocket']); + assert.deepEqual(diff.restartRequiredFields, ['websocket.port']); }); test('classifyConfigHotReloadDiff treats safe nested config paths as hot-reloadable', () => { @@ -101,7 +136,11 @@ test('classifyConfigHotReloadDiff keeps unsafe nested siblings restart-required' const diff = classifyConfigHotReloadDiff(prev, next); assert.deepEqual(diff.hotReloadFields, []); - assert.deepEqual(diff.restartRequiredFields, ['ankiConnect', 'stats']); + assert.deepEqual(diff.restartRequiredFields, [ + 'ankiConnect.url', + 'ankiConnect.ai.model', + 'stats.serverPort', + ]); }); test('config hot reload runtime debounces rapid watch events', () => { diff --git a/src/core/services/config-hot-reload.ts b/src/core/services/config-hot-reload.ts index 83e02967..9cdaed05 100644 --- a/src/core/services/config-hot-reload.ts +++ b/src/core/services/config-hot-reload.ts @@ -1,3 +1,4 @@ +import { getConfigHotReloadField } from '../../config/hot-reload'; import { type ReloadConfigStrictResult } from '../../config'; import type { ConfigValidationWarning } from '../../types'; import type { ResolvedConfig } from '../../types'; @@ -33,10 +34,6 @@ function isRecord(value: unknown): value is Record { return value !== null && typeof value === 'object' && !Array.isArray(value); } -function pathStartsWith(path: string, prefix: string): boolean { - return path === prefix || path.startsWith(`${prefix}.`); -} - function collectChangedPaths(prev: unknown, next: unknown, prefix = ''): string[] { if (isEqual(prev, next)) { return []; @@ -52,60 +49,6 @@ function collectChangedPaths(prev: unknown, next: unknown, prefix = ''): string[ ); } -const HOT_RELOAD_ROOTS = ['subtitleStyle', 'keybindings', 'shortcuts', 'subtitleSidebar'] as const; - -const HOT_RELOAD_EXACT_OR_PREFIX_PATHS = [ - 'secondarySub.defaultMode', - 'mpv.aniskipEnabled', - 'mpv.aniskipButtonKey', - 'ankiConnect.ai.enabled', - 'stats.toggleKey', - 'stats.markWatchedKey', - 'logging.level', - 'logging.rotation', - 'logging.files', - 'youtube.primarySubLanguages', - 'jimaku', - 'subsync', - 'ankiConnect.deck', - 'ankiConnect.media.normalizeAudio', - 'ankiConnect.media.mirrorMpvVolume', - 'ankiConnect.media.reviewTiming', - 'ankiConnect.behavior.autoUpdateNewCards', - 'ankiConnect.knownWords.highlightEnabled', - 'ankiConnect.knownWords.refreshMinutes', - 'ankiConnect.knownWords.addMinedWordsImmediately', - 'ankiConnect.knownWords.matchMode', - 'ankiConnect.knownWords.decks', - 'ankiConnect.nPlusOne.enabled', - 'ankiConnect.nPlusOne.minSentenceWords', - 'ankiConnect.fields.word', - 'ankiConnect.fields.audio', - 'ankiConnect.fields.image', - 'ankiConnect.fields.sentence', - 'ankiConnect.fields.miscInfo', - 'ankiConnect.isLapis.sentenceCardModel', - 'ankiConnect.isKiku.fieldGrouping', - 'ankiConnect.isSenren.fieldGrouping', - 'ankiConnect.lapisKiku.wordCardKind', -] as const; - -function hotReloadFieldForChangedPath(path: string): string | null { - for (const root of HOT_RELOAD_ROOTS) { - if (pathStartsWith(path, root)) { - return root; - } - } - - for (const hotPath of HOT_RELOAD_EXACT_OR_PREFIX_PATHS) { - if (pathStartsWith(path, hotPath)) { - return hotPath === 'jimaku' || hotPath === 'subsync' ? path : hotPath; - } - } - - return null; -} - function classifyDiff(prev: ResolvedConfig, next: ResolvedConfig): ConfigHotReloadDiff { const hotReloadFields: string[] = []; const restartRequiredFields: string[] = []; @@ -113,33 +56,11 @@ function classifyDiff(prev: ResolvedConfig, next: ResolvedConfig): ConfigHotRelo const changedPaths = collectChangedPaths(prev, next); for (const path of changedPaths) { - const hotReloadField = hotReloadFieldForChangedPath(path); + const hotReloadField = getConfigHotReloadField(path); if (hotReloadField) { hotReloadFieldSet.add(hotReloadField); - } - } - - const keys = new Set([ - ...(Object.keys(prev) as Array), - ...(Object.keys(next) as Array), - ]); - - for (const key of keys) { - if ( - key === 'subtitleStyle' || - key === 'keybindings' || - key === 'shortcuts' || - key === 'subtitleSidebar' - ) { - continue; - } - - const changedPathsForKey = changedPaths.filter((path) => pathStartsWith(path, String(key))); - const hasRestartRequiredChange = changedPathsForKey.some( - (path) => !hotReloadFieldForChangedPath(path), - ); - if (hasRestartRequiredChange) { - restartRequiredFields.push(String(key)); + } else { + restartRequiredFields.push(path); } } diff --git a/src/main/runtime/config-settings-runtime.test.ts b/src/main/runtime/config-settings-runtime.test.ts index 77d2e714..85ebbdec 100644 --- a/src/main/runtime/config-settings-runtime.test.ts +++ b/src/main/runtime/config-settings-runtime.test.ts @@ -5,9 +5,91 @@ import os from 'node:os'; import path from 'node:path'; import { DEFAULT_CONFIG, deepCloneConfig } from '../../config'; import { resolveConfig } from '../../config/resolve'; +import { buildConfigSettingsRegistry } from '../../config/settings/registry'; +import type { RawConfig } from '../../types/config'; import { IPC_CHANNELS } from '../../shared/ipc/contracts'; import { createConfigSettingsRuntime } from './config-settings-runtime'; +test('settings saves report live changes and only the sections that actually need restart', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'subminer-settings-live-')); + const configPath = path.join(dir, 'config.jsonc'); + let rawConfig: RawConfig = {}; + let resolvedConfig = resolveConfig(rawConfig).resolved; + const applied: string[][] = []; + const runtime = createConfigSettingsRuntime({ + fields: buildConfigSettingsRegistry(DEFAULT_CONFIG), + getConfigPath: () => configPath, + getRawConfig: () => rawConfig, + getConfig: () => resolvedConfig, + getWarnings: () => [], + reloadConfigStrict: () => { + rawConfig = JSON.parse(fs.readFileSync(configPath, 'utf-8')); + const result = resolveConfig(rawConfig); + resolvedConfig = result.resolved; + return { ok: true, config: resolvedConfig, warnings: result.warnings, path: configPath }; + }, + onHotReloadApplied: (diff) => { + applied.push(diff.hotReloadFields); + }, + getSettingsWindow: () => null, + setSettingsWindow: () => {}, + createSettingsWindow: () => { + throw new Error('Save must not open a window'); + }, + settingsHtmlPath: '/tmp/settings.html', + openPath: async () => '', + defaultAnkiConnectUrl: DEFAULT_CONFIG.ankiConnect.url, + createAnkiClient: () => { + throw new Error('Save must not query Anki'); + }, + ipcMain: { handle: () => {} }, + ipcChannels: IPC_CHANNELS.request, + }); + + try { + const live = runtime.savePatch({ + operations: [ + { op: 'set', path: 'notifications.overlayPosition', value: 'top' }, + { + op: 'set', + path: 'subtitleGeneration.threads', + value: DEFAULT_CONFIG.subtitleGeneration.threads + 1, + }, + ], + }); + assert.equal(live.ok, true); + assert.deepEqual(live.restartRequiredFields, []); + assert.deepEqual(live.restartRequiredSections, []); + assert.deepEqual( + new Set(live.hotReloadFields), + new Set(['notifications.overlayPosition', 'subtitleGeneration.threads']), + ); + assert.deepEqual(applied, [live.hotReloadFields]); + + const mixed = runtime.savePatch({ + operations: [ + { op: 'set', path: 'ankiConnect.deck', value: 'Mining' }, + { op: 'set', path: 'ankiConnect.url', value: 'http://127.0.0.1:9999' }, + ], + }); + assert.equal(mixed.ok, true); + assert.deepEqual(mixed.hotReloadFields, ['ankiConnect.deck']); + assert.deepEqual(mixed.restartRequiredSections, ['AnkiConnect']); + + const reset = runtime.savePatch({ + operations: [ + { op: 'reset', path: 'notifications.overlayPosition' }, + { op: 'reset', path: 'subtitleGeneration.threads' }, + ], + }); + assert.equal(reset.ok, true); + assert.deepEqual(reset.restartRequiredSections, []); + assert.deepEqual(new Set(reset.hotReloadFields), new Set(live.hotReloadFields)); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +}); + test('config settings runtime exposes inferred Yomitan Anki deck lookup', async () => { const handlers = new Map unknown>(); const runtime = createConfigSettingsRuntime({