Add more documentation

Especially in and around LongPressAction.kt
Also add some logging / Toast when fetchAllAndShuffle() has to stop early
This commit is contained in:
Stypox 2026-02-09 22:48:45 +01:00
parent d075539a8a
commit 3aa5edc453
No known key found for this signature in database
GPG Key ID: 4BDF1B40A49FDD23
10 changed files with 250 additions and 75 deletions

View File

@ -295,6 +295,8 @@ public abstract class BaseListFragment<I, N> extends BaseStateFragment<I>
@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<I, N> extends BaseStateFragment<I>
/**
* @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<PlayQueue> getPlayQueueStartingAt(@NonNull final StreamInfoItem item) {

View File

@ -491,6 +491,8 @@ public final class BookmarkFragment extends BaseLocalListFragment<List<PlaylistL
LongPressable.fromPlaylistMetadataEntry(selectedItem),
LongPressAction.fromPlaylistMetadataEntry(
selectedItem,
// Note: this does a blocking I/O call to the database. Use coroutines when
// migrating to Kotlin/Compose instead.
localPlaylistManager.getIsPlaylistThumbnailPermanent(selectedItem.getUid()),
// TODO passing this parameter is bad and should be fixed when migrating the
// bookmark fragment to Compose, for more info see method javadoc

View File

@ -584,6 +584,11 @@ public class LocalPlaylistFragment extends BaseLocalListFragment<List<PlaylistSt
disposables.add(disposable);
}
/**
* Changes the playlist's non-permanent thumbnail to the one of the stream entity with the
* provided ID, but only does so if the user did not set a permanent thumbnail themselves.
* @param thumbnailStreamId the ID of the stream entity whose thumbnail will be displayed
*/
private void changeThumbnailStreamIdIfNotPermanent(final long thumbnailStreamId) {
if (playlistManager == null
|| playlistManager.getIsPlaylistThumbnailPermanent(playlistId)) {

View File

@ -119,6 +119,11 @@ class SubscriptionManager(context: Context) {
subscriptionTable.delete(subscriptionEntity)
}
/**
* Checks if the user is subscribed to a channel, in a blocking manner. Since the data being
* loaded from the database is very little, this should be fine. However once the migration to
* Kotlin coroutines will be finished, the blocking computation should be removed.
*/
fun blockingIsSubscribed(serviceId: Int, url: String): Boolean {
return !subscriptionTable.getSubscription(serviceId, url).isEmpty.blockingGet()
}

View File

@ -19,16 +19,20 @@ import io.reactivex.rxjava3.schedulers.Schedulers;
public final class ChannelTabPlayQueue extends AbstractInfoPlayQueue<ChannelTabInfo> {
/**
* 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<StreamInfoItem> 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<ChannelTabI
this(serviceId, linkHandler, null, Collections.emptyList(), 0);
}
// Plays the first
/**
* Plays the streams in the channel tab where {@link ChannelTabHelper#isStreamsTab} returns
* true, choosing the first such tab among the ones returned by {@code ChannelInfo.getTabs()}.
* @param serviceId the service ID of the channel
* @param channelUrl the channel URL
*/
public ChannelTabPlayQueue(final int serviceId,
final String channelUrl) {
super(serviceId, channelUrl, null, Collections.emptyList(), 0);
linkHandler = null;
tabHandler = null;
}
@Override
@ -51,10 +60,11 @@ public final class ChannelTabPlayQueue extends AbstractInfoPlayQueue<ChannelTabI
@Override
public void fetch() {
if (isInitial) {
if (linkHandler == null) {
if (tabHandler == null) {
// we still have not chosen a tab, so we need to fetch the channel
ExtractorHelper.getChannelInfo(this.serviceId, this.baseUrl, false)
.flatMap(channelInfo -> {
linkHandler = channelInfo.getTabs()
tabHandler = channelInfo.getTabs()
.stream()
.filter(ChannelTabHelper::isStreamsTab)
.findFirst()
@ -62,20 +72,22 @@ public final class ChannelTabPlayQueue extends AbstractInfoPlayQueue<ChannelTabI
"No playable channel tab found"));
return ExtractorHelper
.getChannelTab(this.serviceId, this.linkHandler, false);
.getChannelTab(this.serviceId, this.tabHandler, false);
})
.subscribeOn(Schedulers.io())
.observeOn(AndroidSchedulers.mainThread())
.subscribe(getHeadListObserver());
} else {
ExtractorHelper.getChannelTab(this.serviceId, this.linkHandler, false)
// fetch the initial page of the channel tab
ExtractorHelper.getChannelTab(this.serviceId, this.tabHandler, false)
.subscribeOn(Schedulers.io())
.observeOn(AndroidSchedulers.mainThread())
.subscribe(getHeadListObserver());
}
} else {
ExtractorHelper.getMoreChannelTabItems(this.serviceId, this.linkHandler, this.nextPage)
// fetch the successive page of the channel tab
ExtractorHelper.getMoreChannelTabItems(this.serviceId, this.tabHandler, this.nextPage)
.subscribeOn(Schedulers.io())
.observeOn(AndroidSchedulers.mainThread())
.subscribe(getNextPageObserver());

View File

@ -9,6 +9,9 @@ import org.schabi.newpipe.NewPipeDatabase
import org.schabi.newpipe.database.playlist.PlaylistMetadataEntry
import org.schabi.newpipe.local.playlist.LocalPlaylistManager
/**
* A play queue that fetches all of the items in a local playlist.
*/
class LocalPlaylistPlayQueue(info: PlaylistMetadataEntry) : PlayQueue(0, listOf()) {
private val playlistId: Long = info.uid
private var fetchDisposable: Disposable? = null

View File

@ -1,5 +1,9 @@
package org.schabi.newpipe.player.playqueue
import android.os.Handler
import android.os.Looper
import android.util.Log
import android.widget.Toast
import io.reactivex.rxjava3.android.schedulers.AndroidSchedulers
import io.reactivex.rxjava3.core.BackpressureStrategy
import io.reactivex.rxjava3.core.Flowable
@ -8,6 +12,9 @@ import java.io.Serializable
import java.util.Collections
import java.util.concurrent.atomic.AtomicInteger
import kotlinx.coroutines.reactive.awaitFirst
import org.schabi.newpipe.App
import org.schabi.newpipe.BuildConfig
import org.schabi.newpipe.R
import org.schabi.newpipe.player.playqueue.PlayQueueEvent.AppendEvent
import org.schabi.newpipe.player.playqueue.PlayQueueEvent.ErrorEvent
import org.schabi.newpipe.player.playqueue.PlayQueueEvent.InitEvent
@ -16,7 +23,6 @@ import org.schabi.newpipe.player.playqueue.PlayQueueEvent.RecoveryEvent
import org.schabi.newpipe.player.playqueue.PlayQueueEvent.RemoveEvent
import org.schabi.newpipe.player.playqueue.PlayQueueEvent.ReorderEvent
import org.schabi.newpipe.player.playqueue.PlayQueueEvent.SelectEvent
import org.schabi.newpipe.player.playqueue.PlayQueueItem
/**
* PlayQueue is responsible for keeping track of a list of streams and the index of
@ -460,10 +466,29 @@ abstract class PlayQueue internal constructor(
// servers by making too many requests. For reference, making 10 fetch requests
// will mean fetching at most 1000 items on YouTube playlists, though this
// changes among services.
Log.w(
TAG,
"Stopped after $MAX_FETCHES_BEFORE_SHUFFLING calls to fetch() " +
"(for a total of ${streams.size} streams) to avoid rate limits"
)
Handler(Looper.getMainLooper()).post {
Toast.makeText(
App.instance,
App.instance.getString(
R.string.queue_fetching_stopped_early,
MAX_FETCHES_BEFORE_SHUFFLING,
streams.size
),
Toast.LENGTH_SHORT
).show()
}
break
}
fetchCount += 1
if (BuildConfig.DEBUG) {
Log.d(TAG, "fetchAllAndShuffle(): fetching a page, current stream count ${streams.size}")
}
fetch()
// Since `fetch()` does not return a Completable we can listen on, we have to wait
@ -562,4 +587,9 @@ abstract class PlayQueue internal constructor(
private fun broadcast(event: PlayQueueEvent) {
eventBroadcast?.onNext(event)
}
companion object {
val TAG: String = PlayQueue::class.java.simpleName
const val MAX_FETCHES_BEFORE_SHUFFLING = 10
}
}

View File

@ -52,6 +52,8 @@ public class VideoAudioSettingsFragment extends BasePreferenceFragment {
} else if (getString(R.string.show_higher_resolutions_key).equals(key)) {
updateResolutionOptions();
} else if (getString(R.string.show_play_with_kodi_key).equals(key)) {
// Adds or removes the kodi action from the long press menu (but the user may still
// remove/add it again independently of the show_play_with_kodi_key setting).
addOrRemoveKodiLongPressAction(requireContext());
}
};

View File

@ -73,17 +73,41 @@ import org.schabi.newpipe.util.external_communication.ShareUtils
typealias ActionList = MutableList<LongPressAction>
/**
* 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<StreamInfoItem/StreamEntity/...>` 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<StreamInfoItem/StreamEntity/...>` 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<LongPressAction>()
.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<LongPressAction>()
.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<LongPressAction>()
.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<LongPressAction>()
.addPlayerActions { PlaylistPlayQueue(item.serviceId, item.url) }
.addPlayerShuffledActions { PlaylistPlayQueue(item.serviceId, item.url) }
.addShareActions(item)
}
}
}

View File

@ -917,4 +917,5 @@
<string name="popup_shuffled">Popup\nshuffled</string>
<string name="play_shuffled">Play\nshuffled</string>
<string name="items_in_playlist">%d items in playlist</string>
<string name="queue_fetching_stopped_early">Stopped loading after %1$d pages and %2$d items to avoid rate limits</string>
</resources>