From 1cdf4b454707db2695a85e7c2dbd9bfc353bffb0 Mon Sep 17 00:00:00 2001 From: zarzet Date: Mon, 27 Jul 2026 14:24:39 +0700 Subject: [PATCH] fix(library): retain records when deletion fails --- lib/screens/downloaded_album_screen.dart | 5 ++- lib/screens/local_album_screen.dart | 5 ++- lib/screens/queue_tab_selection.dart | 7 ++--- lib/screens/track_metadata_actions.dart | 35 +++++++++++++-------- lib/utils/file_access.dart | 40 ++++++++++++++++++------ lib/widgets/duplicate_review_sheet.dart | 18 ++++++----- test/models_and_utils_test.dart | 24 ++++++++++++++ 7 files changed, 94 insertions(+), 40 deletions(-) diff --git a/lib/screens/downloaded_album_screen.dart b/lib/screens/downloaded_album_screen.dart index 579bc993..8bb3958d 100644 --- a/lib/screens/downloaded_album_screen.dart +++ b/lib/screens/downloaded_album_screen.dart @@ -157,9 +157,8 @@ class _DownloadedAlbumScreenState extends ConsumerState deleteItem: (id) async { final item = tracksById[id]; if (item == null) return false; - try { - await deleteFile(item.filePath); - } catch (_) {} + final deleted = await deleteFile(item.filePath); + if (!deleted) return false; historyNotifier.removeFromHistory(id); return true; }, diff --git a/lib/screens/local_album_screen.dart b/lib/screens/local_album_screen.dart index 4d8cb257..a531431d 100644 --- a/lib/screens/local_album_screen.dart +++ b/lib/screens/local_album_screen.dart @@ -130,9 +130,8 @@ class _LocalAlbumScreenState extends ConsumerState final item = tracksById[id]; if (item == null) return false; if (!isCueVirtualPath(item.filePath)) { - try { - await deleteFile(item.filePath); - } catch (_) {} + final deleted = await deleteFile(item.filePath); + if (!deleted) return false; } await libraryNotifier.removeItem(id); return true; diff --git a/lib/screens/queue_tab_selection.dart b/lib/screens/queue_tab_selection.dart index d1b943e2..231574f2 100644 --- a/lib/screens/queue_tab_selection.dart +++ b/lib/screens/queue_tab_selection.dart @@ -476,10 +476,9 @@ extension _QueueTabSelectionActions on _QueueTabState { for (final id in _selectedIds) { final item = itemsById[id]; if (item != null) { - try { - final cleanPath = _cleanFilePath(item.filePath); - await deleteFile(cleanPath); - } catch (_) {} + final cleanPath = _cleanFilePath(item.filePath); + final fileDeleted = await deleteFile(cleanPath); + if (!fileDeleted) continue; if (item.source == LibraryItemSource.downloaded) { historyNotifier.removeFromHistory(item.historyItem!.id); diff --git a/lib/screens/track_metadata_actions.dart b/lib/screens/track_metadata_actions.dart index f0519b73..0398fd91 100644 --- a/lib/screens/track_metadata_actions.dart +++ b/lib/screens/track_metadata_actions.dart @@ -120,33 +120,42 @@ extension _TrackMetadataFileActions on _TrackMetadataScreenState { ), TextButton( onPressed: () async { + var fileDeleted = true; if (_isLocalItem) { if (_isCueVirtualTrack && _localLibraryItem != null) { await ref .read(localLibraryProvider.notifier) .removeItem(_localLibraryItem!.id); } else { - try { - await deleteFile(cleanFilePath); - } catch (e) { - debugPrint('Failed to delete file: $e'); - } - if (_localLibraryItem != null) { + fileDeleted = await deleteFile(cleanFilePath); + if (fileDeleted && _localLibraryItem != null) { await ref .read(localLibraryProvider.notifier) .removeItem(_localLibraryItem!.id); } } } else { - try { - await deleteFile(cleanFilePath); - } catch (e) { - debugPrint('Failed to delete file: $e'); + fileDeleted = await deleteFile(cleanFilePath); + if (fileDeleted) { + ref + .read(downloadHistoryProvider.notifier) + .removeFromHistory(_downloadItem!.id); } + } - ref - .read(downloadHistoryProvider.notifier) - .removeFromHistory(_downloadItem!.id); + if (!fileDeleted) { + if (screenContext.mounted) { + ScaffoldMessenger.of(screenContext).showSnackBar( + SnackBar( + content: Text( + screenContext.l10n.snackbarError( + screenContext.l10n.snackbarFailedToWriteStorage, + ), + ), + ), + ); + } + return; } if (dialogContext.mounted) { diff --git a/lib/utils/file_access.dart b/lib/utils/file_access.dart index 309bfc9c..4db8ace8 100644 --- a/lib/utils/file_access.dart +++ b/lib/utils/file_access.dart @@ -272,21 +272,43 @@ Future fileExists(String? path) async { return File(realPath).exists(); } -Future deleteFile(String? path) async { - if (path == null || path.isEmpty) return; +/// Deletes [path] and reports whether the file is confirmed absent afterward. +/// +/// SAF providers are allowed to reject a delete request by returning `false`. +/// Callers that also remove a Library row must only do so when this returns +/// `true`, otherwise the app would hide a file that still exists on storage. +Future deleteFile(String? path) async { + if (path == null || path.isEmpty) return false; // CUE virtual paths should NOT be deleted through this function — // deleting album.cue would remove ALL tracks. Callers should handle // CUE deletion specially (e.g. only delete when all tracks are removed). - if (isCueVirtualPath(path)) return; + if (isCueVirtualPath(path)) return false; if (isContentUri(path)) { - await PlatformBridge.safDelete(path); - await musicPlayerHandler?.onSourceDeleted(path); - return; + try { + final deleted = await PlatformBridge.safDelete(path); + final confirmedAbsent = deleted || !await PlatformBridge.safExists(path); + if (confirmedAbsent) { + await musicPlayerHandler?.onSourceDeleted(path); + } + return confirmedAbsent; + } catch (_) { + return false; + } } + + final file = File(path); try { - await File(path).delete(); - } catch (_) {} - await musicPlayerHandler?.onSourceDeleted(path); + if (await file.exists()) { + await file.delete(); + } + final confirmedAbsent = !await file.exists(); + if (confirmedAbsent) { + await musicPlayerHandler?.onSourceDeleted(path); + } + return confirmedAbsent; + } catch (_) { + return false; + } } Future fileStat(String? path) async { diff --git a/lib/widgets/duplicate_review_sheet.dart b/lib/widgets/duplicate_review_sheet.dart index 55498ec2..164f3bbc 100644 --- a/lib/widgets/duplicate_review_sheet.dart +++ b/lib/widgets/duplicate_review_sheet.dart @@ -74,7 +74,9 @@ class _DuplicateReviewSheetState extends ConsumerState { if (bitDepth > 0 && sampleRate > 0) { final khz = sampleRate / 1000; final khzText = khz % 1 == 0 ? khz.toInt().toString() : khz.toString(); - return format.isEmpty ? '$bitDepth/$khzText' : '$bitDepth/$khzText $format'; + return format.isEmpty + ? '$bitDepth/$khzText' + : '$bitDepth/$khzText $format'; } final bitrate = entry.bitrate ?? 0; if (bitrate > 0) { @@ -112,11 +114,10 @@ class _DuplicateReviewSheetState extends ConsumerState { final historyNotifier = ref.read(downloadHistoryProvider.notifier); var deleted = 0; for (final entry in entries) { - try { - await deleteFile( - DownloadedEmbeddedCoverResolver.cleanFilePath(entry.filePath), - ); - } catch (_) {} + final fileDeleted = await deleteFile( + DownloadedEmbeddedCoverResolver.cleanFilePath(entry.filePath), + ); + if (!fileDeleted) continue; if (entry.source == 'downloaded') { historyNotifier.removeFromHistory(entry.id); } else { @@ -200,8 +201,9 @@ class _DuplicateReviewSheetState extends ConsumerState { padding: const EdgeInsets.fromLTRB(24, 8, 24, 32), child: Text( context.l10n.duplicatesEmpty, - style: Theme.of(context).textTheme.bodyMedium - ?.copyWith(color: colorScheme.onSurfaceVariant), + style: Theme.of(context).textTheme.bodyMedium?.copyWith( + color: colorScheme.onSurfaceVariant, + ), ), ); } diff --git a/test/models_and_utils_test.dart b/test/models_and_utils_test.dart index e6059204..c9943cf8 100644 --- a/test/models_and_utils_test.dart +++ b/test/models_and_utils_test.dart @@ -14,11 +14,35 @@ import 'package:spotiflac_android/services/history_database.dart'; import 'package:spotiflac_android/utils/artist_utils.dart'; import 'package:spotiflac_android/utils/audio_conversion_utils.dart'; import 'package:spotiflac_android/utils/audio_format_utils.dart'; +import 'package:spotiflac_android/utils/file_access.dart'; import 'package:spotiflac_android/utils/mime_utils.dart'; import 'package:spotiflac_android/utils/path_match_keys.dart'; import 'package:spotiflac_android/utils/string_utils.dart'; void main() { + group('file deletion', () { + test('confirms a local file is absent before reporting success', () async { + final tempDir = await Directory.systemTemp.createTemp( + 'spotiflac-delete-test-', + ); + addTearDown(() async { + if (await tempDir.exists()) { + await tempDir.delete(recursive: true); + } + }); + final file = File('${tempDir.path}${Platform.pathSeparator}track.flac'); + await file.writeAsBytes([1, 2, 3]); + + expect(await deleteFile(file.path), isTrue); + expect(await file.exists(), isFalse); + expect(await deleteFile(file.path), isTrue); + }); + + test('refuses to delete a virtual CUE track path', () async { + expect(await deleteFile('/music/album.cue#track01'), isFalse); + }); + }); + group('native worker progress', () { test('does not publish 100 percent while finalization is pending', () { expect(nativeWorkerFinalizingProgress(0), 0.95);