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

PICARD-1849: add album_save_post_processors to plug-in API. #1565

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thomasvs
Copy link

Fixes https://tickets.metabrainz.org/browse/PICARD-1849

  • This is a…
    • Bug fix
    • Feature addition
    • Refactoring
    • Minor / simple change (like a typo)
    • Other
  • Describe this change in 1-2 sentences:

I wanted to extend the M3U playlist plugin to automatically generate an .m3u8
when Albums get saved.

To be able to do so, I needed to add album_save_post_processors so that I could
get access to an album on save.

Solution

File save is done in a thread, so I needed to implement a way to wait for those
threads to finish by using the file_save_post_processors hook.

Thomas Vander Stichele added 2 commits June 21, 2020 10:57
I wanted to extend the M3U playlist plugin to automatically generate an .m3u8
when Albums get saved.

To be able to do so, I needed to add album_save_post_processors so that I could
get access to an album on save.

File save is done in a thread, so I needed to implement a way to wait for those
threads to finish by using the file_save_post_processors hook.
Copy link
Collaborator

@zas zas left a comment

Choose a reason for hiding this comment

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

Please ensure tests are passing (Codacy is complaining)

files = self.get_files_from_objects(objects, save=True)
for file in files:
file.save()
for o in objects:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use obj instead of o

# run album_save hooks when the last file is saved
def _file_post_save(self, file):
for album, files in self._album_saves:
if file in files:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since file will not be found in empty files:

if not files:
    ...
elif file in files:
   ...

file.save()
for o in objects:
log.debug('save object %r', o)
files = self.get_files_from_objects([o, ], save=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This defeats uniquification (?) implemented in

picard/picard/tagger.py

Lines 663 to 665 in 7b41e12

def get_files_from_objects(self, objects, save=False):
"""Return list of files from list of albums, clusters, tracks or files."""
return uniqify(chain(*[obj.iterfiles(save) for obj in objects]))

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this can potentially lead to the same file saved twice: You can have both the album and the files selected for saving, but saving should only happen once.

Also we need to be clear what is expected to happen:

  1. If I select the album and save, should the album_save_post_processor be run? (I assume this is a clear YES)
  2. If I select all files of an album, but not the album itself, should the album_save_post_processor be run?
  3. If I select only some files of an album, but not the album itself, should the album_save_post_processor be run?

I tend to say yes to all of the above. But for case 3 it might be useful if the registered processor gets some information, that only a subset has been saved. Optimal would be to pass a list of actually saved files to the processor, so it could figure out what to do with this based on the use case.

log.debug("Album %r saved, running post_save_processors", album)
run_album_post_save_processors(album)
self._album_saves.remove((album, files))
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure to understand why we return immediatly here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants