-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Error handling in subscribe() in DownloadDialog #6118
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for fixing this issue!
With your implementation NewPipe is going to ignore the error which makes it hard to fix the, / find the underlying cause. For this reason, I suggest to do something like this to allow users reporting the problem:
throwable -> ErrorActivity.reportErrorInSnackbar(context,
new ErrorInfo(throwable, UserAction.DOWNLOAD_GET_SIZE,
"Downloading video stream size", currentInfo.getServiceId()))));
Replace "Downloading video stream size" with the proper description for each error.I think showing the snackbar should be ok. I did not test this, so there might be issues with that. E.g. it could be hidden by the download dialog overlay and thus be not accessible. If this is the case, we need to find another solution.
The change above requires
DOWNLOAD_STREAM_SIZE("download stream size")
or something similar to be added to org.schabi.newpipe.error.UserAction
As @TobiGr mentioned "ErroSanckbar could be hidden by the download dialog overlay and thus be not accessible. If this is the case, we need to find another solution." so it must be tested before merging. |
I am not able to reproduce the crash, but I think the snackbar will not be hidden and if it is, the user can still try to send the report by closing the download dialog and using the report button. If we do not get a response in the original issue, I suggest to merge this PR by the end of next week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I forgot about this PR
No worries :) @TobiGr |
What is it?
Description of the changes in your PR
Fixes the following issue(s)
Due diligence