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

Error handling in subscribe() in DownloadDialog #6118

Merged
merged 2 commits into from
May 29, 2021

Conversation

sauravrao637
Copy link
Contributor

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

  • The result output in subscribe may throw error and thus need to be handled, so I did it :)

Fixes the following issue(s)

Due diligence

Copy link
Member

@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.

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

@TobiGr TobiGr added bug Issue is related to a bug downloader Issue is related to the downloader labels Apr 20, 2021
@sauravrao637
Copy link
Contributor Author

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.
The bug couldn't be reproduced as no case link was provided with the bug report .

@TobiGr
Copy link
Member

TobiGr commented May 4, 2021

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.

Copy link
Member

@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.

Sorry, I forgot about this PR

@TobiGr TobiGr merged commit 62b593d into TeamNewPipe:dev May 29, 2021
@sauravrao637
Copy link
Contributor Author

No worries :) @TobiGr

This was referenced Jun 5, 2021
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 downloader Issue is related to the downloader
Projects
None yet
Development

Successfully merging this pull request may close these issues.

App crashes when i download in Pop-up mode
2 participants