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

Fix download component behavior when network connection is cut. #167

Merged
merged 1 commit into from Nov 8, 2021

Conversation

dero
Copy link
Collaborator

@dero dero commented Oct 7, 2021

Summary

Fixes #126

Currently when network connection is lost while a video is being downloaded, the downloader component gets stuck and doesn't either explicitly pause the download or wait for the connection to come back up and resume automatically.

To address this, this PR makes sure that when there is a connection related error encountered by the DownloadManager, the current downloads are paused automatically and the UI of the downloader component changes from the "downloading" state to "paused". At the same time a console warning is generated. User can then manually resume the affected downloads once connection is restored.

While this is likely not a perfect solution, it may be good enough or may serve as a first step towards a more robust solution. Please let me know your thoughts, @derekherman and / or @beaufortfrancois.

Screencast of the updated behavior

web-dev-download-interrupted-warning.mp4

@dero dero requested a review from derekherman October 7, 2021 16:05
@dero dero self-assigned this Oct 7, 2021
@beaufortfrancois
Copy link
Member

This PR definitely improves the current state of downloading in kino. Thank you @dero!

I was surprised to notice background fetch was not used there. If background fetch is there, kino should use it, otherwise it should fallback to "regular" fetch. Is there a feature request for this yet? @rkochman FYI

@dero
Copy link
Collaborator Author

dero commented Oct 11, 2021

Background fetch was originally intended to be implemented during phase 2 (this phase), see #25, but I'm not sure if that still is the plan.

@rkochman
Copy link
Collaborator

Not including Background Fetch was an oversight on my part. We should try to get it in Phase 2.

Copy link
Collaborator

@derekherman derekherman left a comment

Choose a reason for hiding this comment

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

This works to address the original issue and can be merged. Additionally, we have now prioritized adding background fetch to this phase—we can do that in a new PR.

@derekherman derekherman merged commit c4e1216 into develop Nov 8, 2021
@derekherman derekherman deleted the fix/download-interrupted branch November 8, 2021 02:39
@felipebochehin87
Copy link

Bug found: #176

@felipebochehin87
Copy link

Bug found: #183

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants