From bf3facf5c618664f98283ef380fb9823ecf0c070 Mon Sep 17 00:00:00 2001 From: Stuart Cameron Date: Sat, 12 Sep 2026 17:50:05 +1000 Subject: [PATCH] feat(presets): import and export preset files (#81) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #81's fourth ask was for "JSON profiles as implemented in Massie VFX Open Media Encoder". The format already existed and is richer than what was asked for — a preset is JSON carrying the whole pipeline *and* the encoding settings, in plain files under ~/.vapourbox/presets. What was missing is everything around it, so this is an import/export and error-surfacing change wearing a format request's clothes. Import is two-phase, and that is the substance of the change rather than a structural nicety. A preset carries `customVapoursynth` — Python the worker executes — and `customFfmpegArgs`, both verified to survive a round trip. Importing someone else's preset is therefore closer to running their script than to loading their settings, and nothing else in the app would reveal it: those fields only render in advanced mode. So `inspectPresetFile` validates without committing, the dialog shows any custom code verbatim, and `commitImport` takes `stripCustomCode` — the middle option between trusting everything and importing nothing. Import deliberately does not apply the preset. "Add this to my presets" should not replace the user's current pipeline as a side effect, and there is no undo. Three bugs found while writing it, each with a regression test named after the behaviour. All are pre-existing and all become far easier to hit with import, because an imported preset is very likely to be named like one the user already has: - The on-disk filename came from the preset's *name*, and the sanitizer lowercases and collapses whitespace and separators — so "VHS Cleanup", "vhs cleanup", "VHS/Cleanup" and "VHS Cleanup" all produced vhs_cleanup.json. Saving one destroyed another. Filenames are now the preset id; savePreset sweeps any other file carrying the same id, which migrates legacy files as they are touched. - Renaming a preset wrote a second file and left the original, so it appeared twice after a reload. - deletePreset removed the name-derived file *before* looking by id, so deleting one preset took a similarly-named one's file with it. `isBuiltIn` is never taken from a file. The flag survives fromJson, and a built-in cannot be deleted or overwritten, so a preset claiming to be one would be stuck in the menu permanently. Load failures are collected instead of printed. A `print()` per bad file means that in a release build the preset simply never appears, which is indistinguishable from one that was never saved — tolerable while the app wrote every preset itself, wrong once files arrive from elsewhere. They now surface as a red row in the preset menu with the filename, the reason in plain words, and the full path. Also fixes `saveAsPreset` never calling notifyListeners(), so a newly-saved preset only appeared once something unrelated rebuilt the toolbar. app/test/preset_service_test.dart is the first test over this service at all, which is how the three bugs survived; `directoryOverride` exists so it can run against a temp directory rather than the real $HOME. Not done: encoder-only profiles. A preset always carries the whole pipeline, so "just my ProRes output settings" cannot be expressed without also pinning someone's deinterlacer. That needs an optional-pipeline model change and is its own piece of work. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 44 +++- README.md | 4 +- app/lib/services/preset_service.dart | 272 ++++++++++++++++++--- app/lib/viewmodels/main_viewmodel.dart | 19 ++ app/lib/views/main_window.dart | 310 +++++++++++++++++++++++ app/test/preset_service_test.dart | 324 +++++++++++++++++++++++++ 6 files changed, 943 insertions(+), 30 deletions(-) create mode 100644 app/test/preset_service_test.dart diff --git a/CLAUDE.md b/CLAUDE.md index c049bdb7..3d99e467 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -802,7 +802,49 @@ a path. Presets save complete filter pipeline + encoding settings. The built-in presets are static factories in `app/lib/models/processing_preset.dart`, collected by `ProcessingPreset.builtInPresets()`, which `PresetService` calls in -`initialize()`/`reload()`. User presets save to `~/.vapourbox/presets/*.json`. +`initialize()`/`reload()`. User presets save to `~/.vapourbox/presets/*.json`, +and can be exported to and imported from any file (issue #81). + +> **A preset is executable, and import has to say so.** It carries +> `customVapoursynth` — Python the worker runs — and `customFfmpegArgs`, both of +> which survive a round trip. Importing someone else's preset is closer to +> running their script than to loading their settings, and **nothing else in the +> app would reveal it**: those fields only render in advanced mode. So import is +> two-phase. `inspectPresetFile` reads and validates without committing; +> the dialog shows any custom code verbatim; `commitImport` installs it, with +> `stripCustomCode` as the middle option between trusting everything and +> importing nothing. Don't collapse that into a one-shot `importPreset`. + +> **Never take `isBuiltIn` from a file** — the flag survives `fromJson`, and a +> built-in cannot be deleted or overwritten, so a preset claiming to be one +> would be stuck in the menu permanently. Both the load path and the import +> path force it false. + +> **The on-disk filename is the preset's id, not its name.** It used to be +> `.json`, and the sanitizer lowercases and collapses +> whitespace and separators — so `VHS Cleanup`, `vhs cleanup`, `VHS/Cleanup` +> and `VHS Cleanup` all produced `vhs_cleanup.json`. Saving one destroyed +> another; renaming left the original behind as a duplicate; and `deletePreset` +> removed the name-derived file *before* looking by id, so deleting one preset +> took a similarly-named one's file with it. Import makes all three easy to hit, +> since an imported preset is very likely to be named like an existing one. +> `savePreset` now also sweeps any other file carrying the same id, which +> migrates legacy name-based files as they are touched. + +> **Load failures are collected, not printed.** `_loadUserPresets` used to +> `print()` per bad file, which in a release build means the preset silently +> never appears — indistinguishable from one that was never saved. They now land +> in `PresetService.loadFailures` and the preset menu shows a red row. Tolerable +> when the app wrote every preset itself; wrong once files arrive from elsewhere. + +`app/test/preset_service_test.dart` is the first test over this service — set +`directoryOverride` to a temp dir, since the real path comes from `$HOME`. All +three filename bugs above have a regression test named after the behaviour. + +**Not done:** encoder-only profiles. A preset always carries the whole pipeline, +so "just my ProRes output settings" cannot be expressed without also pinning +someone's deinterlacer. That needs an optional-pipeline model change and is its +own piece of work. ## Hardware Encoders (issue #51) diff --git a/README.md b/README.md index e38cbd02..7f2d43bb 100644 --- a/README.md +++ b/README.md @@ -175,7 +175,9 @@ Presets store the whole pipeline plus encoding settings, and the menu splits the **Quality Only** — Fast, Balanced and High Quality just deinterlace, at three levels of effort. Use one when the picture is already clean. -Your own presets save alongside them and persist across sessions. +Your own presets save alongside them and persist across sessions, and can be **exported to a file and imported** — so a setup that works for a particular camcorder or capture card can be handed to someone else. + +An imported preset is shown to you before it is installed, because a preset can carry custom VapourSynth and FFmpeg arguments, and those run on your machine when you process a video. If it does, the code is displayed and you can import the preset without it. diff --git a/app/lib/services/preset_service.dart b/app/lib/services/preset_service.dart index 93d33b8b..46a54ed5 100644 --- a/app/lib/services/preset_service.dart +++ b/app/lib/services/preset_service.dart @@ -1,10 +1,77 @@ import 'dart:convert'; import 'dart:io'; +import 'package:flutter/foundation.dart'; import 'package:path/path.dart' as path; import '../models/processing_preset.dart'; +/// A preset file on disk that could not be read. +/// +/// These used to be swallowed by a `print()`, which in a release build means +/// the preset simply never appears and nothing says why. That is tolerable for +/// a file the app wrote itself; it is the wrong behaviour for one a user +/// hand-edited or was sent, which is exactly what import makes common. +class PresetLoadFailure { + const PresetLoadFailure(this.path, this.reason); + + /// Full path of the file that failed. + final String path; + + /// Why, in terms a user can act on. + final String reason; + + /// Just the filename, for display. + String get filename => path.split(RegExp(r'[/\\]')).last; +} + +/// What reading a preset file found, before anything is committed to disk. +/// +/// Import is two-phase on purpose: a preset carries `customVapoursynth` and +/// `customFfmpegArgs`, and the first of those is Python that the worker +/// executes. Importing someone else's preset is therefore closer to running +/// their script than to loading their settings, and the user has to be able to +/// see that before saying yes — so inspecting a file and committing it are +/// separate steps. +class PresetImportPreview { + const PresetImportPreview({ + this.preset, + this.error, + this.customVapoursynth = '', + this.customFfmpegArgs = '', + this.existingWithSameId, + this.existingWithSameName, + }); + + /// The preset read from the file, or null when [error] is set. + final ProcessingPreset? preset; + + /// Why the file could not be read, in terms a user can act on. + final String? error; + + /// Custom VapourSynth the preset would bring with it. **This is Python, run + /// in the worker process.** + final String customVapoursynth; + + /// Custom FFmpeg arguments the preset would bring with it. + final String customFfmpegArgs; + + /// An already-installed preset with the same id — i.e. this file is another + /// copy of one the user already has, so importing updates it in place. + final ProcessingPreset? existingWithSameId; + + /// An already-installed preset with the same *name* but a different id. + /// Both can coexist, but the menu would show two identical labels. + final ProcessingPreset? existingWithSameName; + + bool get ok => preset != null; + + /// Whether the file carries executable extras that deserve an explicit + /// yes before they are installed. + bool get carriesCustomCode => + customVapoursynth.trim().isNotEmpty || customFfmpegArgs.trim().isNotEmpty; +} + /// Service for loading and saving processing presets. /// /// Presets are stored in ~/.vapourbox/presets/ as JSON files. @@ -14,10 +81,22 @@ class PresetService { bool _isInitialized = false; final List _presets = []; + final List _loadFailures = []; + + /// Overrides the presets directory. Tests only — the real path is derived + /// from the home directory, which a test cannot safely move. + Directory? directoryOverride; /// Whether the preset service has been initialized. bool get isInitialized => _isInitialized; + /// Preset files that could not be read on the last load. + /// + /// Surfaced in the preset menu rather than only logged: a preset that + /// silently fails to appear is indistinguishable from one that was never + /// saved. + List get loadFailures => List.unmodifiable(_loadFailures); + /// All available presets (built-in + user). List get presets => List.unmodifiable(_presets); @@ -40,11 +119,15 @@ class PresetService { await _loadUserPresets(); _isInitialized = true; - print('PresetService: Loaded ${_presets.length} presets (${userPresets.length} user)'); + debugPrint('PresetService: loaded ${_presets.length} presets ' + '(${userPresets.length} user, ${_loadFailures.length} failed)'); } /// Get the presets directory path. Future getPresetsDirectory() async { + final override = directoryOverride; + if (override != null) return override; + String? home; if (Platform.isWindows) { home = Platform.environment['USERPROFILE']; @@ -70,6 +153,7 @@ class PresetService { /// Load user presets from disk. Future _loadUserPresets() async { + _loadFailures.clear(); try { final dir = await getPresetsDirectory(); if (!await dir.exists()) return; @@ -80,18 +164,39 @@ class PresetService { final content = await entity.readAsString(); final json = jsonDecode(content) as Map; final preset = ProcessingPreset.fromJson(json); - // Mark user presets as non-built-in + // Never take `isBuiltIn` from a file. A built-in cannot be deleted + // or overwritten, so a preset claiming to be one would be stuck in + // the menu permanently — and the flag survives decoding, verified. _presets.add(preset.copyWith(isBuiltIn: false)); } catch (e) { - print('Failed to load preset from ${entity.path}: $e'); + _loadFailures.add(PresetLoadFailure(entity.path, describeJsonError(e))); } } } } catch (e) { - print('Failed to load user presets: $e'); + _loadFailures.add(PresetLoadFailure( + (await getPresetsDirectory()).path, + 'The presets folder could not be read: $e', + )); } } + /// Turn a decode failure into something a user can act on. + /// + /// The raw exceptions are accurate and unreadable — a `FormatException` from + /// `jsonDecode` quotes a byte offset, and json_serializable throws a bare + /// `TypeError` naming Dart types the user has never heard of. + static String describeJsonError(Object e) { + if (e is FormatException) { + return 'The file is not valid JSON.'; + } + if (e is TypeError) { + return 'The file is JSON but not a VapourBox preset, or a setting in it ' + 'has the wrong type.'; + } + return e.toString(); + } + /// Save a preset to disk. Future savePreset(ProcessingPreset preset) async { if (preset.isBuiltIn) { @@ -99,11 +204,15 @@ class PresetService { } final dir = await _ensurePresetsDirectory(); - final filename = _sanitizeFilename(preset.name) + '.json'; - final file = File(path.join(dir.path, filename)); + final file = File(path.join(dir.path, _filenameFor(preset))); + + await file.writeAsString(encodePreset(preset), flush: true); - final json = jsonEncode(preset.toJson()); - await file.writeAsString(json, flush: true); + // Remove any older file holding this same preset under a different name. + // Filenames used to be derived from the preset's name, so renaming one + // left the original behind as a duplicate, and two names that sanitize + // alike ("VHS Cleanup" and "vhs cleanup") overwrote each other silently. + await _removeOtherFilesWithId(dir, preset.id, keep: file.path); // Update in-memory list final existingIndex = _presets.indexWhere((p) => p.id == preset.id); @@ -114,32 +223,135 @@ class PresetService { } } + /// The on-disk filename for a preset: its id, not its name. + /// + /// The name is still what the user sees — it lives inside the file — but it + /// makes a poor filename. Two different presets can share one, renaming + /// changes it, and sanitizing collapses distinct names together. + static String _filenameFor(ProcessingPreset preset) => '${preset.id}.json'; + + /// Pretty-printed, because a preset is now a file people send each other and + /// read; one long line is hostile to both. + static String encodePreset(ProcessingPreset preset) => + const JsonEncoder.withIndent(' ').convert(preset.toJson()); + + /// Delete every file in [dir] whose JSON carries [id], except [keep]. + Future _removeOtherFilesWithId( + Directory dir, + String id, { + required String keep, + }) async { + await for (final entity in dir.list()) { + if (entity is! File || !entity.path.endsWith('.json')) continue; + if (entity.path == keep) continue; + try { + final json = jsonDecode(await entity.readAsString()); + if (json is Map && json['id'] == id) { + await entity.delete(); + } + } catch (_) { + // An unreadable file is not ours to delete — it is reported as a load + // failure instead, where the user can decide. + } + } + } + + /// Write [preset] to [destinationPath] so it can be shared. + /// + /// Deliberately the same JSON as the on-disk copy, so exporting is a copy + /// and importing is a validated copy back. A separate "export format" would + /// be a second thing to keep in step for no gain. + Future exportPreset(ProcessingPreset preset, String destinationPath) async { + await File(destinationPath).writeAsString(encodePreset(preset), flush: true); + } + + /// The filename to suggest when exporting [preset]. + static String suggestedExportFilename(ProcessingPreset preset) { + final base = _sanitizeFilenameStatic(preset.name); + return '${base.isEmpty ? 'preset' : base}.json'; + } + + /// Read a preset file and report what importing it would mean, without + /// changing anything. + Future inspectPresetFile(String sourcePath) async { + final file = File(sourcePath); + if (!await file.exists()) { + return const PresetImportPreview(error: 'That file no longer exists.'); + } + + ProcessingPreset preset; + try { + final json = jsonDecode(await file.readAsString()); + if (json is! Map) { + return const PresetImportPreview( + error: 'That file is JSON, but not a preset.'); + } + // As on the load path: never trust `isBuiltIn` from a file. + preset = ProcessingPreset.fromJson(json).copyWith(isBuiltIn: false); + } catch (e) { + return PresetImportPreview(error: describeJsonError(e)); + } + + if (preset.name.trim().isEmpty) { + return const PresetImportPreview( + error: 'That preset has no name, so there would be nothing to pick ' + 'it by.'); + } + + return PresetImportPreview( + preset: preset, + customVapoursynth: preset.encodingSettings.customVapoursynth, + customFfmpegArgs: preset.encodingSettings.customFfmpegArgs, + existingWithSameId: findById(preset.id), + existingWithSameName: _presets + .where((p) => p.name == preset.name && p.id != preset.id) + .firstOrNull, + ); + } + + /// Install a preset that [inspectPresetFile] already validated. + /// + /// [stripCustomCode] drops the custom VapourSynth and FFmpeg arguments, + /// which is the safe way to accept someone else's filter settings without + /// also accepting their Python. + Future commitImport( + PresetImportPreview preview, { + bool stripCustomCode = false, + }) async { + final source = preview.preset; + if (source == null) { + throw StateError('commitImport called on a preview that failed to read'); + } + + var preset = source.copyWith(isBuiltIn: false); + if (stripCustomCode) { + preset = preset.copyWith( + encodingSettings: preset.encodingSettings.copyWith( + customVapoursynth: '', + customFfmpegArgs: '', + ), + ); + } + + await savePreset(preset); + return preset; + } + /// Delete a user preset. Future deletePreset(ProcessingPreset preset) async { if (preset.isBuiltIn) { throw ArgumentError('Cannot delete built-in presets'); } - // Remove from disk + // Remove from disk, by id only. + // + // This used to delete `.json` first and then scan for the + // id, which deletes the wrong file whenever two presets' names sanitize to + // the same thing — "VHS Cleanup" and "vhs cleanup" both produce + // `vhs_cleanup.json`. Deleting one would take the other's file with it. final dir = await getPresetsDirectory(); - final filename = _sanitizeFilename(preset.name) + '.json'; - final file = File(path.join(dir.path, filename)); - if (await file.exists()) { - await file.delete(); - } - - // Also try finding by ID in case filename doesn't match - await for (final entity in dir.list()) { - if (entity is File && entity.path.endsWith('.json')) { - try { - final content = await entity.readAsString(); - final json = jsonDecode(content) as Map; - if (json['id'] == preset.id) { - await entity.delete(); - break; - } - } catch (_) {} - } + if (await dir.exists()) { + await _removeOtherFilesWithId(dir, preset.id, keep: ''); } // Remove from in-memory list @@ -164,7 +376,11 @@ class PresetService { } /// Sanitize a filename by removing invalid characters. - String _sanitizeFilename(String name) { + /// + /// No longer decides where a preset is stored — that is the id now — so this + /// only has to produce something reasonable to *suggest* in a save dialog. + /// Collisions here are harmless: the user sees the name and can change it. + static String _sanitizeFilenameStatic(String name) { return name .replaceAll(RegExp(r'[<>:"/\\|?*]'), '_') .replaceAll(RegExp(r'\s+'), '_') diff --git a/app/lib/viewmodels/main_viewmodel.dart b/app/lib/viewmodels/main_viewmodel.dart index 140a0077..0c98635f 100644 --- a/app/lib/viewmodels/main_viewmodel.dart +++ b/app/lib/viewmodels/main_viewmodel.dart @@ -1733,9 +1733,28 @@ class MainViewModel extends ChangeNotifier { ); await PresetService.instance.savePreset(preset); + // The preset menu is built from the service, so it has to be told — this + // was missing, and a newly saved preset only appeared after some unrelated + // change happened to rebuild the toolbar. + notifyListeners(); return preset; } + /// Install a preset read from a file, and tell the UI it exists. + /// + /// Deliberately does not apply it: importing is "add this to my presets", + /// and replacing the user's current pipeline as a side effect of that would + /// be destructive with no undo. The menu is one click away. + Future importPreset( + PresetImportPreview preview, { + bool stripCustomCode = false, + }) async { + final imported = await PresetService.instance + .commitImport(preview, stripCustomCode: stripCustomCode); + notifyListeners(); + return imported; + } + /// Update an existing user preset with current settings. Future updatePreset(ProcessingPreset existing) async { final updated = existing.copyWith( diff --git a/app/lib/views/main_window.dart b/app/lib/views/main_window.dart index 5b5c8a58..15fbe7cd 100644 --- a/app/lib/views/main_window.dart +++ b/app/lib/views/main_window.dart @@ -1,5 +1,6 @@ import 'dart:io'; +import 'package:file_picker/file_picker.dart'; import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; import 'package:provider/provider.dart'; @@ -22,6 +23,7 @@ import 'progress_panel.dart'; import 'queue_panel.dart'; import 'settings/settings_dialog.dart'; import '../widgets/resizable_split.dart'; +import '../widgets/warning_banner.dart'; class MainWindow extends StatelessWidget { const MainWindow({super.key}); @@ -96,6 +98,18 @@ class MainWindow extends StatelessWidget { onSelected: (value) async { if (value == 'save') { _showSavePresetDialog(context, viewModel); + } else if (value == 'import') { + await _importPreset(context, viewModel); + } else if (value == 'loadErrors') { + await _showPresetLoadFailures(context); + } else if (value.startsWith('export:')) { + final presetId = value.substring(7); + final preset = viewModel.availablePresets + .where((p) => p.id == presetId) + .firstOrNull; + if (preset != null) { + await _exportPreset(context, preset); + } } else if (value.startsWith('load:')) { final presetId = value.substring(5); final preset = viewModel.availablePresets.where((p) => p.id == presetId).firstOrNull; @@ -142,6 +156,7 @@ class MainWindow extends StatelessWidget { }, itemBuilder: (context) { final presets = viewModel.availablePresets; + final failures = PresetService.instance.loadFailures; final user = presets.where((p) => !p.isBuiltIn).toList(); // Split the built-ins by what question they answer. "How hard @@ -215,8 +230,16 @@ class MainWindow extends StatelessWidget { Navigator.pop(context, 'update:${p.id}'); }, ), + IconButton( + icon: const Icon(Icons.ios_share, size: 18), + tooltip: 'Export to a file', + onPressed: () { + Navigator.pop(context, 'export:${p.id}'); + }, + ), IconButton( icon: const Icon(Icons.delete, size: 18), + tooltip: 'Delete', onPressed: () { Navigator.pop(context, 'delete:${p.id}'); }, @@ -235,6 +258,37 @@ class MainWindow extends StatelessWidget { title: Text('Save Current Settings...'), ), ), + const PopupMenuItem( + value: 'import', + child: ListTile( + contentPadding: EdgeInsets.zero, + dense: true, + leading: Icon(Icons.file_open), + title: Text('Import Preset...'), + ), + ), + // Preset files that could not be read. Silence here is what + // made a corrupt or hand-edited preset indistinguishable from + // one that was never saved. + if (failures.isNotEmpty) ...[ + const PopupMenuDivider(), + PopupMenuItem( + value: 'loadErrors', + child: ListTile( + contentPadding: EdgeInsets.zero, + dense: true, + leading: Icon(Icons.error_outline, + color: Theme.of(context).colorScheme.error), + title: Text( + failures.length == 1 + ? '1 preset could not be loaded' + : '${failures.length} presets could not be loaded', + style: TextStyle( + color: Theme.of(context).colorScheme.error), + ), + ), + ), + ], ]; }, ), @@ -693,6 +747,252 @@ class MainWindow extends StatelessWidget { } } + /// Write a preset to a file the user picks, so it can be shared. + Future _exportPreset(BuildContext context, ProcessingPreset preset) async { + final messenger = ScaffoldMessenger.of(context); + // Captured before any await: the context may be gone by the time a + // failure needs reporting. + final errorColour = Theme.of(context).colorScheme.error; + try { + final destination = await FilePicker.platform.saveFile( + dialogTitle: 'Export "${preset.name}"', + fileName: PresetService.suggestedExportFilename(preset), + type: FileType.custom, + allowedExtensions: const ['json'], + ); + // A cancelled picker returns null; that is not a failure. + if (destination == null) return; + + // Some platforms return the name without the extension the filter implies. + final path = + destination.toLowerCase().endsWith('.json') ? destination : '$destination.json'; + + await PresetService.instance.exportPreset(preset, path); + messenger.showSnackBar( + SnackBar(content: Text('Exported "${preset.name}"')), + ); + } catch (e) { + messenger.showSnackBar(SnackBar( + content: Text('Could not export the preset: $e'), + backgroundColor: errorColour, + )); + } + } + + /// Read a preset file the user picks, confirm anything executable in it, and + /// install it. + Future _importPreset(BuildContext context, MainViewModel viewModel) async { + final messenger = ScaffoldMessenger.of(context); + final errorColour = Theme.of(context).colorScheme.error; + + final result = await FilePicker.platform.pickFiles( + dialogTitle: 'Import a preset', + type: FileType.custom, + allowedExtensions: const ['json'], + ); + if (result == null || result.files.isEmpty) return; + final sourcePath = result.files.first.path; + if (sourcePath == null) return; + + final preview = await PresetService.instance.inspectPresetFile(sourcePath); + if (!preview.ok) { + messenger.showSnackBar(SnackBar( + content: Text(preview.error ?? 'That file is not a preset.'), + backgroundColor: errorColour, + )); + return; + } + + if (!context.mounted) return; + final decision = await _confirmImport(context, preview); + if (decision == null) return; + + try { + final imported = await viewModel.importPreset( + preview, + stripCustomCode: decision == _ImportChoice.withoutCustomCode, + ); + messenger.showSnackBar(SnackBar( + content: Text(preview.existingWithSameId != null + ? 'Updated "${imported.name}"' + : 'Imported "${imported.name}"'), + )); + } catch (e) { + messenger.showSnackBar(SnackBar( + content: Text('Could not import the preset: $e'), + backgroundColor: errorColour, + )); + } + } + + /// Ask before installing an imported preset. + /// + /// A plain preset gets a short confirmation. One carrying custom VapourSynth + /// gets the code shown verbatim, because that is Python the worker will run + /// — importing someone's preset is closer to running their script than to + /// loading their settings, and nothing else in the app would reveal it: the + /// custom-code fields are hidden unless advanced mode is on. + Future<_ImportChoice?> _confirmImport( + BuildContext context, PresetImportPreview preview) async { + final preset = preview.preset!; + final theme = Theme.of(context); + + return showDialog<_ImportChoice>( + context: context, + builder: (context) => AlertDialog( + title: Text(preview.existingWithSameId != null + ? 'Update "${preview.existingWithSameId!.name}"?' + : 'Import "${preset.name}"?'), + content: ConstrainedBox( + constraints: const BoxConstraints(maxWidth: 520), + child: SingleChildScrollView( + child: Column( + mainAxisSize: MainAxisSize.min, + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + if (preset.description != null && + preset.description!.trim().isNotEmpty) ...[ + Text(preset.description!), + const SizedBox(height: 12), + ], + Text( + '${preset.pipeline.enabledPassCount} processing ' + '${preset.pipeline.enabledPassCount == 1 ? "pass" : "passes"}, ' + 'output as ${preset.encodingSettings.codec.displayName}.', + style: theme.textTheme.bodySmall, + ), + if (preview.existingWithSameId != null) ...[ + const SizedBox(height: 12), + const WarningBanner( + message: 'You already have this preset. Importing replaces ' + 'it with the version in this file.', + ), + ] else if (preview.existingWithSameName != null) ...[ + const SizedBox(height: 12), + WarningBanner( + message: 'You already have a different preset called ' + '"${preset.name}". Both will be kept, so the menu will ' + 'show the name twice.', + ), + ], + if (preview.carriesCustomCode) ...[ + const SizedBox(height: 16), + const WarningBanner( + message: 'This preset carries custom code, which runs on ' + 'your machine when you process a video. Only accept it ' + 'from someone you trust.', + ), + const SizedBox(height: 12), + if (preview.customVapoursynth.trim().isNotEmpty) + _codeBlock(context, 'Custom VapourSynth (Python)', + preview.customVapoursynth), + if (preview.customFfmpegArgs.trim().isNotEmpty) + _codeBlock(context, 'Custom FFmpeg arguments', + preview.customFfmpegArgs), + ], + ], + ), + ), + ), + actions: [ + TextButton( + onPressed: () => Navigator.pop(context), + child: const Text('Cancel'), + ), + // Offered only when there is something to leave out. Taking the + // filter settings without the code is the useful middle option, and + // without it the choice is trust-everything or nothing. + if (preview.carriesCustomCode) + TextButton( + onPressed: () => + Navigator.pop(context, _ImportChoice.withoutCustomCode), + child: const Text('Import without the code'), + ), + FilledButton( + onPressed: () => Navigator.pop(context, _ImportChoice.asIs), + child: Text(preview.existingWithSameId != null ? 'Replace' : 'Import'), + ), + ], + ), + ); + } + + Widget _codeBlock(BuildContext context, String label, String code) { + final theme = Theme.of(context); + return Padding( + padding: const EdgeInsets.only(bottom: 12), + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + Text(label, style: theme.textTheme.labelSmall), + const SizedBox(height: 4), + Container( + width: double.infinity, + padding: const EdgeInsets.all(8), + decoration: BoxDecoration( + color: theme.colorScheme.surfaceContainerHighest, + borderRadius: BorderRadius.circular(4), + ), + child: SelectableText( + code.trim(), + style: const TextStyle(fontFamily: 'monospace', fontSize: 12), + ), + ), + ], + ), + ); + } + + /// List the preset files that could not be read, and where they are. + Future _showPresetLoadFailures(BuildContext context) async { + final failures = PresetService.instance.loadFailures; + if (failures.isEmpty) return; + + await showDialog( + context: context, + builder: (context) => AlertDialog( + title: Text(failures.length == 1 + ? 'A preset could not be loaded' + : '${failures.length} presets could not be loaded'), + content: ConstrainedBox( + constraints: const BoxConstraints(maxWidth: 560), + child: SingleChildScrollView( + child: Column( + mainAxisSize: MainAxisSize.min, + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + for (final f in failures) ...[ + Text(f.filename, + style: const TextStyle(fontWeight: FontWeight.bold)), + Text(f.reason), + const SizedBox(height: 4), + SelectableText( + f.path, + style: TextStyle( + fontFamily: 'monospace', + fontSize: 11, + color: Theme.of(context) + .colorScheme + .onSurface + .withValues(alpha: 0.6), + ), + ), + const SizedBox(height: 16), + ], + ], + ), + ), + ), + actions: [ + TextButton( + onPressed: () => Navigator.pop(context), + child: const Text('Close'), + ), + ], + ), + ); + } + void _showSavePresetDialog(BuildContext context, MainViewModel viewModel) { final nameController = TextEditingController(); final descriptionController = TextEditingController(); @@ -879,3 +1179,13 @@ class MainWindow extends StatelessWidget { return existingFiles; } } + +/// What the user chose in the import confirmation. +enum _ImportChoice { + /// Install the preset exactly as the file describes it. + asIs, + + /// Install it without the custom VapourSynth and FFmpeg arguments — the + /// useful middle option between trusting everything and importing nothing. + withoutCustomCode, +} diff --git a/app/test/preset_service_test.dart b/app/test/preset_service_test.dart new file mode 100644 index 00000000..ee2a5729 --- /dev/null +++ b/app/test/preset_service_test.dart @@ -0,0 +1,324 @@ +// Disk behaviour of PresetService: saving, deleting, and the import/export +// added for issue #81's fourth ask. +// +// There were no tests over this service at all, which is how three bugs +// survived in it — see the groups below. `directoryOverride` exists so these +// can run against a temp directory instead of the real ~/.vapourbox/presets. +// +// Run with: flutter test test/preset_service_test.dart + +import 'dart:convert'; +import 'dart:io'; + +import 'package:flutter_test/flutter_test.dart'; +import 'package:vapourbox/models/encoding_settings.dart'; +import 'package:vapourbox/models/processing_pipeline.dart'; +import 'package:vapourbox/models/processing_preset.dart'; +import 'package:vapourbox/models/qtgmc_parameters.dart'; +import 'package:vapourbox/services/preset_service.dart'; + +void main() { + late Directory tempDir; + late PresetService service; + + setUp(() async { + tempDir = await Directory.systemTemp.createTemp('vapourbox-presets-test'); + service = PresetService.instance; + service.directoryOverride = tempDir; + await service.reload(); + }); + + tearDown(() async { + service.directoryOverride = null; + if (await tempDir.exists()) { + await tempDir.delete(recursive: true); + } + }); + + ProcessingPreset makePreset( + String name, { + String? id, + String customVapoursynth = '', + String customFfmpegArgs = '', + }) => + ProcessingPreset( + id: id, + name: name, + pipeline: const ProcessingPipeline( + deinterlace: QTGMCParameters(enabled: false), + ), + encodingSettings: EncodingSettings( + customVapoursynth: customVapoursynth, + customFfmpegArgs: customFfmpegArgs, + ), + ); + + Future> presetFiles() async => tempDir + .listSync() + .whereType() + .where((f) => f.path.endsWith('.json')) + .map((f) => f.uri.pathSegments.last) + .toList() + ..sort(); + + group('saving', () { + test('round-trips through disk', () async { + await service.savePreset(makePreset('My Preset')); + await service.reload(); + + final loaded = service.findByName('My Preset'); + expect(loaded, isNotNull); + expect(loaded!.isBuiltIn, isFalse); + }); + + test('names the file after the id, not the name', () async { + final preset = makePreset('My Preset'); + await service.savePreset(preset); + expect(await presetFiles(), ['${preset.id}.json']); + }); + + test('renaming does not leave the old file behind', () async { + // The filename used to come from the name, so a rename wrote a second + // file and the original stayed — the preset appeared twice after reload. + final preset = makePreset('Before'); + await service.savePreset(preset); + await service.savePreset(preset.copyWith(name: 'After')); + + expect(await presetFiles(), hasLength(1)); + await service.reload(); + expect(service.userPresets.map((p) => p.name), ['After']); + }); + + test('two names that sanitize alike do not overwrite each other', () async { + // Verified against the old sanitizer: "VHS Cleanup", "vhs cleanup", + // "VHS/Cleanup" and "VHS Cleanup" all produced vhs_cleanup.json, so + // saving one destroyed the other. Import makes this easy to hit, because + // the imported preset is very likely to be named like an existing one. + await service.savePreset(makePreset('VHS Cleanup')); + await service.savePreset(makePreset('vhs cleanup')); + await service.savePreset(makePreset('VHS/Cleanup')); + + expect(await presetFiles(), hasLength(3)); + await service.reload(); + expect(service.userPresets, hasLength(3)); + }); + + test('refuses to save a built-in', () async { + final builtIn = ProcessingPreset.builtInPresets().first; + expect(() => service.savePreset(builtIn), throwsArgumentError); + }); + }); + + group('deleting', () { + test('removes only the named preset', () async { + // The old implementation deleted `.json` first and only + // then looked by id, so deleting "vhs cleanup" took "VHS Cleanup"'s file. + final keep = makePreset('VHS Cleanup'); + final drop = makePreset('vhs cleanup'); + await service.savePreset(keep); + await service.savePreset(drop); + + await service.deletePreset(drop); + await service.reload(); + + expect(service.userPresets.map((p) => p.name), ['VHS Cleanup']); + }); + }); + + group('load failures are reported, not swallowed', () { + test('malformed JSON is named with a readable reason', () async { + await File('${tempDir.path}/broken.json').writeAsString('{not json'); + await service.reload(); + + expect(service.loadFailures, hasLength(1)); + expect(service.loadFailures.single.filename, 'broken.json'); + expect(service.loadFailures.single.reason, contains('not valid JSON')); + }); + + test('valid JSON that is not a preset is distinguished', () async { + await File('${tempDir.path}/other.json').writeAsString('{"hello":"world"}'); + await service.reload(); + + expect(service.loadFailures, hasLength(1)); + expect(service.loadFailures.single.reason, contains('not a VapourBox preset')); + }); + + test('one bad file does not stop the others loading', () async { + await service.savePreset(makePreset('Good')); + await File('${tempDir.path}/broken.json').writeAsString('nonsense'); + await service.reload(); + + expect(service.findByName('Good'), isNotNull); + expect(service.loadFailures, hasLength(1)); + }); + + test('a clean folder reports nothing', () async { + await service.savePreset(makePreset('Good')); + await service.reload(); + expect(service.loadFailures, isEmpty); + }); + }); + + group('export', () { + test('writes JSON that imports back unchanged', () async { + final preset = makePreset('Shared'); + final dest = '${tempDir.path}/exported-elsewhere.json'; + await service.exportPreset(preset, dest); + + final preview = await service.inspectPresetFile(dest); + expect(preview.ok, isTrue, reason: preview.error); + expect(preview.preset!.name, 'Shared'); + expect(preview.preset!.id, preset.id); + }); + + test('is pretty-printed, because people read and send these', () async { + final dest = '${tempDir.path}/exported.json'; + await service.exportPreset(makePreset('Shared'), dest); + expect(await File(dest).readAsString(), contains('\n ')); + }); + + test('suggests a filename from the name', () { + expect(PresetService.suggestedExportFilename(makePreset('My VHS Preset')), + 'my_vhs_preset.json'); + // A name made entirely of separators must still produce something. + expect(PresetService.suggestedExportFilename(makePreset('///')), + isNot(startsWith('.'))); + }); + }); + + group('import', () { + Future writeFile(String name, Object json) async { + final f = File('${tempDir.path}/$name'); + await f.writeAsString(jsonEncode(json)); + return f.path; + } + + test('reports a readable error rather than throwing', () async { + final p = await writeFile('bad.json', {'nope': true}); + final preview = await service.inspectPresetFile(p); + + expect(preview.ok, isFalse); + expect(preview.error, contains('not a VapourBox preset')); + }); + + test('reports a missing file', () async { + final preview = + await service.inspectPresetFile('${tempDir.path}/nothing.json'); + expect(preview.ok, isFalse); + expect(preview.error, contains('no longer exists')); + }); + + test('rejects a preset with no name', () async { + final p = await writeFile('noname.json', makePreset(' ').toJson()); + final preview = await service.inspectPresetFile(p); + expect(preview.ok, isFalse); + expect(preview.error, contains('no name')); + }); + + test('never trusts isBuiltIn from the file', () async { + // Verified: the flag survives fromJson. A preset claiming to be built-in + // could not then be deleted or overwritten, so it would be stuck in the + // menu permanently. + final json = makePreset('Sneaky').toJson()..['isBuiltIn'] = true; + final p = await writeFile('sneaky.json', json); + + final preview = await service.inspectPresetFile(p); + expect(preview.preset!.isBuiltIn, isFalse); + + final imported = await service.commitImport(preview); + expect(imported.isBuiltIn, isFalse); + await service.reload(); + expect(service.findByName('Sneaky')!.isBuiltIn, isFalse); + }); + + test('flags custom code, which is the whole reason import confirms', () async { + final p = await writeFile( + 'custom.json', + makePreset('Loaded', + customVapoursynth: 'import os', + customFfmpegArgs: '-metadata x=1') + .toJson(), + ); + + final preview = await service.inspectPresetFile(p); + expect(preview.carriesCustomCode, isTrue); + expect(preview.customVapoursynth, 'import os'); + expect(preview.customFfmpegArgs, '-metadata x=1'); + }); + + test('an ordinary preset carries no custom code', () async { + final p = await writeFile('plain.json', makePreset('Plain').toJson()); + expect((await service.inspectPresetFile(p)).carriesCustomCode, isFalse); + }); + + test('whitespace-only custom code does not count', () async { + final p = await writeFile('ws.json', + makePreset('WS', customVapoursynth: ' \n ').toJson()); + expect((await service.inspectPresetFile(p)).carriesCustomCode, isFalse); + }); + + test('can strip the custom code and keep the settings', () async { + final p = await writeFile( + 'custom.json', + makePreset('Loaded', + customVapoursynth: 'import os', customFfmpegArgs: '-x 1') + .toJson(), + ); + final preview = await service.inspectPresetFile(p); + + final imported = await service.commitImport(preview, stripCustomCode: true); + expect(imported.encodingSettings.customVapoursynth, isEmpty); + expect(imported.encodingSettings.customFfmpegArgs, isEmpty); + + await service.reload(); + final onDisk = service.findByName('Loaded')!; + expect(onDisk.encodingSettings.customVapoursynth, isEmpty); + }); + + test('keeps the custom code when not asked to strip it', () async { + final p = await writeFile( + 'custom.json', makePreset('Kept', customVapoursynth: 'x = 1').toJson()); + final preview = await service.inspectPresetFile(p); + + final imported = await service.commitImport(preview); + expect(imported.encodingSettings.customVapoursynth, 'x = 1'); + }); + + test('spots an update to a preset already installed', () async { + final original = makePreset('Mine'); + await service.savePreset(original); + + final p = await writeFile('again.json', + original.copyWith(name: 'Mine, revised').toJson()); + final preview = await service.inspectPresetFile(p); + + expect(preview.existingWithSameId, isNotNull); + expect(preview.existingWithSameName, isNull); + + // Importing it updates in place rather than adding a second copy. + await service.commitImport(preview); + await service.reload(); + expect(service.userPresets, hasLength(1)); + expect(service.userPresets.single.name, 'Mine, revised'); + }); + + test('spots a different preset sharing a name', () async { + await service.savePreset(makePreset('VHS Cleanup')); + final p = await writeFile('theirs.json', makePreset('VHS Cleanup').toJson()); + + final preview = await service.inspectPresetFile(p); + expect(preview.existingWithSameId, isNull); + expect(preview.existingWithSameName, isNotNull); + + // Both survive — the ids differ, so they are genuinely two presets. + await service.commitImport(preview); + await service.reload(); + expect(service.userPresets, hasLength(2)); + }); + + test('commitImport refuses a preview that failed', () async { + const bad = PresetImportPreview(error: 'nope'); + expect(() => service.commitImport(bad), throwsStateError); + }); + }); +}