Skip to content
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

Sort "What's New" page chronologically (latest-oldest) #2547

Closed
wants to merge 21 commits into from

Conversation

timfbfbfbfb
Copy link

@timfbfbfbfb timfbfbfbfb commented Aug 20, 2019

Features:

  1. Sort What's New page chronologically, in latest-oldest order
  2. Fetch recent videos from subscribed channels in multiple background threads parallel, this speeds up the download process quite a lot
  3. Incrementally insert video items to What's New list according to the publish time, so users could see the first meaning paint earlier

Limitations:

  1. Publish time is retrieved from Youtube RSS feeds. This would cost users extra mobile data, though the RSS feed XML files are quite small compared to the Youtube channel video HTML pages (each RSS feed XML file is approximately 1/5 to 1/4 file size of the HTML page)
  2. Since Youtube RSS feed provides at most the latest 15 videos from the channel, the What's New page only shows the latest 15 videos from each subscribed channel

Remarks:

I missed the PR #2309 at first and had just noticed it implemented the similar chronological sort feature in the middle of the development, but I still decided to create this PR for two reasons.

  1. I found that this PR is quite different from Feed order and subscriptions groups #2309, for instance feature 3 has not been implemented in Feed order and subscriptions groups #2309. I have downloaded the apk of Feed order and subscriptions groups #2309 and tried it out, the channel grouping feature is really cool, I think this PR and Feed order and subscriptions groups #2309 could complement each other well.
  2. Many NewPipe users want the chronological sort feature so badly. This PR is relatively small, maybe it would be easier for other contributors to review and merge

APK for reviewers to test:

Self-signed release APK
app-afa9fa55.apk.zip

Since this APK is self-signed, Play Protect may show a warning with that, just select "Install anyway" to install

Related issues

Close #739
Close #822

Related PRs

Merge together with TeamNewPipe/NewPipeExtractor#189

@snappyapple632
Copy link

snappyapple632 commented Aug 23, 2019

I'm seeing duplication of certain videos.
Capture _2019-08-23-06-13-07
Capture _2019-08-23-06-13-19
Capture _2019-08-23-06-13-33

@timfbfbfbfb
Copy link
Author

@snappyapple632 Thanks for your feedback!
I think I just accidentally call startLoading() twice when the app is first started. I did not even notice the duplicated videos because I tested the app with too many subscribed channels.
Anyway, I have fixed that and tested on my side, and I updated the APK in the description. Could you please help to verify that?

@snappyapple632
Copy link

snappyapple632 commented Aug 23, 2019

The issue seems to be gone on the latest test!

@chilliger
Copy link

chilliger commented Aug 23, 2019

Tested the new apk. No duplication for me. But every time loading the whats new feed I get the following exception. This is also happening when I switch back to Newpipe from an other app

Exception

Crash log

org.schabi.newpipe.extractor.exceptions.ParsingException: Could not get channel name
	at org.schabi.newpipe.extractor.services.youtube.extractors.YoutubeChannelExtractor.getName(YoutubeChannelExtractor.java:126)
	at org.schabi.newpipe.extractor.channel.ChannelInfo.getInfo(ChannelInfo.java:62)
	at org.schabi.newpipe.extractor.channel.ChannelInfo.getInfo(ChannelInfo.java:49)
	at org.schabi.newpipe.util.ExtractorHelper.lambda$getChannelInfo$4(ExtractorHelper.java:123)
	at org.schabi.newpipe.util.-$$Lambda$ExtractorHelper$u5W7VszTe8AoEexIsFM9huQfbkM.call(Unknown Source:4)
	at io.reactivex.internal.operators.single.SingleFromCallable.subscribeActual(SingleFromCallable.java:44)
	at io.reactivex.Single.subscribe(Single.java:3438)
	at io.reactivex.internal.operators.single.SingleDoOnSuccess.subscribeActual(SingleDoOnSuccess.java:35)
	at io.reactivex.Single.subscribe(Single.java:3438)
	at io.reactivex.internal.operators.maybe.MaybeFromSingle.subscribeActual(MaybeFromSingle.java:41)
	at io.reactivex.Maybe.subscribe(Maybe.java:4154)
	at io.reactivex.internal.operators.maybe.MaybeConcatArray$ConcatMaybeObserver.drain(MaybeConcatArray.java:153)
	at io.reactivex.internal.operators.maybe.MaybeConcatArray$ConcatMaybeObserver.request(MaybeConcatArray.java:78)
	at io.reactivex.internal.operators.flowable.FlowableElementAtMaybe$ElementAtSubscriber.onSubscribe(FlowableElementAtMaybe.java:66)
	at io.reactivex.internal.operators.maybe.MaybeConcatArray.subscribeActual(MaybeConcatArray.java:42)
	at io.reactivex.Flowable.subscribe(Flowable.java:14479)
	at io.reactivex.internal.operators.flowable.FlowableElementAtMaybe.subscribeActual(FlowableElementAtMaybe.java:36)
	at io.reactivex.Maybe.subscribe(Maybe.java:4154)
	at io.reactivex.internal.operators.maybe.MaybeToSingle.subscribeActual(MaybeToSingle.java:46)
	at io.reactivex.Single.subscribe(Single.java:3438)
	at io.reactivex.internal.operators.maybe.MaybeFromSingle.subscribeActual(MaybeFromSingle.java:41)
	at io.reactivex.Maybe.subscribe(Maybe.java:4154)
	at io.reactivex.internal.operators.maybe.MaybeSubscribeOn$SubscribeTask.run(MaybeSubscribeOn.java:54)
	at io.reactivex.internal.schedulers.ScheduledDirectTask.call(ScheduledDirectTask.java:38)
	at io.reactivex.internal.schedulers.ScheduledDirectTask.call(ScheduledDirectTask.java:26)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
	at java.lang.Thread.run(Thread.java:764)
