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: Handle copy of folders containing live photos #49293

Merged
merged 5 commits into from
Dec 4, 2024

Conversation

artonge
Copy link
Contributor

@artonge artonge commented Nov 14, 2024

We need to recursively look for live photos in the folder, and then handle them as usual.

Fix #49289
Fix #49307

@kesselb
Copy link
Contributor

kesselb commented Nov 15, 2024

getDirectoryListing may return a listing like

  • a.jpg
  • a.mov
  • b.jpg
  • b.mov

Loop:

  1. handleCopy for a.jpg will copy a.mov too.
  2. handleCopy for a.mov will then copy a.jpg again.

It would be nice to skip the copy process for a.mov and a.jpg for the second execution if that's possible without making the whole logic much more complex.

@artonge artonge force-pushed the artonge/fix/handle_folders_copy_live_photos branch 5 times, most recently from 48eee31 to edf6450 Compare November 20, 2024 14:27
@artonge artonge changed the base branch from master to artonge/chore/update_nc_cypress_beta.11 November 20, 2024 14:27
@artonge artonge force-pushed the artonge/fix/handle_folders_copy_live_photos branch 2 times, most recently from 5de0228 to c74b1ba Compare November 20, 2024 16:38
@artonge artonge force-pushed the artonge/fix/handle_folders_copy_live_photos branch from c74b1ba to 892349f Compare November 21, 2024 16:54
@kesselb

This comment was marked as resolved.

@kesselb

This comment was marked as resolved.

@kesselb
Copy link
Contributor

kesselb commented Nov 21, 2024

Test cases:

Scenario Expected Result (2024-11-21) Result (2024-12-04)
Copy live photo "a.jpg" to folder where "a.mov" exists Copy not possible
Copy live photo "a.jpg" to folder where "a.mov" exists (batch action) Copy not possible
Move live photo "a.jpg" to folder where "a.mov" exists Move not possible
Move live photo "a.jpg" to folder where "a.mov" exists (batch action) Move not possible
Copy a folder with live photos (same storage) Copy possible
Copy a folder with live photos (same storage, batch action) Copy possible
Copy a folder with live photos (external local to primary s3) Copy possible
Copy a folder with live photos (external local to primary s3, batch action) Copy possible
Move a folder with live photos (same storage) Live photos work after move
Move a folder with live photos (same storage, batch action) Live photos work after move
Move a folder with live photos (external local to primary s3) Live photos work after move
Move a folder with live photos (external local to primary s3, batch action) Live photos work after move

@artonge artonge force-pushed the artonge/chore/update_nc_cypress_beta.11 branch 5 times, most recently from 5966751 to ebda7b4 Compare November 25, 2024 15:22
@artonge artonge force-pushed the artonge/fix/handle_folders_copy_live_photos branch from 892349f to 5e692da Compare November 25, 2024 16:52
Base automatically changed from artonge/chore/update_nc_cypress_beta.11 to master November 26, 2024 09:17
@artonge artonge force-pushed the artonge/fix/handle_folders_copy_live_photos branch 3 times, most recently from 43ed165 to 5b01166 Compare November 28, 2024 16:35
@artonge artonge force-pushed the artonge/fix/handle_folders_copy_live_photos branch 2 times, most recently from 3568df4 to bafc1e5 Compare December 2, 2024 13:03
@artonge artonge requested a review from kesselb December 2, 2024 13:27
@artonge
Copy link
Contributor Author

artonge commented Dec 2, 2024

/backport to stable30

@kesselb

This comment was marked as resolved.

@kesselb

This comment was marked as resolved.

@kesselb

This comment was marked as resolved.

@kesselb
Copy link
Contributor

kesselb commented Dec 4, 2024

I've tested "Copying a folder with Live Photos" using desktop client 3.15 (no vfs) with bulkupload.enabled true and false: The metadata is not copied, the live photo state is lost.

bulkupload.enabled requests
true mkcol + post for the bulk entpoint
false mkcol + put for each file in the folder

Signed-off-by: Louis Chemineau <louis@chmn.me>
We need to recursively look for live photos in the folder,
and then handle them as usual.

Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Louis Chemineau <louis@chmn.me>
@artonge
Copy link
Contributor Author

artonge commented Dec 4, 2024

@kesselb I was not able to reproduce all of your failed scenario. Can you make sure that you pulled the latest version of the PR?

Regarding the Desktop client behavior, please see with a desktop engineer.

@kesselb

This comment was marked as resolved.

@kesselb

This comment was marked as outdated.

@kesselb

This comment was marked as outdated.

@kesselb

This comment was marked as resolved.

Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Louis Chemineau <louis@chmn.me>
@artonge artonge force-pushed the artonge/fix/handle_folders_copy_live_photos branch from bafc1e5 to 8be6a7c Compare December 4, 2024 15:23
@kesselb
Copy link
Contributor

kesselb commented Dec 4, 2024

The peer id for a.jpg was wrong, and therefore the copy operations failed.
It now works as expected.


The situation with Nautilus is not ideal. Copying a folder sends a mkcol request and then a copy request for each file. The user is then asked if a.mov should be replaced because a.mov was already copied with a.jpg. Similar case for deleting a folder with live photos, a delete request is sent for each child and Nautilus show's a warning that a.mov is not found.

@artonge artonge merged commit 1ef3e3e into master Dec 4, 2024
188 checks passed
@artonge artonge deleted the artonge/fix/handle_folders_copy_live_photos branch December 4, 2024 16:02
@skjnldsv skjnldsv mentioned this pull request Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants