search

icloud-photos-downloader / icloud_photos_downloader

A command-line tool to download photos from iCloud
MIT License
6.88k stars 553 forks source link

MOV files of Live Photos are not deleted #515

Open l4t3b0 opened 1 year ago

l4t3b0 commented 1 year ago

Overview

Im using the command line option --auto-delete to delete files in the recently deleted folder. The image is a Live Photo, so it consists 2 files. The HEIC file and the corresponding _HEVC.MOV file. The HEIC gets deleted, but the MOV does not.

Steps to Reproduce

  1. take a live photo
  2. sync it to the computer. HEIC and _HEVC.MOV files are synced.
  3. delete the photo on the iPhone
  4. sync it again, wit the auto-delete option

Expected Behavior

Not only the HEIC file should be deleted after the 4th step, but the corresponding _HEVC.MOV file also

Actual Behavior

HEIC is deleted, but the _HEVC,MOV file does not.

Context

interpretor commented 1 year ago

@AndreyNikiforov Any updates on this? That's breaking core functionality. Would it be enough to also scan for the additional *_HEVC.MOV in icloudpd/autodelete.py?

AndreyNikiforov commented 1 year ago

That's breaking core functionality

What version broke that functionality?

interpretor commented 1 year ago

I can't say if or when it worked before, as I am a new user of icloudpd. I didn't test older versions than 1.8.1.

AndreyNikiforov commented 1 year ago

I can't say if or when it worked before, as I am a new user of icloudpd. I didn't test older versions than 1.8.1.

I'll assume that it is missing functionality, rather than broken one until proven otherwise.

interpretor commented 1 year ago

I think you're right, it seems it never worked. In test_autodelete_photos.py there is also no testcase for autodeleting live photos.

Implementing this should be really simple, as there is just another check for os.path.exists() needed, where the path is media with the additional _HEVC.MOV in autodelete.py, and then also delete that file, or am I missing something? Probably a new function like live_photo_filename(media) in paths.py would be helpful.

AndreyNikiforov commented 1 year ago

Implementing this should be really simple, as there is just another check for os.path.exists() needed, where the path is media with the additional _HEVC.MOV in autodelete.py, and then also delete that file, or am I missing something? Probably a new function like live_photo_filename(media) in paths.py would be helpful.

I looked at the code a number of weeks back and it seemed to me that to solve this problem, one needs to implement reverse de-duping. To complete that reversal, I envisioned that it would be better to refactor file naming logic into separate function first. Since that seemed reasonably big chunk of work, I decided to tackle smaller changes first to refresh my memory of the codebase, while keeping code separation as a sub-goal.

If you want to contribute even with a small improvement, you are welcome to do so. Please keep tests updated, so when more changes come, we are protected from regressions.

ddavtian commented 1 year ago

+1 on this, would really love to see this implemented when possible. Right now some local assets are being removed and some are not.

interpretor commented 1 year ago

@AndreyNikiforov any updates on this? I could write a simple fix, but I'm having problems getting proper test data.

AndreyNikiforov commented 1 year ago

@AndreyNikiforov any updates on this? I could write a simple fix, but I'm having problems getting proper test data.

No progress. Updating existing and writing new tests that require setting proper state in the icloud account are main blockers for any major refactoring for anybody, including myself. I am looking for ways to decouple logic from icloud interface, so they can be tested separately, but not successful yet.

interpretor commented 1 year ago

Hi @AndreyNikiforov, I am using now a quick and dirty solution to also auto delete live photos. I only added a function to paths.py and added the logic to delete live photos in autodelete.py, so that merging from upstream will unlikely conflict with my changes. I could make a pull request, but I don't have any unit tests for that functionality, because I am not able to get proper test data.

syl779 commented 1 year ago

Could any created .jpg files be deleted too?

AndreyNikiforov commented 1 year ago

Could any created .jpg files be deleted too?

@syl779 what is the use case where jpg files are not deleted? IIUC jpg can only be a main file and there are no known problems with deleting main file. This issue is about deleting secondary file for the asset.

syl779 commented 1 year ago

I mean I take a photo, it gets downloaded in .heic format, the .mov file for the liveview also gets downloaded and a .jpg is created (convert_heic_to_jpeg).

Then I delete the photo on my phone. The .heic file gets deleted next time icloudpd runs but the .mov and .jpg files remain.

AndreyNikiforov commented 1 year ago

I mean I take a photo, it gets downloaded in .heic format, the .mov file for the liveview also gets downloaded and a .jpg is created (convert_heic_to_jpeg).

What container are you using? Heic to jpg conversion is not provided by icloudpd container in this project.

boredazfcuk commented 1 year ago

If it's my container, the function that's being requested already exists, and is detailed in the documentation.

bowencool commented 7 months ago

Any news?

AndreyNikiforov commented 5 months ago

Seems to be working fine in latest version (1.19.0) #863