-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[Bug Fixed] Playlists With No Uploader No Longer Crash The App When Added To Users Library #2724
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
Conversation
|
Previously playlists with a null uploader (such as album playlists generated by YouTube music) crashed the app (error screen, returns to playlist list). Now the uploader name is left blank and playlists can be used without an uploader. To replicate problem, try adding this playlist (an album generated by YT-Music) to your library. |
|
A future commit may be made which fills the uploader with the uploader of the first video/song in the playlist. |
Stypox
left a comment
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.
Thank you! I only pointed out a style issue.
app/src/main/java/org/schabi/newpipe/database/playlist/model/PlaylistRemoteEntity.java
Outdated
Show resolved
Hide resolved
|
I think that fixes it |
... and some comments to the code
…tion" This reverts commit cb5c219.
|
Just added some doc-strings before final merge. (Wrong branch for that other commit, so I reverted that, it will be a different pr) |
…notification"" This reverts commit 646e327.
Stypox
left a comment
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.
Thank you! :-D
This should be an easy merge, only one line was changed. @TobiGr
|
what happens, if the channel name gets changed? https://support.google.com/youtube/answer/2657964 is the channelName equal to uploader? also the import of TextUtils can be removed |
Thanks for the catch I hadn't considered that. Ill use the == fix instead. Ill push the fix later today |
|
Fix pushed |
|
== will most likely never be true (good for null checks, but bad for String comparison) What about readding the import of TextUtils and use This is a null safe comparison of those Strings and should fix it. Sorry, I just did see the unused TextUtils import earlier and didn't think about using it as the solution. |
|
ill check out some solutions and push a fix later today |
|
Ok, that should be better. Sorry for the larger change, but it now lists "Auto-Generated" as the uploader when null, and has debug loging! I tried to follow style from 'PlaylistFragment.java'. |
|
did you test, whether simply replacing |
|
Yes when loading a playlist with no uploader a little error dialogue shows at the bottom of the screen. It's a bit annoying, but it doesn't crash the app like it did before. A fix to the extractor should make the remaining error go away. |
Stypox
left a comment
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.
This is good to go, I also tested with 4 other playlists/albums and they worked (taken from here, on the right).
The ui changes look good, and I agree that a notice is to be shown in the playlist page (as it is now).
Thank you for the contribution :-D
Stypox
left a comment
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.
Sorry for the double review, I forgot to point out this ;-)
Co-Authored-By: Stypox <[email protected]>
|
allright everything looks good. I removed some unnecessary changes. (I had planed do make a more compact layout without the uploader if there was none found, but it didnt work out) |
app/src/main/java/org/schabi/newpipe/local/holder/RemotePlaylistItemHolder.java
Outdated
Show resolved
Hide resolved
…stItemHolder.java Co-Authored-By: Redirion <[email protected]>
|
Cool so does this mean it will be in the next release target? |
|
@PeterHindes yeah :-) |
|
@Stypox and @Redirion thanks for reviewing and merging and thanks to @PeterHindes for creating the fix. |
|
Ok, thanks for the tip, I didn't know about the drafted release notes 😉 |
|
The pr for the extractor fix should also be reviewed for the next release. Thanks @Redirion for the second half of the fix. |
|
@PeterHindes I merged the extractor PR and updated to the latest extractor commit (see b883ad1 and TeamNewPipe/NewPipeExtractor@8cb3250) |
Uh oh!
There was an error while loading. Please reload this page.