notify ShuffleOrder when media item(s) are moved #2226
Conversation
|
@rohitjoins @icbaker any chance one of you could take a look? |
|
@marcbaechinger ping since it has been a month, any chance you could take a look at this PR? |
| * the move occurs. | ||
| * @return A copy of this {@link ShuffleOrder} with the elements moved. | ||
| */ | ||
| default ShuffleOrder cloneAndMove(int indexFrom, int indexToExclusive, int newIndexFrom) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
I'm going to send this for internal review now. You may see some more commits being added as I make changes in response to review feedback. Please refrain from pushing any more substantive changes as it will complicate the internal review - thanks! |
Issue: #1932
Issue: #1381
--
(Extracting a seperate method
moveMediaSourceHolderswas done to align with insert/remove code. A newdefaultmethod instead of just implementing that inDefaultShuffleOrderwas done because I believe the move detection is not required by simpler shuffle order implementations, and this makes upgrades easier as devs don't have to implement a new function.)