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

Add support for tabs #3201

Closed
wants to merge 22 commits into from
Closed

Add support for tabs #3201

wants to merge 22 commits into from

Conversation

wb9688
Copy link
Contributor

@wb9688 wb9688 commented Mar 8, 2020

Currently it looks like the following:

Videos tab Playlists tab
Screenshot of Videos tab Screenshot of Playlists tab

You could download a test APK here.

To do:

  • Show the subscribe count and button
  • Show placeholder for avatar and banner while loading or when there is none
  • Show 'No items' instead of 'No videos'
  • Make the queue working again (though I've no idea how to do that as a tab could contain both streams and playlists just for the stream-only tabs)
  • Show tab names at the top instead of dots at the bottom
  • Clean up the code
  • Fix crash when opening item in channel on homepage
  • Localize tab names
  • Fix location of loading animation and network error

Fixes #3237

Also see TeamNewPipe/NewPipeExtractor#279.

@Stypox
Copy link
Member

Stypox commented Mar 8, 2020

I am not sure it is a good idea to use those bubbles on the bottom to switch to different tabs. I think a pager like in the MainActivity would suit better, since in the future we could add new tabs for channels (e.g. related/suggested channels, channel description, ...), and having a small label for every tab would make things more clear and easily-accessible to the user imo.

@wb9688
Copy link
Contributor Author

wb9688 commented Mar 8, 2020

@Stypox: I agree. What exactly do you mean with 'a small label' though? Just the name of the tab (e.g. 'Videos' or 'Popular tracks')?

@wb9688 wb9688 added the feature request Issue is related to a feature in the app label Mar 8, 2020
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.

The approach is ok, in my opinion

@Stypox
Copy link
Member

Stypox commented Mar 8, 2020

@Stypox: I agree. What exactly do you mean with 'a small label' though? Just the name of the tab (e.g. 'Videos' or 'Popular tracks')?

Yeah ;-)

@heberjeur

This comment has been minimized.

@B0pol

This comment has been minimized.

@heberjeur
Copy link

I found this error
https://youtu.be/90wuJhtQlqE

@wb9688
Copy link
Contributor Author

wb9688 commented Mar 14, 2020

I updated the screenshots and test APK.

@opusforlife2
Copy link
Collaborator

Would you please rename the package for adjacent installation with other apks?

@B0pol
Copy link
Member

B0pol commented Mar 14, 2020

Would you please rename the package for adjacent installation with other apks?

Rebase because it will also include fixed feed

Copy link
Member

@B0pol B0pol left a comment

Choose a reason for hiding this comment

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

