From f63d0fe49ea0944bdadf5a67092952605b70cffb Mon Sep 17 00:00:00 2001 From: edde746 <86283021+edde746@users.noreply.github.com> Date: Thu, 6 Aug 2026 06:10:35 +0200 Subject: [PATCH] fix(music): shuffle the head of a shuffled queue too Starting a music playlist, album, or artist on shuffle always opened on the list's first track: MusicQueueController.load anchored _order[cursor] and shuffled only the rest, and _startQueue collapsed "no start track" into startIndex 0, so the anchor was always the head. Anchoring is right for the two callers that do have a track which must play first -- the now-playing shuffle toggle, and a load with an explicit start track -- so make "no explicit start" representable instead of inferring it from the index: load takes int? startIndex and shuffles the whole list, head included, when it is null. A start track the list turns out not to contain now drops the anchor rather than falling back to 0. Video playback was never affected: Plex shuffles server-side via /playQueues and Jellyfin already shuffles its full local list. The queue's Random is injectable so the service-level regression is deterministic without depending on the SDK's seeded-PRNG sequence. Close #1811 --- .../music/music_playback_service.dart | 5 +- .../music/music_playback_service_impl.dart | 15 +++- .../music/music_queue_controller.dart | 31 ++++++-- .../music/music_playback_service_test.dart | 74 ++++++++++++++++++- .../music/music_queue_controller_test.dart | 15 ++++ 5 files changed, 126 insertions(+), 14 deletions(-) diff --git a/lib/services/music/music_playback_service.dart b/lib/services/music/music_playback_service.dart index d73d433b..c992fb9b 100644 --- a/lib/services/music/music_playback_service.dart +++ b/lib/services/music/music_playback_service.dart @@ -73,8 +73,9 @@ abstract class MusicPlaybackService extends ChangeNotifier { int get queueSessionRevision; /// Start a new queue from [tracks], optionally at [startTrack] (defaults - /// to the first track). [shuffle] shuffles with the start track anchored - /// first. + /// to the first track). [shuffle] anchors [startTrack] first and shuffles + /// the rest after it; with no [startTrack] the whole list shuffles, so the + /// queue opens on a random track rather than always the first one (#1811). Future playFromList({ required List tracks, MediaItem? startTrack, diff --git a/lib/services/music/music_playback_service_impl.dart b/lib/services/music/music_playback_service_impl.dart index 3a51b394..b3b14f67 100644 --- a/lib/services/music/music_playback_service_impl.dart +++ b/lib/services/music/music_playback_service_impl.dart @@ -1,4 +1,5 @@ import 'dart:async'; +import 'dart:math'; import 'package:flutter/foundation.dart' show ValueListenable, visibleForTesting; import 'package:flutter/widgets.dart'; @@ -70,10 +71,12 @@ class MusicPlaybackServiceImpl extends MusicPlaybackService with WidgetsBindingO this._completedConfirmDelay = const Duration(milliseconds: 400), PlaybackCoordinator? coordinator, @visibleForTesting Future Function(double)? volumePersistenceWriter, + @visibleForTesting Random? queueRandom, }) : assert(resolver != null || database != null, 'database is required to build the default resolver'), _serverManager = serverManager, _resolver = resolver ?? ServerMusicSourceResolver(serverManager: serverManager, database: database!), _coordinator = coordinator ?? PlaybackCoordinator.instance, + _queue = MusicQueueController(random: queueRandom), _volumePersistenceWriter = volumePersistenceWriter ?? _writePersistedVolume { _coordinator.registerMusicSession(stopAndDispose: _stopForVideoClaim); // tvOS has no background-audio session in v1, so it pauses on @@ -110,7 +113,7 @@ class MusicPlaybackServiceImpl extends MusicPlaybackService with WidgetsBindingO final PlaybackCoordinator _coordinator; final Future Function(double) _volumePersistenceWriter; - final MusicQueueController _queue = MusicQueueController(); + final MusicQueueController _queue; /// Persisted music volume (0–100), applied to every audio player instance /// (the core is recreated after video claims playback). Falls back to full @@ -336,10 +339,14 @@ class MusicPlaybackServiceImpl extends MusicPlaybackService with WidgetsBindingO final generation = ++_generation; _invalidateArmRequests(); _finalizeCurrentTrack(); - var startIndex = 0; + // Null start index = "no track has to play first", which is what lets a + // shuffled queue randomize its head too. Collapsing that into 0 pinned + // every shuffle launch to the list's first track (#1811) — including a + // start track the list turns out not to contain. + int? startIndex; if (startTrack != null) { - startIndex = tracks.indexWhere((t) => t.globalKey == startTrack.globalKey); - if (startIndex < 0) startIndex = 0; + final index = tracks.indexWhere((t) => t.globalKey == startTrack.globalKey); + if (index >= 0) startIndex = index; } _queue.load(tracks, startIndex: startIndex, shuffle: shuffle); _playContext = playContext; diff --git a/lib/services/music/music_queue_controller.dart b/lib/services/music/music_queue_controller.dart index 3a3867a4..deed7480 100644 --- a/lib/services/music/music_queue_controller.dart +++ b/lib/services/music/music_queue_controller.dart @@ -45,17 +45,28 @@ class MusicQueueController { MediaItem? trackAt(int queueIndex) => queueIndex >= 0 && queueIndex < _order.length ? _items[_order[queueIndex]] : null; - /// Replace the queue with [tracks], starting at [startIndex]. With - /// [shuffle] the start track is anchored first and the rest shuffle after - /// it (it keeps playing / plays first). - void load(List tracks, {int startIndex = 0, bool shuffle = false}) { + /// Replace the queue with [tracks], starting at [startIndex]. A null + /// [startIndex] (the default) means *no explicit start* — playback simply + /// begins at the head. + /// + /// [shuffle] reads that distinction. With an explicit [startIndex] the + /// start track is anchored first and the rest shuffle after it (it keeps + /// playing / plays first); with none the whole list shuffles, head + /// included. Collapsing "no start track" into index 0 is what pinned every + /// shuffled playlist to its first track (#1811). + void load(List tracks, {int? startIndex, bool shuffle = false}) { _items ..clear() ..addAll(tracks); _order = List.generate(tracks.length, (i) => i); _shuffled = false; - _cursor = tracks.isEmpty ? -1 : startIndex.clamp(0, tracks.length - 1); - if (shuffle && tracks.isNotEmpty) _shuffleAnchoringCurrent(); + _cursor = tracks.isEmpty ? -1 : (startIndex ?? 0).clamp(0, tracks.length - 1); + if (!shuffle || tracks.isEmpty) return; + if (startIndex == null) { + _shuffleAll(); + } else { + _shuffleAnchoringCurrent(); + } } void clear() { @@ -174,6 +185,14 @@ class MusicQueueController { _shuffled = true; } + /// Shuffle every entry, head included — a session started *as* shuffled + /// has no track that must play first. + void _shuffleAll() { + _order.shuffle(_random); + _cursor = 0; + _shuffled = true; + } + /// Drop everything after the current entry (playback order), including /// the underlying canonical items. void clearUpcoming() { diff --git a/test/services/music/music_playback_service_test.dart b/test/services/music/music_playback_service_test.dart index c637c896..0703854d 100644 --- a/test/services/music/music_playback_service_test.dart +++ b/test/services/music/music_playback_service_test.dart @@ -1,4 +1,5 @@ import 'dart:async'; +import 'dart:math'; import 'package:flutter_test/flutter_test.dart'; import 'package:os_media_controls/os_media_controls.dart'; @@ -547,8 +548,36 @@ class _GatedVolumeWriter { } } +/// Queue [Random] the tests can script. Real randomness by default; +/// [lowestDraw] makes a shuffle deterministic without depending on the SDK's +/// seeded-PRNG sequence (which carries no cross-release guarantee). +class _ScriptedRandom implements Random { + final Random _real = Random(); + late int Function(int max) _draw = _real.nextInt; + + /// Every draw picks the lowest candidate index. + void lowestDraw() => _draw = (_) => 0; + + @override + int nextInt(int max) => _draw(max); + + @override + bool nextBool() => _real.nextBool(); + + @override + double nextDouble() => _real.nextDouble(); +} + class _Harness { - _Harness._(this.service, this.resolver, this.client, this.controls, this.players, this.serverManager); + _Harness._( + this.service, + this.resolver, + this.client, + this.controls, + this.players, + this.serverManager, + this.queueRandom, + ); final MusicPlaybackServiceImpl service; final FakeMusicSourceResolver resolver; @@ -557,6 +586,9 @@ class _Harness { final List players; final MultiServerManager serverManager; + /// Drives the queue's shuffle; script it before starting a shuffled queue. + final _ScriptedRandom queueRandom; + /// Seeded into every created FakePlayer — lets a test configure arm /// failures before the first player exists. final Set failingSetNextUris = {}; @@ -564,6 +596,7 @@ class _Harness { FakePlayer get player => players.last; factory _Harness.create({Future Function(double)? volumePersistenceWriter}) { + final queueRandom = _ScriptedRandom(); final client = FakeMediaServerClient(); final resolver = FakeMusicSourceResolver(client: client); final controls = FakeMediaControlsManager(); @@ -584,8 +617,9 @@ class _Harness { // paths resolve within pumpEventQueue. completedConfirmDelay: Duration.zero, volumePersistenceWriter: volumePersistenceWriter, + queueRandom: queueRandom, ); - harness = _Harness._(service, resolver, client, controls, players, serverManager); + harness = _Harness._(service, resolver, client, controls, players, serverManager, queueRandom); return harness; } @@ -622,6 +656,42 @@ void main() { h.serverManager.dispose(); }); + group('shuffled session start (#1811)', () { + final playlist = [for (var i = 0; i < 6; i++) _track('p$i')]; + + test('no start track shuffles the head too instead of pinning the first track', () async { + h.queueRandom.lowestDraw(); + + await h.playTracks(playlist, shuffle: true); + + final opened = h.service.currentTrack!; + expect(opened.id, isNot('p0'), reason: 'shuffle opened on the list head'); + expect(h.service.queue.first.id, opened.id); + expect(h.service.queue.map((t) => t.id).toSet(), playlist.map((t) => t.id).toSet()); + expect(h.service.shuffled, isTrue); + expect(h.player.openedUris, [_urlFor(opened)]); + }); + + test('an explicit start track still anchors the shuffled queue', () async { + h.queueRandom.lowestDraw(); + + await h.playTracks(playlist, startTrack: playlist[3], shuffle: true); + + expect(h.service.currentTrack!.id, 'p3'); + expect(h.service.queue.first.id, 'p3'); + expect(h.service.queue.map((t) => t.id).toSet(), playlist.map((t) => t.id).toSet()); + }); + + test('a start track absent from the list drops the anchor rather than pinning the first track', () async { + h.queueRandom.lowestDraw(); + + await h.playTracks(playlist, startTrack: _track('not-in-list'), shuffle: true); + + expect(h.service.currentTrack!.id, isNot('p0')); + expect(h.service.queue.map((t) => t.id).toSet(), playlist.map((t) => t.id).toSet()); + }); + }); + test('volume updates notify only the dedicated volume listenable', () async { await h.playTracks([t1]); var serviceNotifications = 0; diff --git a/test/services/music/music_queue_controller_test.dart b/test/services/music/music_queue_controller_test.dart index 7eb43152..47ac0f6c 100644 --- a/test/services/music/music_queue_controller_test.dart +++ b/test/services/music/music_queue_controller_test.dart @@ -36,6 +36,21 @@ void main() { expect(_ids(q.queue).toSet(), _ids(tracks).toSet()); }); + test('shuffle without a start index randomizes the head too (#1811)', () { + // A shuffle launch has no track that must play first, so the head must + // be drawn from the whole list. Anchoring the default index 0 made + // every shuffled playlist open on its first track. + final heads = {}; + for (var seed = 0; seed < 8; seed++) { + final q = controller(seed: seed)..load(tracks, shuffle: true); + expect(q.shuffled, isTrue); + expect(q.cursor, 0); + expect(_ids(q.queue).toSet(), _ids(tracks).toSet()); + heads.add(q.current!.id); + } + expect(heads.length, greaterThan(1), reason: 'head stayed pinned to one track across seeds'); + }); + test('empty load leaves an idle queue', () { final q = controller()..load(const []); expect(q.isEmpty, isTrue);