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

[New Arch] Download a single file #2872

Closed
17 of 24 tasks
jesmrec opened this issue Apr 30, 2020 · 7 comments · Fixed by #3169
Closed
17 of 24 tasks

[New Arch] Download a single file #2872

jesmrec opened this issue Apr 30, 2020 · 7 comments · Fixed by #3169

Comments

@jesmrec
Copy link
Collaborator

jesmrec commented Apr 30, 2020

Synchronization epic: #2818

Postponed for the moment. To do in upcoming PRs.

  • Group notifications with progress. We need to decide how to show them when several downloads are performed parallelly. This will have the same behavior for uploads. To be done together.
  • No information will be shown in the adapters due to performance issues. (Icon downloading in the file list)
  • Tests will be included in upcoming PR because several things will change till the final version.

AC:

  • By tapping a file in the list, a copy of the remote file is downloaded and stored in the device.
  • If the file is currently downloaded, check before if new version exists in the server.
  • Notifications: many notifications can happen at the time if many files are downloaded. They can be grouped from 3 different (pending to check)
  • Progress bar to be updated during the action with the %. Also visible in the notification.
  • Once the file finishes to download, notification is gone.
  • Errors in download shown in notification
  • File is previewed after being downloaded

Errors:

  • No connectivity
  • Server unreachable
  • Error servers (file was remotely deleted, moved, maintenance mode)

TASKS

  • Research which is the best way to do it
  • Create branch new_sync/download_file
  • Development tasks
    • Migrate remote operation to kotlin
    • Create a file provider in data module to handle file storage [Not needed for downloads]
    • Integrate workmanager to schedule downloads
    • Create a one time worker to download a file, cancelable, and listenable
    • Integrate this task with the new architecture
    • Show notifications with download progress
    • Use new worker to download and send files
    • Use new worker to download and show files in documents provider
    • Observe worker in the viewpager adapter
    • Use new worker to download and show images
    • Use new worker to download and open with
    • Use new worker and show something downloading in adapters
    • Show a custom message in notification
    • Create a transfer provider to handle transfers. Similar to TransferRequester but with new Arch
    • Clean old downloader stuff
    • Error handling
    • Unit tests
  • Code review and apply changes requested
  • Design test plan
  • QA
  • Merge branch new_sync/download_file into new_arch/sync

PR

@abelgardep
Copy link
Contributor

We will stop this issue for the moment. It is needed to complete #2955 first.

@abelgardep
Copy link
Contributor

I have some questions about how downloads work now and how they should work after this synchronization refactor.

Let's start showing the current behavior.

Downloads are stored in the shared storage. There, any application can access them. There is no restriction about that, files are "public" once we download them to the device. Similar to Telegram, WhatsApp, and a lot of other apps. If we uninstall the app, those downloaded files will be persisted, public for any app that has access to the shared storage. We can find more information about this here.

At the moment, we download files when previewing them (except videos, that are streamed), making them Av. Offline, or selecting them and download.

Downloads are performed sequentially. This means that we download files one by one.

Every download shares the notification. This means that we show a notification with the download progress, and then we show a notification when the download is finished.

When we download just one file, there is no problem with this behavior. We show a notification with the progress, and when it finishes the notification progress is replaced with the finished notification. But, when we download several files, the notification is replaced for each download and it may cause them to show and hide a notification too fast that they are not properly shown.

One Several 
downloads_one downlaods_seeveral

Let's discuss the expected behavior

Related to scoped storage #2877. And taking this guide as a reference, we have several topics to discuss. We could consider ownCloud files as app-specific files and we will make them more secure, other apps could access them using the SAF that we support via our DocumentsProvider but it will be more restricted. This will make our files more secure, and those files will be removed from the device if the app is uninstalled. But we need to take care of the space we use since it is limited

This way, we could differentiate at least some types of downloads:

  1. Previews: They could be downloaded to the app-specific storage and maybe to the cached one. This way we could free some space when needed
  2. Av. Offline: They could be downloaded to the app-specific storage and it may have sense to store them in the persisted one
  3. Select and download: This behavior could be changed and make it download to the downloads folder if the user considers it important to persist it.

In any case, if we decide to keep the previous behavior or to change it, downloads are not performed sequentially anymore. Several files can be downloaded at the same time. And there are several ways to notify the user about this:

