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.

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

feat(playlist): remove MediaSource range #4564

Merged
merged 4 commits into from
Aug 16, 2018
Merged

feat(playlist): remove MediaSource range #4564

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c9b6a73
feat(playlist): add API to remove range of indices
GiuseppePiscopo Jul 24, 2018
2c801ca
docs(playlist): fix javadoc comments
GiuseppePiscopo Jul 24, 2018
cc7f011
feat(playlist): stick to list.subList API
GiuseppePiscopo Jul 25, 2018
7878bf6
chore(playlist): reorder message IDs, fix tabbing
GiuseppePiscopo Jul 25, 2018
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
94 changes: 93 additions & 1 deletion ...ary/core/src/main/java/com/google/android/exoplayer2/source/ConcatenatingMediaSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ public class ConcatenatingMediaSource extends CompositeMediaSource<MediaSourceHo
private static final int MSG_ADD = 0;
private static final int MSG_ADD_MULTIPLE = 1;
private static final int MSG_REMOVE = 2;
private static final int MSG_REMOVE_RANGE = 200;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you renumber this to be 3 and change the numbers below accordingly?

private static final int MSG_MOVE = 3;
private static final int MSG_CLEAR = 4;
private static final int MSG_NOTIFY_LISTENER = 5;
Expand Down Expand Up @@ -265,6 +266,9 @@ public final synchronized void addMediaSources(
* <p>Note: If you want to move the instance, it's preferable to use {@link #moveMediaSource(int,
* int)} instead.
*
* <p>Note: If you want to remove a set of contiguous sources, it's preferable to use
* {@link #removeMediaSourceRange(int, int)} instead.
*
* @param index The index at which the media source will be removed. This index must be in the
* range of 0 &lt;= index &lt; {@link #getSize()}.
*/
Expand All @@ -276,7 +280,10 @@ public final synchronized void removeMediaSource(int index) {
* Removes a {@link MediaSource} from the playlist and executes a custom action on completion.
*
* <p>Note: If you want to move the instance, it's preferable to use {@link #moveMediaSource(int,
* int)} instead.
* int, Runnable)} instead.
*
* <p>Note: If you want to remove a set of contiguous sources, it's preferable to use
* {@link #removeMediaSourceRange(int, int, Runnable)} instead.
*
* @param index The index at which the media source will be removed. This index must be in the
* range of 0 &lt;= index &lt; {@link #getSize()}.
Expand All @@ -297,6 +304,79 @@ public final synchronized void removeMediaSource(
}
}

/**
* Removes a range of {@link MediaSource}s from the playlist, by specifying an initial index
* (included) and a final index (excluded).
*
* <p>Note: when specified range is empty, no actual media source is removed and no exception
* is thrown.
*
* @param fromIndex The initial range index, pointing to the first media source that will be
* removed. This index must be in the range of 0 &lt;= index &lt; {@link #getSize()}.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Both ranges should be "in the range of 0 <= index <= {@link #getSize()}." That is, including the getSize index.
Otherwise you wouldn't be able to do removeMediaSourceRange(getSize(), getSize()) as an edge case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At a given time, "occupied" track indices are 0, 1, ... getSize() - 1. fromIndex should point to an occupied index, as the initial range index is included in the range. How can one remove the track at index getSize(), if there's no track at that index? Just trying to understand

Copy link
Collaborator

Choose a reason for hiding this comment

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

As the end index is exclusive, the method must allow getSize() as end index.

And to support the example you gave in the issue, to keep an element at index N only, it would be nice to be able to call
removeMediaSourceRange(0, N) followed by removeMediaSourceRange(1,getSize()). If this is supposed to remove the last element, the second call will result in removeMediaSourceRange(1, 1) which should be a no-op.

Copy link
Contributor Author

@BrainCrumbz BrainCrumbz Jul 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We've abandoned the "simplicity" implied by initial issue example when we "gave up" on API tolerance (so to say). Since then, the idea seemed: have a clear API with errors when arguments are not correct. And for fromIndex to be equal to getSize() seems not correct. Of course we noticed that you cannot call removeMediaSourceRange(getSize(), getSize()) anymore. And also you cannot call removeMediaSourceRange(1, getSize()) in issue second call, that would throw too.

Staying consistent with allowed param values in signature seems quite a thing. By allowing passing getIndex() in fromIndex, it seems to go back to the "tolerant" approach somehow, which is fine anyway. Don't know, your call.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say it's best to follow the existing behaviour of List.subList and the like. That is, throwing if startIndex<0, endIndex>size or startIndex>endIndex. This ensures the operation is valid, but is also tolerant enough to support common use cases like "remove everything behind element N" with removeRange(N, size) which will work even if N is the last element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. Just to tease a little bit, in case like "remove everything behind element N" when N is the last element, then N = size - 1, and removeMediaSourceRange(size - 1, size) already works as is. What instead currently throws (and probably looks not tolerant) is removeMediaSourceRange(size, size). Right?

Anyway, changes are coming

Copy link
Collaborator

Choose a reason for hiding this comment

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

removeMediaSourceRange(size - 1, size) would remove the element at N = size - 1 though, right?

And if I'm not mistaken removeMediaSourceRange(size, size) won't throw too because List.subList doesn't throw for this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Q1: correct, current implementation would remove element at size - 1, i.e. "the last element"

Q2: removeMediaSourceRange(size, size) throws in current (<=> initial version) of this PR. It will not throw after applying your proposed changes, as List.subList(size, size) does not throw. I was referring to current implementation.

Cool

* @param toIndex The final range index, pointing to the first media source that will be left
* untouched. The last media source to be removed is at index {@code toIndex} - 1.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"The last media source to be removed is at index {@code toIndex} - 1." - That seems redundant and can be removed.

* This index must be in the range of 0 &lt;= index &lt; {@link #getSize()}.
*
Copy link
Collaborator

Choose a reason for hiding this comment

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

We usually leave no empty line between @param and @throws.

* @throws IndexOutOfBoundsException when either index is out of playlist bounds, i.e. {@code
* fromIndex} &lt; 0 or &gt;= {@link #getSize()}, {@code toIndex} &lt; 0 or &gt; {@link
* #getSize()}
* @throws IndexOutOfBoundsException when range is malformed, i.e. {@code fromIndex} &gt;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you combine them in one @throws doc? Maybe just as "When the range is malformed, i.e. {@code fromIndex} < 0, {@code toIndex} > {@link #getSize()}, or {@code fromIndex} > {@code toIndex}."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree with grouping. Are you also suggesting to slim down the number of (broken) examples provided in JavaDoc?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. As we already require fromIndex>lastIndex, the other two examples are not needed.

* {@code toIndex}
*/
public final synchronized void removeMediaSourceRange(int fromIndex, int toIndex) {
removeMediaSourceRange(fromIndex, toIndex, null);
}

/**
* Removes a range of {@link MediaSource}s from the playlist, by specifying an initial index
* (included) and a final index (excluded), and executes a custom action on completion..
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove second full stop.

*
* <p>Note: when specified range is empty, no actual media source is removed and no exception
* is thrown.
*
* @param fromIndex The initial range index, pointing to the first media source that will be
* removed. This index must be in the range of 0 &lt;= index &lt; {@link #getSize()}.
* @param toIndex The final range index, pointing to the first media source that will be left
* untouched. The last media source to be removed is at index {@code toIndex} - 1.
* This index must be in the range of 0 &lt;= index &lt; {@link #getSize()}.
*
* @throws IndexOutOfBoundsException when either index is out of playlist bounds, i.e. {@code
* fromIndex} &lt; 0 or &gt;= {@link #getSize()}, {@code toIndex} &lt; 0 or &gt; {@link
* #getSize()}
* @throws IndexOutOfBoundsException when range is malformed, i.e. {@code fromIndex} &gt;
* {@code toIndex}
* @param actionOnCompletion A {@link Runnable} which is executed immediately after the media
Copy link
Collaborator

Choose a reason for hiding this comment

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

This param should be together with the other params above.

* source range has been removed from the playlist.
*/
public final synchronized void removeMediaSourceRange(
int fromIndex, int toIndex, @Nullable Runnable actionOnCompletion) {
if (fromIndex < 0 || fromIndex >= mediaSourcesPublic.size()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to do the explicit bound checks here. As soon as you call the subList method, the IndexOutOfBoundsException will be thrown anyway.

throw new IndexOutOfBoundsException(String.format("Cannot remove source range: initial index (%d) out of bounds", fromIndex));
}
if (toIndex < 0 || toIndex > mediaSourcesPublic.size()) {
throw new IndexOutOfBoundsException(String.format("Cannot remove source range: final index (%d) out of bounds", toIndex));
}
if (fromIndex > toIndex) {
throw new IndexOutOfBoundsException(String.format("Cannot remove source range: range malformed (%d, %d)", fromIndex, toIndex));
}
if (fromIndex == toIndex) {
if (actionOnCompletion != null) {
actionOnCompletion.run();
}
return;
}
mediaSourcesPublic.subList(fromIndex, toIndex).clear();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you replace that with Util.removeRange(mediaSourcesPublic, fromIndex, toIndex)? That calls the same method but is slightly more readable.

Also move it above the if(fromIndex == toIndex) check to ensure the exceptions are thrown first.

if (player != null) {
player
.createMessage(this)
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should be indented by 4 spaces (relative to "player")

.setType(MSG_REMOVE_RANGE)
.setPayload(new MessageData<>(fromIndex, toIndex, actionOnCompletion))
.send();
} else if (actionOnCompletion != null) {
actionOnCompletion.run();
}
}

/**
* Moves an existing {@link MediaSource} within the playlist.
*
Expand Down Expand Up @@ -489,6 +569,18 @@ public final void handleMessage(int messageType, Object message) throws ExoPlayb
removeMediaSourceInternal(removeMessage.index);
scheduleListenerNotification(removeMessage.actionOnCompletion);
break;
case MSG_REMOVE_RANGE:
MessageData<Integer> removeRangeMessage = (MessageData<Integer>) message;
int fromIndex = removeRangeMessage.index;
int toIndex = removeRangeMessage.customData;
for (int index = toIndex - 1; index >= fromIndex; index--) {
shuffleOrder = shuffleOrder.cloneAndRemove(index);
}
for (int index = toIndex - 1; index >= fromIndex; index--) {
removeMediaSourceInternal(index);
}
scheduleListenerNotification(removeRangeMessage.actionOnCompletion);
break;
case MSG_MOVE:
MessageData<Integer> moveMessage = (MessageData<Integer>) message;
shuffleOrder = shuffleOrder.cloneAndRemove(moveMessage.index);
Expand Down