-
-
Notifications
You must be signed in to change notification settings - Fork 490
Bandcamp support #232
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
Bandcamp support #232
Conversation
Unfortunately a little messy
Additionally, get cover art from json instead of html
Is the CI failing my fault? I believe it is not; one of my tests was failing, but I fixed it with 46e1f39. |
@fynngodau you can see whether it's your fault or not by clicking on |
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.
From a quick look it looks ok for me. Except for the license line, the things I commented on can be addresses in seperate PRs.
I did not look at the tests
Also is there anywhere global state? From my cursory glace it does not look like it.
extractor/src/main/java/org/schabi/newpipe/extractor/services/bandcamp/BandcampService.java
Show resolved
Hide resolved
.../java/org/schabi/newpipe/extractor/services/bandcamp/extractors/BandcampExtractorHelper.java
Outdated
Show resolved
Hide resolved
.../java/org/schabi/newpipe/extractor/services/bandcamp/extractors/BandcampExtractorHelper.java
Outdated
Show resolved
Hide resolved
public void onFetchPage(@Nonnull Downloader downloader) throws IOException, ExtractionException { | ||
|
||
} |
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.
why is this empty
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.
Can the reason for why its empty be added here
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.
it is not empty anymore. i moved some of the code which was falsely placed in getInitialPage()
...ava/org/schabi/newpipe/extractor/services/bandcamp/extractors/BandcampFeaturedExtractor.java
Outdated
Show resolved
Hide resolved
.../org/schabi/newpipe/extractor/services/bandcamp/extractors/BandcampRadioStreamExtractor.java
Show resolved
Hide resolved
...a/org/schabi/newpipe/extractor/services/bandcamp/extractors/BandcampSuggestionExtractor.java
Outdated
Show resolved
Hide resolved
...a/org/schabi/newpipe/extractor/services/bandcamp/extractors/BandcampSuggestionExtractor.java
Outdated
Show resolved
Hide resolved
...a/org/schabi/newpipe/extractor/services/bandcamp/extractors/BandcampSuggestionExtractor.java
Outdated
Show resolved
Hide resolved
...e/extractor/services/bandcamp/extractors/streaminfoitem/BandcampStreamInfoItemExtractor.java
Outdated
Show resolved
Hide resolved
Made BandCampRadioExtractor a Kiosk which holds StreamInfoItems and not InfoItems.
use interface
…Utils.nonEmptyAndNullJoin(String, String[])
When using the index here, it the index needs to be decremented once an element is removed. To cirecumvent this, the native Collections.removeIf() method is used.
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.
Reviewing something this big again is really time consuming. If the points from my previous review were addressed, then its good to go.
The small stuff can be handled in seperate PR, which are then way easiert to review. Like adding the reason for empty methods and replace the remaining occurences of bandcamp.com
On a side note: Since all of the most recent commits were made by you @TobiGr, are you going to continue maintaining this service or is @fynngodau going to help too?
public void onFetchPage(@Nonnull Downloader downloader) throws IOException, ExtractionException { | ||
|
||
} |
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.
Can the reason for why its empty be added here
Hi, I'm still interested in maintaining this, however after having maintained this PR without it actually being used for a year or so, I'm glad that I don't have to do these final steps myself. Then I can open some other, smaller PRs with fixes as needed or follow-up features. In case you're wondering why I was not in the IRC channel, I was probably kicked for inactivity, or I forgot to join with my new matrix account. |
Featured
andRadio
and for Bandcamp itself. Bandcamp support NewPipe#3741Fixes #138
Fixes TeamNewPipe/NewPipe#1284
I had to add theorg.json
library because the website contains some JSON that has unquoted keys.Descriptions are suboptimal because their newlines get eaten and then lyrics are really difficult to readNot anymore for some reason.Actually implemented, but depends on Playlist in channels #227
Playlists with a lot of streams take ages to load, because for every playlist info item, a separate request needs to be made to find out the URL of its cover. I'll see whether I can add a workaround just for playlists with more than a number (e.g. 10) of streamsWorkaround ba967f1 only loads cover art of each track individually for playlists <= 10 streamsFeatured
kioskCurrently only loads nine items currently can't load further "pages"; can contain preorder albums whose cover is not always displayed correctly and which shows errors but works once opened
Radio
) kioskClicking uploader's (primary host's) name causes the crash reporter to open as it is a link to a fan page
Are extra work because they look different and don't really provide a lot of value I think
(
fanthanks
/writings
)But I'm not sure of their quality