Fetch albums in folders for iCloud Photos #297
Conversation
noizwaves
changed the title
pyicloud/services/photos.py
Outdated
| @@ -474,7 +514,8 @@ def _list_query_gen(self, offset, list_type, direction, query_filter=None): | |||
| return query | |||
|
|
|||
| def __unicode__(self): | |||
| return self.title | |||
| names = self.lineage + [self.title] | |||
| return '/'.join(names) | |||
There was a problem hiding this comment.
I do not believe, this is a good logic here. In iCloud Photos, a folder name can contain a "/".
So either you need to escape the / character in foldernames (and possible also Album and Filenames) or you skip the whole thing.
Considering that none of the current services (ubuiquity, files and photos) support Unix-Styled foldernames/paths but instead do it via chaining of list items I'm thinking that maybe dropping this kind of feature completely is not the worst idea.
But let#s see what @Quentame thinks...
There was a problem hiding this comment.
I don't feel strongly either that this change adds value. It was added initially to aid debugging/development, but ended up being unused for my use case. I've removed it to make the PR smaller & simpler.
|
@noizwaves I saw the changes. I liked them. Thanks for that. But yes, @Quentame has to review. I got no commit rights on this repo. And he's on vacation for a few more days. So patience. 😁 |
pyicloud/services/photos.py
Outdated
| for sub_folder in self._fetch_sub_folders(folder_id): | ||
| if sub_folder["recordName"] == "----Root-Folder----" or ( | ||
| sub_folder["fields"].get("isDeleted") | ||
| and sub_folder["fields"]["isDeleted"]["value"] |
There was a problem hiding this comment.
Why checking the value here ?
Should we fetch sub folders if the parent is deleted ?
There was a problem hiding this comment.
Great point; this logic exists already at https://github.com/picklepete/pyicloud/pull/297/files/2e4d3eb62ffc3b36bf3d30e60ac526f48cd366c4#diff-dbc5e8af14fd471636e609e5ecd1fe3bR178. I'll take this out and see what happens.
There was a problem hiding this comment.
I'm inclined to leave this logic in, in other words, I'm included to not fetch sub folders / albums for deleted parent folders.
In the iCloud GUI, if you delete a folder that contains sub folders, it will delete both the folder and the sub-folder. I think it may be confusing/inconsistent if some deleted folders are traversed when looking for albums, as the results generated by pyicloud will be different than what is visible in the UI. This also remains aligned with the existing code where deleted albums were not returned.
I could see it being valuable to allow the user to choose how deleted folders/abums should be treated when fetching albums in a future PR that extends this functionality.
There was a problem hiding this comment.
OK but I think we should add a quick comment to explain how it is.
There was a problem hiding this comment.
Also we can extract the code here in a little private function as, as you pointed out, the logic exists already at L178
There was a problem hiding this comment.
@Quentame I've implemented both of your suggestions, thank you for making them. Sorry for the slow delay in implementing them!
I think all of the feedback has been addressed and this can be merged. Please let me know if you have other feedback.
Co-authored-by: Quentame <[email protected]>
|
Hello @Quentame, what is the the status on this PR? |
@noizwaves and @Quentame, is there still something that needs to be done for this PR or do you need help on anything? |
|
Hi, can this be pulled please? This is a crucial feature for a project I'm working on. Thanks! |
Proposed change
This PR enhances the PhotosService to traverse sub-folders when it retrieves albums. The initial implementation only shallowly iterated over items in the root folder. The code has been changed to first determine if an item is actually a folder, and if so to query iCloud for any albums inside of it.
The ancestry or "lineage" of an album is also tracked in a new field
PhotoAlbum.lineage; the names of each parent directory are included in this list. The definition ofPhotoAlbum.__unicode__has been enhanced to include this information in a path-esque way. The aim is to better represent the actual "path" within an iCloud Library.Type of change
Example of code:
Additionally, I used a local build of this package in this script to download the details of all of my albums. My library consists of albums at the root level, albums in a single folder, and albums nested within multiple levels of folders.
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: