-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Load full stream info when enqueuing a stream #7036
Conversation
I think how you're doing it now will always cause a full network fetch. The best way would probably be to check if (for example) the Here's the before and after of a similar case of adding the network fetch I recently implemented: Before:
After:
(In the after, NewPipe/app/src/main/java/org/schabi/newpipe/util/StreamDialogEntry.java Lines 209 to 216 in faa7a91
|
Hey, brother, thank you for solving the problem that my device YouTube can't get |
Yes, good idea. The stream info would anyway be fetched when the stream is loaded and played, but not fetching it beforehand is a good idea nonetheless. |
|
Changes made in #6872 will conflict with this PR so I will wait for it to be merged before continuing here. |
There are "three" "stages" of stream loading:
Stream info items are those loaded alongside other stream info items (i.e. when fetching a list, e.g. search results, trending, feed, ...), while stream info is loaded e.g. when a video is opened or played. If when calling |
GitHub added some keybinding to fast close PRs, and I seem to accidentally keep to press it
That's not a problem, |
This commit calls getStreamInfo causing a full network fetch of stream info (I believe only if required) when adding a stream item to the queue. This should prevent UI issues of missing metadata when queueing videos that have been fast-loaded and are missing metadata. Fixes TeamNewPipe#7035
54d8f2e
to
62d3612
Compare
When new info is fetched the feed item still doesn't update, I'm not sure where to trigger this update. |
Nvm that was the wrong thing... |
Working now :) |
lmk if this needs a refactor because I'm not sure if it the best way of doing. |
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.
I can confirm it works when enqueueing a stream whose duration is unknown, thank you!
I get this crash when I long press an item in the fast feed, press on "Start playing in the background" and just after that I long press on another item. (both items are without duration before long pressing).
Exception
- User Action: ui error
- Request: ACRA report
- Content Language: en
- Service: none
- Version: 0.21.13
- OS: Linux Android 10 - 29
Crash log
java.lang.NullPointerException: Attempt to invoke virtual method 'int org.schabi.newpipe.player.playqueue.PlayQueue.size()' on a null object reference
at org.schabi.newpipe.player.helper.PlayerHolder.getQueueSize(PlayerHolder.java:74)
at org.schabi.newpipe.local.feed.FeedFragment.showStreamDialog(FeedFragment.kt:336)
at org.schabi.newpipe.local.feed.FeedFragment.access$showStreamDialog(FeedFragment.kt:80)
at org.schabi.newpipe.local.feed.FeedFragment$listenerStreamItem$1.onItemLongClick(FeedFragment.kt:395)
at com.xwray.groupie.GroupieViewHolder$2.onLongClick(GroupieViewHolder.java:32)
at android.view.View.performLongClickInternal(View.java:7339)
at android.view.View.performLongClick(View.java:7297)
at android.view.View.performLongClick(View.java:7315)
at android.view.View$CheckForLongPress.run(View.java:27850)
at android.os.Handler.handleCallback(Handler.java:883)
at android.os.Handler.dispatchMessage(Handler.java:100)
at android.os.Looper.loop(Looper.java:214)
at android.app.ActivityThread.main(ActivityThread.java:7356)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:491)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:940)
app/src/main/java/org/schabi/newpipe/local/feed/FeedFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/util/StreamDialogEntry.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/util/StreamDialogEntry.java
Outdated
Show resolved
Hide resolved
enqueue(R.string.enqueue_stream, (fragment, item) -> { | ||
NavigationHelper.enqueueOnPlayer(fragment.getContext(), new SinglePlayQueue(item)); | ||
fetchItemInfoIfSparse(fragment, item, | ||
fullItem -> NavigationHelper.enqueueOnPlayer(fragment.getContext(), fullItem) | ||
); | ||
}), | ||
|
||
enqueue_next(R.string.enqueue_next_stream, (fragment, item) -> { | ||
NavigationHelper.enqueueNextOnPlayer(fragment.getContext(), new SinglePlayQueue(item)); | ||
fetchItemInfoIfSparse(fragment, item, | ||
fullItem -> NavigationHelper.enqueueNextOnPlayer(fragment.getContext(), fullItem) | ||
); | ||
}), |
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 did you only do it for enqueue? I also get the problem of the missing duration field in the play queue when I press on "Start playing 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.
Ah, for some reason I neglected to check those options :)
Co-authored-by: Stypox <stypox@pm.me>
Co-authored-by: Stypox <stypox@pm.me>
In order to fix the exception I posted above I think you just need to alter the if (!isPlayerOpen()) return 0;
final PlayQueue playQueue = player.getPlayQueue();
return playQueue == null ? 0 : playQueue.size(); |
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.
Code now looks good, the formatting just needs to be fixed
app/src/main/java/org/schabi/newpipe/util/StreamDialogEntry.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/util/StreamDialogEntry.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/util/StreamDialogEntry.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Stypox <stypox@pm.me>
Co-authored-by: Stypox <stypox@pm.me>
cbe986e
to
3ff00ff
Compare
app/src/main/java/org/schabi/newpipe/util/StreamDialogEntry.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Stypox <stypox@pm.me> Replace the unecessary callback interface InfoCallback in favour of the standard type Consumer<SinglePlayQueue>
Kudos, SonarCloud Quality Gate passed! |
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.
Works perfectly, thank you!
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.
While rebasing #7570 I ran into a few smaller things which I would like to get an answer / opinion on.
// helper functions // | ||
///////////////////////////////////////////// | ||
|
||
private static void fetchItemInfoIfSparse(final Fragment fragment, |
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 the fragment used as param here? The only usage for fragment is fragment.getContext()
in this method.
.doOnError(throwable -> Log.e("StreamDialogEntry", | ||
throwable.toString())) | ||
.subscribe(); | ||
|
||
callback.accept(new SinglePlayQueue(result)); | ||
}, throwable -> Log.e("StreamDialogEntry", throwable.toString())); |
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.
I think we should report these error via the UI. Otherwise the user gets no feedback on his action if something fails. However, I am not sure how to handle network related errors here. @Stypox Any idea?
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.
Maybe we toast the user and fallback to returning the sparse info item then, although that still has its issues.
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 could use an error notification, just like https://github.com/TeamNewPipe/NewPipe/pull/7538/files#diff-212f79b4111281cdfbffed5f79bca8922f99be14642ef90d524e6c9de9a436f7R66
Please feel free to criticize the code here, I am not sure if I am using workers correctly or if there is a better way to do this. I essentially copied the code from
NewPipe/app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java
Lines 878 to 906 in faa7a91
I decided loading the information when adding to the queue is better than loading when the video starts playing as it allows the queue progress text to be correct as well.
What is it?
Description of the changes in your PR
This PR calls getStreamInfo causing a full network fetch of stream info (I believe only if required) when adding a stream item to the queue. This should prevent UI issues of missing metadata when queuing videos that have been fast-loaded and are missing metadata.
Fixes the following issue(s)
APK testing
The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.
Due diligence