-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Show overall duration of videos in playlist #6045
Conversation
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 won't work like this. The remote playlist are paginated, meaning you only ever load a fraction of longer playlist. The playlist also can't be preloaded, because it could potentially be an infinite amount of videos (YouTube Mix)
It also looks like as if the change will break the existing counter for long playlist, for the same reason.
This would need to be done in the extractor, in order to be done properly. And it would only work if the services provide that specific information for us to parse |
Hi @XiangRongLin, thank you very much for reviewing. Now reworked to show overall duration only for local lists - they are not so tricky I hope. Can you give an example-link of an infinite list by the way? |
https://www.youtube.com/watch?v=FfAjtZgLkPQ&list=RDFfAjtZgLkPQ Its basically youtube.com/watch?v={videoID}&list=RD{videoID} |
so now improved enough, tried with that infinite list. Next thing would be to extend the service to deliver the duration, but this service is not in our hands, right?:) |
The service is in our hands https://github.com/TeamNewPipe/NewPipeExtractor And like i said before, it needs to be fixed there. From a quick glance the duration would increase as more items are loaded. |
I am a user, and i am already satisfied with this - i did this implementation for me in the first line;) |
I think this could be made as an option disabled by default, with a note that for larger playlists, it might not display the actual duration immediately. |
now made it configurable |
Hi @XiangRongLin , if I do this via the extractor I have to return the overall duration in PlaylistInfo which is called per current design only once - when opening the view for a playlist. Because the online-service (e.g. youtube) does not provide playlist-duration it is needed to loop through all items, sum up the duration and provide it via something like PlaylistInfo.getStreamDuration() of extractor - for multipage playlists (>100 videos) it is needed to load all pages which takes too long (about 12s to show a playlist with 1615 videos I had tried - all this time the list itself appears to be loading and no items of it is shown to the user) which makes user experience too bad. So either we implement outside of extractor showing only partial duration in case of long playlist but having good performance, or we implement inside of extractor but kill the performance and show the exact duration which nobody needs in long playlists:) What do you think, am I missing something? |
That is unfortunate that youtube does not provide that information. Since the services (youtube) themself do not provide that information, it would then also not belong inside the extractor.
I don't personally see much use in this solution, but I also don't like to decide about which features to add and which not. These discussions are best made in an issue and not the PR itself. |
"without opening the playlist" is a good point, let me try. Concerning units: I made it already without units, it looks like this now: 01:35:17 in case complete or in case incomplete 12:08:09+, About "first 50 or 100 videos": it works already like this out-of-the-box just because this amount of videos the extrator delivers. |
hi @SameenAhnaf , Showing duration for a channel (when one is open on the screen) I would consider as a new PullRequest, and seems to be possible as I understand the corresponding youtube page for that. But on my phone-screen there seems to be no space where to add duration - even the number of videos is not shown, see And some good news: on your screenshot with infinite number of videos no duration is shown I bet because it was not enabled in Settings as requested by @mhmdanas:), I now updated the link to the latest APK, sorry for confusion, can you please check again. Cool that you've found my feature useful, looking forward to hear from you. |
Any chances of merging this feature ? |
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 for this nice addition. Code looks almost good, and is also really simple. I think this PR should be merged, but we should not proceed further imo (e.g. by implementing it in the extractor), because we would soon get into niche territory.
By the way, sorry, I completely missed this PR for some reason
app/src/main/java/org/schabi/newpipe/fragments/list/playlist/PlaylistFragment.java
Outdated
Show resolved
Hide resolved
headerBinding.playlistStreamCount.setText(Localization | ||
.localizeStreamCount(activity, count)); | ||
final long videoCount = itemsList.size(); | ||
final long playlistOverallDurationSeconds = itemsList.stream().mapToLong(x -> |
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 would first filter by PlaylistStreamEntry.class::isInstance
, then map with PlaylistStreamEntry.class::cast
, then map to PlaylistStreamEntry::getStreamEntity
, finally mapToLong StreamEntryOrWhatever::getDuration
and sum.
Hi @Stypox, @Akv2021, thank you for your comments and attention to this pull-request! Screenrecorder-2022-07-19-19-29-17-300.mp4Screenrecorder-2022-07-19-19-30-53-869.mp4 |
I think this PR would be useful anyway, even if a similar feature exists in another place |
6d02491
to
2b684ad
Compare
Kudos, SonarCloud Quality Gate passed! |
earlier only overall amount of videos was shown. Now overall duration is shown there too - as formatted by existing Localization.concatenateStrings() and Localization.getDurationString(). show all videos OverallDuration in local Playlist too refactor to make implementation in LocalPlaylistFragment and PlaylistFragment more obviously similar unfortunately could not refactor upto BaseLocalListFragment revert the changes for online Playlists because they are paginated and may be infinite i.e. correct count may come only from the service->extractor chain which unfortunately does not give overall duration yet next try to improve user-experience with online Playlist just show that duration is longer (">") than the calculated value in case there is more page(s) even more improve user-experience for online Playlist by adding the duration of next items as soon as they are made visible make showing of playlists duration configurable, disabled by default adjusted duration to be handled as long because it comes as long from extractor no idea why I handled it as int earlier Revert "make showing of playlists duration configurable, disabled by default", refactor This reverts commit bc1ba17. Fix rebase Apply review Rename video -> stream Remove unused settings keys
72f34ee
to
bef5907
Compare
|
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 to everyone involved! I rebased and squashed, and removed some leftover strings that were used in the settings. I also tested both local and remote playlists, adding and removing items, and everything worked.
It feels like the current UI is a bit non-intuitive. Maybe there should be a prefix before it, like "Total duration: 45:03" or something. |
A nice suggestion! I don't know how to update this pull-request here probably because it is marked as closed, but I have an idea how to fulfill this request - see dev...bg172:NewPipe:showOverallDurationInPlaylist . Maybe somebody can take it over in a meaningful way? ![]() ![]() |
@Stypox Does this need a new PR? |
Yes, this should go in a new PR, but I guess it will be a quite simple change, so it can be merged in 0.27.0 |
@bg172 ^ ^_^ |
|
earlier only overall amount of videos was shown in playlist (in both searched online playlist and local playlist). Now overall duration is shown there too - as formatted by existing Localization.concatenateStrings() and Localization.getDurationString().
What is it?
Description of the changes in your PR
earlier only overall amount of videos was shown. Now overall duration is shown there too - as formatted by existing Localization.concatenateStrings() and Localization.getDurationString().
APK testing
see https://github.com/TeamNewPipe/NewPipe/pull/6045/checks (artefacts) for the uptodate APK as soon as built by github - tested OK on Xiaomi Mi 9 SE
Due diligence