Merge pull request #1938 from phw/PICARD-2324-fix-wvc-rename

PICARD-2324: Fix renaming of WavPack correction files

picard/file.py

@@ -353,8 +353,7 @@ class File(QtCore.QObject, Item):
         if config.setting["rename_files"] or config.setting["move_files"]:
             new_filename = self._rename(old_filename, metadata, config.setting)
         # Move extra files (images, playlists, etc.)
-        if config.setting["move_files"] and config.setting["move_additional_files"]:
-            self._move_additional_files(old_filename, new_filename)
+        self._move_additional_files(old_filename, new_filename, config)
         # Delete empty directories
         if config.setting["delete_empty_dirs"]:
             dirname = os.path.dirname(old_filename)
@@ -546,14 +545,22 @@ class File(QtCore.QObject, Item):
         for image in images:
             image.save(dirname, metadata, counters)
 
-    def _move_additional_files(self, old_filename, new_filename):
+    def _move_additional_files(self, old_filename, new_filename, config):
         """Move extra files, like images, playlists..."""
+        if not config.setting["move_files"] or not config.setting["move_additional_files"]:
+            return
         new_path = os.path.dirname(new_filename)
         old_path = os.path.dirname(old_filename)
         if new_path == old_path:
             # skip, same directory, nothing to move
             return
-        config = get_config()
+        pattern_regexes = self._compile_move_additional_files_pattern(config)
+        if not pattern_regexes:
+            return
+        moves = self._get_additional_files_moves(old_path, new_path, pattern_regexes)
+        self._apply_additional_files_moves(moves)
+
+    def _compile_move_additional_files_pattern(self, config):
         patterns = config.setting["move_additional_files_pattern"]
         pattern_regexes = set()
         for pattern in patterns.split():
@@ -563,24 +570,26 @@ class File(QtCore.QObject, Item):
             pattern_regex = re.compile(fnmatch.translate(pattern), re.IGNORECASE)
             match_hidden = pattern.startswith('.')
             pattern_regexes.add((pattern_regex, match_hidden))
-        if not pattern_regexes:
-            return
+        return pattern_regexes
+
+    def _get_additional_files_moves(self, old_path, new_path, patterns):
         moves = set()
         try:
-            # TODO: use with statement with python 3.6+
-            for entry in os.scandir(old_path):
-                is_hidden = entry.name.startswith('.')
-                for pattern_regex, match_hidden in pattern_regexes:
-                    if is_hidden and not match_hidden:
-                        continue
-                    if pattern_regex.match(entry.name):
-                        new_file_path = os.path.join(new_path, entry.name)
-                        moves.add((entry.path, new_file_path))
-                        break  # we are done with this file
+            with os.scandir(old_path) as scan:
+                for entry in scan:
+                    is_hidden = entry.name.startswith('.')
+                    for pattern_regex, match_hidden in patterns:
+                        if is_hidden and not match_hidden:
+                            continue
+                        if pattern_regex.match(entry.name):
+                            new_file_path = os.path.join(new_path, entry.name)
+                            moves.add((entry.path, new_file_path))
+                            break  # we are done with this file
         except OSError as why:
             log.error("Failed to scan %r: %s", old_path, why)
-            return
+        return moves
 
+    def _apply_additional_files_moves(self, moves):
         for old_file_path, new_file_path in moves:
             # FIXME we shouldn't do this from a thread!
             if self.tagger.files.get(decode_filename(old_file_path)):

picard/formats/apev2.py

@@ -49,6 +49,11 @@ from picard.util import (
     encode_filename,
     sanitize_date,
 )
