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

feat: Copy free upload #449

Open
wants to merge 38 commits into
base: main
Choose a base branch
from
Open

feat: Copy free upload #449

wants to merge 38 commits into from

Conversation

LouisCAD
Copy link
Contributor

@LouisCAD LouisCAD commented Mar 3, 2025

Now, the app starts a foreground service as soon as user-picked files arrive in the app.

That way, we can keep the URI permissions until the upload is complete, without having to import/copying the data into the app storage.

That lets us avoid filling the user device, and avoids no free storage issues.

Change operations:

  1. Replace DB-backed upload session with in-memory UploadState
  2. Replace UploadWorker with UploadForegroundService
  3. Replace UploadFileTask with TransferUploader
  4. Replace TransferSendManager with UploadSessionStarter
  5. Replace ImportFilesScreen with PickFilesScreen
  6. Replace ImportFilesViewModel with PickFilesViewModel
  7. Merge all upload related screen (progress, errors, success, email validation…)
  8. Simplify navigation
  9. Remove ImportationFilesManager
  10. Remove ImportLocalStorage
  11. Ensure the old importation directory is removed if present, freeing storage
  12. Extract a few classes into their own file
  13. Update NotificationUtils code for naming semantics

Depends on Infomaniak/android-core#305

@LouisCAD LouisCAD force-pushed the copy-free-upload branch 5 times, most recently from 02ed398 to 69bf195 Compare March 3, 2025 16:33
@LouisCAD LouisCAD changed the title WIP: Copy free upload feat: Copy free upload Mar 3, 2025
@LouisCAD LouisCAD force-pushed the copy-free-upload branch 2 times, most recently from 3648621 to 3fcd200 Compare March 4, 2025 09:51
@LouisCAD LouisCAD force-pushed the copy-free-upload branch 2 times, most recently from 546e766 to 9abebf2 Compare March 4, 2025 14:04
@LouisCAD LouisCAD marked this pull request as ready for review March 4, 2025 14:31
@github-actions github-actions bot removed the dependent label Mar 5, 2025
Copy link

github-actions bot commented Mar 5, 2025

@LouisCAD
Copy link
Contributor Author

LouisCAD commented Mar 5, 2025

Things to do in this PR, or after:

  • Thumbnails:
  • Implement upload cancellation with a worker
    • Think through retries backoff and hard limit for giving-up.
  • Change notification wording from "draft" to "pending"
  • Check BackHandlers of UploadScreen and nested composables don't clash
  • Think through the canRetry in the notification, we might want a more precise error message (reuse the one shown in the app?)
  • Check the notification message stays consistent across updates

Things to do definitely after:

  • Extract stuff from UploadForegroundService's companion object.

For the android-core repo:

  • Move ForegroundService into its own module and add the android.permission.FOREGROUND_SERVICE for older Android versions.
  • Add @RequiresPermission for withPartialWakelock

@LouisCAD LouisCAD force-pushed the copy-free-upload branch 4 times, most recently from b754c35 to 11b101d Compare March 5, 2025 14:59
LouisCAD added 8 commits March 5, 2025 17:39
Now, the app starts a foreground service as soon as user-picked files
arrive in the app.
That way, we can keep the URI permissions until the upload
is complete, without having to import/copying the data
into the app storage. That lets us avoid filling the user device,
and avoids no free storage issues.

Change operations:

 1. Replace DB-backed upload session with in-memory UploadState
 2. Replace UploadWorker with UploadForegroundService
 3. Replace UploadFileTask with TransferUploader
 4. Replace TransferSendManager with UploadSessionStarter
 5. Replace ImportFilesScreen with PickFilesScreen
 6. Replace ImportFilesViewModel with PickFilesViewModel
 7. Merge all upload related screen (progress, errors, success, email validation…)
 8. Simplify navigation
 9. Remove ImportationFilesManager
10. Remove ImportLocalStorage
11. Ensure the old importation directory is removed if present, freeing storage
12. Extract a few classes into their own file
13. Update NotificationUtils code for naming semantics
14. Move files from the importfiles package to the pickfiles package
LouisCAD added 4 commits March 5, 2025 17:44
The back buttons/gestures are enough and more standard.
The close button previously removed picked files after
a popup confirmation, which requires the same amount of steps
as going back twice.
Copy link
Member

@sirambd sirambd left a comment

Choose a reason for hiding this comment

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

Don't forget to reformat all files, and group properties at the top of files

)
UploadState.Failure.RestrictedLocation -> UploadFailureScreen(
exitNewTransfer = cancel,
desc = "SwissTransfer doesn't work here yet"
Copy link
Member

Choose a reason for hiding this comment

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

Add a translated string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, reused the one shown in the bottom sheet. That said, I think we should think through this redundancy.

If we show a bottom sheet, what are supposed to show in the screen below?
If we show the error below, do we need a bottom sheet in the first place?

Also, do we allow retrying somehow, in case the user leaves a restricted location?

notification.post(Ids.LastUpload)
}

fun buildUploadFailedNotification(canRetry: Boolean): Notification {
Copy link
Member

Choose a reason for hiding this comment

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

Check this canRetry because it's always true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted in my comment above. Maybe check that after this PR?

@LouisCAD LouisCAD requested a review from sirambd March 6, 2025 13:42
@LouisCAD
Copy link
Contributor Author

LouisCAD commented Mar 6, 2025

I did my best to address everything in new commits (without touching the previous ones since your review, @sirambd).

I hope it's good to go, and that we can do what I wrote down in the updated comment above after.

@LouisCAD
Copy link
Contributor Author

LouisCAD commented Mar 6, 2025

BTW, about notifications, we need to allow the upload to start even if the permission is denied.

Foreground services can still run without that, so we don't absolutely need it to proceed with the transfer upload.

We probably want an alternative to guardedCallback { …} that gets called whatever the permission grant result is (but still after we receive the result). Maybe a name like a requestingCallback { … } extension for the permission state.

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

Successfully merging this pull request may close these issues.

2 participants