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

googlephotos: Use encoder for album names #5535

Merged
merged 1 commit into from Aug 19, 2021

Conversation

timevortex
Copy link
Contributor

What is the purpose of this change?

This fixes the problem of album names containing \n which caused problems as local directories could not be created during sync.

Was the change discussed in an issue or in the forum before?

Yes: https://forum.rclone.org/t/google-photos-sync-failed-for-multi-lined-album-names/25589

Checklist

  • I have read the contribution guidelines.
  • I have added tests for all changes in this PR if appropriate.
  • I have added documentation for the changes if appropriate.
  • All commit messages are in house style.
  • I'm done, this Pull Request is ready for review :-)

Manual Testing

Please note the diff is intentionally redacted - it show cases the two primary differences in album names due to this change.

$ rclone_new lsf -Fp --csv gphotos:shared-album &> rclone_new_2021-08-15_shared-album_lsf.txt
$ rclone lsf -Fp --csv gphotos:shared-album &> rclone_2021-08-15_shared-album_lsf.txt
$ diff rclone_2021-08-15_shared-album_lsf.txt rclone_new_2021-08-15_shared-album_lsf.txt
88c88
< "2018 US Trip - San Francisco, Seattle/"
---
> "2018 US Trip - San Francisco, Seattle/Kirkland, New York/"
164,166c164
< "California Citrus State Historic Park
< 
<  29 Dec 2019/"
---
> California Citrus State Historic Park␊␊ 29 Dec 2019/
$

@ncw
Copy link
Member

ncw commented Aug 16, 2021

I think this will break existing users as rclone uses the / to make a heirarchical directory structure - this may not be what you want but you'll see it is documented here

88c88
< "2018 US Trip - San Francisco, Seattle/"
---
> "2018 US Trip - San Francisco, Seattle/Kirkland, New York/"

Directories within the album directory are also writeable and you may create new directories (albums) under album. If you copy files with a directory hierarchy in there then rclone will create albums with the / character in them. For example if you do

This is easy to fix though - just use anAlbum.Title = f.opt.Enc.FromStandardPath(anAlbum.Title) instead of anAlbum.Title = f.opt.Enc.FromStandardName(anAlbum.Title)

Other thoughts

  • Is this going to need a ToStandardPath for the albums too? I guess maybe not since that I think is the only place rclone finds albums
  • What about the photo names? Do you think those need encoding too?

@timevortex
Copy link
Contributor Author

Thanks for taking a look. I've updated the branch to use f.opt.Enc.FromStandardPath instead.

As expected the / diffs found initially are gone:

$ diff rclone_2021-08-15_shared-album_lsf.txt rclone_new_2021-08-15_shared-album_lsf.txt
164,166c164
< "California Citrus State Historic Park
< 
<  29 Dec 2019/"
---
> California Citrus State Historic Park␊␊ 29 Dec 2019/
366,368c364
$

My thoughts to your questions:

Is this going to need a ToStandardPath for the albums too? I guess maybe not since that I think is the only place rclone finds albums

From what I can tell both /albums and /shared-album use this function listAlbums where I have made this change so I think this should cover both use cases. I don't have any way to test or check with /albums though.

What about the photo names? Do you think those need encoding too?

I haven't seen any issues yet and I think it makes sense as photo names are least likely to have human input - automatically named by camera or phone using the most widely accepted set of characters. Also even manually named files imported into Google Photos will have to have come from an existing filesystem with character limitations. The problem with Album names is that Google Photos hasn't done any sanity checking but with photo names this seems to be automatically taken care of?

I had a quick look and I think we would add this after this line:

o.remote = f.opt.Enc.FromStandardName(o.remote)

I have no way to test it though so I wouldn't feel comfortable making this change :/

@timevortex
Copy link
Contributor Author

I had a quick look at the failing test - doesn't seem to be related to this change?

Copy link
Member

@ncw ncw left a comment

Choose a reason for hiding this comment

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

I think that looks great now - thank you :-)

Let's ignore files for the moment - I think you are right about them being sanitised already.

I'll merge it now.

@ncw ncw merged commit 60323dc into rclone:master Aug 19, 2021
@timevortex timevortex deleted the googlephotos_encode branch August 19, 2021 18:23
@timevortex
Copy link
Contributor Author

Awesome, thanks!

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.

None yet

2 participants