Caused by: java.lang.NullPointerException: Attempt to invoke virtual method 'java.lang.String org.jsoup.nodes.Element.attr(java.lang.String)' on a null object reference
	at org.schabi.newpipe.extractor.services.youtube.extractors.YoutubeChannelExtractor.getName(YoutubeChannelExtractor.java:124)
	... 28 more


I don't know if this is related to this change.
Edit: Ok seems to be a non existent channel in my subscription list.

@chilliger
Copy link

Another note on this. Is it possible to cache the order in some way to improve the loading time and to minimize the jumping effect when new found videos get inserted. Maybe setting up the background fetching with how often it should be done could be an option?

@snappyapple632
Copy link

Use the Markdown converter next time @chilliger

@timfbfbfbfb
Copy link
Author

Is it possible to cache the order in some way to improve the loading time and to minimize the jumping effect when new found videos get inserted.

I think finding the correct positions of the video items would not contribute too much to the loading time and the jumping effect.
Like I explained in the description, I fetch video items from all subscribed channels in the background threads and insert the video items to the list incrementally in the main UI thread, a little bit of jumping effect is unavoidable. I think this is a trade-off. I could avoid jumping effect if I wait for all HTTP responses before displaying the list, however, the loading time would be very long.

Maybe setting up the background fetching with how often it should be done could be an option?

The current implementation would just fetch the videos from the subscribed channels once.

@Stypox Stypox mentioned this pull request Aug 26, 2019
@cool-student
Copy link
Contributor

@timfbfbfbfb can you add a notification with progress so that the user knows when all subscriptions have been checked ? This was implemented in #2309 . In the current implementation it is unclear when the subscription check is done and while I like already seeing the videos and being able to scroll already, I have to scroll back up constantly since I never really know when all subscriptions have been checked. IMO this could be added later and I just want to see this merged asap but that's just my opinion.

@timfbfbfbfb
Copy link
Author

@coolstudent123 That is a good suggestion, I think we can add a horizontal progress bar at the very top to indicate the loading progress. But as you have said, I want to keep this PR small enough to merge, maybe we can create a separate issue for that. I can also help working on that later.

@opusforlife2
Copy link
Collaborator

Also closes #822 .

@rakshazi
Copy link

Hello, any update on this?
No news for last 2 weeks :(

@Stypox Stypox mentioned this pull request Sep 12, 2019
@esalgado
Copy link

I tested the build for several days and runs very well without issues for me.

@rakshazi
Copy link

So. may be it's time to merge that PR?

@theScrabi theScrabi added deep review required Maintainer must double check/test/review this due to changes in API or architecture feature request Issue is related to a feature in the app labels Sep 24, 2019
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your work, code looks good!
I do not have a deep understaing of ReactiveX, so feel free to ignore the requested changes about Schedulers.io() if they are wrong ;-)

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I double-checked everything and I think it can now be merged!

Again, thank you for your work :-D

@Stypox Stypox self-requested a review October 1, 2019 13:05
@timfbfbfbfb
Copy link
Author

@ozyc
The first and the third exceptions are fixed in the latest version, though the third exception is not introduced by my code changes.
I could not reproduce the second exception, maybe you can try if it is still reproducible in the latest version.

@n-xlkt @cool-student
I added the progress bar in the latest version. Since there are not so many code changes, maybe it is a good idea to just add the progress bar in this PR.

