diff --git a/beetsplug/alternatives.py b/beetsplug/alternatives.py index 7565563..9c448e2 100644 --- a/beetsplug/alternatives.py +++ b/beetsplug/alternatives.py @@ -16,6 +16,7 @@ import argparse from concurrent import futures import six +import traceback import beets from beets import util, art @@ -23,11 +24,27 @@ from beets.ui import Subcommand, get_path_formats, input_yn, UserError, \ print_, decargs from beets.library import parse_query_string, Item -from beets.util import syspath, displayable_path, cpu_count, bytestring_path +from beets.util import syspath, displayable_path, cpu_count, bytestring_path, \ + FilesystemError from beetsplug import convert +def _remove(path, soft=True): + """Remove the file. If `soft`, then no error will be raised if the + file does not exist. + In contrast to beets' util.remove, this uses lexists such that it can + actually remove symlink links. + """ + path = syspath(path) + if soft and not os.path.lexists(path): + return + try: + os.remove(path) + except (OSError, IOError) as exc: + raise FilesystemError(exc, 'delete', (path,), traceback.format_exc()) + + class AlternativesPlugin(BeetsPlugin): def __init__(self): @@ -166,29 +183,40 @@ def parse_config(self, config): dir = os.path.join(self.lib.directory, dir) self.directory = dir + def item_change_actions(self, item, path, dest): + """ Returns the necessary actions for items that were previously in the + external collection, but might require metadata updates. + """ + actions = [] + + if not util.samefile(path, dest): + actions.append(self.MOVE) + + item_mtime_alt = os.path.getmtime(syspath(path)) + if (item_mtime_alt < os.path.getmtime(syspath(item.path))): + actions.append(self.WRITE) + album = item.get_album() + + if album: + if (album.artpath and + os.path.isfile(syspath(album.artpath)) and + (item_mtime_alt + < os.path.getmtime(syspath(album.artpath)))): + actions.append(self.SYNC_ART) + + return actions + def matched_item_action(self, item): path = self.get_path(item) - if path and os.path.isfile(syspath(path)): + if path and os.path.lexists(syspath(path)): dest = self.destination(item) _, path_ext = os.path.splitext(path) _, dest_ext = os.path.splitext(dest) if not path_ext == dest_ext: # formats config option changed return (item, [self.REMOVE, self.ADD]) - actions = [] - if not util.samefile(path, dest): - actions.append(self.MOVE) - item_mtime_alt = os.path.getmtime(syspath(path)) - if (item_mtime_alt < os.path.getmtime(syspath(item.path))): - actions.append(self.WRITE) - album = item.get_album() - if album: - if (album.artpath and - os.path.isfile(syspath(album.artpath)) and - (item_mtime_alt - < os.path.getmtime(syspath(album.artpath)))): - actions.append(self.SYNC_ART) - return (item, actions) + else: + return (item, self.item_change_actions(item, path, dest)) else: return (item, [self.ADD]) @@ -276,7 +304,7 @@ def get_path(self, item): def remove_item(self, item): path = self.get_path(item) - util.remove(path) + _remove(path) util.prune_dirs(path, root=self.directory) del item[self.path_key] @@ -359,6 +387,21 @@ def parse_config(self, config): super(SymlinkView, self).parse_config(config) + def item_change_actions(self, item, path, dest): + """ Returns the necessary actions for items that were previously in the + external collection, but might require metadata updates. + """ + actions = [] + + if not path == dest: + # The path of the link itself changed + actions.append(self.MOVE) + elif not util.samefile(path, item.path): + # link target changed + actions.append(self.MOVE) + + return actions + def update(self, create=None): for (item, actions) in self.items_actions(): dest = self.destination(item) @@ -389,6 +432,10 @@ def create_symlink(self, item): if self.relativelinks == self.LINK_RELATIVE else item.path) util.link(link, dest) + def sync_art(self, item, path): + # FIXME: symlink art + pass + class Worker(futures.ThreadPoolExecutor): diff --git a/test/cli_test.py b/test/cli_test.py index d159bdb..100a08f 100644 --- a/test/cli_test.py +++ b/test/cli_test.py @@ -155,6 +155,44 @@ def test_add_move_remove_album_relative(self): self.alt_config['link_type'] = 'relative' self.test_add_move_remove_album(absolute=False) + def test_add_update_move_album(self): + """ Test that symlinks are properly updated and no broken links left + when an item's path in the library changes. + Since moving the items causes the links in the symlink view to be + broken, this situation used to be incorrectly detected as + addition of new items, such that the old links weren't removed. + Contrast this to the `test_add_move_remove_album` test, in which the + old links do not break upon changing the path format. + * An album is added. + * The album name is changed, which also causes the tracks to be moved. + * The symlink view is updated. + """ + self.add_album(artist='Michael Jackson', album='Thriller', year='1990') + + self.runcli('alt', 'update', 'by-year') + + by_year_path = self.lib_path(b'by-year/1990/Thriller/track 1.mp3') + self.assertSymlink( + link=by_year_path, + target=self.lib_path(b'Michael Jackson/Thriller/track 1.mp3'), + absolute=True, + ) + + # `-y` skips the prompt, `-a` updates album-level fields, `-m` forces + # actually moving the files + self.runcli('mod', '-y', '-a', '-m', + 'Thriller', 'album=Thriller (Remastered)') + self.runcli('alt', 'update', 'by-year') + + self.assertIsNotFile(by_year_path) + self.assertSymlink( + link=self.lib_path( + b'by-year/1990/Thriller (Remastered)/track 1.mp3'), + target=self.lib_path( + b'Michael Jackson/Thriller (Remastered)/track 1.mp3'), + absolute=True, + ) + def test_valid_options(self): """ Test that an error is raised when option is invalid * Config link type is invalid