Please avoid duplicates next time. Otherwise you're ordering community to translate something they already translated, we should not do that.

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@@ -27,7 +27,7 @@ class SubscriptionManager(context: Context) {

database.runInTransaction {
infoList.forEachIndexed { index, info ->
feedDatabaseManager.upsertAll(listEntities[index].uid, info.relatedItems)
feedDatabaseManager.upsertAll(listEntities[index].uid, info.tabs[0].relatedItems as List<StreamInfoItem>)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a better solution for this?

@wb9688
Copy link
Contributor Author

wb9688 commented Mar 15, 2020

@opusforlife2 and @B0pol: I just rebased and updated the APK.

@wb9688 wb9688 marked this pull request as ready for review March 15, 2020 15:29
@@ -39,9 +32,9 @@ public void fetch() {
ExtractorHelper.getChannelInfo(this.serviceId, this.baseUrl, false)
.subscribeOn(Schedulers.io())
.observeOn(AndroidSchedulers.mainThread())
.subscribe(getHeadListObserver());
.subscribe((Consumer<? super ChannelInfo>) getHeadListObserver());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think I did this part correctly and I'm not sure what this even is supposed to do, though I don't think it breaks anything when I remove it.

@wb9688
Copy link
Contributor Author

wb9688 commented Mar 15, 2020

@TobiGr: I think this PR is almost finished now. I highlighted 2 issues above, plus I need to make the playlist controls not sticky. @B0pol also discovered some SoundCloud issue, where it sometimes stops working completely for a few minutes, which I've reproduced once, but I'm not sure what causes it.

@wb9688
Copy link
Contributor Author

wb9688 commented Mar 15, 2020

Another bug found by @B0pol: when a channel only has one tab (i.e. media.ccc.de or PeerTube) and you press back, it crashes.

Edit: fixed.

@opusforlife2
Copy link
Collaborator

Something is wrong. The feed update process is broken. It doesn't load even a single one successfully. Tapping on Report doesn't even show the errors. It just closes the app. I'm not sure if that's caused by this PR or mauricio's code.

@wb9688
Copy link
Contributor Author

wb9688 commented Mar 15, 2020

@opusforlife2: I could confirm that, however it shows the errors for me. I'll look into that.

@wb9688
Copy link
Contributor Author

wb9688 commented Mar 15, 2020

@opusforlife2: I fixed your issue and rebased it (again). I'll upload the new APK in a minute.

@opusforlife2
Copy link
Collaborator

opusforlife2 commented Mar 15, 2020

Confirmed fixed.

Edit: Wow! Channel loading is super fast. Placebo or your code?

@opusforlife2
Copy link
Collaborator

Another bug, but it is also present in cool-student's PR. When you open the app through a link from another app, the name of the app "Newpipe tabs" flashes for a moment at the place where the resolution drop down is. For his PR, it's "Newpipe notification", naturally. This doesn't happen in the release apk.

Which means it's something in the dev branch causing the issue.

@opusforlife2 opusforlife2 mentioned this pull request Mar 16, 2020
1 task
@wb9688
Copy link
Contributor Author

wb9688 commented Mar 16, 2020

@opusforlife2: Please just create a new issue for that as that isn't caused by this PR.

@wb9688 wb9688 mentioned this pull request Mar 16, 2020
3 tasks
@opusforlife2
Copy link
Collaborator

That... is a very good point. Not sure what I was thinking. Brain fart.

@opusforlife2
Copy link
Collaborator

Error when switching tabs on a SoundCloud user page.

Also, why does Newpipe lose the backstack when you open the error report page? Isn't that a bug?

Exception

Crash log

org.schabi.newpipe.extractor.exceptions.ExtractionException: Could not get next page
	at org.schabi.newpipe.extractor.services.soundcloud.SoundcloudChannelAlbumsExtractor.computeNextPageAndGetPlaylists(SoundcloudChannelAlbumsExtractor.java:61)
	at org.schabi.newpipe.extractor.services.soundcloud.SoundcloudChannelAlbumsExtractor.getInitialPage(SoundcloudChannelAlbumsExtractor.java:37)
	at org.schabi.newpipe.extractor.utils.ExtractorHelper.getItemsPageOrLogError(ExtractorHelper.java:19)
	at org.schabi.newpipe.extractor.channel.ChannelTabInfo.loadTab(ChannelTabInfo.java:67)
	at org.schabi.newpipe.fragments.list.channel.ChannelTabFragment.lambda$loadResult$0$ChannelTabFragment(ChannelTabFragment.java:84)
	at org.schabi.newpipe.fragments.list.channel.-$$Lambda$ChannelTabFragment$WuJ37aw-7UKTbW8f72jXH9kxJGI.call(Unknown Source:2)
	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.SingleSubscribeOn$SubscribeOnObserver.run(SingleSubscribeOn.java:89)
	at io.reactivex.Scheduler$DisposeTask.run(Scheduler.java:578)
	at io.reactivex.internal.schedulers.ScheduledRunnable.run(ScheduledRunnable.java:66)
	at io.reactivex.internal.schedulers.ScheduledRunnable.call(ScheduledRunnable.java:57)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:301)
	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.io.InterruptedIOException: interrupted
	at okio.Timeout.throwIfReached(Timeout.java:146)
	at okio.Okio$1.write(Okio.java:76)
	at okio.AsyncTimeout$1.write(AsyncTimeout.java:180)
	at okio.RealBufferedSink.flush(RealBufferedSink.java:224)
	at okhttp3.internal.http1.Http1Codec.finishRequest(Http1Codec.java:166)
	at okhttp3.internal.http.CallServerInterceptor.intercept(CallServerInterceptor.java:84)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:121)
	at com.facebook.stetho.okhttp3.StethoInterceptor.intercept(StethoInterceptor.java:56)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
	at okhttp3.internal.connection.ConnectInterceptor.intercept(ConnectInterceptor.java:45)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:121)
	at okhttp3.internal.cache.CacheInterceptor.intercept(CacheInterceptor.java:93)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:121)
	at okhttp3.internal.http.BridgeInterceptor.intercept(BridgeInterceptor.java:93)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
	at okhttp3.internal.http.RetryAndFollowUpInterceptor.intercept(RetryAndFollowUpInterceptor.java:127)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:121)
	at okhttp3.RealCall.getResponseWithInterceptorChain(RealCall.java:257)
	at okhttp3.RealCall.execute(RealCall.java:93)
	at org.schabi.newpipe.DownloaderImpl.execute(DownloaderImpl.java:159)
	at org.schabi.newpipe.extractor.downloader.Downloader.get(Downloader.java:70)
	at org.schabi.newpipe.extractor.downloader.Downloader.get(Downloader.java:42)
	at org.schabi.newpipe.extractor.services.soundcloud.SoundcloudParsingHelper.getPlaylistsFromApi(SoundcloudParsingHelper.java:312)
	at org.schabi.newpipe.extractor.services.soundcloud.SoundcloudParsingHelper.getPlaylistsFromApiMinItems(SoundcloudParsingHelper.java:300)
	at org.schabi.newpipe.extractor.services.soundcloud.SoundcloudChannelAlbumsExtractor.computeNextPageAndGetPlaylists(SoundcloudChannelAlbumsExtractor.java:59)
	... 16 more


