-
Notifications
You must be signed in to change notification settings - Fork 422
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.
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
Add support for channel tabs #951
Conversation
YouTube is currently A/B testing a new layout on their channel pages, which uses a RichGridRenderer.
- extract YouTube channel tabs: playlists, channels, shorts, live
- fix checkstyle errors
Are YouTube music / topic channels supported? #501 |
1 minute was incorrectly parsed as 1s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Now we just need to wait for AudricV's review and we are done. Thanks!
Having the same URL for different tabs will lead to cache conflicts. That's why I added the URL suffixes, even though they are not present on the actual Bandcamp page. /**
* Check if we can load it from the cache (forceLoad parameter), if we can't,
* load from the network (Single loadFromNetwork)
* and put the results in the cache.
*
* @param <I> the item type's class that extends {@link Info}
* @param forceLoad whether to force loading from the network instead of from the cache
* @param serviceId the service to load from
* @param url the URL to load
* @param infoType the {@link InfoItem.InfoType} of the item
* @param loadFromNetwork the {@link Single} to load the item from the network
* @return a {@link Single} that loads the item
*/
private static <I extends Info> Single<I> checkCache(final boolean forceLoad,
final int serviceId, final String url,
final InfoItem.InfoType infoType,
final Single<I> loadFromNetwork) { |
Mmmh, you are right, but then the two urls should be ".../tracks" and ".../albums", for consistency. Can you do this change and fix merge conflicts? |
I changed the URL suffixes to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, before reading the review, don't be afraid: I don't ask you to change everything by yourself, I can do a part of my requested changes if you want.
As some code changes have been made by @Stypox, they should reply for comments on their changes, so you probably don't need to answer on every comment.
Note that a lot of review comments are asking for code style or formatting changes, so changes are easier that what you can think on a first look.
Thank you very much for your effort on this feature! The general structure and code changes look fine with the current extractor code, but improvements are definitively needed in the future (and that's what #904 does partially).
We need a list of API changes which will be added in the next extractor release, especially because this PR introduces breaking changes. It should be added in the PR description for easier access.
I noticed changes that should be made globally/in multiple files (some changes are not listed here but on review comments):
- better documentation/addition of documentation on methods and classes, at least for the ones exposed in the API;
- proper/better usage of Java's Stream API (I also highlighted this in a few review comments): when working on streams of
JsonArray
s, in most cases you filter/keep only objects ofJsonObject
's class instance then casting and working with theseJsonObject
s instead of doing the same process or parts of it multiple times (e.g. multiple casts). Here is an example:myJsonArray.stream() .filter(JsonObject.class::isInstance) .map(JsonObject.class::cast) // Your operations here
- on several tests, you are removing test
Override
annotations, replacing them byTest
ones. Can you explain why are you doing this?
Finally, it seems that some commits have a message different of what they are doing. Could you fix that and also, if possible, switching to a rebase on our dev
branch instead of using a lot of merge commits and fixing merge conflicts on the fly? It would make the commits of your branch, and so this PR, a lot cleaner in my opinion. Thank you in advance!
@@ -0,0 +1,12 @@ | |||
package org.schabi.newpipe.extractor.linkhandler; | |||
|
|||
public final class ChannelTabs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are using strings for channel tabs names, however this open the door to several NullPointerException
s in your code. You should make sure that they are avoided as much as possible. Using an enum would have been a great idea, like described in the conversation beyond #951 (comment), but as it has been decided to not do so, don't change anything here.
I think a dedicated base (abstract) structure should be created for tabs, and this structure should be implemented in services, because different services may have different tab names for the same type of contents.
This structure should allow getting its name, what it contains (streams, playlists, channels, [...] or multiple type of contents) and maybe the service ID and/or more.
But as you rely on content and sort filters, which are currently strings, this is out of the scope of this PR. These filters should be refactored too (made in #904).
...org/schabi/newpipe/extractor/services/youtube/extractors/YoutubeStreamInfoItemExtractor.java
Outdated
Show resolved
Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/channel/ChannelTabExtractor.java
Outdated
Show resolved
Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/channel/ChannelTabInfo.java
Outdated
Show resolved
Hide resolved
...rg/schabi/newpipe/extractor/services/bandcamp/extractors/BandcampAlbumInfoItemExtractor.java
Outdated
Show resolved
Hide resolved
...ava/org/schabi/newpipe/extractor/services/youtube/extractors/YoutubeChannelTabExtractor.java
Outdated
Show resolved
Hide resolved
...c/test/java/org/schabi/newpipe/extractor/services/bandcamp/BandcampChannelExtractorTest.java
Outdated
Show resolved
Hide resolved
...c/test/java/org/schabi/newpipe/extractor/services/bandcamp/BandcampChannelExtractorTest.java
Outdated
Show resolved
Hide resolved
...c/test/java/org/schabi/newpipe/extractor/services/peertube/PeertubeAccountExtractorTest.java
Show resolved
Hide resolved
.../test/java/org/schabi/newpipe/extractor/services/youtube/YouTubeChannelTabExtractorTest.java
Outdated
Show resolved
Hide resolved
Tests do not seem to run when they are not annotated with |
fix: Bandcamp channel link handler factory
use shared method for channel header extraction
extractor/src/main/java/org/schabi/newpipe/extractor/channel/ChannelInfo.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I don't like about the YouTube implementation is that we fetch the videos tab, but we don't extract the StreamInfoItem
contents from it, just the tabs.
This requires 2 (potentially the same) requests now to get the video tab information, which makes things a lot slower for Piped :/
Could we somehow have something like a default tab extracted if the service needs to extract a tab in order to get the available tabs?
Please do not push any commits to the branch of this pull request, I am working on rebasing this branch on top of the |
Closing in favor of #1082. Thank you very much for your work which you can find in this new, improved and updated PR of this one! |
YouTube is planning to update their channel page layout (as announced here) and divide videos into 3 tabs (regular videos, shorts, livestreams).
I did open pull request #944 to address the updated video tab layout, but that would still leave shorts and livestreams inaccessible. So I decided to implement channel tabs for NewPipe that allow the extraction of additional content besides the main video list.
Channel tabs may contain any
InfoItem
, which makes it possible to not only extract video tabs but also playlists and featured channels. So this also resolves, for the extractor part, TeamNewPipe/NewPipe#2414.I added support for the following services and tabs:
Closes #227