From 253a19f17f0ec4ee93661e88759c7e71b74aae4c Mon Sep 17 00:00:00 2001 From: zarzet <42882290+zarzet@users.noreply.github.com> Date: Fri, 2 Oct 2026 18:34:57 +0700 Subject: [PATCH] fix(library): preserve folders during upgrades and load failures Load configured sources before storage probes and the track index, retain visible configuration on failures, and expose loading and retry states. Wait for saved settings before legacy migration and recreate a missing source placeholder without replacing existing identities. --- lib/l10n/arb/app_en.arb | 2 + lib/l10n/arb/app_id.arb | 4 +- lib/providers/local_library_provider.dart | 90 +++++-- .../settings/library_settings_page.dart | 37 +++ lib/services/library_database.dart | 60 ++++- test/library_loading_test.dart | 243 ++++++++++++++++++ test/library_settings_cleanup_test.dart | 51 ++++ test/library_source_migration_test.dart | 98 +++++++ 8 files changed, 545 insertions(+), 40 deletions(-) create mode 100644 test/library_loading_test.dart create mode 100644 test/library_source_migration_test.dart diff --git a/lib/l10n/arb/app_en.arb b/lib/l10n/arb/app_en.arb index 4889b636..66a3c3f9 100644 --- a/lib/l10n/arb/app_en.arb +++ b/lib/l10n/arb/app_en.arb @@ -7064,6 +7064,8 @@ "weeklyReleasesUnavailable": "Some artists could not be checked. Their saved results are kept. Providers may not supply artist catalogs or exact release dates.", "weeklyReleasesLoadError": "Could not check new releases. Pull to refresh.", "motionArtworkDescription": "Loop video covers supplied by metadata providers. Turn off to use still artwork and reduce battery use.", + "libraryLoading": "Loading Library…", + "libraryLoadFailed": "Library could not be loaded. Retry to load the saved folders and track index.", "optionsEmbeddedCoverSizeOriginal": "Original resolution", "@optionsEmbeddedCoverSizeOriginal": { "description": "Option that preserves the provider artwork at its original resolution" diff --git a/lib/l10n/arb/app_id.arb b/lib/l10n/arb/app_id.arb index 8784b69d..a4fc8fc0 100644 --- a/lib/l10n/arb/app_id.arb +++ b/lib/l10n/arb/app_id.arb @@ -6979,5 +6979,7 @@ "weeklyReleasesEmpty": "Tidak ditemukan rilis dengan tanggal dalam periode ini. Tambahkan artis favorit atau coba periode lebih panjang.", "weeklyReleasesUnavailable": "Beberapa artis tidak dapat diperiksa. Hasil tersimpan tetap dipertahankan. Penyedia mungkin tidak memberikan katalog artis atau tanggal rilis lengkap.", "weeklyReleasesLoadError": "Rilis baru tidak dapat diperiksa. Tarik untuk memuat ulang.", - "motionArtworkDescription": "Putar berulang cover video dari penyedia metadata. Matikan untuk memakai cover diam dan menghemat baterai." + "motionArtworkDescription": "Putar berulang cover video dari penyedia metadata. Matikan untuk memakai cover diam dan menghemat baterai.", + "libraryLoading": "Memuat Library…", + "libraryLoadFailed": "Library tidak dapat dimuat. Coba lagi untuk memuat folder tersimpan dan indeks lagu." } diff --git a/lib/providers/local_library_provider.dart b/lib/providers/local_library_provider.dart index a9ec8a38..bdbb58a3 100644 --- a/lib/providers/local_library_provider.dart +++ b/lib/providers/local_library_provider.dart @@ -19,6 +19,8 @@ const _excludedDownloadedCountKey = 'local_library_excluded_downloaded_count'; final _prefs = SharedPreferences.getInstance(); class LocalLibraryState { + final bool isLoading; + final bool loadFailed; final bool isScanning; final bool scanIsFinalizing; final bool scanIsPaused; @@ -38,6 +40,8 @@ class LocalLibraryState { final Set _isrcSet; LocalLibraryState({ + this.isLoading = false, + this.loadFailed = false, this.isScanning = false, this.scanIsFinalizing = false, this.scanIsPaused = false, @@ -76,6 +80,8 @@ class LocalLibraryState { } LocalLibraryState copyWith({ + bool? isLoading, + bool? loadFailed, bool? isScanning, bool? scanIsFinalizing, bool? scanIsPaused, @@ -97,6 +103,8 @@ class LocalLibraryState { Set? isrcSet, }) { return LocalLibraryState( + isLoading: isLoading ?? this.isLoading, + loadFailed: loadFailed ?? this.loadFailed, isScanning: isScanning ?? this.isScanning, scanIsFinalizing: scanIsFinalizing ?? this.scanIsFinalizing, scanIsPaused: scanIsPaused ?? this.scanIsPaused, @@ -124,7 +132,10 @@ class LocalLibraryState { } class LocalLibraryNotifier extends Notifier { - final LibraryDatabase _db = LibraryDatabase.instance; + LocalLibraryNotifier({LibraryDatabase? database}) + : _db = database ?? LibraryDatabase.instance; + + final LibraryDatabase _db; final NotificationService _notificationService = NotificationService(); static const _progressPollingInterval = Duration(milliseconds: 350); static const _progressStreamBootstrapTimeout = Duration(milliseconds: 900); @@ -184,7 +195,7 @@ class LocalLibraryNotifier extends Notifier { } Future.microtask(_ensureLoadedFromDatabase); - return LocalLibraryState(); + return LocalLibraryState(isLoading: true); } Future _ensureLoadedFromDatabase() { @@ -200,17 +211,27 @@ class LocalLibraryNotifier extends Notifier { return _loadFuture ?? Future.value(); } _isLoaded = true; + state = state.copyWith(isLoading: true, loadFailed: false); try { + // Folder configuration is independent of the potentially expensive + // track index and storage probes. Publish it before either can fail. + final configuredSources = await _db.getSources(); + if (!ref.mounted) return; + state = state.copyWith(sources: configuredSources); + await ref.read(settingsProvider.notifier).ensureLoaded(); await _migrateLegacySource(); final reconnectedSources = await _refreshSourceAvailabilityInDatabase(); - final countFuture = _db.getCount(); - final indexFuture = _db.getLookupIndex(); - final sourcesFuture = _db.getSources(); + final sources = await _db.getSources(); + if (!ref.mounted) return; + state = state.copyWith(sources: sources); + final summary = await Future.wait([ + _db.getCount(), + _db.getLookupIndex(), + ]); final prefsFuture = _prefs; - final count = await countFuture; - final lookupIndex = await indexFuture; - final sources = await sourcesFuture; + final count = summary[0] as int; + final lookupIndex = summary[1] as LocalLibraryLookupIndex; DateTime? lastScannedAt; var excludedDownloadedCount = 0; @@ -223,6 +244,7 @@ class LocalLibraryNotifier extends Notifier { _log.w('Failed to load lastScannedAt: $e'); } + if (!ref.mounted) return; state = state.copyWith( totalCount: count, loadedIndexVersion: state.loadedIndexVersion + 1, @@ -246,13 +268,20 @@ class LocalLibraryNotifier extends Notifier { } } catch (e, stack) { _isLoaded = false; + if (ref.mounted) state = state.copyWith(loadFailed: true); _log.e('Failed to load library from database: $e', e, stack); } finally { _loadFuture = null; + if (ref.mounted) state = state.copyWith(isLoading: false); } } Future reloadFromStorage() async { + final loading = _loadFuture; + if (loading != null) { + await loading; + return; + } _isLoaded = false; _hasLoadedFromDatabase = false; _loadFuture = null; @@ -388,12 +417,16 @@ class LocalLibraryNotifier extends Notifier { DateTime? lastScannedAt, int? excludedDownloadedCount, }) async { - final countFuture = _db.getCount(); - final indexFuture = _db.getLookupIndex(); - final sourcesFuture = _db.getSources(); - final count = await countFuture; - final index = await indexFuture; - final sources = await sourcesFuture; + final sources = await _db.getSources(); + if (!ref.mounted) return; + state = state.copyWith(sources: sources); + final summary = await Future.wait([ + _db.getCount(), + _db.getLookupIndex(), + ]); + final count = summary[0] as int; + final index = summary[1] as LocalLibraryLookupIndex; + if (!ref.mounted) return; final latestSourceScan = sources .map((source) => source.lastScannedAt) .whereType() @@ -403,6 +436,7 @@ class LocalLibraryNotifier extends Notifier { latest == null || value.isAfter(latest) ? value : latest, ); state = state.copyWith( + loadFailed: false, totalCount: count, loadedIndexVersion: state.loadedIndexVersion + 1, lastScannedAt: lastScannedAt ?? latestSourceScan, @@ -478,19 +512,25 @@ class LocalLibraryNotifier extends Notifier { } Future _probePathAvailability(String path, {String? bookmark}) async { - if (Platform.isAndroid && path.startsWith('content://')) { - return PlatformBridge.probeSafTreeReadAccess(path); - } - if (Platform.isIOS && bookmark != null && bookmark.trim().isNotEmpty) { - final access = await PlatformBridge.startAccessingIosBookmark(bookmark); - if (access == null) return false; - try { - return await Directory(access.path).exists(); - } finally { - await PlatformBridge.stopAccessingIosBookmark(access); + try { + if (Platform.isAndroid && path.startsWith('content://')) { + return await PlatformBridge.probeSafTreeReadAccess(path); } + if (Platform.isIOS && bookmark != null && bookmark.trim().isNotEmpty) { + final access = await PlatformBridge.startAccessingIosBookmark(bookmark); + if (access == null) return false; + try { + return await Directory(access.path).exists(); + } finally { + await PlatformBridge.stopAccessingIosBookmark(access); + } + } + return await Directory(path).exists(); + } catch (error) { + // A busy provider or a filesystem exception cannot prove detachment. + _log.w('Library source access probe failed: $error'); + return null; } - return await Directory(path).exists(); } Future> _refreshSourceAvailabilityInDatabase() => diff --git a/lib/screens/settings/library_settings_page.dart b/lib/screens/settings/library_settings_page.dart index 265b55c2..d177e922 100644 --- a/lib/screens/settings/library_settings_page.dart +++ b/lib/screens/settings/library_settings_page.dart @@ -636,6 +636,43 @@ class _LibrarySettingsPageState extends ConsumerState { title: context.l10n.libraryScanSettings, ), ), + SliverToBoxAdapter( + child: Consumer( + builder: (context, ref, _) { + final status = ref.watch( + localLibraryProvider.select( + (state) => + (loading: state.isLoading, failed: state.loadFailed), + ), + ); + if (!status.loading && !status.failed) { + return const SizedBox.shrink(); + } + return ListTile( + leading: status.loading + ? const SizedBox( + width: 24, + height: 24, + child: CircularProgressIndicator(strokeWidth: 2), + ) + : Icon(Icons.error_outline, color: colorScheme.error), + title: Text( + status.loading + ? context.l10n.libraryLoading + : context.l10n.libraryLoadFailed, + ), + trailing: status.loading + ? null + : TextButton( + onPressed: () => ref + .read(localLibraryProvider.notifier) + .reloadFromStorage(), + child: Text(context.l10n.dialogRetry), + ), + ); + }, + ), + ), SliverToBoxAdapter( child: SettingsGroup( children: [ diff --git a/lib/services/library_database.dart b/lib/services/library_database.dart index bce5721f..1393587e 100644 --- a/lib/services/library_database.dart +++ b/lib/services/library_database.dart @@ -1282,20 +1282,19 @@ class LibraryDatabase { }) async { if (path.trim().isEmpty) return; final db = await database; - await db.update( - 'library_sources', - { - 'path': path, - 'display_name': displayName, - 'bookmark': bookmark, - 'volume_id': volumeId, - 'is_removable': isRemovable ? 1 : 0, - 'available': available ? 1 : 0, - 'last_scanned_at': lastScannedAt?.toIso8601String(), - 'last_seen_at': available ? DateTime.now().toIso8601String() : null, - }, - where: 'id = ?', - whereArgs: [legacySourceId], + await persistLegacyLibrarySource( + db, + LocalLibrarySource( + id: legacySourceId, + path: path, + displayName: displayName, + bookmark: bookmark, + volumeId: volumeId, + isRemovable: isRemovable, + available: available, + lastScannedAt: lastScannedAt, + lastSeenAt: available ? DateTime.now() : null, + ), ); } @@ -2277,3 +2276,36 @@ class LibraryDatabase { return hash; } } + +/// Migrates the pre-multiple-folder setting even when its placeholder row is +/// absent. Do not REPLACE an existing source or change the IDs of its tracks. +Future persistLegacyLibrarySource( + Database db, + LocalLibrarySource source, +) async { + final values = { + 'path': source.path, + 'display_name': source.displayName, + 'bookmark': source.bookmark, + 'volume_id': source.volumeId, + 'is_removable': source.isRemovable ? 1 : 0, + 'available': source.available ? 1 : 0, + 'last_scanned_at': source.lastScannedAt?.toIso8601String(), + 'last_seen_at': source.lastSeenAt?.toIso8601String(), + }; + await db.transaction((txn) async { + final updated = await txn.update( + 'library_sources', + values, + where: 'id = ?', + whereArgs: [source.id], + ); + if (updated == 0) { + await txn.insert('library_sources', { + 'id': source.id, + 'enabled': source.enabled ? 1 : 0, + ...values, + }); + } + }); +} diff --git a/test/library_loading_test.dart b/test/library_loading_test.dart new file mode 100644 index 00000000..67e526f6 --- /dev/null +++ b/test/library_loading_test.dart @@ -0,0 +1,243 @@ +import 'dart:async'; +import 'dart:io'; + +import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:shared_preferences/shared_preferences.dart'; +import 'package:spotiflac_android/models/settings.dart'; +import 'package:spotiflac_android/providers/local_library_provider.dart'; +import 'package:spotiflac_android/providers/settings_provider.dart'; +import 'package:spotiflac_android/services/library_database.dart'; + +class _Settings extends SettingsNotifier { + _Settings({Future? ready, String path = ''}) + : _ready = ready, + _path = path; + + final Future? _ready; + final String _path; + + @override + AppSettings build() => const AppSettings(); + + @override + Future ensureLoaded() async { + await _ready; + state = state.copyWith(localLibraryPath: _path); + } +} + +class _LibraryDatabase implements LibraryDatabase { + _LibraryDatabase(this.sources); + + List sources; + Future Function() count = () async => 2; + Future Function() index = () async => + const LocalLibraryLookupIndex(matchKeys: {'song|artist'}); + bool failSources = false; + + @override + Future> getSources() async { + if (failSources) throw StateError('database is busy'); + return List.of(sources); + } + + @override + Future getCount() => count(); + + @override + Future getLookupIndex() => index(); + + @override + Future getSourceByPath(String path) async => + sources.where((source) => source.path == path).firstOrNull; + + @override + Future migrateLegacySource({ + required String path, + required String displayName, + String? bookmark, + String? volumeId, + bool isRemovable = false, + bool available = true, + DateTime? lastScannedAt, + }) async { + sources = [ + ...sources, + LocalLibrarySource( + id: LibraryDatabase.legacySourceId, + path: path, + displayName: displayName, + available: available, + ), + ]; + } + + @override + Future updateSourceState( + String sourceId, { + bool? enabled, + bool? available, + DateTime? lastScannedAt, + DateTime? lastSeenAt, + String? lastScanError, + bool clearLastScanError = false, + }) async { + sources = [ + for (final source in sources) + source.id == sourceId + ? source.copyWith(enabled: enabled, available: available) + : source, + ]; + } + + @override + dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); +} + +Future _waitFor(bool Function() condition) async { + for (var i = 0; i < 50 && !condition(); i++) { + await Future.delayed(Duration.zero); + } + expect(condition(), isTrue); +} + +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + late Directory music; + late _LibraryDatabase database; + late ProviderContainer container; + + setUp(() async { + SharedPreferences.setMockInitialValues({}); + music = await Directory.systemTemp.createTemp('library_loading_'); + database = _LibraryDatabase([ + LocalLibrarySource( + id: 'music', + path: music.path, + displayName: 'Music', + trackCount: 2, + ), + ]); + container = ProviderContainer( + overrides: [ + settingsProvider.overrideWith(_Settings.new), + localLibraryProvider.overrideWith( + () => LocalLibraryNotifier(database: database), + ), + ], + ); + }); + + tearDown(() async { + container.dispose(); + await music.delete(recursive: true); + }); + + test( + 'upgrade waits for saved settings before migrating the old folder', + () async { + final ready = Completer(); + container.dispose(); + database.sources = []; + container = ProviderContainer( + overrides: [ + settingsProvider.overrideWith( + () => _Settings(ready: ready.future, path: music.path), + ), + localLibraryProvider.overrideWith( + () => LocalLibraryNotifier(database: database), + ), + ], + ); + container.read(localLibraryProvider); + await Future.delayed(Duration.zero); + expect(container.read(localLibraryProvider).isLoading, isTrue); + ready.complete(); + await _waitFor(() => !container.read(localLibraryProvider).isLoading); + expect(container.read(localLibraryProvider).loadFailed, isFalse); + expect( + container.read(localLibraryProvider).sources.single.path, + music.path, + ); + expect( + container.read(localLibraryProvider).sources.single.id, + LibraryDatabase.legacySourceId, + ); + }, + ); + + test( + 'saved folders appear while a large track index is still loading', + () async { + final pending = Completer(); + database.index = () => pending.future; + container.read(localLibraryProvider); + await _waitFor( + () => container.read(localLibraryProvider).sources.isNotEmpty, + ); + final loading = container.read(localLibraryProvider); + expect(loading.sources.single.path, music.path); + expect(loading.isLoading, isTrue); + pending.complete(const LocalLibraryLookupIndex()); + await _waitFor(() => !container.read(localLibraryProvider).isLoading); + expect(container.read(localLibraryProvider).totalCount, 2); + }, + ); + + test( + 'index failure retains folders and retry recovers without reattaching', + () async { + database.index = () async => throw StateError('index read failed'); + container.read(localLibraryProvider); + await _waitFor(() => !container.read(localLibraryProvider).isLoading); + final failed = container.read(localLibraryProvider); + expect(failed.loadFailed, isTrue); + expect(failed.sources.single.path, music.path); + database.index = () async => const LocalLibraryLookupIndex(); + await container.read(localLibraryProvider.notifier).reloadFromStorage(); + expect(container.read(localLibraryProvider).loadFailed, isFalse); + expect(container.read(localLibraryProvider).totalCount, 2); + expect(container.read(localLibraryProvider).sources.single.id, 'music'); + }, + ); + + test( + 'failed reload keeps the previously loaded folders and song count', + () async { + container.read(localLibraryProvider); + await _waitFor(() => !container.read(localLibraryProvider).isLoading); + database.failSources = true; + await container.read(localLibraryProvider.notifier).reloadFromStorage(); + final failed = container.read(localLibraryProvider); + expect(failed.loadFailed, isTrue); + expect(failed.totalCount, 2); + expect(failed.sources.single.id, 'music'); + database.failSources = false; + await container.read(localLibraryProvider.notifier).reloadFromStorage(); + expect(container.read(localLibraryProvider).loadFailed, isFalse); + }, + ); + + test('an unavailable folder remains attached instead of vanishing', () async { + await music.delete(recursive: true); + database.sources = [ + LocalLibrarySource( + id: 'missing', + path: music.path, + displayName: 'Offline music', + isRemovable: true, + available: false, + ), + ]; + database.count = () async => 0; + container.read(localLibraryProvider); + await _waitFor(() => !container.read(localLibraryProvider).isLoading); + final loaded = container.read(localLibraryProvider); + expect(loaded.loadFailed, isFalse); + expect(loaded.sources.single.available, isFalse); + expect(loaded.sources.single.path, music.path); + // Keep teardown simple after deliberately removing the storage above. + await music.create(); + }); +} diff --git a/test/library_settings_cleanup_test.dart b/test/library_settings_cleanup_test.dart index ba079f84..5234cf2a 100644 --- a/test/library_settings_cleanup_test.dart +++ b/test/library_settings_cleanup_test.dart @@ -48,6 +48,32 @@ class _History extends DownloadHistoryNotifier { Future cleanupOrphanedDownloads() => _cleanup(); } +class _RetryLibrary extends _Library { + _RetryLibrary() + : super( + LocalLibraryState( + loadFailed: true, + sources: const [ + LocalLibrarySource( + id: 'sd-card', + path: 'content://storage/tree/sd-card', + displayName: 'Saved music', + available: false, + ), + ], + ), + () async => 0, + ); + + int _reloadCalls = 0; + + @override + Future reloadFromStorage() async { + _reloadCalls++; + state = state.copyWith(loadFailed: false, totalCount: 5); + } +} + Finder get _cleanupRow => find.byWidgetPredicate( (widget) => widget is SettingsItem && widget.title == 'Cleanup Missing Files', ); @@ -84,6 +110,31 @@ Future _mount( void main() { for (final mornye in [false, true]) { + testWidgets('failed load shows saved folder and offers retry ($mornye)', ( + tester, + ) async { + final library = _RetryLibrary(); + await _mount( + tester, + mornye: mornye, + library: library, + history: _History(0, () async => 0), + ); + await tester.scrollUntilVisible(find.text('Retry'), -300); + await tester.pumpAndSettle(); + expect( + find.textContaining('Library could not be loaded'), + findsOneWidget, + ); + expect(find.text('Saved music'), findsOneWidget); + await tester.tap(find.text('Retry')); + await tester.pumpAndSettle(); + expect(library._reloadCalls, 1); + expect(find.textContaining('Library could not be loaded'), findsNothing); + expect(find.text('Saved music'), findsOneWidget); + expect(tester.takeException(), isNull); + }); + testWidgets('cleanup works for download-only Library ($mornye)', ( tester, ) async { diff --git a/test/library_source_migration_test.dart b/test/library_source_migration_test.dart new file mode 100644 index 00000000..948c3b5e --- /dev/null +++ b/test/library_source_migration_test.dart @@ -0,0 +1,98 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:spotiflac_android/services/library_database.dart'; +import 'package:sqflite/sqflite.dart'; + +class _SourcesDatabase implements Database, Transaction { + final rows = >{}; + int transactions = 0; + int inserts = 0; + + @override + Future transaction( + Future Function(Transaction txn) action, { + bool? exclusive, + }) async { + transactions++; + return action(this); + } + + @override + Future update( + String table, + Map values, { + String? where, + List? whereArgs, + ConflictAlgorithm? conflictAlgorithm, + }) async { + expect(table, 'library_sources'); + final row = rows[whereArgs!.single]; + if (row == null) return 0; + row.addAll(values); + return 1; + } + + @override + Future insert( + String table, + Map values, { + String? nullColumnHack, + ConflictAlgorithm? conflictAlgorithm, + }) async { + expect(table, 'library_sources'); + inserts++; + rows[values['id'] as String] = Map.of(values); + return 1; + } + + @override + dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); +} + +void main() { + const source = LocalLibrarySource( + id: LibraryDatabase.legacySourceId, + path: 'content://example.documents/tree/card%3AMusic', + displayName: 'Music', + isRemovable: true, + available: false, + bookmark: 'saved-bookmark', + ); + + test( + 'upgrade restores the old folder when the placeholder is missing', + () async { + final database = _SourcesDatabase(); + await persistLegacyLibrarySource(database, source); + final restored = database.rows[LibraryDatabase.legacySourceId]!; + expect(restored['path'], source.path); + expect(restored['bookmark'], source.bookmark); + expect(restored['available'], 0); + expect(restored['enabled'], 1); + expect(restored['is_removable'], 1); + expect(database.transactions, 1); + await persistLegacyLibrarySource(database, source); + expect(database.rows.length, 1); + expect(database.inserts, 1); + }, + ); + + test( + 'migration preserves the existing source identity and disabled state', + () async { + final database = _SourcesDatabase(); + database.rows[LibraryDatabase.legacySourceId] = { + 'id': LibraryDatabase.legacySourceId, + 'path': 'legacy://local-library', + 'enabled': 0, + 'last_scan_error': 'previous scan error', + }; + await persistLegacyLibrarySource(database, source); + final restored = database.rows[LibraryDatabase.legacySourceId]!; + expect(restored['id'], LibraryDatabase.legacySourceId); + expect(restored['enabled'], 0); + expect(restored['last_scan_error'], 'previous scan error'); + expect(restored['path'], source.path); + expect(database.inserts, 0); + }, + ); +}