From 7cab070f747d369cb3488c7cfa2343597446ed8c Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Tue, 9 Nov 2021 08:51:25 +0100 Subject: [PATCH 1/4] PICARD-2324: Fix renaming of WavPack correction files --- picard/formats/apev2.py | 17 ++++++++++++----- picard/util/filenaming.py | 15 +++++++++++++++ test/formats/test_apev2.py | 5 +++-- test/test_util_filenaming.py | 9 +++++++++ 4 files changed, 39 insertions(+), 7 deletions(-) diff --git a/picard/formats/apev2.py b/picard/formats/apev2.py index 48e9b4ab9..e9b59450f 100644 --- a/picard/formats/apev2.py +++ b/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, @@ -313,12 +318,14 @@ class WavPackFile(APEv2File): def _save_and_rename(self, old_filename, metadata): """Includes an additional check for WavPack correction files""" - wvc_filename = old_filename.replace(".wv", ".wvc") + new_filename = super()._save_and_rename(old_filename, metadata) + wvc_filename = replace_extension(old_filename, ".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) + 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) + return new_filename class OptimFROGFile(APEv2File): diff --git a/picard/util/filenaming.py b/picard/util/filenaming.py index 6ae70a44a..a073e02c5 100644 --- a/picard/util/filenaming.py +++ b/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('.') diff --git a/test/formats/test_apev2.py b/test/formats/test_apev2.py index a8b12028e..a075ed655 100644 --- a/test/formats/test_apev2.py +++ b/test/formats/test_apev2.py @@ -201,9 +201,10 @@ class WavPackTest(CommonApeTests.ApeTestCase): 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) diff --git a/test/test_util_filenaming.py b/test/test_util_filenaming.py index aad6f64ab..050bc8fd8 100644 --- a/test/test_util_filenaming.py +++ b/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')) From d8406b5159f42ae4203396c0249c05dea6ab197d Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Tue, 9 Nov 2021 10:35:22 +0100 Subject: [PATCH 2/4] PICARD-2324: Move .wvc files before moving additional files --- picard/file.py | 5 +++-- picard/formats/apev2.py | 5 ++--- test/formats/test_apev2.py | 27 +++++++++++++++++++++++--- test/test_filesystem.py | 39 ++++++++++++++++++++++---------------- 4 files changed, 52 insertions(+), 24 deletions(-) diff --git a/picard/file.py b/picard/file.py index fc4cd85e6..3be94b43b 100644 --- a/picard/file.py +++ b/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) # Delete empty directories if config.setting["delete_empty_dirs"]: dirname = os.path.dirname(old_filename) @@ -554,6 +553,8 @@ class File(QtCore.QObject, Item): # skip, same directory, nothing to move return config = get_config() + if not config.setting["move_files"] or not config.setting["move_additional_files"]: + return patterns = config.setting["move_additional_files_pattern"] pattern_regexes = set() for pattern in patterns.split(): diff --git a/picard/formats/apev2.py b/picard/formats/apev2.py index e9b59450f..49f68a58c 100644 --- a/picard/formats/apev2.py +++ b/picard/formats/apev2.py @@ -316,16 +316,15 @@ class WavPackFile(APEv2File): NAME = "WavPack" _File = mutagen.wavpack.WavPack - def _save_and_rename(self, old_filename, metadata): + def _move_additional_files(self, old_filename, new_filename): """Includes an additional check for WavPack correction files""" - new_filename = super()._save_and_rename(old_filename, metadata) wvc_filename = replace_extension(old_filename, ".wvc") if isfile(wvc_filename): 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) - return new_filename + return super()._move_additional_files(old_filename, new_filename) class OptimFROGFile(APEv2File): diff --git a/test/formats/test_apev2.py b/test/formats/test_apev2.py index a075ed655..c7ce2dd95 100644 --- a/test/formats/test_apev2.py +++ b/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,8 +196,9 @@ 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) @@ -209,6 +210,26 @@ class WavPackTest(CommonApeTests.ApeTestCase): # 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)) diff --git a/test/test_filesystem.py b/test/test_filesystem.py index 8c966a425..7bce0d718 100644 --- a/test/test_filesystem.py +++ b/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 @@ -101,42 +101,49 @@ class TestFileSystem(PicardTestCase): f = picard.formats.open_(files['old_mp3']) f._move_additional_files(files['old_mp3'], files['new_mp3']) + 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) From a66b2ad779fe568909eb46e392b5f56d098ae484 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Tue, 9 Nov 2021 11:01:11 +0100 Subject: [PATCH 3/4] PICARD-2324: Run .wvc rename code only if rename_files or move_files are set --- picard/file.py | 9 ++++----- picard/formats/apev2.py | 6 +++--- test/test_filesystem.py | 2 +- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/picard/file.py b/picard/file.py index 3be94b43b..a54671da8 100644 --- a/picard/file.py +++ b/picard/file.py @@ -353,7 +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.) - 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) @@ -545,16 +545,15 @@ 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() - if not config.setting["move_files"] or not config.setting["move_additional_files"]: - return patterns = config.setting["move_additional_files_pattern"] pattern_regexes = set() for pattern in patterns.split(): diff --git a/picard/formats/apev2.py b/picard/formats/apev2.py index 49f68a58c..19844255d 100644 --- a/picard/formats/apev2.py +++ b/picard/formats/apev2.py @@ -316,15 +316,15 @@ class WavPackFile(APEv2File): NAME = "WavPack" _File = mutagen.wavpack.WavPack - def _move_additional_files(self, old_filename, new_filename): + def _move_additional_files(self, old_filename, new_filename, config): """Includes an additional check for WavPack correction files""" wvc_filename = replace_extension(old_filename, ".wvc") - if isfile(wvc_filename): + if (config.setting["rename_files"] or config.setting["move_files"]) and isfile(wvc_filename): 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) - return super()._move_additional_files(old_filename, new_filename) + return super()._move_additional_files(old_filename, new_filename, config) class OptimFROGFile(APEv2File): diff --git a/test/test_filesystem.py b/test/test_filesystem.py index 7bce0d718..0bec4f8db 100644 --- a/test/test_filesystem.py +++ b/test/test_filesystem.py @@ -99,7 +99,7 @@ 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) From 7683ca06885965cc09f05c207836023f6a8db3ca Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Tue, 9 Nov 2021 11:44:58 +0100 Subject: [PATCH 4/4] Reduce code complexity in File._move_additional_files --- picard/file.py | 35 ++++++++++++++++++++++------------- picard/formats/apev2.py | 17 +++++++++++------ 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/picard/file.py b/picard/file.py index a54671da8..06fa73390 100644 --- a/picard/file.py +++ b/picard/file.py @@ -554,6 +554,13 @@ class File(QtCore.QObject, Item): if new_path == old_path: # skip, same directory, nothing to move return + 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)): diff --git a/picard/formats/apev2.py b/picard/formats/apev2.py index 19844255d..746b07b51 100644 --- a/picard/formats/apev2.py +++ b/picard/formats/apev2.py @@ -316,14 +316,19 @@ class WavPackFile(APEv2File): NAME = "WavPack" _File = mutagen.wavpack.WavPack + 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 = replace_extension(old_filename, ".wvc") - if (config.setting["rename_files"] or config.setting["move_files"]) and isfile(wvc_filename): - 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) + 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)