From c35b11a155e3deb758905769468f27d1f447b9fc Mon Sep 17 00:00:00 2001 From: zarzet Date: Thu, 23 Jul 2026 17:40:09 +0700 Subject: [PATCH] fix(library): preserve downloaded file records --- lib/providers/download_history_provider.dart | 233 +++++++++++++++---- lib/services/history_database.dart | 25 ++ test/download_history_logic_test.dart | 133 +++++++++++ 3 files changed, 345 insertions(+), 46 deletions(-) create mode 100644 test/download_history_logic_test.dart diff --git a/lib/providers/download_history_provider.dart b/lib/providers/download_history_provider.dart index f89f97f2..fe11ed5a 100644 --- a/lib/providers/download_history_provider.dart +++ b/lib/providers/download_history_provider.dart @@ -11,9 +11,39 @@ import 'package:spotiflac_android/utils/file_access.dart'; import 'package:spotiflac_android/utils/string_utils.dart'; import 'package:spotiflac_android/utils/audio_format_utils.dart'; import 'package:spotiflac_android/utils/int_utils.dart'; +import 'package:spotiflac_android/utils/path_match_keys.dart'; final _historyLog = AppLogger('DownloadHistory'); +typedef StartupOrphanDecision = ({ + Set confirmedIds, + Set pendingIds, +}); + +StartupOrphanDecision reconcileStartupOrphanSuspects({ + required Set checkedIds, + required Set missingIds, + required Set previousSuspectIds, +}) { + final currentMissing = missingIds.intersection(checkedIds); + return ( + confirmedIds: currentMissing.intersection(previousSuspectIds), + pendingIds: currentMissing.difference(previousSuspectIds), + ); +} + +bool historyItemsReferToSameStoredFile( + DownloadHistoryItem first, + DownloadHistoryItem second, +) { + if (first.id == second.id) return true; + final firstKeys = buildPathMatchKeys(first.filePath); + if (firstKeys.isEmpty) return false; + return buildPathMatchKeys( + second.filePath, + ).any((key) => firstKeys.contains(key)); +} + class DownloadHistoryItem { final String id; final String trackName; @@ -216,6 +246,20 @@ class DownloadHistoryItem { } } +Map _indexRecentHistoryItems( + Iterable items, + String? Function(DownloadHistoryItem item) keyOf, +) { + final index = {}; + for (final item in items) { + final key = keyOf(item); + if (key != null && key.isNotEmpty) { + index.putIfAbsent(key, () => item); + } + } + return index; +} + class DownloadHistoryState { final List items; final int totalCount; @@ -231,27 +275,17 @@ class DownloadHistoryState { this.loadedIndexVersion = 0, List? lookupItems, }) : _lookupItems = List.unmodifiable(lookupItems ?? items), - _bySpotifyId = Map.fromEntries( - (lookupItems ?? items) - .where( - (item) => item.spotifyId != null && item.spotifyId!.isNotEmpty, - ) - .map((item) => MapEntry(item.spotifyId!, item)), + _bySpotifyId = _indexRecentHistoryItems( + lookupItems ?? items, + (item) => item.spotifyId, ), - _byIsrc = Map.fromEntries( - (lookupItems ?? items) - .where((item) => item.isrc != null && item.isrc!.isNotEmpty) - .map((item) => MapEntry(item.isrc!, item)), + _byIsrc = _indexRecentHistoryItems( + lookupItems ?? items, + (item) => item.isrc, ), - _byTrackArtistKey = Map.fromEntries( - (lookupItems ?? items) - .map( - (item) => MapEntry( - _trackArtistKey(item.trackName, item.artistName), - item, - ), - ) - .where((entry) => entry.key.isNotEmpty), + _byTrackArtistKey = _indexRecentHistoryItems( + lookupItems ?? items, + (item) => _trackArtistKey(item.trackName, item.artistName), ); static String _trackArtistKey(String trackName, String artistName) { @@ -306,6 +340,8 @@ class DownloadHistoryNotifier extends Notifier { 'history_startup_saf_repair_cursor_v1'; static const _startupOrphanCursorKey = 'history_startup_orphan_cursor_v1'; static const _startupOrphanSuspectPrefix = + 'history_startup_orphan_suspect_v2_'; + static const _legacyStartupOrphanSuspectPrefix = 'history_startup_orphan_suspect_v1_'; static const _startupAudioCursorKey = 'history_startup_audio_cursor_v1'; static const _audioProbeFailedPathsKey = @@ -971,26 +1007,22 @@ class DownloadHistoryNotifier extends Notifier { DownloadHistoryItem item, ) async { DownloadHistoryItem? existing; - if (item.spotifyId != null && item.spotifyId!.isNotEmpty) { - existing = state.getBySpotifyId(item.spotifyId!); - } - if (existing == null && item.isrc != null && item.isrc!.isNotEmpty) { - existing = state.getByIsrc(item.isrc!); + for (final candidate in state.lookupItems) { + if (historyItemsReferToSameStoredFile(candidate, item)) { + existing = candidate; + break; + } } + if (existing == null) { - final json = await _db.findExisting( - spotifyId: item.spotifyId, - isrc: item.isrc, - ); + final json = await _db.getById(item.id); if (json != null) { existing = DownloadHistoryItem.fromJson(json); } } + if (existing == null) { - final json = await _db.findByTrackAndArtist( - item.trackName, - item.artistName, - ); + final json = await _db.findByFilePath(item.filePath); if (json != null) { existing = DownloadHistoryItem.fromJson(json); } @@ -1457,12 +1489,16 @@ class DownloadHistoryNotifier extends Notifier { ({ List orphanedIds, Map replacementPaths, + Map replacementFileNames, + Map replacementRelativeDirs, Map pathById, }) > _inspectOrphanedEntries(List> entries) async { final orphanedIds = []; final replacementPaths = {}; + final replacementFileNames = {}; + final replacementRelativeDirs = {}; final pathById = {}; const checkChunkSize = 16; @@ -1472,7 +1508,7 @@ class DownloadHistoryNotifier extends Notifier { : entries.length; final chunk = entries.sublist(i, end); - final checks = await Future.wait?>( + final checks = await Future.wait?>( chunk.map((entry) async { final id = entry['id'] as String; final filePath = entry['file_path'] as String?; @@ -1481,6 +1517,63 @@ class DownloadHistoryNotifier extends Notifier { try { if (await fileExists(filePath)) return MapEntry(id, true); + if (entry['storage_mode'] == 'saf') { + final treeUri = (entry['download_tree_uri'] as String? ?? '') + .trim(); + var fileName = (entry['saf_file_name'] as String? ?? '').trim(); + if (fileName.isEmpty && isContentUri(filePath)) { + fileName = _fileNameFromUri(filePath); + } + if (treeUri.isEmpty || fileName.isEmpty) { + return MapEntry(id, null); + } + + bool treeAccessible; + try { + treeAccessible = await PlatformBridge.validateSafTreeAccess( + treeUri, + ); + } catch (error) { + _historyLog.w( + 'Unable to verify SAF tree while checking $id: $error', + ); + return MapEntry(id, null); + } + if (!treeAccessible) { + return MapEntry(id, null); + } + + for (final candidate in _conversionRenameCandidates( + fileName, + includeAlternateExtensions: true, + )) { + try { + final resolved = await PlatformBridge.resolveSafFile( + treeUri: treeUri, + relativeDir: entry['saf_relative_dir'] as String? ?? '', + fileName: candidate, + ); + final uri = (resolved['uri'] as String? ?? '').trim(); + if (uri.isEmpty) continue; + replacementPaths[id] = uri; + replacementFileNames[id] = candidate; + final relativeDir = + (resolved['relative_dir'] as String? ?? '').trim(); + if (relativeDir.isNotEmpty) { + replacementRelativeDirs[id] = relativeDir; + } + pathById[id] = uri; + return MapEntry(id, true); + } catch (error) { + _historyLog.w( + 'Unable to resolve SAF file while checking $id: $error', + ); + return MapEntry(id, null); + } + } + return MapEntry(id, false); + } + final sibling = await _findConvertedSibling(filePath); if (sibling != null) { _historyLog.i( @@ -1494,13 +1587,13 @@ class DownloadHistoryNotifier extends Notifier { return MapEntry(id, false); } catch (e) { _historyLog.w('Error checking file existence for $id: $e'); - return MapEntry(id, false); + return MapEntry(id, null); } }), ); for (final check in checks) { - if (check == null || check.value) continue; + if (check == null || check.value != false) continue; orphanedIds.add(check.key); _historyLog.d( 'Found orphaned entry: ${check.key} (${pathById[check.key] ?? ''})', @@ -1511,6 +1604,8 @@ class DownloadHistoryNotifier extends Notifier { return ( orphanedIds: orphanedIds, replacementPaths: replacementPaths, + replacementFileNames: replacementFileNames, + replacementRelativeDirs: replacementRelativeDirs, pathById: pathById, ); } @@ -1518,6 +1613,8 @@ class DownloadHistoryNotifier extends Notifier { void _applyHistoryPathAndDeletionChanges({ required List deletedIds, required Map replacementPaths, + Map replacementFileNames = const {}, + Map replacementRelativeDirs = const {}, }) { if (deletedIds.isEmpty && replacementPaths.isEmpty) { return; @@ -1529,8 +1626,21 @@ class DownloadHistoryNotifier extends Notifier { continue; } final replacementPath = replacementPaths[item.id]; - if (replacementPath != null && replacementPath != item.filePath) { - updatedItems.add(item.copyWith(filePath: replacementPath)); + final replacementFileName = replacementFileNames[item.id]; + final replacementRelativeDir = replacementRelativeDirs[item.id]; + if (replacementPath != null && + (replacementPath != item.filePath || + (replacementFileName != null && + replacementFileName != item.safFileName) || + (replacementRelativeDir != null && + replacementRelativeDir != item.safRelativeDir))) { + updatedItems.add( + item.copyWith( + filePath: replacementPath, + safFileName: replacementFileName, + safRelativeDir: replacementRelativeDir, + ), + ); } else { updatedItems.add(item); } @@ -1562,24 +1672,39 @@ class DownloadHistoryNotifier extends Notifier { } final result = await _inspectOrphanedEntries(entries); - final confirmedOrphanIds = []; - for (final id in result.orphanedIds) { + final checkedIds = result.pathById.keys.toSet(); + final previousSuspectIds = { + for (final id in checkedIds) + if (prefs.getBool('$_startupOrphanSuspectPrefix$id') == true) id, + }; + final decision = reconcileStartupOrphanSuspects( + checkedIds: checkedIds, + missingIds: result.orphanedIds.toSet(), + previousSuspectIds: previousSuspectIds, + ); + for (final id in checkedIds) { final key = '$_startupOrphanSuspectPrefix$id'; - if (prefs.getBool(key) == true) { - confirmedOrphanIds.add(id); - await prefs.remove(key); - } else { + await prefs.remove('$_legacyStartupOrphanSuspectPrefix$id'); + if (decision.pendingIds.contains(id)) { await prefs.setBool(key, true); _historyLog.d( 'Deferring orphan removal until next pass: $id (${result.pathById[id] ?? ''})', ); + } else { + await prefs.remove(key); } } for (final replacement in result.replacementPaths.entries) { - await _db.updateFilePath(replacement.key, replacement.value); + await _db.updateFilePath( + replacement.key, + replacement.value, + newSafFileName: result.replacementFileNames[replacement.key], + newSafRelativeDir: result.replacementRelativeDirs[replacement.key], + ); await prefs.remove('$_startupOrphanSuspectPrefix${replacement.key}'); } + final confirmedOrphanIds = decision.confirmedIds.toList(growable: false); final deletedCount = confirmedOrphanIds.isEmpty ? 0 : await _db.deleteByIds(confirmedOrphanIds); @@ -1587,6 +1712,8 @@ class DownloadHistoryNotifier extends Notifier { _applyHistoryPathAndDeletionChanges( deletedIds: confirmedOrphanIds, replacementPaths: result.replacementPaths, + replacementFileNames: result.replacementFileNames, + replacementRelativeDirs: result.replacementRelativeDirs, ); if (entries.length < maxItems) { @@ -1610,6 +1737,8 @@ class DownloadHistoryNotifier extends Notifier { _historyLog.i('Starting orphaned downloads cleanup...'); final orphanedIds = []; final replacementPaths = {}; + final replacementFileNames = {}; + final replacementRelativeDirs = {}; const pageSize = 256; var offset = 0; @@ -1625,15 +1754,25 @@ class DownloadHistoryNotifier extends Notifier { final result = await _inspectOrphanedEntries(entries); orphanedIds.addAll(result.orphanedIds); replacementPaths.addAll(result.replacementPaths); + replacementFileNames.addAll(result.replacementFileNames); + replacementRelativeDirs.addAll(result.replacementRelativeDirs); if (entries.length < pageSize) { break; } - offset += entries.length - result.orphanedIds.length; + // Deletions are applied only after inspection finishes, so advance by + // the full page. Subtracting missing rows here can repeatedly fetch the + // same page forever when an entire page is orphaned. + offset += entries.length; } for (final replacement in replacementPaths.entries) { - await _db.updateFilePath(replacement.key, replacement.value); + await _db.updateFilePath( + replacement.key, + replacement.value, + newSafFileName: replacementFileNames[replacement.key], + newSafRelativeDir: replacementRelativeDirs[replacement.key], + ); } if (orphanedIds.isEmpty && replacementPaths.isEmpty) { @@ -1647,6 +1786,8 @@ class DownloadHistoryNotifier extends Notifier { _applyHistoryPathAndDeletionChanges( deletedIds: orphanedIds, replacementPaths: replacementPaths, + replacementFileNames: replacementFileNames, + replacementRelativeDirs: replacementRelativeDirs, ); _historyLog.i( diff --git a/lib/services/history_database.dart b/lib/services/history_database.dart index 13f89d49..57663624 100644 --- a/lib/services/history_database.dart +++ b/lib/services/history_database.dart @@ -5,6 +5,7 @@ import 'package:path_provider/path_provider.dart'; import 'package:shared_preferences/shared_preferences.dart'; import 'package:spotiflac_android/services/sqlite_helpers.dart' as sqlite; import 'package:spotiflac_android/utils/logger.dart'; +import 'package:spotiflac_android/utils/path_match_keys.dart'; final _log = AppLogger('HistoryDatabase'); final Future _prefs = SharedPreferences.getInstance(); @@ -648,12 +649,31 @@ class HistoryDatabase { return _dbRowToJson(rows.first); } + Future?> findByFilePath(String filePath) async { + final pathKeys = buildPathMatchKeys(filePath).toList(growable: false); + if (pathKeys.isEmpty) return null; + + final db = await database; + final placeholders = List.filled(pathKeys.length, '?').join(','); + final rows = await db.rawQuery(''' + SELECT h.* + FROM history h + JOIN history_path_keys hpk ON hpk.item_id = h.id + WHERE hpk.path_key IN ($placeholders) + ORDER BY h.downloaded_at DESC + LIMIT 1 + ''', pathKeys); + if (rows.isEmpty) return null; + return _dbRowToJson(rows.first); + } + Future?> getBySpotifyId(String spotifyId) async { final db = await database; final rows = await db.query( 'history', where: 'spotify_id = ?', whereArgs: [spotifyId], + orderBy: 'downloaded_at DESC', limit: 1, ); if (rows.isEmpty) return null; @@ -666,6 +686,7 @@ class HistoryDatabase { 'history', where: 'isrc = ?', whereArgs: [isrc], + orderBy: 'downloaded_at DESC', limit: 1, ); if (rows.isEmpty) return null; @@ -971,6 +992,7 @@ class HistoryDatabase { String id, String newFilePath, { String? newSafFileName, + String? newSafRelativeDir, String? newQuality, int? newBitDepth, int? newSampleRate, @@ -983,6 +1005,9 @@ class HistoryDatabase { if (newSafFileName != null) { values['saf_file_name'] = newSafFileName; } + if (newSafRelativeDir != null) { + values['saf_relative_dir'] = newSafRelativeDir; + } if (newQuality != null) { values['quality'] = newQuality; } diff --git a/test/download_history_logic_test.dart b/test/download_history_logic_test.dart new file mode 100644 index 00000000..1f95b842 --- /dev/null +++ b/test/download_history_logic_test.dart @@ -0,0 +1,133 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:spotiflac_android/providers/download_history_provider.dart'; + +DownloadHistoryItem _historyItem({ + required String id, + required String filePath, + required DateTime downloadedAt, + String albumName = 'Album', +}) { + return DownloadHistoryItem( + id: id, + trackName: 'Same Song', + artistName: 'Same Artist', + albumName: albumName, + filePath: filePath, + service: 'test', + downloadedAt: downloadedAt, + spotifyId: 'spotify:track:same', + isrc: 'SAMEISRC', + ); +} + +void main() { + group('download history identity', () { + test('same track metadata does not merge files from different albums', () { + final first = _historyItem( + id: 'first', + filePath: '/music/First Album/Same Song.flac', + downloadedAt: DateTime.utc(2026, 7, 22), + albumName: 'First Album', + ); + final second = _historyItem( + id: 'second', + filePath: '/music/Second Album/Same Song.flac', + downloadedAt: DateTime.utc(2026, 7, 23), + albumName: 'Second Album', + ); + + expect(historyItemsReferToSameStoredFile(first, second), isFalse); + }); + + test('equivalent paths still update one stored file record', () { + final original = _historyItem( + id: 'original', + filePath: '/music/Album/Same Song.flac', + downloadedAt: DateTime.utc(2026, 7, 22), + ); + final converted = _historyItem( + id: 'converted', + filePath: '/music/Album/Same Song.opus', + downloadedAt: DateTime.utc(2026, 7, 23), + ); + + expect(historyItemsReferToSameStoredFile(original, converted), isTrue); + }); + + test('duplicate track identities keep the newest lookup item', () { + final newest = _historyItem( + id: 'newest', + filePath: '/music/New Album/Same Song.flac', + downloadedAt: DateTime.utc(2026, 7, 23), + albumName: 'New Album', + ); + final older = _historyItem( + id: 'older', + filePath: '/music/Old Album/Same Song.flac', + downloadedAt: DateTime.utc(2026, 7, 22), + albumName: 'Old Album', + ); + + final state = DownloadHistoryState( + items: [newest, older], + lookupItems: [newest, older], + totalCount: 2, + ); + + expect(state.getBySpotifyId('spotify:track:same')?.id, 'newest'); + expect(state.getByIsrc('SAMEISRC')?.id, 'newest'); + expect( + state.findByTrackAndArtist('Same Song', 'Same Artist')?.id, + 'newest', + ); + expect(state.items, hasLength(2)); + }); + }); + + group('startup orphan reconciliation', () { + test('requires consecutive missing checks before confirmation', () { + final firstMissing = reconcileStartupOrphanSuspects( + checkedIds: {'track'}, + missingIds: {'track'}, + previousSuspectIds: {}, + ); + expect(firstMissing.confirmedIds, isEmpty); + expect(firstMissing.pendingIds, {'track'}); + + final recovered = reconcileStartupOrphanSuspects( + checkedIds: {'track'}, + missingIds: {}, + previousSuspectIds: firstMissing.pendingIds, + ); + expect(recovered.confirmedIds, isEmpty); + expect(recovered.pendingIds, isEmpty); + + final missingAgain = reconcileStartupOrphanSuspects( + checkedIds: {'track'}, + missingIds: {'track'}, + previousSuspectIds: recovered.pendingIds, + ); + expect(missingAgain.confirmedIds, isEmpty); + expect(missingAgain.pendingIds, {'track'}); + + final consecutiveMissing = reconcileStartupOrphanSuspects( + checkedIds: {'track'}, + missingIds: {'track'}, + previousSuspectIds: missingAgain.pendingIds, + ); + expect(consecutiveMissing.confirmedIds, {'track'}); + expect(consecutiveMissing.pendingIds, isEmpty); + }); + + test('ignores unchecked stale suspects', () { + final decision = reconcileStartupOrphanSuspects( + checkedIds: {'checked'}, + missingIds: {'unchecked'}, + previousSuspectIds: {'unchecked'}, + ); + + expect(decision.confirmedIds, isEmpty); + expect(decision.pendingIds, isEmpty); + }); + }); +}