From e01ec99c5e83bcb02c8bd8bf6d74485e52681da4 Mon Sep 17 00:00:00 2001 From: zarzet <42882290+zarzet@users.noreply.github.com> Date: Thu, 8 Oct 2026 09:27:26 +0700 Subject: [PATCH] perf(downloads): enqueue selected playlists in one update --- .../download_queue_selection_test.dart | 189 ++++++++++++++++++ lib/models/download_item.dart | 13 ++ lib/providers/download_queue_provider.dart | 92 ++++++--- lib/screens/queue_tab_selection.dart | 20 +- test/download_queue_selection_test.dart | 178 ++++++++++++++++- test/local_library_lazy_loading_test.dart | 141 ++++++++++++- 6 files changed, 586 insertions(+), 47 deletions(-) diff --git a/integration_test/download_queue_selection_test.dart b/integration_test/download_queue_selection_test.dart index 994688ac..d8573b63 100644 --- a/integration_test/download_queue_selection_test.dart +++ b/integration_test/download_queue_selection_test.dart @@ -48,6 +48,8 @@ void main() { 'platform': Platform.operatingSystem, 'note': 'Five alternating pairs of synchronous enqueue on the real notifier, paused during startup. Restore and persistence use private platform SQLite and are verified outside timing. Not download speed or FPS.', + 'playlist_baseline_note': + 'Per-playlist loop uses the current single-batch API, which already publishes once. The separate pre-change baseline captures its original two publications. This paired comparison isolates the additional benefit of grouping playlists.', }; var completed = 0; late Directory root; @@ -211,4 +213,191 @@ void main() { } completed++; }); + + for (final count in [20, 200]) { + testWidgets('enqueue $count playlists with independent batch metadata', ( + tester, + ) async { + final tracks = await playlistBatchFixture(count * 20); + final groups = [ + for (var i = 0; i < count; i++) tracks.sublist(i * 20, (i + 1) * 20), + ]; + final batches = [ + for (var i = 0; i < groups.length; i++) + DownloadQueueBatch(tracks: groups[i], playlistName: 'Playlist $i'), + ]; + final expectedTracks = groups.expand(normalizeBatchAlbumArtists).toList(); + final old = DownloadItem( + id: 'persisted', + track: tracks.first, + service: 'example', + createdAt: DateTime.utc(2026), + qualityOverride: 'LOSSLESS', + ); + final samples = >>{ + 'per_playlist': [], + 'bulk': [], + }; + for (var sample = 0; sample < 5; sample++) { + for (final bulk in sample.isEven ? [false, true] : [true, false]) { + await db.delete('download_queue_items'); + final stamp = old.createdAt.toIso8601String(); + await db.insert('download_queue_items', { + 'id': old.id, + 'item_json': jsonEncode(old.toJson()), + 'status': 'queued', + 'created_at': stamp, + 'updated_at': stamp, + }); + final container = scope(); + try { + final queue = container.read(downloadQueueProvider.notifier); + queue.pauseQueue(persistAcrossRestarts: false); + var publications = 0; + final subscription = container.listen( + downloadQueueProvider, + (_, _) => publications++, + ); + final watch = Stopwatch()..start(); + if (bulk) { + queue.addBatchesToQueue( + batches, + ' example ', + qualityOverride: 'HIGH', + ); + } else { + for (var i = 0; i < groups.length; i++) { + queue.addMultipleToQueue( + groups[i], + ' example ', + qualityOverride: 'HIGH', + playlistName: 'Playlist $i', + ); + } + } + watch.stop(); + subscription.close(); + samples[bulk ? 'bulk' : 'per_playlist']!.add({ + 'microseconds': watch.elapsedMicroseconds, + 'publications': publications, + }); + expect(publications, bulk ? 1 : count); + final early = container.read(downloadQueueProvider).items.toList(); + expect(early, hasLength(tracks.length)); + expect( + early.map((item) => item.id).toSet(), + hasLength(tracks.length), + ); + await queue.flushQueuePersistence(); + final state = container.read(downloadQueueProvider); + expect(state.items.skip(1), early); + expect( + jsonEncode(state.items.first.toJson()), + jsonEncode(old.toJson()), + ); + expect(state.isPaused, true); + expect(state.isProcessing, false); + final rows = await AppStateDatabase.instance + .getPendingDownloadQueueRows(); + expect( + rows.map((row) => row['id']), + state.items.map((item) => item.id), + ); + for (var i = 0; i < early.length; i++) { + final item = early[i]; + expect(item.playlistName, 'Playlist ${i ~/ 20}'); + expect(item.playlistPosition, i % 20 + 1); + expect(item.fromBatch, true); + expect(item.qualityOverride, 'HIGH'); + expect(item.service, 'example'); + expect(item.preserveQualityVariant, true); + expect(item.networkDownloadFolder, 'smb://example.test/music'); + expect( + jsonEncode(item.track.toJson()), + jsonEncode(expectedTracks[i].toJson()), + ); + expect( + jsonDecode(rows[i + 1]['item_json'] as String), + jsonDecode(jsonEncode(item.toJson())), + ); + } + } finally { + container.dispose(); + } + } + } + report['playlists_$count'] = samples; + completed++; + }); + } + + testWidgets( + 'playlist batches keep prior storage on failure then retry together', + (tester) async { + await db.delete('download_queue_items'); + final tracks = await playlistBatchFixture(300); + final old = DownloadItem( + id: 'keep', + track: tracks.first, + service: 'example', + createdAt: DateTime.utc(2026), + ); + final stamp = old.createdAt.toIso8601String(); + await db.insert('download_queue_items', { + 'id': old.id, + 'item_json': jsonEncode(old.toJson()), + 'status': 'queued', + 'created_at': stamp, + 'updated_at': stamp, + }); + await db.execute( + r'''CREATE TRIGGER fail_batches BEFORE INSERT ON download_queue_items + WHEN json_extract(NEW.item_json, '$.track.id') = '129' + BEGIN SELECT RAISE(ABORT, 'fixture row failure'); END''', + ); + final container = scope(); + try { + final queue = container.read(downloadQueueProvider.notifier); + queue.pauseQueue(persistAcrossRestarts: false); + queue.addBatchesToQueue([ + DownloadQueueBatch( + tracks: tracks.take(150).toList(), + playlistName: 'First', + ), + DownloadQueueBatch( + tracks: tracks.skip(150).toList(), + playlistName: 'Second', + ), + ], 'example'); + final early = container.read(downloadQueueProvider).items; + await queue.flushQueuePersistence(); + expect( + (await db.query('download_queue_items')).map((row) => row['id']), + ['keep'], + ); + expect(container.read(downloadQueueProvider).items.skip(1), early); + await db.execute('DROP TRIGGER fail_batches'); + await queue.flushQueuePersistence(); + final rows = await AppStateDatabase.instance + .getPendingDownloadQueueRows(); + expect(rows.map((row) => row['id']), [ + 'keep', + ...early.map((item) => item.id), + ]); + expect(container.read(downloadQueueProvider).isPaused, true); + expect( + early.take(150).map((item) => item.playlistName), + everyElement('First'), + ); + expect( + early.skip(150).map((item) => item.playlistName), + everyElement('Second'), + ); + } finally { + await db.execute('DROP TRIGGER IF EXISTS fail_batches'); + container.dispose(); + } + completed++; + }, + ); } diff --git a/lib/models/download_item.dart b/lib/models/download_item.dart index 691c2695..9c9feed7 100644 --- a/lib/models/download_item.dart +++ b/lib/models/download_item.dart @@ -5,6 +5,19 @@ part 'download_item.g.dart'; const Object _downloadItemUnset = Object(); +/// Tracks that share playlist naming and album-artist normalization rules. +class DownloadQueueBatch { + final List tracks; + final String? playlistName; + final List? playlistPositions; + + const DownloadQueueBatch({ + required this.tracks, + this.playlistName, + this.playlistPositions, + }); +} + enum DownloadStatus { queued, downloading, diff --git a/lib/providers/download_queue_provider.dart b/lib/providers/download_queue_provider.dart index d6f4a5aa..20f45233 100644 --- a/lib/providers/download_queue_provider.dart +++ b/lib/providers/download_queue_provider.dart @@ -975,42 +975,68 @@ class DownloadQueueNotifier extends Notifier { String? playlistName, List? playlistPositions, }) { + addBatchesToQueue( + [ + DownloadQueueBatch( + tracks: tracks, + playlistName: playlistName, + playlistPositions: playlistPositions, + ), + ], + service, + qualityOverride: qualityOverride, + ); + } + + /// Publishes several independent album/playlist batches together. Keep each + /// group's artist normalization, batch flag and playlist positions separate. + void addBatchesToQueue( + List batches, + String service, { + String? qualityOverride, + }) { + if (batches.isEmpty) return; final settings = ref.read(settingsProvider); - updateSettings(settings); - + String? normalizedService; final takenIds = state.items.map((item) => item.id).toSet(); - final shouldAssignPlaylistPositions = - playlistName != null && playlistName.trim().isNotEmpty; - final normalizedTracks = normalizeBatchAlbumArtists(tracks); - final fromBatch = normalizedTracks.length > 1; - final newItems = normalizedTracks.asMap().entries.map((entry) { - final track = entry.value; - final index = entry.key; - final explicitPosition = - playlistPositions != null && - index < playlistPositions.length && - (playlistPositions[index] ?? 0) > 0 - ? playlistPositions[index] - : null; - final id = _newQueueItemId(track, takenIds: takenIds); - takenIds.add(id); - return DownloadItem( - id: id, - track: track, - service: _normalizeQueuedService(service), - createdAt: DateTime.now(), - qualityOverride: qualityOverride, - playlistName: playlistName, - playlistPosition: - explicitPosition ?? - (shouldAssignPlaylistPositions ? index + 1 : null), - fromBatch: fromBatch, - preserveQualityVariant: settings.allowQualityVariants, - networkDownloadFolder: settings.networkDownloadFolder, - ); - }).toList(); + final newItems = []; + for (final batch in batches) { + final playlistName = batch.playlistName; + final playlistPositions = batch.playlistPositions; + final shouldAssignPlaylistPositions = + playlistName != null && playlistName.trim().isNotEmpty; + final normalizedTracks = normalizeBatchAlbumArtists(batch.tracks); + final fromBatch = normalizedTracks.length > 1; + for (var index = 0; index < normalizedTracks.length; index++) { + final track = normalizedTracks[index]; + final explicitPosition = + playlistPositions != null && + index < playlistPositions.length && + (playlistPositions[index] ?? 0) > 0 + ? playlistPositions[index] + : null; + final id = _newQueueItemId(track, takenIds: takenIds); + takenIds.add(id); + newItems.add( + DownloadItem( + id: id, + track: track, + service: normalizedService ??= _normalizeQueuedService(service), + createdAt: DateTime.now(), + qualityOverride: qualityOverride, + playlistName: playlistName, + playlistPosition: + explicitPosition ?? + (shouldAssignPlaylistPositions ? index + 1 : null), + fromBatch: fromBatch, + preserveQualityVariant: settings.allowQualityVariants, + networkDownloadFolder: settings.networkDownloadFolder, + ), + ); + } + } - state = state.copyWith(items: [...state.items, ...newItems]); + state = _withSettings(settings, items: [...state.items, ...newItems]); _saveQueueToStorage(); if (!state.isProcessing) { diff --git a/lib/screens/queue_tab_selection.dart b/lib/screens/queue_tab_selection.dart index 687b08ee..a8e63af9 100644 --- a/lib/screens/queue_tab_selection.dart +++ b/lib/screens/queue_tab_selection.dart @@ -263,15 +263,17 @@ extension _QueueTabSelectionActions on _QueueTabState { } return; } - for (final playlist in selectedPlaylists) { - final tracks = playlist.tracks.map((e) => e.track).toList(); - queueNotifier.addMultipleToQueue( - tracks, - svc, - qualityOverride: qualityOverride, - playlistName: playlist.name, - ); - } + queueNotifier.addBatchesToQueue( + [ + for (final playlist in selectedPlaylists) + DownloadQueueBatch( + tracks: playlist.tracks.map((entry) => entry.track).toList(), + playlistName: playlist.name, + ), + ], + svc, + qualityOverride: qualityOverride, + ); } if (settings.askQualityBeforeDownload || settings.allowQualityVariants) { diff --git a/test/download_queue_selection_test.dart b/test/download_queue_selection_test.dart index dcfcd9e1..16834f6a 100644 --- a/test/download_queue_selection_test.dart +++ b/test/download_queue_selection_test.dart @@ -1,3 +1,5 @@ +import 'dart:convert'; + import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:spotiflac_android/models/download_item.dart'; @@ -76,9 +78,10 @@ class _Queue extends DownloadQueueNotifier { } } -Map _stableItem(DownloadItem item) => item.toJson() - ..remove('id') - ..remove('createdAt'); +Map _stableItem(DownloadItem item) => + (jsonDecode(jsonEncode(item.toJson())) as Map) + ..remove('id') + ..remove('createdAt'); void main() { TestWidgetsFlutterBinding.ensureInitialized(); @@ -210,4 +213,173 @@ void main() { } }, ); + + for (final processing in [false, true]) { + test( + 'multiple batches keep independent metadata (processing: $processing)', + () { + final existing = DownloadItem( + id: 'existing', + track: _tracks.first, + service: 'example', + createdAt: DateTime.utc(2026), + ); + final initial = const DownloadQueueState(outputDir: '/old').copyWith( + items: [existing], + isPaused: true, + isProcessing: processing, + currentDownload: processing ? existing : null, + ); + final baseline = scope(initial); + final bulk = scope(initial); + final mutable = _tracks.toList(); + final batches = [ + DownloadQueueBatch( + tracks: mutable, + playlistName: ' One ', + playlistPositions: [7, null], + ), + DownloadQueueBatch( + tracks: [_tracks.first], + playlistName: 'Second', + playlistPositions: [0], + ), + DownloadQueueBatch( + tracks: [_tracks.first, _tracks.first], + playlistName: ' ', + playlistPositions: [-1, 9, 99], + ), + const DownloadQueueBatch(tracks: [], playlistName: 'Empty'), + DownloadQueueBatch(tracks: [_tracks.last], playlistPositions: [4]), + ]; + try { + final loop = baseline.read(downloadQueueProvider.notifier); + for (final batch in batches) { + loop.addMultipleToQueue( + batch.tracks, + ' example ', + qualityOverride: 'HIGH', + playlistName: batch.playlistName, + playlistPositions: batch.playlistPositions, + ); + } + final queue = bulk.read(downloadQueueProvider.notifier); + var publications = 0; + bulk.listen(downloadQueueProvider, (_, _) => publications++); + queue.addBatchesToQueue( + batches, + ' example ', + qualityOverride: 'HIGH', + ); + final state = bulk.read(downloadQueueProvider); + final expected = baseline.read(downloadQueueProvider); + expect(state.items.map(_stableItem), expected.items.map(_stableItem)); + expect(publications, 1); + expect(state.items.first, same(existing)); + expect(state.items.map((item) => item.id).toSet(), hasLength(7)); + expect(state.isPaused, true); + expect(state.isProcessing, processing); + expect(state.currentDownload, same(initial.currentDownload)); + expect(state.outputDir, _settings.downloadDirectory); + expect(state.filenameFormat, _settings.filenameFormat); + expect(state.audioQuality, _settings.audioQuality); + expect(state.autoFallback, _settings.autoFallback); + final added = state.items.skip(1).toList(); + expect(added.map((item) => item.track.albumArtist), [ + 'Artist', + 'Artist', + 'Artist & Guest', + 'Artist & Guest', + 'Artist & Guest', + _tracks.last.albumArtist, + ]); + expect(added.map((item) => item.fromBatch), [ + true, + true, + false, + true, + true, + false, + ]); + expect(added.map((item) => item.playlistPosition), [ + 7, + 2, + 1, + null, + 9, + 4, + ]); + expect(added.map((item) => item.playlistName), [ + ' One ', + ' One ', + 'Second', + ' ', + ' ', + null, + ]); + expect( + added.map((item) => item.service), + everyElement('replacement'), + ); + expect( + added.map((item) => item.qualityOverride), + everyElement('HIGH'), + ); + expect( + added.map((item) => item.preserveQualityVariant), + everyElement(true), + ); + expect( + added.map((item) => item.networkDownloadFolder), + everyElement(_settings.networkDownloadFolder), + ); + mutable.clear(); + batches.clear(); + expect(state.items, hasLength(7)); + queue.addBatchesToQueue([ + DownloadQueueBatch(tracks: [_tracks.first]), + ], 'example'); + expect( + bulk + .read(downloadQueueProvider) + .items + .map((item) => item.id) + .toSet(), + hasLength(8), + ); + } finally { + bulk.dispose(); + baseline.dispose(); + } + }, + ); + } + + test( + 'no batches is a no-op, an explicit empty batch still applies settings', + () { + const initial = DownloadQueueState(outputDir: '/old', isPaused: true); + final container = scope(initial); + try { + final queue = container.read(downloadQueueProvider.notifier); + var publications = 0; + container.listen(downloadQueueProvider, (_, _) => publications++); + queue.addBatchesToQueue([], 'example'); + expect(container.read(downloadQueueProvider), same(initial)); + expect(publications, 0); + queue.addBatchesToQueue([ + const DownloadQueueBatch(tracks: []), + ], 'example'); + expect(publications, 1); + expect(container.read(downloadQueueProvider).items, isEmpty); + expect(container.exists(extensionProvider), false); + expect( + container.read(downloadQueueProvider).outputDir, + _settings.downloadDirectory, + ); + } finally { + container.dispose(); + } + }, + ); } diff --git a/test/local_library_lazy_loading_test.dart b/test/local_library_lazy_loading_test.dart index c76e7d83..6e885bcd 100644 --- a/test/local_library_lazy_loading_test.dart +++ b/test/local_library_lazy_loading_test.dart @@ -9,6 +9,7 @@ import 'package:spotiflac_android/models/download_item.dart'; import 'package:spotiflac_android/models/settings.dart'; import 'package:spotiflac_android/models/track.dart'; import 'package:spotiflac_android/providers/download_queue_provider.dart'; +import 'package:spotiflac_android/providers/extension_provider.dart'; import 'package:spotiflac_android/providers/library_browse_provider.dart'; import 'package:spotiflac_android/providers/library_collections_provider.dart'; import 'package:spotiflac_android/providers/library_search_provider.dart'; @@ -73,9 +74,49 @@ class _Queue extends DownloadQueueNotifier { DownloadQueueState build() => DownloadQueueState(); } +class _DownloadSettings extends SettingsNotifier { + @override + AppSettings build() => const AppSettings( + defaultLibraryView: 'all', + defaultService: 'example', + askQualityBeforeDownload: false, + ); +} + +class _DownloadExtensions extends ExtensionNotifier { + @override + ExtensionState build() => const ExtensionState( + extensions: [ + Extension( + id: 'example', + name: 'example', + displayName: 'Example', + version: '1', + description: '', + enabled: true, + status: 'loaded', + hasDownloadProvider: true, + ), + ], + ); +} + +class _BatchQueue extends _Queue { + _BatchQueue(this._record); + final void Function(List, String, String?) _record; + @override + void addBatchesToQueue( + List batches, + String service, { + String? qualityOverride, + }) => _record(batches, service, qualityOverride); +} + class _SelectionCollections extends LibraryCollectionsNotifier { - _SelectionCollections(this._record); + _SelectionCollections(this._record, {bool withTracks = false}) + : _withTracks = withTracks; final void Function(List) _record; + final bool _withTracks; @override LibraryCollectionsState build() => LibraryCollectionsState( @@ -87,11 +128,29 @@ class _SelectionCollections extends LibraryCollectionsNotifier { name: id, createdAt: DateTime.utc(2026), updatedAt: DateTime.utc(2026), - tracks: const [], + tracks: _withTracks + ? [ + for (var i = 0; i < (id == 'Second' ? 2 : 1); i++) + CollectionTrackEntry( + key: '$id-$i', + addedAt: DateTime.utc(2026), + track: Track( + id: '$id-$i', + name: '$id-$i', + artistName: 'Artist', + albumName: 'Album', + duration: 180, + ), + ), + ] + : const [], ), ], ); + @override + Future ensurePlaylistsLoaded(Iterable ids) async {} + @override Future deletePlaylists(Iterable ids) async { final selected = ids.toSet(); @@ -155,6 +214,84 @@ void main() { }); for (final mornye in [false, true]) { + testWidgets( + 'playlist download confirms one grouped enqueue (Mornye: $mornye)', + (tester) async { + tester.view.physicalSize = const Size(800, 1200); + tester.view.devicePixelRatio = 1; + addTearDown(tester.view.reset); + final calls = >[]; + final services = []; + await tester.pumpWidget( + ProviderScope( + overrides: [ + settingsProvider.overrideWith(_DownloadSettings.new), + extensionProvider.overrideWith(_DownloadExtensions.new), + downloadHistoryProvider.overrideWith(() => history), + localLibraryProvider.overrideWith(() => local), + libraryCollectionsProvider.overrideWith( + () => _SelectionCollections((_) {}, withTracks: true), + ), + downloadQueueProvider.overrideWith( + () => _BatchQueue((batches, service, quality) { + calls.add(batches); + services.add(service); + expect(quality, isNull); + }), + ), + ], + child: MaterialApp( + theme: mornye + ? MornyeTheme.build(Brightness.dark) + : AppTheme.light(), + localizationsDelegates: AppLocalizations.localizationsDelegates, + supportedLocales: AppLocalizations.supportedLocales, + home: const Scaffold(body: QueueTab(librarySection: 'playlists')), + ), + ), + ); + await tester.pumpAndSettle(); + final l10n = AppLocalizations.of(tester.element(find.byType(QueueTab))); + await tester.ensureVisible(find.text('First')); + await tester.longPress(find.text('First')); + await tester.pumpAndSettle(); + await tester.ensureVisible(find.text('Second')); + await tester.tap(find.text('Second')); + await tester.pumpAndSettle(); + await tester.tap(find.text(l10n.bulkDownloadPlaylistsButton(2))); + await tester.pumpAndSettle(); + expect( + find.text(l10n.dialogDownloadPlaylistsMessage(3, 2)), + findsOneWidget, + ); + await tester.tap(find.text(l10n.dialogCancel)); + await tester.pumpAndSettle(); + expect(calls, isEmpty); + await tester.tap(find.text(l10n.bulkDownloadPlaylistsButton(2))); + await tester.pumpAndSettle(); + await tester.tap(find.text(l10n.dialogDownload)); + await tester.pumpAndSettle(); + expect(calls, hasLength(1)); + expect(services, ['example']); + expect(calls.single.map((batch) => batch.playlistName), [ + 'First', + 'Second', + ]); + expect( + calls.single.map( + (batch) => batch.tracks.map((track) => track.id).toList(), + ), + [ + ['First-0'], + ['Second-0', 'Second-1'], + ], + ); + expect(find.text(l10n.snackbarAddedTracksToQueue(3)), findsOneWidget); + expect(find.text(l10n.bulkDownloadPlaylistsButton(2)), findsNothing); + expect(tester.takeException(), isNull); + }, + ); + testWidgets('playlist selection confirms one deletion (Mornye: $mornye)', ( tester, ) async {