Add more specific error messages and deduplicate their handling#12477
Add more specific error messages and deduplicate their handling#12477Stypox wants to merge 3 commits intoTeamNewPipe:release-0.28.0from
Conversation
Stypox
force-pushed
the
better-error-messages
branch
from
9f727b0 to
13d1917
Compare
litetex
left a comment
litetex
left a comment
There was a problem hiding this comment.
Stypox on fire today 🔥
Thank you for the PR!
I did some testing with some common exceptions but it's hard to test all possible combinations.
However Code LGTM
| // If there’s already a git hash, just add more of it to the end (or remove a letter) | ||
| // to cause jitpack to regenerate the artifact. | ||
| implementation 'com.github.TeamNewPipe:NewPipeExtractor:7adbc48a0aa872c016b8ec089e278d5e12772054' | ||
| implementation 'com.github.Stypox:NewPipeExtractor:1c04bd88c3f1e6b1e0c912814035745e61bb9aba' |
There was a problem hiding this comment.
This needs to be changed before merging
| @StringRes | ||
| private fun getMessageStringId( | ||
| fun getMessageStringId( | ||
| throwable: Throwable?, |
There was a problem hiding this comment.
Might be a stupid question but why are we catching Throwables in the first place and not Exceptions?
If we encounter an Error that usually means a critical error that must result in a VM crash, like OutOfMemoryError, etc
There was a problem hiding this comment.
For example when the app crashes and ACRA catches the throwable and starts the error report screen, that throwable could be anything. In other places... well yeah maybe we don't need to catch Throwables, but just to be sure sometimes we do. E.g. StackOverflowError is not an Exception, but if we have a recursive stack overflow somewhere (e.g. the extractor) we still want to handle it nicely instead of crashing the app.
|
Was closing this intentional? |
|
This pointed to the release branch which was deleted and thus the PR closed automatically. This should be reopened targeting |
|
@Stypox |
3 participants
What is it?
Description of the changes in your PR
This PR deduplicates the decision of which error message to show for which throwable (it was done in 3 different places previously). It also adds some nice new error messages for specific errors, so users understand better what is going on (and maybe complain less when YouTube breaks something ;-) ).
RouterActivity, a toast is shown like before if the error is not something that usually needs to be reported, as to not distract the user with a notification.ContentNotAvailableExceptionnow have nice specific error messages, and in particular this PR addsUnsupportedContentInCountryException(from @AudricV's new Youtube trending implementation) andYoutubeSignInConfirmNotBotException( [YouTube] Add custom error for "Sign in to confirm ..." NewPipeExtractor#1352). The latter in particular will hopefully help reduce the amount of duplicate comments in [YouTube] "Sign in to confirm..."/"Watch on the latest version..." #11139.Relies on the following changes
APK testing
The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR. You can find more info and a video demonstration on this wiki page.
Debug APK from CI: app(1).zip
Due diligence