Design and implementation of lists in the rewrite

[Stypox]

This issue aims to collect information and decide how to implement lists in the refactor. A few PRs already touch on this topic (playlist: #11259, history: #11947, errors: #12404, long press menu: #12032, uploader icon for replied-to comments: #11991), while some have already been merged (related items: #11383, comments: #11060 #11978, empty state fragments: #11725 #12217 #12604). However, I think we should stop a bit to think about our requirements and the current implementation, to make sure that what is being designed anew properly implements everything we need without design flaws.

_{I'm sorry if this came off as a big wall of text. Hopefully it will give some guidance to whoever wants to contribute to this issue!}

Desired features

Common functionality

In the app, all lists share a lot of common functionality, although specific features might not exist in all lists, and might behave differently here and there:

• They contain items of mixed types in different places: e.g. related items contain both streams and playlists, and e.g. streams appear in playlists, related videos, feed, etc.
• The types of items used in NewPipe at the moment are:
  • Streams (i.e. videos)
  • Playlists
  • Channels
  • Channel groups
  • Comments
• The UI of the list adapts to the screen size: it's a list or rows on phones, a grid of rectangles on tablets, and users may even enable "card view" with full-blown thumbnails
• The items may sometimes appear in different forms: e.g. channels in the subscription list appear in a smaller view than channels in search results.
• Items can be clicked to do a certain action or long-pressed to open a menu. (Redesigned long press menu #12032)
• Actions can be performed on the whole list, e.g. playlists have a background button, and we should add a way to add all videos in a list to a playlist.
• In case a loading error happens, an error panel is shown (if nothing was loaded yet) or an error notification is shown (if some items had already been loaded). (Implement Compose-based Error Panel, Error UI Model, and Tests for Comments #12404)
• In case there is nothing to show, an empty view is shown instead. (Migrate empty_state_view xml/view to Jetpack Compose #11725 Refactor EmptyStateComposable to remove modifier from EmptyStateSpec #12217 Clean up EmptyStateComposable code #12604)
• The items may come from different places:
  • from the database (e.g. history or local playlists), aka "local" items
  • from the internet (e.g. search or trending), aka "remote" items
  • from another screen in the app (this is a special case for related items, which are loaded together with video details)
• Loading items from db or from the internet involves loading one page at once, and lists are able to detect when the user scrolls to the bottom and start loading new data only then, and also show proper loading indications.
• Showing progress of items is currently only implemented in the feed and does not even update in real time, but should ideally be shown everywhere.
• Some lists allow reordering items (e.g. streams in playlists, and playlists in bookmarks)
• Some lists can be reloaded (only the feed fragment at the moment)
• In some lists you can multiple-select items (e.g. the channels in the feed group selection dialog)
• The data that lists load is cached, and the state of the list is persisted if the app is resumed from disk (note that this does not currently happen for rewritten comments: CommentRepliesSource: App Crash after being offline #11717 CommentRepliesSource: Add cache for reply fetching #11718 Rewrite: New Comment fragment makes the app crash for comment errors #11728).
• In local lists it is possible to delete items.
• Some lists have additional general data about the list that should be shown somewhere (e.g. the playlist description, the feed load status)
• The UI of items in the comment list depends on data loaded elsewhere to show the uploader icon next to items where the uploader replied: Show stream uploader icon on comments they have replied to #11991

I would argue that most, if not all, of the above desired features can (and should) be implemented in a general way, so they can be used in all lists.

_{Note: I am not considering the list of items in the queue here, nor am I considering the download list, since those have many different requirements and not much functionality in common.}

Item types

As highlighted above, we have 5 types of fundamental item types at the moment, but we may also distinguish between remote items and different sub-types of local items:

• Stream:
  • StreamInfoItem (remote) comes from the extractor
  • StreamWithState (local) used in the feed list
  • StreamStatisticsEntry (local) used in the history
  • PlaylistStreamEntry (local) used for streams in local playlists
• Playlist:
  • PlaylistInfoItem (remote) comes from the extractor
  • PlaylistMetadataEntry (local) for fully local playlists
  • PlaylistRemoteEntity (local) for local bookmarks of remote playlists
• Channel:
  • ChannelInfoItem (remote) comes from the extractor
  • SubscriptionEntity (local) used in the subscription item (although in current NewPipe it's converted to ChannelInfoItem)
• Channel group: FeedGroupEntity (local)
• Comment: CommentInfoItem (remote) comes from the extractor

Pre-refactor implementation details

In an attempt to deduplicate code, the pre-refactor NewPipe codebase features a few base classes that are extended by all lists. However, there are actually two sets of such base classes: those for local items (see here), and those for remote items (see here). Unfortunately this means still having duplicate code, but just between the two base classes.

The base classes take care of:

• loading items from database/internet
• handling the loading of more items when the user scrolls to the bottom (paging)
• building UI items based on their type, and showing them in the list lazily (RecyclerView)
• errors
• empty states
• showing a header with actions to perform on the whole list
• changing item layout (list/grid/card) when settings change
• (partially) handling long-presses
• ...

Generally this system has worked well, but there are major problems because:

• the UI layer is mixed with business logic
• although conceptually local and remote items should behave the same (except for some additional actions or some additional information), they are treated very differently, for example they have ItemHolders. E.g. implementing a common long press menu for all items required needing to go through all possible local and remote item sub-types (see Item types above) and extracting data from them (see Handling item types below).

For the channel groups in the subscriptions tab, the Groupie library is used instead of the two base classes, which adds confusion. This was added back in #2309, with the aim to migrate all lists to it, but this never happened.

Suggestions for the rewrite

I believe we should split the implementation in a few fully independent modules. In order to implement a new list screen, one should be able to combine the features provided by each module. I don't think it's a good idea to rely on base classes and class extensions for all list features at once, since given the huge amount of common behaviors that lists have, it would require having quite a lot of information passing between base- and sub- classes, which makes it much harder to read code. So we should avoid e.g. abstract methods that subclasses implement to say whether some feature is enabled or not or how it should behave, and also e.g. subclasses having to call into the base class to enable/disable/tweak some feature.

Handling item types

Continuing from the Item types section above. I don't think there's anything bad with having so many different sub-types of items: in most cases they represent slightly different concepts and they come from different sources with different data (e.g. different DB tables). However, this complexity should be abstracted away so that the UI layer can handle the 5 main kinds of items without needing to reimplement stuff for each different sub-type.

Ideally we would write a base interface (say UiItem) and 5 interfaces deriving from it (StreamUiItem, PlaylistUiItem, ChannelUiItem, ChannelGroupUiItem, CommentUiItem). Those interfaces would have a few common methods that all sub-types of items need to implement. The interface would allow accessing in a streamlined way:

• data about the item (e.g. name, duration, date, thumbnail, ...)
• actions that can be performed on the item (e.g. play, add to playlist, delete, ...)

However, since remote items come from the extractor, we can't make them implement an interface directly _{(I miss you, Rust traits!)}. So what I did in #12032 was to instead copy all of the data in separate data classes just used for the UI (LongPressable and LongPressAction), and write functions to convert any type/subtype into the data class when needed. This approach, however, involves having another layer of abstraction on top of everything, and also requires copying data (which is cheap if done properly, but may be hard to do properly if you e.g. need to copy around actions that can be performed on an item, which involves copying around lambdas that may accidentally refer to some local object, creating a useless memory leak for each item in a list).

Therefore I think it's best to stick with the *UiItem interfaces, and wrap the extractor classes in a class that implements the interface. This might also make it possible to add more data to the wrapped extractor classes, in case it may turn out to be useful e.g. for #11991. We need to remember to mark all implementations of the interfaces as @Stable to allow for some Compose optimizations.

The Compose UI

Fortunately writing composable UIs in Compose is... well... easy ;-). We should strive to write nice-looking UIs that adapt to many screen sizes.

I'd suggest writing a list @Composable that produces a list of items and takes a bunch of inputs:

• the loading state
• a list of UiItems to render
• any errors that should be shown
• an EmptyStateSpec to render if the list is empty
• some nullable lambda that can be used for reloading
• some paging controller that the list can use to request more items
• the item layout to use (list/grid/card/small_list/small_grid) -> since not all item types may have an implementation for all item layouts, the list composable may choose another one
• a nullable @Composable lambda to render a header item that scrolls with the list
• some null controllers to allow reordering, multi-selecting, and deleting items

I am likely missing some input parameters, however we should strive to keep as much logic as possible out of this composable. It should just render items one by one, choosing their layout based on the super-interface of each UiItem (i.e. StreamUiItem and so on) and based on the requested item layout (list/grid/card/small_list/small_grid), connect (long-)click listeners to the actions available in UiItems, and connect the controllers with the relevant Compose list features.

Loading data

Each list screen in the app should have a corresponding Repository that allows loading the UiItems for that list, and also provide paging, caching and state restore after app resumption.

From the extractor

Loading data from the extractor often means making a request to an Info (e.g. SearchInfo) and then using getRelatedItems() to obtain the first page of InfoItems. From then on successive pages can be obtained from the previous page. In many parts of the app we don't just need the InfoItems but also need the contents of Info (e.g. to show the playlist description in case of PlaylistInfo). Repository implementations should take care of this and e.g. still provide the (partial) Info even when getRelatedItems() returns an error.

Since the above mechanism is common among all (?) extractor requests, it should be abstracted so that every repository doesn't need to reimplement paging, caching and so on.

_{Note: the basic setup for comments is here (already merged), but it's not general at all, it does not have caching and proper error handling, and has a bunch of other issues #11717 #11718 #11728}

Loading stream progress state

Stream progress (i.e. the red bar under videos) should likely be handled by an @Injected Hilt @Singleton, that all StreamUiItems currently shown on screen can subscribe to to get progress updates about themselves through StateFlows. The player will take care of send updates to this @Singleton, which will forward the progress to the right StateFlow and also persist the information to the database.

Dummy data

We should have some dummy items of the various types to render in previews and (possibly) in automated app screenshots.

Long press menu

Since each UiItem implementor has a method that provides the available actions that can be performed on that item, the long press menu can be made a completely separate component that just takes a list of action and shows them. This is already what I did in #12032. Note that this long press menu likely needs to have access to a coroutine context that survives the UI being destroyed, so e.g. if you press on "Start playing in the background" and then close the app immediately, the StreamInfo will finish loading as long as the app survives and then it will be played in the player. This is especially needed for the RouterActivity, which may be replaced entirely with the long press menu, as long as a proper coroutine context is in place. Or do we need WorkManager? I am not so sure about this. @Isira-Seneviratne do you have any suggestion?