+from picard.util.filenaming import (
+    get_available_filename,
+    move_ensure_casing,
+    replace_extension,
+)
 
 from .mutagenext import (
     aac,
@@ -311,14 +316,20 @@ class WavPackFile(APEv2File):
     NAME = "WavPack"
     _File = mutagen.wavpack.WavPack
 
-    def _save_and_rename(self, old_filename, metadata):
+    def _move_or_rename_wvc(self, old_filename, new_filename):
+        wvc_filename = replace_extension(old_filename, ".wvc")
+        if not isfile(wvc_filename):
+            return
+        wvc_new_filename = replace_extension(new_filename, ".wvc")
+        wvc_new_filename = get_available_filename(wvc_new_filename, wvc_filename)
+        log.debug('Moving Wavepack correction file %r => %r', wvc_filename, wvc_new_filename)
+        move_ensure_casing(wvc_filename, wvc_new_filename)
+
+    def _move_additional_files(self, old_filename, new_filename, config):
         """Includes an additional check for WavPack correction files"""
-        wvc_filename = old_filename.replace(".wv", ".wvc")
-        if isfile(wvc_filename):
-            config = get_config()
-            if config.setting["rename_files"] or config.setting["move_files"]:
-                self._rename(wvc_filename, metadata, config.setting)
-        return File._save_and_rename(self, old_filename, metadata)
+        if config.setting["rename_files"] or config.setting["move_files"]:
+            self._move_or_rename_wvc(old_filename, new_filename)
+        return super()._move_additional_files(old_filename, new_filename, config)
 
 
 class OptimFROGFile(APEv2File):

picard/util/filenaming.py

@@ -525,3 +525,18 @@ def get_available_filename(new_path, old_path=None):
         new_path = "%s (%d)%s" % (tmp_filename, i, ext)
         i += 1
     return new_path
+
+
+def replace_extension(filename, new_ext):
+    """Replaces the extension in filename with new_ext.
+
+    If the file has no extension the extension is added.
+
+    Args:
+        filename: A file name
+        new_ext: New file extension
+
+    Returns: filename with replaced file extension
+    """
+    name, ext = os.path.splitext(filename)
+    return name + '.' + new_ext.lstrip('.')

test/formats/test_apev2.py

@@ -184,8 +184,8 @@ class WavPackTest(CommonApeTests.ApeTestCase):
     }
     unexpected_info = ['~video']
 
-    @skipUnlessTestfile
-    def test_save_wavpack_correction_file(self):
+    def setUp(self):
+        super().setUp()
         config.setting['rename_files'] = True
         config.setting['move_files'] = False
         config.setting['ascii_filenames'] = False
@@ -196,18 +196,40 @@ class WavPackTest(CommonApeTests.ApeTestCase):
         config.setting['save_images_to_files'] = False
         config.setting['file_renaming_scripts'] = {'test_id': {'script': '%title%'}}
         config.setting['selected_file_naming_script_id'] = 'test_id'
+
+    def _save_with_wavpack_correction_file(self, source_file_wvc):
         # Create dummy WavPack correction file
-        source_file_wvc = self.filename + 'c'
         open(source_file_wvc, 'a').close()
         # Open file and rename it
         f = open_(self.filename)
-        metadata = Metadata({'title': 'renamed_' + os.path.basename(self.filename)})
+        f._copy_loaded_metadata(f._load(self.filename))
+        f.metadata['title'] = 'renamed_' + os.path.basename(self.filename)
         self.assertTrue(os.path.isfile(self.filename))
-        target_file_wv = f._save_and_rename(self.filename, metadata)
+        target_file_wv = f._save_and_rename(self.filename, f.metadata)
         target_file_wvc = target_file_wv + 'c'
         # Register cleanups
         self.addCleanup(os.unlink, target_file_wv)
         self.addCleanup(os.unlink, target_file_wvc)
+        return (target_file_wv, target_file_wvc)
+
+    @skipUnlessTestfile
+    def test_save_wavpack_correction_file(self):
+        source_file_wvc = self.filename + 'c'
+        (target_file_wv, target_file_wvc) = self._save_with_wavpack_correction_file(source_file_wvc)
+        # Check both the WavPack file and the correction file got moved
+        self.assertFalse(os.path.isfile(self.filename))
+        self.assertFalse(os.path.isfile(source_file_wvc))
+        self.assertTrue(os.path.isfile(target_file_wv))
+        self.assertTrue(os.path.isfile(target_file_wvc))
+
+    @skipUnlessTestfile
+    def test_save_wavpack_correction_file_with_move_additional_files(self):
+        config.setting['move_files'] = True
+        config.setting['move_files_to'] = self.mktmpdir()
+        config.setting['move_additional_files'] = True
+        config.setting['move_additional_files_pattern'] = '*.wvc'
+        source_file_wvc = self.filename + 'c'
+        (target_file_wv, target_file_wvc) = self._save_with_wavpack_correction_file(source_file_wvc)
         # Check both the WavPack file and the correction file got moved
         self.assertFalse(os.path.isfile(self.filename))
         self.assertFalse(os.path.isfile(source_file_wvc))

test/test_filesystem.py

@@ -4,7 +4,7 @@
 #
 # Copyright (C) 2018 Antonio Larrosa
 # Copyright (C) 2018 Wieland Hoffmann
-# Copyright (C) 2018-2019 Philipp Wolfer
+# Copyright (C) 2018-2019, 2021 Philipp Wolfer
 # Copyright (C) 2018-2020 Laurent Monin
 #
 # This program is free software; you can redistribute it and/or
@@ -99,44 +99,51 @@ class TestFileSystem(PicardTestCase):
 
     def _move_additional_files(self, files):
         f = picard.formats.open_(files['old_mp3'])
-        f._move_additional_files(files['old_mp3'], files['new_mp3'])
+        f._move_additional_files(files['old_mp3'], files['new_mp3'], config)
 
+    def _assert_additional_files_moved(self, files):
+        self._move_additional_files(files)
         self._assertFile(files['new_img'])
         self._assertNoFile(files['old_img'])
 
+    def _assert_additional_files_not_moved(self, files):
+        self._move_additional_files(files)
+        self._assertNoFile(files['new_img'])
+        self._assertFile(files['old_img'])
+
     def test_move_additional_files_source_unicode(self):
         files = self._prepare_files(src_rel_path='música')
-
-        self._move_additional_files(files)
+        self._assert_additional_files_moved(files)
 
     def test_move_additional_files_target_unicode(self):
         files = self._prepare_files(tgt_rel_path='música')
-
-        self._move_additional_files(files)
+        self._assert_additional_files_moved(files)
 
     def test_move_additional_files_duplicate_patterns(self):
         files = self._prepare_files()
-
         config.setting['move_additional_files_pattern'] = 'cover.jpg *.jpg'
-
-        self._move_additional_files(files)
+        self._assert_additional_files_moved(files)
 
     def test_move_additional_files_hidden_nopattern(self):
         files = self._prepare_files()
-
         config.setting['move_additional_files_pattern'] = '*.jpg'
-
-        self._move_additional_files(files)
-
+        self._assert_additional_files_moved(files)
         self._assertNoFile(files['new_hidden_img'])
         self._assertFile(files['old_hidden_img'])
 
     def test_move_additional_files_hidden_pattern(self):
         files = self._prepare_files()
-
         config.setting['move_additional_files_pattern'] = '*.jpg .*.jpg'
-
-        self._move_additional_files(files)
-
+        self._assert_additional_files_moved(files)
         self._assertFile(files['new_hidden_img'])
         self._assertNoFile(files['old_hidden_img'])
+
+    def test_move_additional_files_disabled(self):
+        config.setting['move_additional_files'] = False
+        files = self._prepare_files(src_rel_path='música')
+        self._assert_additional_files_not_moved(files)
+
+    def test_move_files_disabled(self):
+        config.setting['move_files'] = False
+        files = self._prepare_files(src_rel_path='música')
+        self._assert_additional_files_not_moved(files)

test/test_util_filenaming.py

@@ -44,6 +44,7 @@ from picard.util.filenaming import (
     make_save_path,
     make_short_filename,
     move_ensure_casing,
+    replace_extension,
     samefile_different_casing,
 )
 
@@ -245,3 +246,11 @@ class GetAvailableFilenameTest(PicardTestCase):
             oldname = self._add_number(filename, expected_number)
             new_filename = get_available_filename(filename, oldname)
             self.assertEqual(self._add_number(filename, expected_number), new_filename)
+
+
+class ReplaceExtensionTest(PicardTestCase):
+
+    def test_replace(self):
+        self.assertEqual('foo/bar.wvc', replace_extension('foo/bar.wv', '.wvc'))
+        self.assertEqual('foo/bar.wvc', replace_extension('foo/bar.wv', 'wvc'))
+        self.assertEqual('foo/bar.wvc', replace_extension('foo/bar', 'wvc'))