Skip to content

notify ShuffleOrder when media item(s) are moved #2226

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

notify ShuffleOrder when media item(s) are moved #2226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,7 +703,7 @@ public void moveMediaItems(int fromIndex, int toIndex, int newFromIndex) {
}
Timeline oldTimeline = getCurrentTimeline();
pendingOperationAcks++;
Util.moveItems(mediaSourceHolderSnapshots, fromIndex, toIndex, newFromIndex);
moveMediaSourceHolders(fromIndex, toIndex, newFromIndex);
Timeline newTimeline = createMaskingTimeline();
PlaybackInfo newPlaybackInfo =
maskTimelineAndPosition(
Expand All @@ -725,6 +725,11 @@ public void moveMediaItems(int fromIndex, int toIndex, int newFromIndex) {
/* repeatCurrentMediaItem= */ false);
}

private void moveMediaSourceHolders(int fromIndex, int toIndex, int newFromIndex) {
Util.moveItems(mediaSourceHolderSnapshots, fromIndex, toIndex, newFromIndex);
shuffleOrder = shuffleOrder.cloneAndMove(fromIndex, toIndex, newFromIndex);
}

@Override
public void replaceMediaItems(int fromIndex, int toIndex, List<MediaItem> mediaItems) {
verifyApplicationThread();
Expand Down
16 changes: 16 additions & 0 deletions libraries/exoplayer/src/main/java/androidx/media3/exoplayer/source/ShuffleOrder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,22 @@ public ShuffleOrder cloneAndClear() {
*/
ShuffleOrder cloneAndInsert(int insertionIndex, int insertionCount);

/**
* Returns a copy of the shuffle order with a range of elements moved.
*
* @param indexFrom The starting index in the unshuffled order of the range to move, from before
* the move occurs.
* @param indexToExclusive The smallest index (must be greater or equal to {@code indexFrom}) that
* is not included in the range of elements moved, from before the move occurs.
* @param newIndexFrom The starting index in the unshuffled order of the range to move, from after
* the move occurs.
* @return A copy of this {@link ShuffleOrder} with the elements moved.
*/
default ShuffleOrder cloneAndMove(int indexFrom, int indexToExclusive, int newIndexFrom) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we implement this method like this, then I think there isn't much benefit, as calling the two other methods would be equivalent and we don't have to introduce this new method.

If we would implement this method standalone without using the other methods, you could raise a point that we can do it in DefaultShuffleOrder with a single implementation that probably has to iterate only once over the array. Do you think there is sufficient benefit in adding this new API and then implementing it here in the suggested way?

However we implement this. If we accept the change we'd need to have a unit test I think. I'd add that myself also, so this is not a condition to take that change if we think we have enough benefit because we can do it with better performance than the higher-order composite calls.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @marcbaechinger,

the benefit of adding a seperate API for this is that shuffle order may decide to not reshuffle in the individual items when a group of items is moved.

My goal with my custom ShuffleOrder is to make add/remove/move operations on the unshuffled ExoPlayer playlist map directly to shuffled playlist. My custom ShuffleOrder will reshuffle the added item group individually for insert action, but I do not want to shuffle them again when moving.

When using the existing API, ShuffleOrder can't know if this is a remove+insert done by the user, or a real move operation.

I'm not sure about the performance, but people who don't care about knowing when moving happens wouldn't need to implement any additional code if there is a default implementation. And of course, if the performance is increased when combining the call, DefaultShuffleOrder can still override cloneAndMove without the default implementation being removed. But personally, I do not feel strongly about the default implementation and it could be removed - but a seperate "move" API is needed for my usecase.

return cloneAndRemove(indexFrom, indexToExclusive)
.cloneAndInsert(newIndexFrom, indexToExclusive - indexFrom);
}

/**
* Returns a copy of the shuffle order with a range of elements removed.
*
Expand Down