For previews, it may have no sense to show a notification if we are showing the progress in the app because the user expects it to be opened as soon as the download finishes.

For 'Select and download' I think that we should keep showing the download notification because the user does not expect a specific action when it finishes downloading.

If we think that it is important to show the download progress, we could try to show a summary of the current downloads, for example:

15 files are being downloaded
50Mb pending

@jesmrec
Copy link
Collaborator Author

jesmrec commented Apr 7, 2021

But we need to take care of the space we use since it is limited

it's a bad new... even worse if they don't tell you how limited it is.

This way, we could differentiate at least some types of downloads:
Previews: They could be downloaded to the app-specific storage and maybe to the cached one. This way we could free
some space when needed
Av. Offline: They could be downloaded to the app-specific storage and it may have sense to store them in the persisted one
Select and download: This behavior could be changed and make it download to the downloads folder if the user considers it > important to persist it.

Previews: i like the idea they can be removed from cache, manually, or automatically (feasible as future improvement? iOS app implements this)

Av. Offline: They must be persisted. I think there is no discussion

Select and download: Would it make sense to get rid of this option? i mean, with Previews you can open and check a file by clicking on it. If this file is important for you, you will make it av. offline and will be always stored and synced in your device. Downloading a file and keep it in the device in a non-synced way does not offer advantages (doesn't it?). Indeed, we have to take care of the space limit. What do you think??

For previews, it may have no sense to show a notification if we are showing the progress in the app because the user expects it to be opened as soon as the download finishes.

Agreed, previews are expected to that moment, with the app in foreground and the user watching the screen. Nothing to notify.

For 'Select and download' I think that we should keep showing the download notification because the user does not expect a specific action when it finishes downloading.

Yes, we can apply this to the av. offline as well

If we think that it is important to show the download progress, we could try to show a summary of the current downloads, for example:

this idea is good for me. Will the summary be always displayed? even when you have 1 download. In case the user wants to download a big amount, he will want to be notified when the whole thing is done. Notifying after every single download will probably bother more than helping

Thanks for sharing your thoughts!! i hope the discussions go on!

@jesmrec
Copy link
Collaborator Author

jesmrec commented Apr 9, 2021

Finally, the implementation of the current issue will keep the behaviour. This is:

  • There will not be concurrency ftm. Downloads will be done one-by-one
  • Therefore, notifications will be shown one-by-one as well
  • Code will be prepared for future changes, in case concurrency is willing to be enabled (currently, with performance problems in low-range devices).

@abelgardep
Copy link
Contributor

Taking into account four scenarios for downloads: Previews, Av. Offline, Select and download, and Documents Provider stuff, we could discuss notifications behavior.

  • Previews: I think that notification progress is not needed at all. We will see the progress in the UI. I consider that success notification is not needed either because after a successful download it will show the preview and that is enough for the user. A credentials error notification would have sense to keep in order to access LoginActivity and relogin after clicking the error notification.
  • Av. Offline: We will discuss this in upcoming issues because Av .Offline needs to be refactored and adapted to new arch.
  • Select and download: I would keep the notifications as they are, taking into account that this feature may disappear in the future.
  • Documents Provider: I would keep the notification because we can not show the progress within the documents provider and it is important to show that we are downloading the file to the user.

@abelgardep
Copy link
Contributor

BTW, showing that we are currently downloading a file in the file list lags the device at the moment. It performs a lot of queries to WorkManager and it makes it lag old devices. This will change in the future when we switch to RecyclerView, use Flow to retrieve the files and use the Database as a unique source of truth.

@jesmrec
Copy link
Collaborator Author

jesmrec commented Apr 12, 2021

Previews: I think that notification progress is not needed at all. We will see the progress in the UI. I consider that success notification is not needed either because after a successful download it will show the preview and that is enough for the user. A credentials error notification would have sense to keep in order to access LoginActivity and relogin after clicking the error notification.

i agree. What happened if, for example:

  1. I click on a big file to download it
  2. Then i leave the app before it finishes because it takes a long time and i want to check my mailbox in the meanwhile

Shouldn't we notify the user when the download finishes?

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

Successfully merging a pull request may close this issue.

2 participants