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 file upload progress #985

Merged
merged 9 commits into from
Nov 25, 2024

Conversation

makeevrserg
Copy link
Collaborator

Background
This PR aims to improve new file manager uploading reactivity and fix ui for multiple files

Changes

  • Remove upload screen and replace it with component
  • Add callbacks for uploaded file progreess - will now be updated in items list
  • Fix multiple files upload display

Test plan

  • Try upload one file fully and see it will appear in list instantly
  • Try upload one file but cancel it in progress. See it also appeared, but with non-full upload size
  • Try upload multiple files

@makeevrserg makeevrserg requested a review from LionZXY as a code owner November 8, 2024 10:47
LionZXY
LionZXY previously approved these changes Nov 12, 2024
@@ -55,10 +57,21 @@ class FilesDecomposeComponentImpl @AssistedInject constructor(
childFactory = { bottomSheetFile, _ -> bottomSheetFile }
)

private val selectionViewModel = instanceKeeper.getOrCreate(path.toString()) {
private val selectionViewModel = instanceKeeper.getOrCreate("selectionViewModel_$path") {
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it's necessary? As far as I remember the viewmodel class name is already used for the unique key. Can you explain why it is necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

filesViewModel and selectionViewModel previously were using same key "$path" before it crashed. The crash was becaseu intanceKeeper got class by key "$path" and the class was filesViewModel. But should be selectionViewModel. So the instance keeper compare only key, not class itself. Due to this implementation classes with "same key" needs to add unique prefix.

The reason why this is required inside InstanceKeeper impl:

check(key !in map) { "Another instance is already associated with the key: $key" }

Copy link
Member

Choose a reason for hiding this comment

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

So the instance keeper compare only key, not class itself.

That's weird, I've been re-doing that part - let's discuss in a PM after your vacation

CHANGELOG.md Outdated
@@ -70,6 +70,7 @@ Attention: don't forget to add the flag for F-Droid before release
- [FIX] Add deeplink fallback for flipper scheme uri
- [FIX] Change description of remotes library card
- [FIX] Fix bytes race in new api
- [FIX] New file manager uploading progress
Copy link
Member

Choose a reason for hiding this comment

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

Also, please, change changelog

@makeevrserg makeevrserg added this pull request to the merge queue Nov 25, 2024
Merged via the queue into flipperdevices:dev with commit b5f3df1 Nov 25, 2024
11 checks passed
@makeevrserg makeevrserg deleted the fix-file-upload-progress branch November 25, 2024 11:02
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