-
Notifications
You must be signed in to change notification settings - Fork 1
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
Backup : create folder structure for Android, fix various bugs #873
Conversation
zatteo
commented
Jul 13, 2023
•
edited
Loading
edited
- Create same folder structure for Android as on device
- Stopping backup return ready state so that we can see number of medias remaining to backup
- Fix various bugs
58806c2
to
5cb0fe1
Compare
}) | ||
.indexFields(['dir_id', 'type']) | ||
.select(['name', 'dir_id', 'type']) | ||
.indexFields(['type', 'metadata.backupDeviceIds']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should invert the order. Always the most restrictive before. I think this is documented somewhere here https://docs.cozy.io/en/tutorials/data/advanced/#indexes-performances-and-design
client, | ||
localBackupConfig, | ||
mediaToUpload | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An async call inside a loop, I don't like it.
nit: Also this method should be rename to something like getOrCreateFolder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I keep this review for a next PR about optimization because I want to create a new working beta today.
case 'mp4': | ||
return 'video/mpeg' | ||
default: | ||
return `video/${extension}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure to want that? Having video/ogx will be weird (instead of video/ogg). Do we really need to pass the extension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does Drive handle that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I add 'mime/lite' to have the best mime guessing. It is 2.5KB. It will be possible to remove it one day if cameraroll return mime type for iOS.
Package is also used by Drive for some purposes.
createdAt | ||
createdAt + | ||
'&MetadataID=' + | ||
metadataId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const toURL = new URL(client.getStackClient().uri)
toURL.pathname = '/files/' + backupFolderId
toURL.searchParams.append('Name', media.name)
// etc
@@ -82,3 +94,7 @@ export const uploadMedia = async ( | |||
}) | |||
}) | |||
} | |||
|
|||
export const cancelUpload = (uploadId: string): Promise<void> => { | |||
return RNFileSystem.stopUpload(parseInt(uploadId)) as unknown as Promise<void> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
184ea52
to
0921eca
Compare
We decided to follow Android folder structure. So if a media is in Download, it will be uploaded in a Download folder (always inside the backup folder). I added a new attribute for media, remotePath, corresponding to this value (like Download in my previous example). I removed temporarly the .select in buildFilesQuery because I did not succeed in getting the path with the .select.
So when the user stop a backup, he sees what is remaining to backup.
Fix wrong thumbnails and medias not appearing in Photos
If it is a normal video, we should return the filepath without any modification like for a photo.
We chose to add this id in metadata because relationships are not possible on files and referenced_by can not remember why there is a reference (and we want to know that is is because it is a Live Photo). For performance reason, this feature depends on medias to upload order. So when there is a Live Photo, the photo part must immediately follow the video part. Added tests to check that.
Previously, every time we deleted local storage and restore a backup, we created new albums. Now, we save in the album the backupDeviceId and restore albums same as we restore medias.
Nothing was uploaded because video part path was not good.
I add 'mime/lite' to have the best mime guessing. It is 2.5KB. It will be possible to remove it one day if cameraroll return mime type for iOS.
0921eca
to
bbc52b3
Compare