The latest apk is updated in the description, feel free to download and test it.

@timfbfbfbfb timfbfbfbfb force-pushed the dev branch 2 times, most recently from d948072 to 0350d05 Compare November 30, 2019 19:13
@Mrmusscle
Copy link

Hi, I had been using your work for days and it was wonderful. Just a small question, my 'live' video keep getting burried for some reason, can you do something about it?
https://i.imgur.com/V6qJyd4.jpg

@Faeb35
Copy link

Faeb35 commented Dec 7, 2019

Hi Buddies
Just tested app-a10e6dde4.apk.zip. I really like this implementation, although there are two things I like to mention:

  1. The "Whats New" is getting reloaded every time I visit this tab. It's very annoying, because the list is constantly changing while loading. And as I have a lot of channels, that always takes a while... Wouldn't it be possible to store the result and reload/update like every day in the background? My suggestion would be to start a manually reload, when you swipe down (like in Chrome or other apps).
  2. I'm having "What's New" configured as my first tab. When I watch a video and come back to the "What's New" list, the list is empty. I'm using Android 5 and the app in german.
    image
    All the best,
    Faeb35

@ADepic ADepic mentioned this pull request Dec 19, 2019
@EthanZeigler
Copy link

This is working flawlessly for me. I think it's stable enough to become an option in prod.

@njmdietrich
Copy link

Love this and have been using it without any issues for the last couple of months now. Last night an issue broke video playback in newspipe and a hotfix has been released. Could you please update the apk here to include this version?

@timfbfbfbfb
Copy link
Author

timfbfbfbfb commented Jan 24, 2020

@njmdietrich The apk file in the description has been updated.

Edit: I have just realized the infinite loading issue, it was fixed in app-fa3a306a.apk

@hunkjazz
Copy link

@timfbfbfbfb

Do you have any other plan for this fork, as in add new features o fixes?

Thank you for the sort feature and maintenece!

@timfbfbfbfb
Copy link
Author

@JLammer
Seems the NewPipe maintainers favor #2309 (see the comment here) and probably this PR would not be getting merged, so I guess I would not add features or make big changes to this PR. But since I use this fork myself, I will keep this forked version of APK usable as long as I am still using it.
Thanks for your support anyway 👍

@njmdietrich
Copy link

I wonder why they prefer the other PR, I think yours is more elegant (from an end user point of view) with the way things load etc, the only thing that's missing from yours to make it perfect in my mind would be a button or pulling down to refresh the list. Props to you and I'm very grateful that you're keeping things updated here :)

@hunkjazz
Copy link

the only thing that's missing from yours to make it perfect in my mind would be a button or pulling down to refresh the list.

Indeed! This is the only thing missing <3.

Because I have got subscribed to many channels, fetching new videos every time I go to "what's new" section takes too long. Doing this without consent of the user is really a pain in the butt. Also, If a button to pull down requires a more complex code (hence coding time), providing just a static button would be fine.

Beyond that (although I don't know if it's possible), would be better to fetch and update the "what's new" section only with the newest videos uploaded from the last time you checked until now. In this way, I think you'd update faster and use less bandwidth. I'm pretty sure the app keeps fetching many metadata it already has stored.

@PeterHindes
Copy link
Contributor

the only thing that's missing from yours to make it perfect in my mind would be a button or pulling down to refresh the list.

Indeed! This is the only thing missing <3.

Because I have got subscribed to many channels, fetching new videos every time I go to "what's new" section takes too long. Doing this without consent of the user is really a pain in the butt. Also, If a button to pull down requires a more complex code (hence coding time), providing just a static button would be fine.

Beyond that (although I don't know if it's possible), would be better to fetch and update the "what's new" section only with the newest videos uploaded from the last time you checked until now. In this way, I think you'd update faster and use less bandwidth. I'm pretty sure the app keeps fetching many metadata it already has stored.

This would probably not use less data. If I'm remembering correctly the way to fetch the subscriptions videos is to check the complete video list xml for each channel, and there was no way to only fetch a specific number of entries.

@ShareASmile
Copy link
Collaborator

@timfbfbfbfb Could you provide an apk with the latest changes?

@timfbfbfbfb
Copy link
Author

@timfbfbfbfb Could you provide an apk with the latest changes?

Sure. Updated in the description.

@wb9688 wb9688 mentioned this pull request Mar 2, 2020
@wb9688
Copy link
Contributor

wb9688 commented Mar 14, 2020

Closed in favor of @mauriciocolli's solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deep review required Maintainer must double check/test/review this due to changes in API or architecture feature request Issue is related to a feature in the app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[enhancement] "What's New" sorting improvement Subscription Order