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

Show improved error panel instead of annoying snackbar or crashing #5148

Merged
merged 11 commits into from
Mar 14, 2021

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Dec 11, 2020

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

This PR improves the error panel that currently is only shown when there are network problems:

  • When ANY error occours, if there are not already some items in the view, always show the error panel, otherwise show the snackbar.
  • Extractor errors now don't crash the app but instead show up in the error panel for the corresponding fragment.
  • The retry button is always shown except for ContentNotSupportedExceptions and ContentNotAvailableExceptions, like before.
  • Add a dynamic button to the error panel that acts as a "Report" button for everything except ReCaptchaExceptions, where instead a "Solve" button is shown.

I also refactored many things related to error reporting, in particular:

  • Move everything inside error subpackage.
  • Add ErrorPanelHelper class to handle the error panel.
  • Use ErrorInfo to store ALL of the information about an error that is needed to perform a report, so that functions can just take an ErrorInfo instead of having to do multiple overloads. This reduced and simplified much the code.
  • Remove multiple overloadable functions about errors in BaseStateFragment and instead make only one, that handles the error panel by itself.
  • Remove returnActivity for ErrorActivity: it was only used in SettingsFragments (set to SettingsActivity, but without it everything just works as intended) and in RouterActivity (set to MainActivity: when something failed while resolving a shared link, the error activity shows up, but now when the user presses back he is just brought to what he was doing before sharing the link to NewPipe, which is expected, instead of unexpectedly going to NewPipe's MainActivity). Also remove ActivityCommunicator, now unneded as it was only needed to pass returnActivity around. This solves the "lost backstack" issue when the error activity opens, so that the user can report something and then continue browsing without losing anything. @opusforlife2 :-D

Other relevant changes:

  • Fix list fragments not being scrollable when the recycler view is hidden, causing problems in VideoDetailFragment. Now, instead of hiding, alpha is just set to 0.0f.
  • Fix EmptyFragment not being scrollable inside VideoDetailFragment.
  • Restore error state in onResume, i.e. after a fragment was paused.
  • Fix SearchFragment onResume behaviour, that was accidentally showing suggestion and meta info panels from the previous search for a brief moment even though it was asked to load something else.
  • Some random code refactorings here and there.
Here there are two errors showing up, but that's expected and I don't think it should be changed.

TODO

  • Add screenshots to this PR
  • Fix tests
  • Fix comments tab with visible error panel not scrolling in video detail fragment
  • Fix search fragment error panel not being restored after resume
  • Test thoroughly (I tested on my Fairphone with /e/, Android 10. These changes should be device independent)

Fixes the following issue(s)

Fixes #4026
Closes #5359 (becomes not relevant)

APK testing

Do you like this behaviour, instead of the old one where the app crashed clearing activity stack and with annoying snackbars preventing access to comments?
This apk throws random errors every once in a while on network requests, to make testing simpler: app-debug.zip

Due diligence

@triallax
Copy link
Contributor

Search and link related issues (@mhmdanas do you have time for this, please? ;-) )

If you summon me, I know I have to oblige. :)

@triallax
Copy link
Contributor

The only two somewhat related issues I found are #1858 and #4026.

@Stypox
Copy link
Member Author

Stypox commented Dec 11, 2020

Yeah, thank you, this fixes #4026, i.e. the error screen only appears if the users asks to or if an unrecoverable error happened (for which it would be impossible to restore the previous state)

@opusforlife2
Copy link
Collaborator

app crashed clearing activity stack

THANK YOU SO MUCH FOR FIXING THIS! CAN I HAVE YOUR BABIES???

@Stypox
Copy link
Member Author

Stypox commented Dec 29, 2020

I rebased, fixed tests and added screenshots to the PR description @TobiGr @B0pol
Here is a debug apk that crashes on every list and on video detail fragment: app-debug.zip
@TiA4f8R That should be done in another PR, since it is a separate issue that needs to be investigated properly.

@opusforlife2
Copy link
Collaborator

Going back from the Error Activity now generates a couple of extra "Back" actions. Switch to Bookmarked Playlists tab > Open a local playlist > Open a video > Open the Error Activity > Go back. You see the video minimized, and you're back at the Bookmarked Playlists tab.

The app should just close the Error Activity and stay on whatever page the Report button is on.

@Stypox Stypox force-pushed the error-panel branch 2 times, most recently from 1790548 to 88c5207 Compare January 9, 2021 15:45
@Stypox
Copy link
Member Author

Stypox commented Jan 9, 2021

@opusforlife2 here you are. I have completely removed "returnActivity", so that now the back arrow and button just do as expected and nothing is ever lost.
This apk throws random errors every once in a while on network requests, to make testing simpler: app-debug.zip
There are still a few things that need to be done for this PR, I will do them tomorrow, then it will be ready

@opusforlife2
Copy link
Collaborator

Confirmed fixed!

@Stypox Stypox mentioned this pull request Jan 14, 2021
4 tasks
@Stypox Stypox force-pushed the error-panel branch 2 times, most recently from 94690ce to 8f21460 Compare January 15, 2021 14:52
@Stypox
Copy link
Member Author

Stypox commented Jan 15, 2021

I rebased and solved the leftover TODOs. This is now ready. @TobiGr @opusforlife2

  • Fix list fragments not being scrollable when the recycler view is hidden, causing problems in VideoDetailFragment. Now, instead of hiding, alpha is just set to 0.0f.
  • Fix EmptyFragment not being scrollable inside VideoDetailFragment.
  • Restore error state in onResume, i.e. after a fragment was paused.
  • Fix SearchFragment onResume behaviour, that was accidentally showing suggestion and meta info panels from the previous search for a brief moment even though it was asked to load something else.

This apk throws random errors every once in a while on network requests, to make testing simpler: app-debug.zip

@opusforlife2
Copy link
Collaborator

The latest APK crashes to home screen for me upon opening. If I then open it again from Recent Apps, it shows me an actual Error Activity.

Also, @TobiGr this Markdown formatted crash report generated using the button doesn't contain the GMT Time and Package headers. Is it not supposed to?

Exception

  • User Action: ui error
  • Request: ACRA report
  • Content Country: CH
  • Content Language: en-IN
  • App Language: en_IN
  • Service: none
  • Version: 0.20.8
  • OS: Linux Android 10 - 29
Crash log

kotlin.UninitializedPropertyAccessException: lateinit property swipeRefreshLayout has not been initialized
	at org.schabi.newpipe.local.feed.FeedFragment.hideLoading(FeedFragment.kt:196)
	at org.schabi.newpipe.local.feed.FeedFragment.handleLoadedState(FeedFragment.kt:266)
	at org.schabi.newpipe.local.feed.FeedFragment.handleResult(FeedFragment.kt:209)
	at org.schabi.newpipe.local.feed.FeedFragment$onViewCreated$2.onChanged(FeedFragment.kt:95)
	at org.schabi.newpipe.local.feed.FeedFragment$onViewCreated$2.onChanged(FeedFragment.kt:54)
	at androidx.lifecycle.LiveData.considerNotify(LiveData.java:131)
	at androidx.lifecycle.LiveData.dispatchingValue(LiveData.java:149)
	at androidx.lifecycle.LiveData.setValue(LiveData.java:307)
	at androidx.lifecycle.MutableLiveData.setValue(MutableLiveData.java:50)
	at androidx.lifecycle.LiveData$1.run(LiveData.java:91)
	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)


@Stypox
Copy link
Member Author

Stypox commented Jan 15, 2021

@opusforlife2 fixed: https:/TeamNewPipe/NewPipe/suites/1839064245/artifacts/35575997
This apk throws random errors every once in a while on network requests, to make testing simpler: app-debug.zip

@opusforlife2
Copy link
Collaborator

Fixed. 👍

I don't see any difference between the last (working) version and the latest one. Is there supposed to be any?

@Stypox
Copy link
Member Author

Stypox commented Jan 15, 2021

@opusforlife2 these: #5148 (comment)

@XiangRongLin XiangRongLin added bug Issue is related to a bug codequality Improvements to the codebase to improve the code quality labels Jan 17, 2021
@Stypox
Copy link
Member Author

Stypox commented Jan 18, 2021

I rebased again, and improved how the loading indicator is shown in the feed fragment
This apk throws random errors every once in a while on network requests, to make testing simpler: app-debug.zip

@Stypox Stypox mentioned this pull request Jan 18, 2021
5 tasks
@triallax
Copy link
Contributor

@mqus I'm not sure if you've missed that, but to be sure, the APK is designed to throw random exceptions for testing purposes.

@mqus
Copy link
Contributor

mqus commented Mar 10, 2021

Yeah, thats why I wrote

So everything seems fine.

And

But nothing that crashes the app.

TobiGr
TobiGr previously approved these changes Mar 10, 2021
@AudricV
Copy link
Member

AudricV commented Mar 10, 2021

There is just one bug with the last debug APK provided: we couldn't share the error report with the share button in the error activity: the shared report is empty.

@mqus
Copy link
Contributor

mqus commented Mar 10, 2021

Ah I noticed that but thought that this was due to signal(where I tried it) not correctly accepting it. My bad for not testing better.

@AudricV AudricV added the feature request Issue is related to a feature in the app label Mar 11, 2021
It will be shown even when nothing could be loaded not due to a network error, and the user can choose to ignore or report it.

Also improve error reporting arguments
Also completely refactor error activity
Also improve some code here and there
Make sure the url not supported dialog is only shown when the url is really not supported, not on any ExtractionException
@Stypox
Copy link
Member Author

Stypox commented Mar 12, 2021

Sorry, I accidentally removed a toString() here. I fixed it. Here is another apk, as usual with random exceptions: app-debug.zip

@AudricV
Copy link
Member

AudricV commented Mar 12, 2021

I can confirm that this issue is fixed on my device.

@TobiGr TobiGr merged commit 404a6c1 into TeamNewPipe:dev Mar 14, 2021
@Stypox Stypox deleted the error-panel branch March 14, 2021 17:02
This was referenced Mar 21, 2021
tossj pushed a commit to tossj/NewPipe-legacy that referenced this pull request Apr 21, 2021
Show improved error panel instead of annoying snackbar or crashing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug codequality Improvements to the codebase to improve the code quality feature request Issue is related to a feature in the app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Instrumented tests don't run Go back to previous screen on Guru Meditation errors
7 participants