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
This commit is contained in:
@@ -73,8 +73,9 @@ abstract class MusicPlaybackService extends ChangeNotifier {
|
|||||||
int get queueSessionRevision;
|
int get queueSessionRevision;
|
||||||
|
|
||||||
/// Start a new queue from [tracks], optionally at [startTrack] (defaults
|
/// Start a new queue from [tracks], optionally at [startTrack] (defaults
|
||||||
/// to the first track). [shuffle] shuffles with the start track anchored
|
/// to the first track). [shuffle] anchors [startTrack] first and shuffles
|
||||||
/// first.
|
/// 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<void> playFromList({
|
Future<void> playFromList({
|
||||||
required List<MediaItem> tracks,
|
required List<MediaItem> tracks,
|
||||||
MediaItem? startTrack,
|
MediaItem? startTrack,
|
||||||
|
|||||||
@@ -1,4 +1,5 @@
|
|||||||
import 'dart:async';
|
import 'dart:async';
|
||||||
|
import 'dart:math';
|
||||||
|
|
||||||
import 'package:flutter/foundation.dart' show ValueListenable, visibleForTesting;
|
import 'package:flutter/foundation.dart' show ValueListenable, visibleForTesting;
|
||||||
import 'package:flutter/widgets.dart';
|
import 'package:flutter/widgets.dart';
|
||||||
@@ -70,10 +71,12 @@ class MusicPlaybackServiceImpl extends MusicPlaybackService with WidgetsBindingO
|
|||||||
this._completedConfirmDelay = const Duration(milliseconds: 400),
|
this._completedConfirmDelay = const Duration(milliseconds: 400),
|
||||||
PlaybackCoordinator? coordinator,
|
PlaybackCoordinator? coordinator,
|
||||||
@visibleForTesting Future<void> Function(double)? volumePersistenceWriter,
|
@visibleForTesting Future<void> Function(double)? volumePersistenceWriter,
|
||||||
|
@visibleForTesting Random? queueRandom,
|
||||||
}) : assert(resolver != null || database != null, 'database is required to build the default resolver'),
|
}) : assert(resolver != null || database != null, 'database is required to build the default resolver'),
|
||||||
_serverManager = serverManager,
|
_serverManager = serverManager,
|
||||||
_resolver = resolver ?? ServerMusicSourceResolver(serverManager: serverManager, database: database!),
|
_resolver = resolver ?? ServerMusicSourceResolver(serverManager: serverManager, database: database!),
|
||||||
_coordinator = coordinator ?? PlaybackCoordinator.instance,
|
_coordinator = coordinator ?? PlaybackCoordinator.instance,
|
||||||
|
_queue = MusicQueueController(random: queueRandom),
|
||||||
_volumePersistenceWriter = volumePersistenceWriter ?? _writePersistedVolume {
|
_volumePersistenceWriter = volumePersistenceWriter ?? _writePersistedVolume {
|
||||||
_coordinator.registerMusicSession(stopAndDispose: _stopForVideoClaim);
|
_coordinator.registerMusicSession(stopAndDispose: _stopForVideoClaim);
|
||||||
// tvOS has no background-audio session in v1, so it pauses on
|
// 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 PlaybackCoordinator _coordinator;
|
||||||
final Future<void> Function(double) _volumePersistenceWriter;
|
final Future<void> Function(double) _volumePersistenceWriter;
|
||||||
|
|
||||||
final MusicQueueController _queue = MusicQueueController();
|
final MusicQueueController _queue;
|
||||||
|
|
||||||
/// Persisted music volume (0–100), applied to every audio player instance
|
/// Persisted music volume (0–100), applied to every audio player instance
|
||||||
/// (the core is recreated after video claims playback). Falls back to full
|
/// (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;
|
final generation = ++_generation;
|
||||||
_invalidateArmRequests();
|
_invalidateArmRequests();
|
||||||
_finalizeCurrentTrack();
|
_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) {
|
if (startTrack != null) {
|
||||||
startIndex = tracks.indexWhere((t) => t.globalKey == startTrack.globalKey);
|
final index = tracks.indexWhere((t) => t.globalKey == startTrack.globalKey);
|
||||||
if (startIndex < 0) startIndex = 0;
|
if (index >= 0) startIndex = index;
|
||||||
}
|
}
|
||||||
_queue.load(tracks, startIndex: startIndex, shuffle: shuffle);
|
_queue.load(tracks, startIndex: startIndex, shuffle: shuffle);
|
||||||
_playContext = playContext;
|
_playContext = playContext;
|
||||||
|
|||||||
@@ -45,17 +45,28 @@ class MusicQueueController {
|
|||||||
MediaItem? trackAt(int queueIndex) =>
|
MediaItem? trackAt(int queueIndex) =>
|
||||||
queueIndex >= 0 && queueIndex < _order.length ? _items[_order[queueIndex]] : null;
|
queueIndex >= 0 && queueIndex < _order.length ? _items[_order[queueIndex]] : null;
|
||||||
|
|
||||||
/// Replace the queue with [tracks], starting at [startIndex]. With
|
/// Replace the queue with [tracks], starting at [startIndex]. A null
|
||||||
/// [shuffle] the start track is anchored first and the rest shuffle after
|
/// [startIndex] (the default) means *no explicit start* — playback simply
|
||||||
/// it (it keeps playing / plays first).
|
/// begins at the head.
|
||||||
void load(List<MediaItem> tracks, {int startIndex = 0, bool shuffle = false}) {
|
///
|
||||||
|
/// [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<MediaItem> tracks, {int? startIndex, bool shuffle = false}) {
|
||||||
_items
|
_items
|
||||||
..clear()
|
..clear()
|
||||||
..addAll(tracks);
|
..addAll(tracks);
|
||||||
_order = List.generate(tracks.length, (i) => i);
|
_order = List.generate(tracks.length, (i) => i);
|
||||||
_shuffled = false;
|
_shuffled = false;
|
||||||
_cursor = tracks.isEmpty ? -1 : startIndex.clamp(0, tracks.length - 1);
|
_cursor = tracks.isEmpty ? -1 : (startIndex ?? 0).clamp(0, tracks.length - 1);
|
||||||
if (shuffle && tracks.isNotEmpty) _shuffleAnchoringCurrent();
|
if (!shuffle || tracks.isEmpty) return;
|
||||||
|
if (startIndex == null) {
|
||||||
|
_shuffleAll();
|
||||||
|
} else {
|
||||||
|
_shuffleAnchoringCurrent();
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
void clear() {
|
void clear() {
|
||||||
@@ -174,6 +185,14 @@ class MusicQueueController {
|
|||||||
_shuffled = true;
|
_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
|
/// Drop everything after the current entry (playback order), including
|
||||||
/// the underlying canonical items.
|
/// the underlying canonical items.
|
||||||
void clearUpcoming() {
|
void clearUpcoming() {
|
||||||
|
|||||||
@@ -1,4 +1,5 @@
|
|||||||
import 'dart:async';
|
import 'dart:async';
|
||||||
|
import 'dart:math';
|
||||||
|
|
||||||
import 'package:flutter_test/flutter_test.dart';
|
import 'package:flutter_test/flutter_test.dart';
|
||||||
import 'package:os_media_controls/os_media_controls.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 {
|
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 MusicPlaybackServiceImpl service;
|
||||||
final FakeMusicSourceResolver resolver;
|
final FakeMusicSourceResolver resolver;
|
||||||
@@ -557,6 +586,9 @@ class _Harness {
|
|||||||
final List<FakePlayer> players;
|
final List<FakePlayer> players;
|
||||||
final MultiServerManager serverManager;
|
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
|
/// Seeded into every created FakePlayer — lets a test configure arm
|
||||||
/// failures before the first player exists.
|
/// failures before the first player exists.
|
||||||
final Set<String> failingSetNextUris = {};
|
final Set<String> failingSetNextUris = {};
|
||||||
@@ -564,6 +596,7 @@ class _Harness {
|
|||||||
FakePlayer get player => players.last;
|
FakePlayer get player => players.last;
|
||||||
|
|
||||||
factory _Harness.create({Future<void> Function(double)? volumePersistenceWriter}) {
|
factory _Harness.create({Future<void> Function(double)? volumePersistenceWriter}) {
|
||||||
|
final queueRandom = _ScriptedRandom();
|
||||||
final client = FakeMediaServerClient();
|
final client = FakeMediaServerClient();
|
||||||
final resolver = FakeMusicSourceResolver(client: client);
|
final resolver = FakeMusicSourceResolver(client: client);
|
||||||
final controls = FakeMediaControlsManager();
|
final controls = FakeMediaControlsManager();
|
||||||
@@ -584,8 +617,9 @@ class _Harness {
|
|||||||
// paths resolve within pumpEventQueue.
|
// paths resolve within pumpEventQueue.
|
||||||
completedConfirmDelay: Duration.zero,
|
completedConfirmDelay: Duration.zero,
|
||||||
volumePersistenceWriter: volumePersistenceWriter,
|
volumePersistenceWriter: volumePersistenceWriter,
|
||||||
|
queueRandom: queueRandom,
|
||||||
);
|
);
|
||||||
harness = _Harness._(service, resolver, client, controls, players, serverManager);
|
harness = _Harness._(service, resolver, client, controls, players, serverManager, queueRandom);
|
||||||
return harness;
|
return harness;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -622,6 +656,42 @@ void main() {
|
|||||||
h.serverManager.dispose();
|
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 {
|
test('volume updates notify only the dedicated volume listenable', () async {
|
||||||
await h.playTracks([t1]);
|
await h.playTracks([t1]);
|
||||||
var serviceNotifications = 0;
|
var serviceNotifications = 0;
|
||||||
|
|||||||
@@ -36,6 +36,21 @@ void main() {
|
|||||||
expect(_ids(q.queue).toSet(), _ids(tracks).toSet());
|
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 = <String>{};
|
||||||
|
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', () {
|
test('empty load leaves an idle queue', () {
|
||||||
final q = controller()..load(const []);
|
final q = controller()..load(const []);
|
||||||
expect(q.isEmpty, isTrue);
|
expect(q.isEmpty, isTrue);
|
||||||
|
|||||||
Reference in New Issue
Block a user