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.

Sign up for GitHub

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
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b4b1743
add album_save_post_processors to plug-in API.
Jun 21, 2020
7b41e12
run isort
Jun 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions picard/album.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,3 +753,20 @@ def register_album_post_removal_processor(function, priority=PluginPriority.NORM

def run_album_post_removal_processors(album_object):
_album_post_removal_processors.run(album_object)

_album_post_save_processors = PluginFunctions(label='album_post_save_processors')


def register_album_post_save_processor(function, priority=PluginPriority.NORMAL):
"""Registers an album-removed processor.
Args:
function: function to call after album save, it will be passed the album object
priority: optional, PluginPriority.NORMAL by default
Returns:
None
"""
_album_post_save_processors.register(function.__module__, function, priority)


def run_album_post_save_processors(album_object):
_album_post_save_processors.run(album_object)
32 changes: 28 additions & 4 deletions picard/tagger.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@
Album,
NatAlbum,
run_album_post_removal_processors,
run_album_post_save_processors,
)
from picard.browser.browser import BrowserIntegration
from picard.browser.filelookup import FileLookup
Expand All @@ -98,7 +99,10 @@
)
from picard.dataobj import DataObject
from picard.disc import Disc
from picard.file import File
from picard.file import (
File,
register_file_post_save_processor,
)
from picard.formats import open_ as open_file
from picard.i18n import setup_gettext
from picard.pluginmanager import PluginManager
Expand Down Expand Up @@ -294,6 +298,10 @@ def __init__(self, picard_args, unparsed_args, localedir, autoupdate):
if self.autoupdate_enabled:
self.updatecheckmanager = UpdateCheckManager(parent=self.window)

# track all file save completions for album save
self._album_saves = []
register_file_post_save_processor(self._file_post_save)

def register_cleanup(self, func):
self.exit_cleanup.append(func)

Expand Down Expand Up @@ -658,9 +666,14 @@ def get_files_from_objects(self, objects, save=False):

def save(self, objects):
"""Save the specified objects."""
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

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.

for file in files:
file.save()
if isinstance(o, Album):
# track the album and its files pending save
self._album_saves.append((o, files))

def load_album(self, album_id, discid=None):
album_id = self.mbid_redirects.get(album_id, album_id)
Expand Down Expand Up @@ -773,6 +786,17 @@ def remove(self, objects):
if files:
self.remove_files(files)

# 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:
   ...

files.remove(file)
if not files:
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


def _lookup_disc(self, disc, result=None, error=None):
self.restore_cursor()
if error is not None:
Expand Down