Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reordering does not work well when combined with shuffle #1042

Open
Chaphasilor opened this issue Aug 10, 2023 · 10 comments
Open

Reordering does not work well when combined with shuffle #1042

Chaphasilor opened this issue Aug 10, 2023 · 10 comments
Assignees
Labels
1 backlog bug Something isn't working

Comments

@Chaphasilor
Copy link

Which API doesn't behave as documented, and how does it misbehave?
Name here the specific methods or fields that are not behaving as documented, and explain clearly what is happening.

ConcatenatingAudioSource.move()

Moves an AudioSource from currentIndex to newIndex

The described behavior only works as long as AudioPlayer.shuffleModeEnabled is false. If this is not the case, the unshuffled indices will be moved around instead, causing the shuffled order to change randomly instead of reflecting the desired change.
As a side effect, this also means that after disabling shuffle, the unshuffled indices are different than before, which is not how playlists should work. The unshuffled indices should never change, unless specifically requested.

In case this method is meant to behave exactly like it does, another method would be needed that only affects shuffled indices. The current one does not, as can be seen from the implementation.

Minimal reproduction project
Provide a link here using one of two options:

  1. Fork this repository and modify the example to reproduce the bug, then provide a link here.
  2. If the unmodified official example already reproduces the bug, just write "The example".

https://github.com/Chaphasilor/audio_service/tree/shuffle-reorder-bug-repro

Only change was renaming the shuffle example to main.dart and adding more dummy items to the playlists.

To Reproduce (i.e. user steps, not code)
Steps to reproduce the behavior:

  1. Run playlist example
  2. Check the playlist/queue. All tracks should be in order (1-6)
  3. Enable shuffle. All tracks after the current track are shuffled, as expected
  4. Drag-and-drop one of the tracks to a different position within the playlist/queue. The track will (most likely) not be reordered correctly. Instead, a seemingly random track will change position. Repeat multiple times to observe erratic and unpredictable behavior.
  5. Disable shuffle. The queue (probably) won't be in the correct order anymore, although it should be. After all, it's not shuffled anymore.

Error messages

If applicable, copy & paste error message here, within the triple quotes to preserve formatting.

Expected behavior
A clear and concise description of what you expected to happen.

  • When shuffle is off, ConcatenatingAudioSource.move() should update the unshuffled indices, i.e. change the actual order of the AudioSource's children
  • When shuffle is on, ConcatenatingAudioSource.move() should update the shuffled indices, i.e. only change the ShuffleOrder.indices and not update the order of the AudioSource's children at all.
  • After reordering tracks while shuffle was on and then turning shuffle off, the original, unshuffled order should be restored. While shuffle is on, it should not be possible to update the unshuffled indices / order of children by calling ConcatenatingAudioSource.move()
  • Alternatively, there could be two distinct methods, one for only moving the unshuffled indices, and one for moving the shuffled ones

Screenshots
If applicable, add screenshots to help explain your problem.

2023-08-10-19-23-27.mp4

Desktop (please complete the following information):

  • OS: Windows 10 22H2
  • Browser: n/a

Smartphone (please complete the following information):

  • Device: ASUS Zenfone 10
  • OS: Android 13 (Zen UI)

Flutter SDK version

Doctor summary (to see all details, run flutter doctor -v):
[√] Flutter (Channel stable, 3.10.0, on Microsoft Windows [Version 10.0.19045.3208], locale en-US)
[√] Windows Version (Installed version of Windows is version 10 or higher)
[!] Android toolchain - develop for Android devices (Android SDK version 34.0.0-rc4)
    X Could not determine java version
[√] Chrome - develop for the web
[√] Visual Studio - develop for Windows (Visual Studio Build Tools 2019 16.11.1)    
[!] Android Studio (version 2022.2)
    X Unable to determine bundled Java version.
[!] Android Studio (version 2022.1)
    X Unable to determine bundled Java version.
[√] VS Code (version 1.80.2)
[√] Connected device (4 available)
[√] Network resources

! Doctor found issues in 3 categories.

Additional context
Add any other context about the problem here.

Source question on StackOverflow: https://stackoverflow.com/questions/76813773/how-to-change-the-shuffled-order-in-just-audio-without-affecting-the-original-or

I've also checkout out various open-source flutter apps that use just_audio, but they either don't support reordering at all, or it is broken in the way I described.

@Chaphasilor
Copy link
Author

@ryanheise are you able to reproduce this? :)

@ryanheise
Copy link
Owner

Hi @Chaphasilor , shuffled indices is just a view of the actual indices, so enabling and disabling shuffle mode simply switches between a shuffled view of the indices, and the "actual" indices. This is intended to allow you to switch back and forth between the two, because the original sequence is still intact. If the user of your app wants to actually treat the shuffled order as if it is the actual order, and can be further mutated with move(), then you might want to consider implementing your own custom subclass of ShuffleOrder and override the insert/remove methods associated with the move operation (or if you don't happen to like the fact that the shuffle order is a view and you want everything to apply to the "actual" list, you could discard all of this shuffle functionality within just_audio and just implement your own shuffle logic that mutates the original list.)

@Chaphasilor
Copy link
Author

I would like to keep the original order and the shuffled order available to switch back and forth. I was just under the impression that, while shuffle is active, the move() operation would update the shuffled indices.

I already attempted to use a custom ShuffleOrder, but still have some hiccups with it. I'll see if I can get working the way I want it to.

Does that mean that this behavior is intended, and you're not planning to change which indices move() affects?

@ryanheise
Copy link
Owner

Yes, this is the intended behaviour, but I believe your use case should be possible to implement on top of this. Although you would have to define your own intended behaviour for the target of a move. For example, if the user is in shuffle mode and drags an item from position [2] to [4] in the shuffle view, it is clear what [2] means. You can look up the shuffle indices to find WHICH item that actually refers to (e.g. this might point to item [7] in the original list). But then what does it mean to drag this to position [4] in the view? You would need to define that. What I know is that you want the shuffled position to be [4] at the destination, but I don't know what position you want this to be in the original list. But once you define this and you know what you want it to be, let's say you want it to end up at [4] in the original list as well, then what you want to do is call move(7, 4), and then override your methods in ShuffleOrder to map [4] of the newly inserted item to the exact same position in the shuffled indices. If instead of making it end up at [4] in the original list as well, perhaps you wanted it to end up say at [9] in the original list, then you would have to define that behaviour (whatever it is) yourself, and then invoke move(7, 9) accordingly, and then implement your custom methods in your ShuffleOrder to map between these indices so that the shuffled position[4] points to [9] in the original list.

For me, it's a little hard to guess what your intended behaviour might be, because I could imagine different apps wanting to implement different shuffle order behaviour (hence why it is customisable). But do let me know if you run into limitations in the API.

@Chaphasilor
Copy link
Author

Thank you for the explanantion.

As stated in the original Stack Overflow quesion, I simply want the shuffle order and original order to be completely separate, so if I move an item within the shuffle view, I just want to update the shuffle indices and nothing else. Turning shuffle back off should show the original, unmodified order.
I do not care what happens to the shuffle order when I move an item in the original order, since I re-shuffle anyway, but potentially the shuffle indices could be modified accordingly to that the resulting shuffle order stays the same.

It seems like in my case I'd simply need to overwrite the move method to not do anything, and then implement a separate move method for my custom shuffle order. I'll have to keep a reference to my shuffle order since I can access it directly through the ConcatenatedAudioSource afaik, but it seems like things are becoming clearer :)

@ryanheise
Copy link
Owner

As stated in the original Stack Overflow quesion, I simply want the shuffle order and original order to be completely separate, so if I move an item within the shuffle view, I just want to update the shuffle indices and nothing else. Turning shuffle back off should show the original, unmodified order.

OK, this is not a use case I had designed it for so I'm not sure if it will be perfectly convenient for you. But you can make your shuffle order implementation modal so that you can set it to a particular mode before its methods are invoked.

For example, one of your situations is that you want to move an item in the shuffle order without affecting the original order. The correct way to affect the shuffle order is to always call player.shuffle(), but before doing so, you would set your shuffle order's "mode" into "shuffle move" mode or something like that, supplying the indices of the move. Then invoke await player.shuffle(), and then set the mode back to normal.

So you might consider having something like:

class MyShuffleOrder extends ... {
  (int, int)? pendingMove;
}

And if it's not null, the pending move indices will be used by your shuffle implementation to execute the shuffle move as opposed to doing a full randomised shuffle.

That's probably going to work even if it's not a usage this API was designed for.

@Chaphasilor
Copy link
Author

Ah okay, thanks for the pointer! So what you're saying is the shuffle indices view is only refreshed when shuffle() is called, so I need to make sure that this happens? Is there a reason why I couldn't move the indices right-away, and the set a updateOnly flag, that makes the ShuffleOrder.shuffle() method have no affect? The ConcatenatingAudioSource should then be able to fetch the latest indices.
Hopefully I'll have some time to play around with this tonight!

By the way, would you be interested in me contributing my shuffle order through a pull request? It might be useful to others trying to build advanced music apps :)

@ryanheise
Copy link
Owner

Well, the view is refreshed in a few other places too, such as when you invoke methods on ConcatenatingAudioSource to change the list of children, but in your case, you want player.shuffle(). I didn't understand your second question unfortunately, but once you at least understand the chain of events that occurs within the plugin, you may find some flexibility in how you can make the right updates happen in the right order.

Since the solution is going to be a bit hacky, I'm not sure yet whether it is something that should be an official example, or anything like that, but if you share your implementation below, that will both help others who search for this page, and also help me to take a look at what you're doing and consider future API changes to make such use cases less hacky.

@kaciula
Copy link

kaciula commented May 20, 2024

@Chaphasilor Have you managed to implement the shuffle logic you wanted? If yes, can you please share your implementation? I'm struggling with this myself.

@richanshah
Copy link

richanshah commented Sep 2, 2024

I am facing same issue in Android auto and below is my code snippet let me know if any solution available

` @OverRide
Future playFromMediaId(String mediaId, [Map<String, dynamic>? extras]) async {
DebugLog.d('audio_player_handler playFromMediaId: $mediaId');
//manage queue for carPlayer
if (mediaId.contains('@')) {
//get parentId using split from last '@' character
//using parentId get the list of mediaItem
//remove attached parentId and add in queue
//and play the song
var parentId = mediaId.split('@').last;
List? songList = _mediaLibrary.items[parentId];
if (songList != null && songList.isNotEmpty) {
for (int i = 0; i < songList.length; i++) {
songList[i].id = songList[i].id.split('@').first;
}
await updateQueue(songList);
MediaItem? mediaItem = songList.firstWhereOrNull((element) => element.id == mediaId.split('@').first);
DebugLog.d('player mediaItem playFromMediaId @: $mediaItem');

    var index = 0;
    if (mediaItem != null) {
      index = songList.indexOf(mediaItem);
    } else {
      DebugLog.d('player mediaItem not found in queue');
    }
    skipToQueueItem(index);
    play();
  }
} else {
  //manage queue for mobile
  //get the queue and find mediaItem using 'mediaId'
  //skip till mediaItem and then after start play.
  List<MediaItem> list = queue.value;
  MediaItem? mediaItem = list.firstWhereOrNull((element) => element.id == mediaId);
  DebugLog.d('player mediaItem playFromMediaId: $mediaItem');
  if (mediaItem != null) {
    int index = list.indexOf(mediaItem);
    if (index == -1) {
      DebugLog.d('player mediaItem not found in queue');
      // do the logic of new queue and playlist
    } else {
      // found the requested mediaItem in current Queue
      // play that mediaItem
      skipToQueueItem(index);
      play();
    }
  }
}

}`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 backlog bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants