From f83044a5d09072fbc06b13d0b5860857e96a899f Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Tue, 1 Dec 2020 08:54:15 +0100 Subject: [PATCH 1/7] PICARD-2028: Update cover art box only on changes Use signalling to update the cover art box when cover art actually changed. Avoid cover art box update on selection changes. --- picard/album.py | 8 ++++---- picard/cluster.py | 6 +++++- picard/track.py | 1 + picard/ui/coverartbox.py | 28 +++++++++++++++++----------- picard/ui/mainwindow.py | 17 ++--------------- 5 files changed, 29 insertions(+), 31 deletions(-) diff --git a/picard/album.py b/picard/album.py index 8b99a5970..3dc685ed9 100644 --- a/picard/album.py +++ b/picard/album.py @@ -113,6 +113,7 @@ class AlbumArtist(DataObject): class Album(DataObject, Item): + metadata_images_changed = QtCore.pyqtSignal() release_group_loaded = QtCore.pyqtSignal() def __init__(self, album_id, discid=None): @@ -131,6 +132,7 @@ class Album(DataObject, Item): self._discids.add(discid) self._after_load_callbacks = [] self.unmatched_files = Cluster(_("Unmatched Files"), special=True, related_album=self, hide_if_empty=True) + self.unmatched_files.metadata_images_changed.connect(self.update_metadata_images) self.status = None self._album_artists = [] self.update_metadata_images_enabled = True @@ -376,6 +378,7 @@ class Album(DataObject, Item): self.enable_update_metadata_images(False) for track in self._new_tracks: track.orig_metadata.copy(track.metadata) + track.metadata_images_changed.connect(self.update_metadata_images) # Prepare parser for user's script for s_name, s_text in enabled_tagger_scripts_texts(): @@ -396,7 +399,6 @@ class Album(DataObject, Item): self._new_metadata.strip_whitespace() for track in self.tracks: - track.metadata_images_changed.connect(self.update_metadata_images) for file in list(track.linked_files): file.move(self.unmatched_files) self.metadata = self._new_metadata @@ -513,12 +515,10 @@ class Album(DataObject, Item): self._files += 1 self.update(update_tracks=False) add_metadata_images(self, [file]) - file.metadata_images_changed.connect(self.update_metadata_images) def _remove_file(self, track, file): self._files -= 1 self.update(update_tracks=False) - file.metadata_images_changed.disconnect(self.update_metadata_images) remove_metadata_images(self, [file]) def _match_files(self, files, threshold=0): @@ -719,8 +719,8 @@ class Album(DataObject, Item): return update_metadata_images(self) - self.update(False) + self.metadata_images_changed.emit() def keep_original_images(self): self.enable_update_metadata_images(False) diff --git a/picard/cluster.py b/picard/cluster.py index 480d527c4..6280f0396 100644 --- a/picard/cluster.py +++ b/picard/cluster.py @@ -72,6 +72,8 @@ from picard.ui.item import Item class FileList(QtCore.QObject, Item): + metadata_images_changed = QtCore.pyqtSignal() + def __init__(self, files=None): QtCore.QObject.__init__(self) self.metadata = Metadata() @@ -81,7 +83,8 @@ class FileList(QtCore.QObject, Item): for file in self.files: if self.can_show_coverart: file.metadata_images_changed.connect(self.update_metadata_images) - update_metadata_images(self) + if self.files: + update_metadata_images(self) def iterfiles(self, save=False): for file in self.files: @@ -100,6 +103,7 @@ class FileList(QtCore.QObject, Item): def update_metadata_images(self): if self.update_metadata_images_enabled and self.can_show_coverart: update_metadata_images(self) + self.metadata_images_changed.emit() def keep_original_images(self): self.enable_update_metadata_images(False) diff --git a/picard/track.py b/picard/track.py index 2dfcf8b18..6f17e22ae 100644 --- a/picard/track.py +++ b/picard/track.py @@ -350,6 +350,7 @@ class Track(DataObject, Item): def update_orig_metadata_images(self): update_metadata_images(self) + self.metadata_images_changed.emit() def keep_original_images(self): for file in self.linked_files: diff --git a/picard/ui/coverartbox.py b/picard/ui/coverartbox.py index d0738d4ee..279f42b58 100644 --- a/picard/ui/coverartbox.py +++ b/picard/ui/coverartbox.py @@ -347,17 +347,29 @@ class CoverArtBox(QtWidgets.QGroupBox): self.cover_art_label.setText(_('New Cover Art')) self.orig_cover_art_label.setText(_('Original Cover Art')) - def show(self): - self.update_display(True) - super().show() + def set_item(self, item): + if not item.can_show_coverart: + return + + if self.item and hasattr(self.item, 'metadata_images_changed'): + self.item.metadata_images_changed.disconnect(self.update_metadata) + self.item = item + if hasattr(self.item, 'metadata_images_changed'): + self.item.metadata_images_changed.connect(self.update_metadata) + self.update_metadata() + + def update_metadata(self): + if not self.item: + return + + metadata = self.item.metadata + orig_metadata = self.item.orig_metadata - def set_metadata(self, metadata, orig_metadata, item): if not metadata or not metadata.images: self.cover_art.set_metadata(orig_metadata) else: self.cover_art.set_metadata(metadata) self.orig_cover_art.set_metadata(orig_metadata) - self.item = item self.update_display() def fetch_remote_image(self, url, fallback_data=None): @@ -461,7 +473,6 @@ class CoverArtBox(QtWidgets.QGroupBox): set_image = set_image_append debug_info = "Appending dropped %r to %r" - update = True if isinstance(self.item, Album): album = self.item album.enable_update_metadata_images(False) @@ -507,14 +518,9 @@ class CoverArtBox(QtWidgets.QGroupBox): file.update() else: debug_info = "Dropping %r to %r is not handled" - update = False log.debug(debug_info, coverartimage, self.item) - if update: - self.cover_art.set_metadata(self.item.metadata) - self.show() - def choose_local_file(self): file_chooser = QtWidgets.QFileDialog(self) file_chooser.setNameFilters([ diff --git a/picard/ui/mainwindow.py b/picard/ui/mainwindow.py index 3d0d1aae6..09a365ae5 100644 --- a/picard/ui/mainwindow.py +++ b/picard/ui/mainwindow.py @@ -1168,8 +1168,6 @@ class MainWindow(QtWidgets.QMainWindow, PreserveGeometry): self.update_actions() - metadata = None - orig_metadata = None obj = None # Clear any existing status bar messages @@ -1181,8 +1179,6 @@ class MainWindow(QtWidgets.QMainWindow, PreserveGeometry): if len(objects) == 1: obj = list(objects)[0] if isinstance(obj, File): - metadata = obj.metadata - orig_metadata = obj.orig_metadata if obj.state == obj.ERROR: msg = N_("%(filename)s (error: %(error)s)") mparms = { @@ -1196,10 +1192,8 @@ class MainWindow(QtWidgets.QMainWindow, PreserveGeometry): } self.set_statusbar_message(msg, mparms, echo=None, history=None) elif isinstance(obj, Track): - metadata = obj.metadata if obj.num_linked_files == 1: file = obj.linked_files[0] - orig_metadata = file.orig_metadata if file.has_error(): msg = N_("%(filename)s (%(similarity)d%%) (error: %(error)s)") mparms = { @@ -1215,22 +1209,15 @@ class MainWindow(QtWidgets.QMainWindow, PreserveGeometry): } self.set_statusbar_message(msg, mparms, echo=None, history=None) - elif isinstance(obj, Album): - metadata = obj.metadata - orig_metadata = obj.orig_metadata - elif obj.can_show_coverart: - metadata = obj.metadata - else: + elif new_selection: # Create a temporary file list which allows changing cover art for all selected files files = self.tagger.get_files_from_objects(objects) obj = FileList(files) - metadata = obj.metadata - orig_metadata = obj.orig_metadata if new_selection: self.metadata_box.selection_dirty = True + self.cover_art_box.set_item(obj) self.metadata_box.update(drop_album_caches=drop_album_caches) - self.cover_art_box.set_metadata(metadata, orig_metadata, obj) self.selection_updated.emit(objects) def refresh_metadatabox(self): From a54090a960ea8d086ef9f354fa54cd4eef7c8946 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Tue, 1 Dec 2020 10:01:00 +0100 Subject: [PATCH 2/7] Let Track inherit from FileList Unifies handling of objects with linked files. Simplifies code, e.g. cover art propagation to parent objects --- picard/album.py | 10 ++++---- picard/tagger.py | 2 +- picard/track.py | 53 ++++++++++++++++----------------------- picard/ui/coverartbox.py | 28 ++++++--------------- picard/ui/infodialog.py | 2 +- picard/ui/itemviews.py | 6 ++--- picard/ui/mainwindow.py | 4 +-- picard/ui/metadatabox.py | 6 ++--- picard/ui/ratingwidget.py | 2 +- picard/ui/scriptsmenu.py | 2 +- picard/util/imagelist.py | 4 +-- test/test_imagelist.py | 24 +++++++++--------- 12 files changed, 60 insertions(+), 83 deletions(-) diff --git a/picard/album.py b/picard/album.py index 3dc685ed9..dc821a3b9 100644 --- a/picard/album.py +++ b/picard/album.py @@ -168,7 +168,7 @@ class Album(DataObject, Item): if track_discids: track.metadata['musicbrainz_discid'] = track_discids track.update() - for file in track.linked_files: + for file in track.files: file.metadata['musicbrainz_discid'] = track_discids file.update() @@ -399,7 +399,7 @@ class Album(DataObject, Item): self._new_metadata.strip_whitespace() for track in self.tracks: - for file in list(track.linked_files): + for file in list(track.files): file.move(self.unmatched_files) self.metadata = self._new_metadata self.orig_metadata.copy(self.metadata) @@ -632,7 +632,7 @@ class Album(DataObject, Item): def is_modified(self): if self.tracks: for track in self.tracks: - for file in track.linked_files: + for file in track.files: if not file.is_saved(): return True return False @@ -640,7 +640,7 @@ class Album(DataObject, Item): def get_num_unsaved_files(self): count = 0 for track in self.tracks: - for file in track.linked_files: + for file in track.files: if not file.is_saved(): count += 1 return count @@ -746,7 +746,7 @@ class NatAlbum(Album): for track in self.tracks: if old_album_title == track.metadata["album"]: track.metadata["album"] = self.metadata["album"] - for file in track.linked_files: + for file in track.files: track.update_file_metadata(file) self.enable_update_metadata_images(True) super().update(update_tracks) diff --git a/picard/tagger.py b/picard/tagger.py index c1ee1e330..77fad8626 100644 --- a/picard/tagger.py +++ b/picard/tagger.py @@ -750,7 +750,7 @@ class Tagger(QtWidgets.QApplication): elif isinstance(obj, NonAlbumTrack): self.remove_nat(obj) elif isinstance(obj, Track): - files.extend(obj.linked_files) + files.extend(obj.files) elif isinstance(obj, Album): self.window.set_statusbar_message( N_("Removing album %(id)s: %(artist)s - %(album)s"), diff --git a/picard/track.py b/picard/track.py index 6f17e22ae..db0d62cdd 100644 --- a/picard/track.py +++ b/picard/track.py @@ -48,6 +48,7 @@ from picard import ( config, log, ) +from picard.cluster import FileList from picard.const import ( DATA_TRACK_TITLE, SILENCE_TRACK_TITLE, @@ -73,12 +74,9 @@ from picard.util.imagelist import ( ImageList, add_metadata_images, remove_metadata_images, - update_metadata_images, ) from picard.util.textencoding import asciipunct -from picard.ui.item import Item - _TRANSLATE_TAGS = { "hip hop": "Hip-Hop", @@ -137,39 +135,42 @@ class TrackArtist(DataObject): super().__init__(ta_id) -class Track(DataObject, Item): +class Track(DataObject, FileList): metadata_images_changed = QtCore.pyqtSignal() def __init__(self, track_id, album=None): DataObject.__init__(self, track_id) + FileList.__init__(self) self.album = album - self.linked_files = [] self.num_linked_files = 0 - self.metadata = Metadata() - self.orig_metadata = Metadata() self.scripted_metadata = Metadata() self._track_artists = [] def __repr__(self): return '' % (self.id, self.metadata["title"]) + @property + def linked_files(self): + """For backward compatibility with old code in plugins""" + return self.files + def add_file(self, file): - if file not in self.linked_files: + if file not in self.files: track_will_expand = self.num_linked_files == 1 - self.linked_files.append(file) + self.files.append(file) self.num_linked_files += 1 self.update_file_metadata(file) add_metadata_images(self, [file]) self.album._add_file(self, file) - file.metadata_images_changed.connect(self.update_orig_metadata_images) + file.metadata_images_changed.connect(self.update_metadata_images) run_file_post_addition_to_track_processors(self, file) if track_will_expand: # Files get expanded, ensure the existing item renders correctly - self.linked_files[0].update_item() + self.files[0].update_item() def update_file_metadata(self, file): - if file not in self.linked_files: + if file not in self.files: return # Run the scripts for the file to allow usage of # file specific metadata and variables @@ -191,12 +192,12 @@ class Track(DataObject, Item): self.update() def remove_file(self, file): - if file not in self.linked_files: + if file not in self.files: return - self.linked_files.remove(file) + self.files.remove(file) self.num_linked_files -= 1 file.copy_metadata(file.orig_metadata, preserve_deleted=False) - file.metadata_images_changed.disconnect(self.update_orig_metadata_images) + file.metadata_images_changed.disconnect(self.update_metadata_images) self.album._remove_file(self, file) remove_metadata_images(self, [file]) run_file_post_removal_from_track_processors(self, file) @@ -218,23 +219,19 @@ class Track(DataObject, Item): if self.item: self.item.update() - def iterfiles(self, save=False): - for file in self.linked_files: - yield file - def is_linked(self): return self.num_linked_files > 0 def can_save(self): """Return if this object can be saved.""" - for file in self.linked_files: + for file in self.files: if file.can_save(): return True return False def can_remove(self): """Return if this object can be removed.""" - for file in self.linked_files: + for file in self.files: if file.can_remove(): return True return False @@ -254,7 +251,7 @@ class Track(DataObject, Item): elif column in m: return m[column] elif self.num_linked_files == 1: - return self.linked_files[0].column(column) + return self.files[0].column(column) def is_video(self): return self.metadata['~video'] == '1' @@ -348,15 +345,9 @@ class Track(DataObject, Item): genre = [join_genres.join(genre)] self.metadata['genre'] = genre - def update_orig_metadata_images(self): - update_metadata_images(self) - self.metadata_images_changed.emit() - def keep_original_images(self): - for file in self.linked_files: - file.keep_original_images() - self.update_orig_metadata_images() - if self.linked_files: + super().keep_original_images() + if self.files: self.metadata.images = self.orig_metadata.images.copy() else: self.metadata.images = ImageList() @@ -411,7 +402,7 @@ class NonAlbumTrack(Track): return try: self._parse_recording(recording) - for file in self.linked_files: + for file in self.files: self.update_file_metadata(file) except Exception: self._set_error(traceback.format_exc()) diff --git a/picard/ui/coverartbox.py b/picard/ui/coverartbox.py index 279f42b58..b75149eb2 100644 --- a/picard/ui/coverartbox.py +++ b/picard/ui/coverartbox.py @@ -55,7 +55,6 @@ from picard.coverart.image import ( CoverArtImageError, ) from picard.file import File -from picard.track import Track from picard.util import imageinfo from picard.util.lrucache import LRUCache @@ -488,29 +487,16 @@ class CoverArtBox(QtWidgets.QGroupBox): album.update_metadata_images() album.update(False) elif isinstance(self.item, FileList): - cluster = self.item - cluster.enable_update_metadata_images(False) - set_image(cluster, coverartimage) - for file in cluster.iterfiles(): + filelist = self.item + filelist.enable_update_metadata_images(False) + set_image(filelist, coverartimage) + for file in filelist.iterfiles(): set_image(file, coverartimage) file.metadata_images_changed.emit() file.update(signal=False) - cluster.enable_update_metadata_images(True) - cluster.update_metadata_images() - cluster.update() - elif isinstance(self.item, Track): - track = self.item - track.album.enable_update_metadata_images(False) - set_image(track, coverartimage) - track.metadata_images_changed.emit() - for file in track.iterfiles(): - set_image(file, coverartimage) - file.metadata_images_changed.emit() - file.update(signal=False) - track.album.enable_update_metadata_images(True) - track.album.update_metadata_images() - track.album.update(False) - track.update() + filelist.enable_update_metadata_images(True) + filelist.update_metadata_images() + filelist.update() elif isinstance(self.item, File): file = self.item set_image(file, coverartimage) diff --git a/picard/ui/infodialog.py b/picard/ui/infodialog.py index 62a3290b0..d4e03d0b4 100644 --- a/picard/ui/infodialog.py +++ b/picard/ui/infodialog.py @@ -399,7 +399,7 @@ class TrackInfoDialog(InfoDialog): tabWidget.setTabText(tab_index, _("&Info")) text = ngettext("%i file in this track", "%i files in this track", track.num_linked_files) % track.num_linked_files - info_files = [format_file_info(file_) for file_ in track.linked_files] + info_files = [format_file_info(file_) for file_ in track.files] text += '
' + '
'.join(info_files) self.ui.info.setText(text) diff --git a/picard/ui/itemviews.py b/picard/ui/itemviews.py index 438705e80..466ff10bc 100644 --- a/picard/ui/itemviews.py +++ b/picard/ui/itemviews.py @@ -1018,7 +1018,7 @@ class TrackItem(TreeItem): def update(self, update_album=True, update_files=True): track = self.obj if track.num_linked_files == 1: - file = track.linked_files[0] + file = track.files[0] file.item = self color = TrackItem.track_colors[file.state] bgcolor = get_match_color(file.similarity, TreeItem.base_color) @@ -1053,14 +1053,14 @@ class TrackItem(TreeItem): oldnum = newnum for i in range(oldnum): # update existing items item = self.child(i) - file = track.linked_files[i] + file = track.files[i] item.obj = file file.item = item item.update(update_track=False) if newnum > oldnum: # add new items items = [] for i in range(newnum - 1, oldnum - 1, -1): - item = FileItem(track.linked_files[i], False) + item = FileItem(track.files[i], False) item.update(update_track=False) items.append(item) self.addChildren(items) diff --git a/picard/ui/mainwindow.py b/picard/ui/mainwindow.py index 09a365ae5..bb0c4c40c 100644 --- a/picard/ui/mainwindow.py +++ b/picard/ui/mainwindow.py @@ -1077,7 +1077,7 @@ class MainWindow(QtWidgets.QMainWindow, PreserveGeometry): def show_more_tracks(self): obj = self.selected_objects[0] if isinstance(obj, Track): - obj = obj.linked_files[0] + obj = obj.files[0] dialog = TrackSearchDialog(self) dialog.load_similar_tracks(obj) dialog.exec_() @@ -1193,7 +1193,7 @@ class MainWindow(QtWidgets.QMainWindow, PreserveGeometry): self.set_statusbar_message(msg, mparms, echo=None, history=None) elif isinstance(obj, Track): if obj.num_linked_files == 1: - file = obj.linked_files[0] + file = obj.files[0] if file.has_error(): msg = N_("%(filename)s (%(similarity)d%%) (error: %(error)s)") mparms = { diff --git a/picard/ui/metadatabox.py b/picard/ui/metadatabox.py index cff5137c0..222932154 100644 --- a/picard/ui/metadatabox.py +++ b/picard/ui/metadatabox.py @@ -395,7 +395,7 @@ class MetadataBox(QtWidgets.QTableWidget): track_albums = set() for file in self.files: objects = [file] - if file.parent in self.tracks and len(self.files & set(file.parent.linked_files)) == 1: + if file.parent in self.tracks and len(self.files & set(file.parent.files)) == 1: objects.append(file.parent) file_tracks.append(file.parent) track_albums.add(file.parent.album) @@ -503,7 +503,7 @@ class MetadataBox(QtWidgets.QTableWidget): files.add(obj) elif isinstance(obj, Track): tracks.add(obj) - files.update(obj.linked_files) + files.update(obj.files) elif isinstance(obj, Cluster) and obj.can_edit_tags(): objects.add(obj) files.update(obj.files) @@ -511,7 +511,7 @@ class MetadataBox(QtWidgets.QTableWidget): objects.add(obj) tracks.update(obj.tracks) for track in obj.tracks: - files.update(track.linked_files) + files.update(track.files) objects.update(files) objects.update(tracks) self.selection_dirty = False diff --git a/picard/ui/ratingwidget.py b/picard/ui/ratingwidget.py index 7fab90ab8..1a4f9fb39 100644 --- a/picard/ui/ratingwidget.py +++ b/picard/ui/ratingwidget.py @@ -95,7 +95,7 @@ class RatingWidget(QtWidgets.QWidget): track = self._track rating = str(self._rating) track.metadata["~rating"] = rating - for file in track.linked_files: + for file in track.files: file.metadata["~rating"] = rating if config.setting["submit_ratings"]: ratings = {("recording", track.id): self._rating} diff --git a/picard/ui/scriptsmenu.py b/picard/ui/scriptsmenu.py index 1020e594d..87786522a 100644 --- a/picard/ui/scriptsmenu.py +++ b/picard/ui/scriptsmenu.py @@ -82,4 +82,4 @@ class ScriptsMenu(QtWidgets.QMenu): yield from self._get_metadata_objects(obj.tracks) yield from self._get_metadata_objects(obj.unmatched_files.iterfiles()) if isinstance(obj, Track): - yield from self._get_metadata_objects(obj.linked_files) + yield from self._get_metadata_objects(obj.files) diff --git a/picard/util/imagelist.py b/picard/util/imagelist.py index 237b95e11..a485690e3 100644 --- a/picard/util/imagelist.py +++ b/picard/util/imagelist.py @@ -147,12 +147,12 @@ def _get_state(obj): if isinstance(obj, Album): for track in obj.tracks: state.sources.append(track) - state.sources += track.linked_files + state.sources += track.files state.sources += obj.unmatched_files.files state.update_new_metadata = True state.update_orig_metadata = True elif isinstance(obj, Track): - state.sources = obj.linked_files + state.sources = obj.files state.update_orig_metadata = True elif isinstance(obj, Cluster): state.sources = obj.files diff --git a/test/test_imagelist.py b/test/test_imagelist.py index 57aea56f1..4b0230184 100644 --- a/test/test_imagelist.py +++ b/test/test_imagelist.py @@ -88,22 +88,22 @@ class UpdateMetadataImagesTest(PicardTestCase): def test_update_track_images(self): track = Track('00000000-0000-0000-0000-000000000000') - track.linked_files = list(self.test_files) + track.files = list(self.test_files) update_metadata_images(track) self.assertEqual(set(self.test_images), set(track.orig_metadata.images)) self.assertFalse(track.orig_metadata.has_common_images) - track.linked_files.remove(self.test_files[2]) + track.files.remove(self.test_files[2]) update_metadata_images(track) self.assertEqual(set(self.test_images), set(track.orig_metadata.images)) self.assertFalse(track.orig_metadata.has_common_images) - track.linked_files.remove(self.test_files[0]) + track.files.remove(self.test_files[0]) update_metadata_images(track) self.assertEqual(set(self.test_images[1:]), set(track.orig_metadata.images)) self.assertTrue(track.orig_metadata.has_common_images) - track.linked_files.append(self.test_files[2]) + track.files.append(self.test_files[2]) update_metadata_images(track) self.assertEqual(set(self.test_images[1:]), set(track.orig_metadata.images)) self.assertTrue(track.orig_metadata.has_common_images) @@ -111,9 +111,9 @@ class UpdateMetadataImagesTest(PicardTestCase): def test_update_album_images(self): album = Album('00000000-0000-0000-0000-000000000000') track1 = Track('00000000-0000-0000-0000-000000000001') - track1.linked_files.append(self.test_files[0]) + track1.files.append(self.test_files[0]) track2 = Track('00000000-0000-0000-0000-000000000002') - track2.linked_files.append(self.test_files[1]) + track2.files.append(self.test_files[1]) album.tracks = [track1, track2] album.unmatched_files.files.append(self.test_files[2]) update_metadata_images(album) @@ -171,27 +171,27 @@ class RemoveMetadataImagesTest(PicardTestCase): def test_remove_from_track(self): track = Track('00000000-0000-0000-0000-000000000000') - track.linked_files = list(self.test_files) + track.files = list(self.test_files) update_metadata_images(track) - track.linked_files.remove(self.test_files[0]) + track.files.remove(self.test_files[0]) remove_metadata_images(track, [self.test_files[0]]) self.assertEqual(set(self.test_images[1:]), set(track.orig_metadata.images)) self.assertTrue(track.orig_metadata.has_common_images) def test_remove_from_track_with_common_images(self): track = Track('00000000-0000-0000-0000-000000000000') - track.linked_files = list(self.test_files[1:]) + track.files = list(self.test_files[1:]) update_metadata_images(track) - track.linked_files.remove(self.test_files[1]) + track.files.remove(self.test_files[1]) remove_metadata_images(track, [self.test_files[1]]) self.assertEqual(set(self.test_images[1:]), set(track.orig_metadata.images)) self.assertTrue(track.orig_metadata.has_common_images) def test_remove_from_empty_track(self): track = Track('00000000-0000-0000-0000-000000000000') - track.linked_files.append(File('test1.flac')) + track.files.append(File('test1.flac')) update_metadata_images(track) - remove_metadata_images(track, [track.linked_files[0]]) + remove_metadata_images(track, [track.files[0]]) self.assertEqual(set(), set(track.orig_metadata.images)) self.assertTrue(track.orig_metadata.has_common_images) From e69f9b7e9f97fb48fc2787bc34ab612cf5f7be4b Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Tue, 1 Dec 2020 14:02:44 +0100 Subject: [PATCH 3/7] Optimised setting image in cover art box Avoid calls to update_metadata_images --- picard/ui/coverartbox.py | 40 ++++++++++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/picard/ui/coverartbox.py b/picard/ui/coverartbox.py index b75149eb2..a947290f2 100644 --- a/picard/ui/coverartbox.py +++ b/picard/ui/coverartbox.py @@ -48,13 +48,17 @@ from picard import ( log, ) from picard.album import Album -from picard.cluster import FileList +from picard.cluster import ( + Cluster, + FileList, +) from picard.const import MAX_COVERS_TO_STACK from picard.coverart.image import ( CoverArtImage, CoverArtImageError, ) from picard.file import File +from picard.track import Track from picard.util import imageinfo from picard.util.lrucache import LRUCache @@ -277,10 +281,22 @@ class CoverArtThumbnail(ActiveLabel): def set_image_replace(obj, coverartimage): obj.metadata.images.strip_front_images() obj.metadata.images.append(coverartimage) + obj.metadata_images_changed.emit() def set_image_append(obj, coverartimage): obj.metadata.images.append(coverartimage) + obj.metadata_images_changed.emit() + + +def iter_file_parents(file): + parent = file.parent + if parent: + yield parent + if isinstance(parent, Track) and parent.album: + yield parent.album + elif isinstance(parent, Cluster) and parent.related_album: + yield parent.related_album HTML_IMG_SRC_REGEX = re.compile(r' Date: Tue, 1 Dec 2020 15:12:32 +0100 Subject: [PATCH 4/7] Avoid emiting metadata_images_changed without changes --- picard/album.py | 6 +++--- picard/cluster.py | 11 ++--------- picard/file.py | 15 ++++++++++----- picard/track.py | 2 +- picard/util/imagelist.py | 17 ++++++++++++++--- test/test_imagelist.py | 24 ++++++++++++------------ 6 files changed, 42 insertions(+), 33 deletions(-) diff --git a/picard/album.py b/picard/album.py index dc821a3b9..f69903375 100644 --- a/picard/album.py +++ b/picard/album.py @@ -718,9 +718,9 @@ class Album(DataObject, Item): if not self.update_metadata_images_enabled: return - update_metadata_images(self) - self.update(False) - self.metadata_images_changed.emit() + if update_metadata_images(self): + self.update(False) + self.metadata_images_changed.emit() def keep_original_images(self): self.enable_update_metadata_images(False) diff --git a/picard/cluster.py b/picard/cluster.py index 6280f0396..a08929a49 100644 --- a/picard/cluster.py +++ b/picard/cluster.py @@ -102,8 +102,8 @@ class FileList(QtCore.QObject, Item): def update_metadata_images(self): if self.update_metadata_images_enabled and self.can_show_coverart: - update_metadata_images(self) - self.metadata_images_changed.emit() + if update_metadata_images(self): + self.metadata_images_changed.emit() def keep_original_images(self): self.enable_update_metadata_images(False) @@ -379,13 +379,6 @@ class Cluster(FileList): yield album_name, artist_name, (files[i] for i in album) - def enable_update_metadata_images(self, enabled): - self.update_metadata_images_enabled = enabled - - def update_metadata_images(self): - if self.update_metadata_images_enabled and self.can_show_coverart: - update_metadata_images(self) - class UnclusteredFiles(Cluster): diff --git a/picard/file.py b/picard/file.py index 715545c0a..aee06cea6 100644 --- a/picard/file.py +++ b/picard/file.py @@ -251,6 +251,7 @@ class File(QtCore.QObject, Item): if tag in preserved_tags or tag in PRESERVED_TAGS: saved_metadata[tag] = values deleted_tags = self.metadata.deleted_tags + images_changed = self.metadata.images != metadata.images self.metadata.copy(metadata) for info in FILE_INFO_TAGS: metadata[info] = self.orig_metadata[info] @@ -262,12 +263,14 @@ class File(QtCore.QObject, Item): if acoustid and "acoustid_id" not in metadata.deleted_tags: self.metadata["acoustid_id"] = acoustid - self.metadata_images_changed.emit() + if images_changed: + self.metadata_images_changed.emit() def keep_original_images(self): - self.metadata.images = self.orig_metadata.images.copy() - self.update(signal=False) - self.metadata_images_changed.emit() + if self.metadata.images != self.orig_metadata.images: + self.metadata.images = self.orig_metadata.images.copy() + self.update(signal=False) + self.metadata_images_changed.emit() def has_error(self): return self.state == File.ERROR @@ -365,6 +368,7 @@ class File(QtCore.QObject, Item): temp_info = {} for info in FILE_INFO_TAGS: temp_info[info] = self.orig_metadata[info] + images_changed = self.orig_metadata.images != self.new_metadata.images # Data is copied from New to Original because New may be # a subclass to handle id3v23 if config.setting["clear_existing_tags"]: @@ -381,7 +385,8 @@ class File(QtCore.QObject, Item): self.clear_errors() self.clear_pending() self._add_path_to_metadata(self.orig_metadata) - self.metadata_images_changed.emit() + if images_changed: + self.metadata_images_changed.emit() # run post save hook run_file_post_save_processors(self) diff --git a/picard/track.py b/picard/track.py index db0d62cdd..f7c894d9f 100644 --- a/picard/track.py +++ b/picard/track.py @@ -196,8 +196,8 @@ class Track(DataObject, FileList): return self.files.remove(file) self.num_linked_files -= 1 - file.copy_metadata(file.orig_metadata, preserve_deleted=False) file.metadata_images_changed.disconnect(self.update_metadata_images) + file.copy_metadata(file.orig_metadata, preserve_deleted=False) self.album._remove_file(self, file) remove_metadata_images(self, [file]) run_file_post_removal_from_track_processors(self, file) diff --git a/picard/util/imagelist.py b/picard/util/imagelist.py index a485690e3..c503136bf 100644 --- a/picard/util/imagelist.py +++ b/picard/util/imagelist.py @@ -53,6 +53,8 @@ class ImageList(MutableSequence): return sorted(self, key=lambda image: image.normalized_types()) def __eq__(self, other): + if len(self) != len(other): + return False return self._sorted() == other._sorted() def copy(self): @@ -124,17 +126,24 @@ def _process_images(state, src_obj): def _update_state(obj, state): + changed = False for src_obj in state.sources: _process_images(state, src_obj) if state.update_new_metadata: - obj.metadata.images = ImageList(state.new_images) + updated_images = ImageList(state.new_images) + changed |= updated_images != obj.metadata.images + obj.metadata.images = updated_images obj.metadata.has_common_images = state.has_common_new_images if state.update_orig_metadata: - obj.orig_metadata.images = ImageList(state.orig_images) + updated_images = ImageList(state.orig_images) + changed |= updated_images != obj.orig_metadata.images + obj.orig_metadata.images = updated_images obj.orig_metadata.has_common_images = state.has_common_orig_images + return changed + # TODO: use functools.singledispatch when py3 is supported def _get_state(obj): @@ -188,8 +197,10 @@ def update_metadata_images(obj): Args: obj: A `Cluster`, `Album` or `Track` object with `metadata` property + Returns: + bool: True, if images where changed, False otherwise """ - _update_state(obj, _get_state(obj)) + return _update_state(obj, _get_state(obj)) def _add_images(metadata, added_images): diff --git a/test/test_imagelist.py b/test/test_imagelist.py index 4b0230184..cd375519e 100644 --- a/test/test_imagelist.py +++ b/test/test_imagelist.py @@ -67,44 +67,44 @@ class UpdateMetadataImagesTest(PicardTestCase): def test_update_cluster_images(self): cluster = Cluster('Test') cluster.files = list(self.test_files) - update_metadata_images(cluster) + self.assertTrue(update_metadata_images(cluster)) self.assertEqual(set(self.test_images), set(cluster.metadata.images)) self.assertFalse(cluster.metadata.has_common_images) cluster.files.remove(self.test_files[2]) - update_metadata_images(cluster) + self.assertFalse(update_metadata_images(cluster)) self.assertEqual(set(self.test_images), set(cluster.metadata.images)) self.assertFalse(cluster.metadata.has_common_images) cluster.files.remove(self.test_files[0]) - update_metadata_images(cluster) + self.assertTrue(update_metadata_images(cluster)) self.assertEqual(set(self.test_images[1:]), set(cluster.metadata.images)) self.assertTrue(cluster.metadata.has_common_images) cluster.files.append(self.test_files[2]) - update_metadata_images(cluster) + self.assertFalse(update_metadata_images(cluster)) self.assertEqual(set(self.test_images[1:]), set(cluster.metadata.images)) self.assertTrue(cluster.metadata.has_common_images) def test_update_track_images(self): track = Track('00000000-0000-0000-0000-000000000000') track.files = list(self.test_files) - update_metadata_images(track) + self.assertTrue(update_metadata_images(track)) self.assertEqual(set(self.test_images), set(track.orig_metadata.images)) self.assertFalse(track.orig_metadata.has_common_images) track.files.remove(self.test_files[2]) - update_metadata_images(track) + self.assertFalse(update_metadata_images(track)) self.assertEqual(set(self.test_images), set(track.orig_metadata.images)) self.assertFalse(track.orig_metadata.has_common_images) track.files.remove(self.test_files[0]) - update_metadata_images(track) + self.assertTrue(update_metadata_images(track)) self.assertEqual(set(self.test_images[1:]), set(track.orig_metadata.images)) self.assertTrue(track.orig_metadata.has_common_images) track.files.append(self.test_files[2]) - update_metadata_images(track) + self.assertFalse(update_metadata_images(track)) self.assertEqual(set(self.test_images[1:]), set(track.orig_metadata.images)) self.assertTrue(track.orig_metadata.has_common_images) @@ -116,23 +116,23 @@ class UpdateMetadataImagesTest(PicardTestCase): track2.files.append(self.test_files[1]) album.tracks = [track1, track2] album.unmatched_files.files.append(self.test_files[2]) - update_metadata_images(album) + self.assertTrue(update_metadata_images(album)) self.assertEqual(set(self.test_images), set(album.orig_metadata.images)) self.assertFalse(album.orig_metadata.has_common_images) album.tracks.remove(track2) - update_metadata_images(album) + self.assertFalse(update_metadata_images(album)) self.assertEqual(set(self.test_images), set(album.orig_metadata.images)) self.assertFalse(album.orig_metadata.has_common_images) # album.unmatched_files.files.remove(self.test_files[2]) album.tracks.remove(track1) - update_metadata_images(album) + self.assertTrue(update_metadata_images(album)) self.assertEqual(set(self.test_images[1:]), set(album.orig_metadata.images)) self.assertTrue(album.orig_metadata.has_common_images) album.tracks.append(track2) - update_metadata_images(album) + self.assertFalse(update_metadata_images(album)) self.assertEqual(set(self.test_images[1:]), set(album.orig_metadata.images)) self.assertTrue(album.orig_metadata.has_common_images) From b31f897ee0090d655f8b36575a5d4689c3b84ef9 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Tue, 1 Dec 2020 15:53:45 +0100 Subject: [PATCH 5/7] Fix propagation of keep_existing_images to tracks Unifies handling of images with new and original cover art for all supported entity types. --- picard/track.py | 8 -------- picard/util/imagelist.py | 9 +-------- 2 files changed, 1 insertion(+), 16 deletions(-) diff --git a/picard/track.py b/picard/track.py index f7c894d9f..8e30a6e77 100644 --- a/picard/track.py +++ b/picard/track.py @@ -71,7 +71,6 @@ from picard.script import ( enabled_tagger_scripts_texts, ) from picard.util.imagelist import ( - ImageList, add_metadata_images, remove_metadata_images, ) @@ -345,13 +344,6 @@ class Track(DataObject, FileList): genre = [join_genres.join(genre)] self.metadata['genre'] = genre - def keep_original_images(self): - super().keep_original_images() - if self.files: - self.metadata.images = self.orig_metadata.images.copy() - else: - self.metadata.images = ImageList() - class NonAlbumTrack(Track): diff --git a/picard/util/imagelist.py b/picard/util/imagelist.py index c503136bf..867c1a910 100644 --- a/picard/util/imagelist.py +++ b/picard/util/imagelist.py @@ -148,8 +148,7 @@ def _update_state(obj, state): # TODO: use functools.singledispatch when py3 is supported def _get_state(obj): from picard.album import Album - from picard.cluster import (Cluster, FileList) - from picard.track import Track + from picard.cluster import FileList state = ImageListState() @@ -160,12 +159,6 @@ def _get_state(obj): state.sources += obj.unmatched_files.files state.update_new_metadata = True state.update_orig_metadata = True - elif isinstance(obj, Track): - state.sources = obj.files - state.update_orig_metadata = True - elif isinstance(obj, Cluster): - state.sources = obj.files - state.update_new_metadata = True elif isinstance(obj, FileList): state.sources = obj.files state.update_new_metadata = True From 43e31fc71c9c48da283ecf0804e0c7b57fa3648c Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Wed, 2 Dec 2020 08:16:11 +0100 Subject: [PATCH 6/7] Avoid multiple inheritance of Track from QObject --- picard/cluster.py | 25 ++++++------------------- picard/track.py | 9 ++++++--- picard/ui/coverartbox.py | 8 +++----- picard/ui/item.py | 29 +++++++++++++++++++++++++++++ picard/util/imagelist.py | 3 ++- 5 files changed, 46 insertions(+), 28 deletions(-) diff --git a/picard/cluster.py b/picard/cluster.py index a08929a49..0c090d212 100644 --- a/picard/cluster.py +++ b/picard/cluster.py @@ -67,19 +67,21 @@ from picard.util.imagelist import ( ) from picard.util.progresscheckpoints import ProgressCheckpoints -from picard.ui.item import Item +from picard.ui.item import ( + FileListItem, + Item, +) -class FileList(QtCore.QObject, Item): +class FileList(QtCore.QObject, FileListItem): metadata_images_changed = QtCore.pyqtSignal() def __init__(self, files=None): QtCore.QObject.__init__(self) + FileListItem.__init__(self, files) self.metadata = Metadata() self.orig_metadata = Metadata() - self.files = files or [] - self.update_metadata_images_enabled = True for file in self.files: if self.can_show_coverart: file.metadata_images_changed.connect(self.update_metadata_images) @@ -97,21 +99,6 @@ class FileList(QtCore.QObject, Item): def can_show_coverart(self): return True - def enable_update_metadata_images(self, enabled): - self.update_metadata_images_enabled = enabled - - def update_metadata_images(self): - if self.update_metadata_images_enabled and self.can_show_coverart: - if update_metadata_images(self): - self.metadata_images_changed.emit() - - def keep_original_images(self): - self.enable_update_metadata_images(False) - for file in list(self.files): - file.keep_original_images() - self.enable_update_metadata_images(True) - self.update_metadata_images() - class Cluster(FileList): diff --git a/picard/track.py b/picard/track.py index 8e30a6e77..34bb73fc7 100644 --- a/picard/track.py +++ b/picard/track.py @@ -48,7 +48,6 @@ from picard import ( config, log, ) -from picard.cluster import FileList from picard.const import ( DATA_TRACK_TITLE, SILENCE_TRACK_TITLE, @@ -76,6 +75,8 @@ from picard.util.imagelist import ( ) from picard.util.textencoding import asciipunct +from picard.ui.item import FileListItem + _TRANSLATE_TAGS = { "hip hop": "Hip-Hop", @@ -134,13 +135,15 @@ class TrackArtist(DataObject): super().__init__(ta_id) -class Track(DataObject, FileList): +class Track(DataObject, FileListItem): metadata_images_changed = QtCore.pyqtSignal() def __init__(self, track_id, album=None): DataObject.__init__(self, track_id) - FileList.__init__(self) + FileListItem.__init__(self) + self.metadata = Metadata() + self.orig_metadata = Metadata() self.album = album self.num_linked_files = 0 self.scripted_metadata = Metadata() diff --git a/picard/ui/coverartbox.py b/picard/ui/coverartbox.py index a947290f2..0d16e5200 100644 --- a/picard/ui/coverartbox.py +++ b/picard/ui/coverartbox.py @@ -48,10 +48,7 @@ from picard import ( log, ) from picard.album import Album -from picard.cluster import ( - Cluster, - FileList, -) +from picard.cluster import Cluster from picard.const import MAX_COVERS_TO_STACK from picard.coverart.image import ( CoverArtImage, @@ -62,6 +59,7 @@ from picard.track import Track from picard.util import imageinfo from picard.util.lrucache import LRUCache +from picard.ui.item import FileListItem from picard.ui.widgets import ActiveLabel @@ -502,7 +500,7 @@ class CoverArtBox(QtWidgets.QGroupBox): track.enable_update_metadata_images(True) album.enable_update_metadata_images(True) album.update(update_tracks=False) - elif isinstance(self.item, FileList): + elif isinstance(self.item, FileListItem): parents = set() filelist = self.item filelist.enable_update_metadata_images(False) diff --git a/picard/ui/item.py b/picard/ui/item.py index 98783e549..f5a6b6dc3 100644 --- a/picard/ui/item.py +++ b/picard/ui/item.py @@ -24,6 +24,7 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. from picard import log +from picard.util.imagelist import update_metadata_images class Item(object): @@ -97,3 +98,31 @@ class Item(object): def clear_errors(self): self._errors = [] + + +class FileListItem(Item): + + def __init__(self, files=None): + super().__init__() + self.files = files or [] + self.update_metadata_images_enabled = True + + def iterfiles(self, save=False): + for file in self.files: + yield file + + def enable_update_metadata_images(self, enabled): + self.update_metadata_images_enabled = enabled + + def update_metadata_images(self): + if self.update_metadata_images_enabled and self.can_show_coverart: + if update_metadata_images(self): + self.metadata_images_changed.emit() + + def keep_original_images(self): + self.enable_update_metadata_images(False) + for file in list(self.files): + if file.can_show_coverart: + file.keep_original_images() + self.enable_update_metadata_images(True) + self.update_metadata_images() diff --git a/picard/util/imagelist.py b/picard/util/imagelist.py index 867c1a910..a8e9c8ad7 100644 --- a/picard/util/imagelist.py +++ b/picard/util/imagelist.py @@ -149,6 +149,7 @@ def _update_state(obj, state): def _get_state(obj): from picard.album import Album from picard.cluster import FileList + from picard.track import Track state = ImageListState() @@ -159,7 +160,7 @@ def _get_state(obj): state.sources += obj.unmatched_files.files state.update_new_metadata = True state.update_orig_metadata = True - elif isinstance(obj, FileList): + elif isinstance(obj, FileList) or isinstance(obj, Track): state.sources = obj.files state.update_new_metadata = True state.update_orig_metadata = True From 1cb8c77d6ed4737e34bdbeec48581d687531afe8 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Wed, 2 Dec 2020 08:59:21 +0100 Subject: [PATCH 7/7] Fixed cover art display for tracks and albums Tracks don't loose cover art when removing files, linked file metadata is used. Albums show original metadata depending on tracks. --- picard/album.py | 1 + picard/ui/coverartbox.py | 8 +++++++- picard/util/imagelist.py | 5 ++++- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/picard/album.py b/picard/album.py index f69903375..cb0ea0842 100644 --- a/picard/album.py +++ b/picard/album.py @@ -403,6 +403,7 @@ class Album(DataObject, Item): file.move(self.unmatched_files) self.metadata = self._new_metadata self.orig_metadata.copy(self.metadata) + self.orig_metadata.images.clear() self.tracks = self._new_tracks del self._new_metadata del self._new_tracks diff --git a/picard/ui/coverartbox.py b/picard/ui/coverartbox.py index 0d16e5200..6ffca5c46 100644 --- a/picard/ui/coverartbox.py +++ b/picard/ui/coverartbox.py @@ -376,7 +376,13 @@ class CoverArtBox(QtWidgets.QGroupBox): return metadata = self.item.metadata - orig_metadata = self.item.orig_metadata + orig_metadata = None + if isinstance(self.item, Track): + track = self.item + if track.num_linked_files == 1: + orig_metadata = track.files[0].orig_metadata + elif hasattr(self.item, 'orig_metadata'): + orig_metadata = self.item.orig_metadata if not metadata or not metadata.images: self.cover_art.set_metadata(orig_metadata) diff --git a/picard/util/imagelist.py b/picard/util/imagelist.py index a8e9c8ad7..6543faa8f 100644 --- a/picard/util/imagelist.py +++ b/picard/util/imagelist.py @@ -160,7 +160,10 @@ def _get_state(obj): state.sources += obj.unmatched_files.files state.update_new_metadata = True state.update_orig_metadata = True - elif isinstance(obj, FileList) or isinstance(obj, Track): + elif isinstance(obj, Track): + state.sources = obj.files + state.update_orig_metadata = True + elif isinstance(obj, FileList): state.sources = obj.files state.update_new_metadata = True state.update_orig_metadata = True