Skip to content

Fix tab handling and enable ignored tests again#2742

Merged
TobiGr merged 6 commits intoTeamNewPipe:devfrom
mauriciocolli:fix-mess-tabs
Nov 21, 2019
Merged

Fix tab handling and enable ignored tests again#2742
TobiGr merged 6 commits intoTeamNewPipe:devfrom
mauriciocolli:fix-mess-tabs

Conversation

@mauriciocolli
Copy link
Contributor

@mauriciocolli mauriciocolli commented Oct 20, 2019

Was testing some other PRs and noticed that the main fragment had some problems, the most important one being the way the view pager was being handled.

  • Remove usage of hardcoded title for default kiosk.
  • Some tests were simply ignored, this PR enables them again.
  • Fix broken view pager tabs implementation
    • Fragments were being recreated from scratch (losing their state) every time some configuration change occurred (e.g. screen rotation).
    • Use FragmentStatePagerAdapter instead, as it is built to work with them and manage their states.
  • Clear the item list when starting loading.
  • Enable toolbar title visibility when setting a new one.
  • Use tab position from parameters instead of relying on the view pager.
  • Make the KioskFragment aware of changes in the preferred content country.

@Stypox
Copy link
Member

Stypox commented Oct 20, 2019

Fragments were being recreated from scratch (losing their state) every time some configuration change occurred (e.g. screen rotation).

Does this imply that now when the user changes i.e. the language, the views do not reload?

@mauriciocolli
Copy link
Contributor Author

Does this imply that now when the user changes i.e. the language, the views do not reload?

Do you mean changes like language of the phone? If you are referring to this, yes, the fragments will reload like any other in the app. Android will recreate and give the correct resources when the fragment is starting up again.

But if you meant the language that is the settings of the app, no, it will not because there is no check to see if it changed or anything. Notice that this isn't the result of this PR, it doesn't work in the current version either. This is a flaw in the current design, things are mixed up in the fragment and the layers (view, data source) are not separated optimally.

And speaking of the language selector, it was decided that pages will be fetched only in english (or whatever the service offer by default), as we don't have to take in account every little change when parsing the page. The content country selector is very useful though.

I will push a commit fixing that in a second.

@Stypox
Copy link
Member

Stypox commented Oct 20, 2019

OK, thank you

Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

Looks good to me. There are only two things:

  • Can you please rebase and address my comment above?
  • Unfortunately you cause a NPE when loading a soundcloud kiosk page which has not the current kioskId. No error with mediaCCC. This does not happen with 0.17.4

Exception

Crash log

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.soundcloud.SoundcloudParsingHelper.clientId(SoundcloudParsingHelper.java:49)
	at org.schabi.newpipe.extractor.services.soundcloud.SoundcloudChartsExtractor.computNextPageAndStreams(SoundcloudChartsExtractor.java:56)
	at org.schabi.newpipe.extractor.services.soundcloud.SoundcloudChartsExtractor.getInitialPage(SoundcloudChartsExtractor.java:85)
	at org.schabi.newpipe.extractor.utils.ExtractorHelper.getItemsPageOrLogError(ExtractorHelper.java:20)
	at org.schabi.newpipe.extractor.kiosk.KioskInfo.getInfo(KioskInfo.java:69)
	at org.schabi.newpipe.extractor.kiosk.KioskInfo.getInfo(KioskInfo.java:55)
	at org.schabi.newpipe.util.ExtractorHelper.lambda$getKioskInfo$10(ExtractorHelper.java:170)
	at org.schabi.newpipe.util.-$$Lambda$ExtractorHelper$TneUC77tEl15twhVz-_2huMZ3Ck.call(lambda)
	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.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:237)
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:269)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1113)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:588)
	at java.lang.Thread.run(Thread.java:818)


- Fix typo in a string resource
- Reorder tabs so the default kiosk is on top of the others
- Fragments were being recreated from scratch (losing their state) every
time some configuration change occurred (e.g. screen rotation).
- Use `FragmentStatePagerAdapter` instead, as it is built to work with
them and manage their states.
@mauriciocolli
Copy link
Contributor Author

Unfortunately you cause a NPE when loading a soundcloud kiosk page which has not the current kioskId. No error with mediaCCC. This does not happen with 0.17.4

The stack trace points to the client id extraction function of the SoundCloud extractor, leading to where the problem is: this branch is outdated so it's still using an old version of the extractor (also in the crash report: Version: 0.17.3).

Can you please rebase and address my comment above?

Done, fixes the other issue as well.

@TobiGr
Copy link
Contributor

TobiGr commented Nov 21, 2019

The stack trace points to the client id extraction function of the SoundCloud extractor, leading to where the problem is: this branch is outdated so it's still using an old version of the extractor (also in the crash report: Version: 0.17.3).

All right, my bad.

Done, fixes the other issue as well.

Perfect

@TobiGr TobiGr merged commit b9d6d55 into TeamNewPipe:dev Nov 21, 2019
This was referenced Dec 12, 2019
@mauriciocolli mauriciocolli deleted the fix-mess-tabs branch March 7, 2020 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants