diff --git a/app/src/main/java/org/schabi/newpipe/fragments/list/BaseListFragment.java b/app/src/main/java/org/schabi/newpipe/fragments/list/BaseListFragment.java index d1dd835ed..faae6d429 100644 --- a/app/src/main/java/org/schabi/newpipe/fragments/list/BaseListFragment.java +++ b/app/src/main/java/org/schabi/newpipe/fragments/list/BaseListFragment.java @@ -295,6 +295,8 @@ public abstract class BaseListFragment extends BaseStateFragment @Override public void held(final ChannelInfoItem item) { + // Note: this does a blocking I/O call to the database. Use coroutines when + // migrating to Kotlin/Compose instead. final boolean isSubscribed = new SubscriptionManager(requireContext()) .blockingIsSubscribed(item.getServiceId(), item.getUrl()); @@ -338,9 +340,9 @@ public abstract class BaseListFragment extends BaseStateFragment /** * @param item an item in the list, from which the built queue should start - * @return a builder for a queue containing all of the items in this list, with the queue index - * set to the item passed as parameter; return {@code null} if no "start playing from here" - * options should be shown + * @return a builder for a queue containing all of the streams items in this list, with the + * queue index set to the stream item passed as parameter; return {@code null} if no "start + * playing from here" options should be shown */ @Nullable protected Function0 getPlayQueueStartingAt(@NonNull final StreamInfoItem item) { diff --git a/app/src/main/java/org/schabi/newpipe/local/bookmark/BookmarkFragment.java b/app/src/main/java/org/schabi/newpipe/local/bookmark/BookmarkFragment.java index b7a3be6d4..a018ffef2 100644 --- a/app/src/main/java/org/schabi/newpipe/local/bookmark/BookmarkFragment.java +++ b/app/src/main/java/org/schabi/newpipe/local/bookmark/BookmarkFragment.java @@ -491,6 +491,8 @@ public final class BookmarkFragment extends BaseLocalListFragment { + /** + * The channel tab link handler. + * If null, it indicates that we have yet to fetch the channel info and choose a tab from it. + */ @Nullable - ListLinkHandler linkHandler; + ListLinkHandler tabHandler; public ChannelTabPlayQueue(final int serviceId, - final ListLinkHandler linkHandler, + final ListLinkHandler tabHandler, final Page nextPage, final List streams, final int index) { - super(serviceId, linkHandler.getUrl(), nextPage, streams, index); - this.linkHandler = linkHandler; + super(serviceId, tabHandler.getUrl(), nextPage, streams, index); + this.tabHandler = tabHandler; } public ChannelTabPlayQueue(final int serviceId, @@ -36,11 +40,16 @@ public final class ChannelTabPlayQueue extends AbstractInfoPlayQueue { - linkHandler = channelInfo.getTabs() + tabHandler = channelInfo.getTabs() .stream() .filter(ChannelTabHelper::isStreamsTab) .findFirst() @@ -62,20 +72,22 @@ public final class ChannelTabPlayQueue extends AbstractInfoPlayQueue +/** + * An action that the user can perform in the long press menu of an item. What matters are lists of + * [LongPressAction], i.e. [ActionList]s, which represent a set of actions that are *applicable* for + * an item. + * + * If an action is present in an [ActionList] it does not necessarily imply that it will be shown to + * the user in the long press menu, because the user may decide which actions to show with the + * `LongPressMenuEditor`. + * + * Also, an [ActionList] may contain actions that are temporarily unavailable (e.g. enqueueing when + * no player is running; see [enabled]), but **should not** contain actions that are not + * *applicable* for an item (i.e. they wouldn't make sense). That's why you will see some actions + * being marked as not [enabled] and some not being included at all in the [ActionList] builders. + * + * @param type the [Type] of the action, describing how to identify it and represent it visually + * @param enabled a lambda that the UI layer can call at any time to check if the action is + * temporarily unavailable (e.g. the enqueue action is available only if the player is running) + * @param action will be called **at most once** to actually perform the action upon selection by + * the user; will be run on [Dispatchers.Main] (i.e. the UI/main thread) and should perform any + * I/O-heavy computation using `withContext(Dispatchers.IO)`; since this is a `suspend` function, it + * is ok if it takes a while to complete, and a loading spinner will be shown in the meantime + */ data class LongPressAction( val type: Type, val enabled: () -> Boolean = { true }, @MainThread val action: suspend (context: Context) -> Unit ) { + /** + * @param id a unique ID that allows saving and restoring a list of action types from settings. + * **MUST NOT CHANGE ACROSS APP VERSIONS!** + * @param label a string label to show in the action's button + * @param icon an icon to show in the action's button + */ enum class Type( - /** - * A unique ID that allows saving and restoring a list of action types from settings. - * MUST NOT CHANGE ACROSS APP VERSIONS! - */ val id: Int, @StringRes val label: Int, val icon: ImageVector @@ -118,6 +142,9 @@ data class LongPressAction( companion object { + /** + * Builds and adds a [LongPressAction] to the list. + */ private fun ActionList.addAction( type: Type, enabled: () -> Boolean = { true }, @@ -127,6 +154,10 @@ data class LongPressAction( return this } + /** + * Builds and adds a [LongPressAction] to the list, but **only if [condition] is `true`**. + * The difference between [condition] and [enabled] is explained in [LongPressAction]. + */ private fun ActionList.addActionIf( condition: Boolean, type: Type, @@ -139,6 +170,10 @@ data class LongPressAction( return this } + /** + * Add the typical player actions that can be performed on any [queue] of streams: + * enqueueing on an existing player and starting one among the three player types. + */ private fun ActionList.addPlayerActions(queue: suspend (Context) -> PlayQueue): ActionList { // TODO once NewPlayer will be used, make it so that the enabled states of Enqueue // and EnqueueNext are a State<> that changes in real time based on the actual evolving @@ -165,12 +200,18 @@ data class LongPressAction( } /** - * Instead of queueFromHere, this function could possibly take a - * `() -> List` plus the `StreamInfoItem/StreamEntity/...` - * that was long-pressed, and take care of searching through the list to find the item - * index, and finally take care of building the queue. It would deduplicate some code in - * fragments, but it's probably not possible to do because of all the different types of - * the items involved. But this should be reconsidered if the types will be unified. + * Add player actions that can be performed when the item (the one that the actions refer + * to), is also part of a list which can be played starting from said item, i.e. "play list + * starting from here" actions. + * + * *Note: instead of [queueFromHere], this function could possibly take a + * `() -> List` plus the `StreamInfoItem/StreamEntity/...` + * that was long-pressed, and take care of searching through the list to find the item + * index, and finally take care of building the queue. It would deduplicate some code in + * fragments, but it's probably not possible to do because of all the different types of + * the items involved. But this should be reconsidered if the types will be unified.* + * + * @param queueFromHere if `null`, this will not modify the list */ private fun ActionList.addPlayerFromHereActions( queueFromHere: (() -> PlayQueue)? @@ -190,6 +231,10 @@ data class LongPressAction( return this } + /** + * Add player actions that make sense only when [queue] (generally) contains multiple + * streams (e.g. playlists, channels), i.e. "play item's streams shuffled" actions. + */ private fun ActionList.addPlayerShuffledActions( queue: suspend (Context) -> PlayQueue ): ActionList { @@ -212,6 +257,10 @@ data class LongPressAction( return this } + /** + * Add actions that allow sharing an [InfoItem] externally. + * Also see the other overload for a more generic version. + */ private fun ActionList.addShareActions(item: InfoItem): ActionList { addAction(Type.Share) { context -> ShareUtils.shareText(context, item.name, item.url, item.thumbnails) @@ -222,6 +271,10 @@ data class LongPressAction( return this } + /** + * Add actions that allow sharing externally an item with [name], [url] and optionally + * [thumbnailUrl]. Also see the other overload for an [InfoItem]-specific version. + */ private fun ActionList.addShareActions( name: String, url: String, @@ -236,6 +289,10 @@ data class LongPressAction( return this } + /** + * Add actions that can be performed on any stream item, be it a remote stream item or a + * stream item stored in history. + */ private fun ActionList.addAdditionalStreamActions(item: StreamInfoItem): ActionList { addAction(Type.Download) { context -> val info = fetchStreamInfoAndSaveToDatabase(context, item.serviceId, item.url) @@ -271,6 +328,7 @@ data class LongPressAction( } } if (KoreUtils.isServiceSupportedByKore(item.serviceId)) { + // offer the option to play with Kodi only if Kore supports the item service addAction( Type.PlayWithKodi, enabled = { KoreUtils.isServiceSupportedByKore(item.serviceId) } @@ -280,8 +338,13 @@ data class LongPressAction( } /** - * @param queueFromHere returns a play queue for the list that contains [item], with the - * queue index pointing to [item], used to build actions like "Play playlist from here". + * *Note: if and when stream item representations will be unified, this should be removed in + * favor a single unified `fromStreamItem` option.* + * + * @param item the remote stream item for which to create a list of possible actions + * @param queueFromHere returns a play queue containing all of the stream items in the list + * that contains [item], with the queue index pointing to [item]; if `null`, no "start + * playing from here" options will be included */ @JvmStatic fun fromStreamInfoItem( @@ -295,47 +358,32 @@ data class LongPressAction( .addAdditionalStreamActions(item) } + /** + * *Note: if and when stream item representations will be unified, this should be removed in + * favor a single unified `fromStreamItem` option.* + * + * @param item the local stream item for which to create a list of possible actions + * @param queueFromHere returns a play queue containing all of the stream items in the list + * that contains [item], with the queue index pointing to [item]; if `null`, no "start + * playing from here" options will be included + */ @JvmStatic fun fromStreamEntity( item: StreamEntity, queueFromHere: (() -> PlayQueue)? ): ActionList { - // TODO decide if it's fine to just convert to StreamInfoItem here (it poses an - // unnecessary dependency on the extractor, when we want to just look at data; maybe - // using something like LongPressable would work) return fromStreamInfoItem(item.toStreamInfoItem(), queueFromHere) } - @JvmStatic - fun fromPlayQueueItem( - item: PlayQueueItem, - playQueueFromWhichToDelete: PlayQueue, - showDetails: Boolean - ): ActionList { - // TODO decide if it's fine to just convert to StreamInfoItem here (it poses an - // unnecessary dependency on the extractor, when we want to just look at data; maybe - // using something like LongPressable would work) - val streamInfoItem = item.toStreamInfoItem() - return ArrayList() - .addShareActions(streamInfoItem) - .addAdditionalStreamActions(streamInfoItem) - .addActionIf(showDetails, Type.ShowDetails) { context -> - // playQueue is null since we don't want any queue change - NavigationHelper.openVideoDetail( - context, - item.serviceId, - item.url, - item.title, - null, - false - ) - } - .addAction(Type.Remove) { - val index = playQueueFromWhichToDelete.indexOf(item) - playQueueFromWhichToDelete.remove(index) - } - } - + /** + * *Note: if and when stream item representations will be unified, this should be removed in + * favor a single unified `fromStreamItem` option.* + * + * @param item the history stream item for which to create a list of possible actions + * @param queueFromHere returns a play queue containing all of the stream items in the list + * that contains [item], with the queue index pointing to [item]; if `null`, no "start + * playing from here" options will be included + */ @JvmStatic fun fromStreamStatisticsEntry( item: StreamStatisticsEntry, @@ -354,11 +402,23 @@ data class LongPressAction( } /** - * TODO [onDelete] is still passed externally to allow the calling fragment to debounce - * many deletions into a single database transaction, improving performance. This is - * however a bad pattern (which has already led to many bugs in NewPipe). Once we migrate - * the playlist fragment to Compose, we should make the database updates immediately, and - * use `collectAsLazyPagingItems()` to load data in chunks and thus avoid slowdowns. + * *Note: if and when stream item representations will be unified, this should be removed in + * favor a single unified `fromStreamItem` option.* + * + * *Note: [onDelete] is still passed externally to allow the calling fragment to debounce + * many deletions into a single database transaction, improving performance. This is + * however a bad pattern (which has already led to many bugs in NewPipe). Once we migrate + * the playlist fragment to Compose, we should make the database updates immediately, and + * use `collectAsLazyPagingItems()` to load data in chunks and thus avoid slowdowns.* + * + * @param item the playlist stream item for which to create a list of possible actions + * @param queueFromHere returns a play queue containing all of the stream items in the list + * that contains [item], with the queue index pointing to [item]; if `null`, no "start + * playing from here" options will be included + * @param playlistId the playlist this stream belongs to, allows setting this item's + * thumbnail as the playlist thumbnail + * @param onDelete the action to run when the user presses on [Type.Delete], see above for + * why it is here and why it is bad */ @JvmStatic fun fromPlaylistStreamEntry( @@ -384,7 +444,60 @@ data class LongPressAction( } /** - * TODO see [fromPlaylistStreamEntry] for why [onDelete] is here and why it's bad + * *Note: if and when stream item representations will be unified, this should be removed in + * favor a single unified `fromStreamItem` option.* + * + * @param item the play queue stream item for which to create a list of possible actions + * @param playQueueFromWhichToDelete the play queue containing [item], and from which [item] + * should be removed in case the user presses the [Type.Remove] action. + * @param showDetails whether to include the option to show stream details, which only makes + * sense if the user is not already on that stream's details page + */ + @JvmStatic + fun fromPlayQueueItem( + item: PlayQueueItem, + playQueueFromWhichToDelete: PlayQueue, + showDetails: Boolean + ): ActionList { + val streamInfoItem = item.toStreamInfoItem() + return ArrayList() + .addShareActions(streamInfoItem) + .addAdditionalStreamActions(streamInfoItem) + .addActionIf(showDetails, Type.ShowDetails) { context -> + // playQueue is null since we don't want any queue change + NavigationHelper.openVideoDetail( + context, + item.serviceId, + item.url, + item.title, + null, + false + ) + } + .addAction(Type.Remove) { + val index = playQueueFromWhichToDelete.indexOf(item) + playQueueFromWhichToDelete.remove(index) + } + } + + /** + * @param item the remote playlist item (e.g. appearing in searches or channel tabs, not the + * remote playlists in bookmarks) for which to create a list of possible actions + */ + @JvmStatic + fun fromPlaylistInfoItem(item: PlaylistInfoItem): ActionList { + return ArrayList() + .addPlayerActions { PlaylistPlayQueue(item.serviceId, item.url) } + .addPlayerShuffledActions { PlaylistPlayQueue(item.serviceId, item.url) } + .addShareActions(item) + } + + /** + * @param item the local playlist item for which to create a list of possible actions + * @param isThumbnailPermanent if true, the playlist's thumbnail was set by the user, and + * can thus also be unset by the user + * @param onDelete the action to run when the user presses on [Type.Delete], see + * [fromPlaylistStreamEntry] for why it is here and why it is bad */ @JvmStatic fun fromPlaylistMetadataEntry( @@ -443,7 +556,10 @@ data class LongPressAction( } /** - * TODO see [fromPlaylistStreamEntry] for why [onDelete] is here and why it's bad + * @param item the remote bookmarked playlist item for which to create a list of possible + * actions + * @param onDelete the action to run when the user presses on [Type.Delete], see + * [fromPlaylistStreamEntry] for why it is here and why it is bad */ @JvmStatic fun fromPlaylistRemoteEntity( @@ -457,6 +573,11 @@ data class LongPressAction( .addAction(Type.Delete) { onDelete.run() } } + /** + * @param item the remote channel item for which to create a list of possible actions + * @param isSubscribed used to decide whether to show the [Type.Subscribe] or + * [Type.Unsubscribe] button + */ @JvmStatic fun fromChannelInfoItem( item: ChannelInfoItem, @@ -492,13 +613,5 @@ data class LongPressAction( .show() } } - - @JvmStatic - fun fromPlaylistInfoItem(item: PlaylistInfoItem): ActionList { - return ArrayList() - .addPlayerActions { PlaylistPlayQueue(item.serviceId, item.url) } - .addPlayerShuffledActions { PlaylistPlayQueue(item.serviceId, item.url) } - .addShareActions(item) - } } } diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index c166a9c52..e68cd351f 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -917,4 +917,5 @@ Popup\nshuffled Play\nshuffled %d items in playlist + Stopped loading after %1$d pages and %2$d items to avoid rate limits