@wb9688
Copy link
Contributor Author

wb9688 commented Mar 20, 2020

@opusforlife2: I can't reproduce that error.

About the backstack: I've no idea. I had code related to that inherited from the VideoDetailFragment, but that seemed to cause crashes here, so I removed that code, which seemed to work.

@opusforlife2
Copy link
Collaborator

Keep opening some User pages. You'll eventually hit it. I originally got it with an unofficial Daft Punk page, but then it stopped happening. I thought it was a one off, but then it happened here.

What basically happens is that Newpipe is either able to load the tabs properly, in which case the error is unlikely to appear at all. But if it takes time to load the first tab, chances are that one of the other ones will give the error.

For example, when I hit the error with the Eminem page, the Tracks tab was empty. But when I opened it again a few times, Tracks was populated.

@Stypox
Copy link
Member

Stypox commented Mar 20, 2020

@opusforlife2 for the backstack: that's not something that can be prevented, I think. When the app crashes... it crashes. The Android is instructed to call a function inside of the app to handle the information about the crash, but everything has already been destroyed.

@opusforlife2
Copy link
Collaborator

@Stypox I'm not talking about the case where the app crashes and then automatically shows the error page. I'm talking about when you're happily browsing, then you see an error at the bottom, which you can choose not to open and continue browsing without any issue. But if you open it, you lose the backstack. I don't see why that should be the case. Maybe there's a way to code the page so that if it is opened manually by the user, the backstack shouldn't be discarded?

Also, the button being called "Report" is a bit misleading. It should be called "View" instead. You get the option to report the error only after viewing it.

@njmdietrich
Copy link

When opening a playlist on YouTube from the playlists tab and then pressing the back button to return to the tab, it opens on the video tab. While the scrolling position of the playlists tab is preserved so switching back to it doesn't cause any issues, the expected behaviour would be to return to the playlist tab immediately.

@TobiGr TobiGr added this to the 0.19.3 milestone Apr 6, 2020
@wb9688 wb9688 removed this from the 0.19.3 milestone Apr 8, 2020
@B0pol B0pol mentioned this pull request Apr 11, 2020
1 task
@wb9688 wb9688 added this to the 0.20.0 milestone Apr 11, 2020
@B0pol B0pol mentioned this pull request Apr 16, 2020
This was referenced Apr 23, 2020
@wb9688 wb9688 removed this from the 0.20.0 milestone Jul 13, 2020
@opusforlife2
Copy link
Collaborator

@NicTanghe Follow this for playlists in channels.

@NicTanghe
Copy link

Does this anly work for playlists with channels own videos or also playlists with others videos?
Also thx for pointing me here. Altyd grappig om mensen Nederland s te zien gebruiken.

@opusforlife2
Copy link
Collaborator

@NicTanghe Probably the channel's own playlists. Can't remember for sure.

@wb9688 wb9688 closed this Oct 24, 2020
@BruceMustache
Copy link

What happens with this PR?

@ghost
Copy link

ghost commented Dec 6, 2020

Curious about the PR as well, checked if there were issues filed for it and found this, not sure if any things come from it.

@MD77MD
Copy link

MD77MD commented May 31, 2021

any hope for this to be merged

@alittlebitofit
Copy link

Is the apk still updated?
Because I see the loading indicator running forever.
And it says "No Results" when I try to search.

@AudricV
Copy link
Member

AudricV commented May 10, 2022

Not at all, this PR was closed and this work has been abandoned.

@heberjeur
Copy link

@TiA4f8R why?, isn't necessary to add support for tabs ?

@Stypox
Copy link
Member

Stypox commented May 10, 2022

Yes it is, since there is an issue open about it which has received endorsements from devs. But nobody is working on this at the moment and the original creator of this PR dropped his work. If somebody wants to work on this, contributions are well accepted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issue is related to a feature in the app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Null object tried to be parsed as JSON