Merge pull request #2488 from zas/image_list_cleanup

Image list cleanup

picard/album.py

@@ -63,6 +63,7 @@ from picard.i18n import (
     N_,
     gettext as _,
 )
+from picard.item import MetadataItem
 from picard.mbjson import (
     medium_to_metadata,
     release_group_to_metadata,
@@ -89,15 +90,8 @@ from picard.util import (
     format_time,
     mbid_validate,
 )
-from picard.util.imagelist import (
-    add_metadata_images,
-    remove_metadata_images,
-    update_metadata_images,
-)
 from picard.util.textencoding import asciipunct
 
-from picard.ui.item import Item
-
 
 RECORDING_QUERY_LIMIT = 100
 
@@ -131,15 +125,11 @@ class ParseResult(IntEnum):
     MISSING_TRACK_RELS = 2
 
 
-class Album(DataObject, Item):
+class Album(DataObject, MetadataItem):
-
-    metadata_images_changed = QtCore.pyqtSignal()
 
     def __init__(self, album_id, discid=None):
         DataObject.__init__(self, album_id)
         self.tagger = QtCore.QCoreApplication.instance()
-        self.metadata = Metadata()
-        self.orig_metadata = Metadata()
         self.tracks = []
         self.loaded = False
         self.load_task = None
@@ -156,7 +146,7 @@ class Album(DataObject, Item):
         self.unmatched_files.metadata_images_changed.connect(self.update_metadata_images)
         self.status = AlbumStatus.NONE
         self._album_artists = []
-        self.update_metadata_images_enabled = True
+        self.update_children_metadata_attrs = {'metadata', 'orig_metadata'}
 
     def __repr__(self):
         return '<Album %s %r>' % (self.id, self.metadata['album'])
@@ -170,9 +160,6 @@ class Album(DataObject, Item):
     def iter_correctly_matched_tracks(self):
         yield from (track for track in self.tracks if track.num_linked_files == 1)
 
-    def enable_update_metadata_images(self, enabled):
-        self.update_metadata_images_enabled = enabled
-
     def append_album_artist(self, album_artist_id):
         """Append artist id to the list of album artists
         and return an AlbumArtist instance"""
@@ -656,13 +643,13 @@ class Album(DataObject, Item):
         self._files_count += 1
         if new_album:
             self.update(update_tracks=False)
-            add_metadata_images(self, [file])
+            self.add_metadata_images_from_children([file])
 
     def remove_file(self, track, file, new_album=True):
         self._files_count -= 1
         if new_album:
             self.update(update_tracks=False)
-            remove_metadata_images(self, [file])
+            self.remove_metadata_images_from_children([file])
 
     @staticmethod
     def _match_files(files, tracks, unmatched_files, threshold=0):
@@ -859,8 +846,8 @@ class Album(DataObject, Item):
         if not self.update_metadata_images_enabled:
             return
 
-        if update_metadata_images(self):
+        if self.update_metadata_images_from_children():
-            self.update(False)
+            self.update(update_tracks=False)
             self.metadata_images_changed.emit()
 
     def keep_original_images(self):
@@ -872,6 +859,12 @@ class Album(DataObject, Item):
         self.enable_update_metadata_images(True)
         self.update_metadata_images()
 
+    def children_metadata_items(self):
+        for track in self.tracks:
+            yield track
+            yield from track.files
+        yield from self.unmatched_files.files
+
 
 class NatAlbum(Album):
 
@@ -891,7 +884,7 @@ class NatAlbum(Album):
             for file in track.files:
                 track.update_file_metadata(file)
         self.enable_update_metadata_images(True)
-        super().update(update_tracks, update_selection)
+        super().update(update_tracks=update_tracks, update_selection=update_selection)
 
     def _finalize_loading(self, error):
         self.update()

picard/cluster.py

@@ -50,27 +50,19 @@ from picard.i18n import (
     N_,
     gettext as _,
 )
-from picard.metadata import (
+from picard.item import (
-    Metadata,
+    FileListItem,
-    SimMatchRelease,
+    Item,
 )
+from picard.metadata import SimMatchRelease
 from picard.track import Track
 from picard.util import (
     album_artist_from_path,
     find_best_match,
     format_time,
 )
-from picard.util.imagelist import (
-    add_metadata_images,
-    remove_metadata_images,
-    update_metadata_images,
-)
 
 from picard.ui.enums import MainAction
-from picard.ui.item import (
-    FileListItem,
-    Item,
-)
 
 
 # Weights for different elements when comparing a cluster to a release
@@ -87,17 +79,13 @@ CLUSTER_COMPARISON_WEIGHTS = {
 
 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()
         if self.files and self.can_show_coverart:
             for file in self.files:
                 file.metadata_images_changed.connect(self.update_metadata_images)
-            update_metadata_images(self)
+            self.update_metadata_images_from_children()
 
     def iterfiles(self, save=False):
         yield from self.files
@@ -142,9 +130,9 @@ class Cluster(FileList):
     def _update_related_album(self, added_files=None, removed_files=None):
         if self.related_album:
             if added_files:
-                add_metadata_images(self.related_album, added_files)
+                self.related_album.add_metadata_images_from_children(added_files)
             if removed_files:
-                remove_metadata_images(self.related_album, removed_files)
+                self.related_album.remove_metadata_images_from_children(removed_files)
             self.related_album.update()
 
     def add_files(self, files, new_album=True):
@@ -161,7 +149,7 @@ class Cluster(FileList):
         self.files.extend(added_files)
         self.update(signal=False)
         if self.can_show_coverart:
-            add_metadata_images(self, added_files)
+            self.add_metadata_images_from_children(added_files)
         self.item.add_files(added_files)
         if new_album:
             self._update_related_album(added_files=added_files)
@@ -177,7 +165,7 @@ class Cluster(FileList):
         self.item.remove_file(file)
         if self.can_show_coverart:
             file.metadata_images_changed.disconnect(self.update_metadata_images)
-            remove_metadata_images(self, [file])
+            self.remove_metadata_images_from_children([file])
         if new_album:
             self._update_related_album(removed_files=[file])
         self.tagger.window.set_processing(False)

picard/file.py

@@ -75,6 +75,7 @@ from picard.i18n import (
     N_,
     gettext as _,
 )
+from picard.item import MetadataItem
 from picard.metadata import (
     Metadata,
     SimMatchTrack,
@@ -111,8 +112,6 @@ from picard.util.tags import (
     PRESERVED_TAGS,
 )
 
-from picard.ui.item import Item
-
 
 FILE_COMPARISON_WEIGHTS = {
     'album': 5,
@@ -136,9 +135,7 @@ class FileErrorType(Enum):
     PARSER = auto()
 
 
-class File(QtCore.QObject, Item):
+class File(QtCore.QObject, MetadataItem):
-
-    metadata_images_changed = QtCore.pyqtSignal()
 
     NAME = None
 
@@ -173,9 +170,6 @@ class File(QtCore.QObject, Item):
         self.state = File.PENDING
         self.error_type = FileErrorType.UNKNOWN
 
-        self.orig_metadata = Metadata()
-        self.metadata = Metadata()
-
         self.similarity = 1.0
         self.parent = None
 

picard/item.py

@@ -25,9 +25,11 @@
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
 
+from PyQt6 import QtCore
+
 from picard import log
 from picard.i18n import ngettext
-from picard.util.imagelist import update_metadata_images
+from picard.metadata import Metadata
 
 
 class Item(object):
@@ -155,22 +157,23 @@ class Item(object):
                             number_of_images) % number_of_images
 
 
-class FileListItem(Item):
+class MetadataItem(Item):
+    metadata_images_changed = QtCore.pyqtSignal()
 
-    def __init__(self, files=None):
+    def __init__(self):
         super().__init__()
-        self.files = files or []
+        self.metadata = Metadata()
+        self.orig_metadata = Metadata()
         self.update_metadata_images_enabled = True
-
+        self.update_children_metadata_attrs = {}
-    def iterfiles(self, save=False):
+        self.iter_children_items_metadata_ignore_attrs = {}
-        yield from self.files
 
     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):
+            if self.update_metadata_images_from_children():
                 self.metadata_images_changed.emit()
 
     def keep_original_images(self):
@@ -180,3 +183,110 @@ class FileListItem(Item):
                 file.keep_original_images()
         self.enable_update_metadata_images(True)
         self.update_metadata_images()
+
+    def children_metadata_items(self):
+        """Yield MetadataItems that are children of the current object"""
+
+    def iter_children_items_metadata(self, metadata_attr):
+        for s in self.children_metadata_items():
+            if metadata_attr in s.iter_children_items_metadata_ignore_attrs:
+                continue
+            yield getattr(s, metadata_attr)
+
+    @staticmethod
+    def get_sources_metadata_images(sources_metadata):
+        images = set()
+        for s in sources_metadata:
+            images = images.union(s.images)
+        return images
+
+    def remove_metadata_images_from_children(self, removed_sources):
+        """Remove the images in the metadata of `removed_sources` from the metadata.
+
+        Args:
+            removed_sources: List of child objects (`Track` or `File`) which's metadata images should be removed from
+        """
+        changed = False
+
+        for metadata_attr in self.update_children_metadata_attrs:
+            removed_images = self.get_sources_metadata_images(getattr(s, metadata_attr) for s in removed_sources)
+            sources_metadata = list(self.iter_children_items_metadata(metadata_attr))
+            metadata = getattr(self, metadata_attr)
+            changed |= metadata.remove_images(sources_metadata, removed_images)
+
+        return changed
+
+    def add_metadata_images_from_children(self, added_sources):
+        """Add the images in the metadata of `added_sources` to the metadata.
+
+        Args:
+            added_sources: List of child objects (`Track` or `File`) which's metadata images should be added to current object
+        """
+        changed = False
+
+        for metadata_attr in self.update_children_metadata_attrs:
+            added_images = self.get_sources_metadata_images(getattr(s, metadata_attr) for s in added_sources)
+            metadata = getattr(self, metadata_attr)
+            changed |= metadata.add_images(added_images)
+
+        return changed
+
+    def update_metadata_images_from_children(self):
+        """Update the metadata images of the current object based on its children.
+
+        Based on the type of the current object, this will update `self.metadata.images` to
+        represent the metadata images of all children (`Track` or `File` objects).
+
+        This method will iterate over all children and completely rebuild
+        `self.metadata.images`. Whenever possible the more specific functions
+        `add_metadata_images_from_children` or `remove_metadata_images_from_children` should be used.
+
+        Returns:
+            bool: True, if images where changed, False otherwise
+        """
+        from picard.util.imagelist import ImageList
+
+        class ImageListState:
+            def __init__(self):
+                self.images = {}
+                self.has_common_images = True
+                self.first_obj = True
+
+            def process_images(self, src_obj_metadata):
+                src_dict = src_obj_metadata.images.hash_dict()
+                prev_len = len(self.images)
+                self.images.update(src_dict)
+                if len(self.images) != prev_len:
+                    if not self.first_obj:
+                        self.has_common_images = False
+                if self.first_obj:
+                    self.first_obj = False
+
+        changed = False
+
+        for metadata_attr in self.update_children_metadata_attrs:
+            state = ImageListState()
+            for src_obj_metadata in self.iter_children_items_metadata(metadata_attr):
+                state.process_images(src_obj_metadata)
+
+            updated_images = ImageList(state.images.values())
+            metadata = getattr(self, metadata_attr)
+            changed |= set(updated_images.hash_dict()) != set(metadata.images.hash_dict())
+            metadata.images = updated_images
+            metadata.has_common_images = state.has_common_images
+
+        return changed
+
+
+class FileListItem(MetadataItem):
+
+    def __init__(self, files=None):
+        super().__init__()
+        self.files = files or []
+        self.update_children_metadata_attrs = {'metadata', 'orig_metadata'}
+
+    def iterfiles(self, save=False):
+        yield from self.files
+
+    def children_metadata_items(self):
+        yield from self.files

picard/metadata.py

@@ -586,6 +586,61 @@ class Metadata(MutableMapping):
     def __str__(self):
         return ("store: %r\ndeleted: %r\nimages: %r\nlength: %r" % (self._store, self.deleted_tags, [str(img) for img in self.images], self.length))
 
+    def add_images(self, added_images):
+        if not added_images:
+            return False
+
+        current_images = set(self.images)
+        if added_images.isdisjoint(current_images):
+            self.images = ImageList(current_images.union(added_images))
+            self.has_common_images = False
+            return True
+
+        return False
+
+    def remove_images(self, sources, removed_images):
+        """Removes `removed_images` from `images`, but only if they are not included in `sources`.
+
+        Args:
+            sources: List of source `Metadata` objects
+            removed_images: Set of `CoverArt` to removed
+
+        Returns:
+            True if self.images was modified, False else
+        """
+        if not self.images or not removed_images:
+            return False
+
+        if not sources:
+            self.images = ImageList()
+            self.has_common_images = True
+            return True
+
+        current_images = set(self.images)
+
+        if self.has_common_images and current_images == removed_images:
+            return False
+
+        common_images = True  # True, if all children share the same images
+        previous_images = None
+
+        # Iterate over all sources and check whether the images proposed to be
+        # removed are used in any sources. Images used in existing sources
+        # must not be removed.
+        for source_metadata in sources:
+            source_images = set(source_metadata.images)
+            if previous_images and common_images and previous_images != source_images:
+                common_images = False
+            previous_images = set(source_metadata.images)  # Remember for next iteration
+            removed_images = removed_images.difference(source_images)
+            if not removed_images and not common_images:
+                return False  # No images left to remove, abort immediately
+
+        new_images = current_images.difference(removed_images)
+        self.images = ImageList(new_images)
+        self.has_common_images = common_images
+        return True
+
 
 class MultiMetadataProxy:
     """

picard/track.py

@@ -61,6 +61,7 @@ from picard.file import (
     run_file_post_removal_from_track_processors,
 )
 from picard.i18n import gettext as _
+from picard.item import FileListItem
 from picard.mbjson import recording_to_metadata
 from picard.metadata import (
     Metadata,
@@ -73,15 +74,9 @@ from picard.script import (
     enabled_tagger_scripts_texts,
 )
 from picard.util import pattern_as_regex
-from picard.util.imagelist import (
+from picard.util.imagelist import ImageList
-    ImageList,
-    add_metadata_images,
-    remove_metadata_images,
-)
 from picard.util.textencoding import asciipunct
 
-from picard.ui.item import FileListItem
-
 
 class TagGenreFilter:
 
@@ -131,18 +126,15 @@ class TrackArtist(DataObject):
 
 class Track(DataObject, FileListItem):
 
-    metadata_images_changed = QtCore.pyqtSignal()
-
     def __init__(self, track_id, album=None):
         DataObject.__init__(self, track_id)
         FileListItem.__init__(self)
         self.tagger = QtCore.QCoreApplication.instance()
-        self.metadata = Metadata()
-        self.orig_metadata = Metadata()
         self.album = album
         self.scripted_metadata = Metadata()
         self._track_artists = []
         self._orig_images = None
+        self.iter_children_items_metadata_ignore_attrs = {'orig_metadata'}
 
     @property
     def num_linked_files(self):
@@ -160,7 +152,7 @@ class Track(DataObject, FileListItem):
                 self.orig_metadata.images = ImageList()
             self.files.append(file)
         self.update_file_metadata(file)
-        add_metadata_images(self, [file])
+        self.add_metadata_images_from_children([file])
         self.album.add_file(self, file, new_album=new_album)
         file.metadata_images_changed.connect(self.update_metadata_images)
         run_file_post_addition_to_track_processors(self, file)
@@ -198,7 +190,7 @@ class Track(DataObject, FileListItem):
         file.metadata_images_changed.disconnect(self.update_metadata_images)
         file.copy_metadata(file.orig_metadata, preserve_deleted=False)
         self.album.remove_file(self, file, new_album=new_album)
-        remove_metadata_images(self, [file])
+        self.remove_metadata_images_from_children([file])
         if not self.files and self._orig_images:
             self.orig_metadata.images = self._orig_images
             self.metadata.images = self._orig_images.copy()
@@ -398,7 +390,7 @@ class NonAlbumTrack(Track):
         self.status = _("[loading recording information]")
         self.clear_errors()
         self.loaded = False
-        self.album.update(True)
+        self.album.update(update_tracks=True)
         config = get_config()
         require_authentication = False
         inc = {
@@ -446,7 +438,7 @@ class NonAlbumTrack(Track):
     def _set_error(self, error):
         self.error_append(error)
         self.status = _("[could not load recording %s]") % self.id
-        self.album.update(True)
+        self.album.update(update_tracks=True)
 
     def _parse_recording(self, recording):
         m = self.metadata
@@ -460,7 +452,7 @@ class NonAlbumTrack(Track):
         if self.callback:
             self.callback()
             self.callback = None
-        self.album.update(True)
+        self.album.update(update_tracks=True)
 
     def _customize_metadata(self):
         super()._customize_metadata()

picard/ui/coverartbox.py

@@ -56,6 +56,7 @@ from picard.coverart.image import (
 )
 from picard.file import File
 from picard.i18n import gettext as _
+from picard.item import FileListItem
 from picard.track import Track
 from picard.util import (
     imageinfo,
@@ -64,7 +65,6 @@ from picard.util import (
 from picard.util.lrucache import LRUCache
 
 from picard.ui.colors import interface_colors
-from picard.ui.item import FileListItem
 from picard.ui.widgets import ActiveLabel
 
 

picard/util/imagelist.py

@@ -32,7 +32,7 @@ class ImageList(MutableSequence):
     def __init__(self, iterable=()):
         self._images = list(iterable)
         self._hash_dict = {}
-        self._changed = True
+        self._dirty = True
 
     def __len__(self):
         return len(self._images)
@@ -41,16 +41,17 @@ class ImageList(MutableSequence):
         return self._images[index]
 
     def __setitem__(self, index, value):
-        self._images[index] = value
+        if self._images[index] != value:
-        self._changed = True
+            self._images[index] = value
+            self._dirty = True
 
     def __delitem__(self, index):
         del self._images[index]
-        self._changed = True
+        self._dirty = True
 
     def insert(self, index, value):
-        self._changed = True
+        self._images.insert(index, value)
-        return self._images.insert(index, value)
+        self._dirty = True
 
     def __repr__(self):
         return '%s(%r)' % (self.__class__.__name__, self._images)
@@ -93,214 +94,10 @@ class ImageList(MutableSequence):
 
     def strip_front_images(self):
         self._images = [image for image in self._images if not image.is_front_image()]
-        self._changed = True
+        self._dirty = True
 
     def hash_dict(self):
-        if self._changed:
+        if self._dirty:
             self._hash_dict = {img.datahash.hash(): img for img in self._images}
-            self._changed = False
+            self._dirty = False
         return self._hash_dict
-
-
-class ImageListState:
-    def __init__(self):
-        self.new_images = {}
-        self.orig_images = {}
-        self.sources = []
-        self.has_common_new_images = True
-        self.has_common_orig_images = True
-        self.first_new_obj = True
-        self.first_orig_obj = True
-        # The next variables specify what will be updated
-        self.update_new_metadata = False
-        self.update_orig_metadata = False
-
-
-def _process_images(state, src_obj, Track):
-    # Check new images
-    if state.update_new_metadata:
-        src_dict = src_obj.metadata.images.hash_dict()
-        prev_len = len(state.new_images)
-        state.new_images.update(src_dict)
-        if len(state.new_images) != prev_len:
-            if not state.first_new_obj:
-                state.has_common_new_images = False
-        if state.first_new_obj:
-            state.first_new_obj = False
-
-    if state.update_orig_metadata and not isinstance(src_obj, Track):
-        # Check orig images, but not for Tracks (which don't have a useful orig_metadata)
-        src_dict = src_obj.orig_metadata.images.hash_dict()
-        prev_len = len(state.orig_images)
-        state.orig_images.update(src_dict)
-        if len(state.orig_images) != prev_len:
-            if not state.first_orig_obj:
-                state.has_common_orig_images = False
-        if state.first_orig_obj:
-            state.first_orig_obj = False
-
-
-def _update_state(obj, state):
-    from picard.track import Track
-
-    changed = False
-    for src_obj in state.sources:
-        _process_images(state, src_obj, Track)
-
-    if state.update_new_metadata:
-        updated_images = ImageList(state.new_images.values())
-        changed |= updated_images.hash_dict().keys() != obj.metadata.images.hash_dict().keys()
-        obj.metadata.images = updated_images
-        obj.metadata.has_common_images = state.has_common_new_images
-
-    if state.update_orig_metadata:
-        updated_images = ImageList(state.orig_images.values())
-        changed |= updated_images.hash_dict().keys() != obj.orig_metadata.images.hash_dict().keys()
-        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):
-    from picard.album import Album
-
-    from picard.ui.item import FileListItem
-
-    state = ImageListState()
-
-    if isinstance(obj, Album):
-        for track in obj.tracks:
-            state.sources.append(track)
-            state.sources += track.files
-        state.sources += obj.unmatched_files.files
-        state.update_new_metadata = True
-        state.update_orig_metadata = True
-    elif isinstance(obj, FileListItem):
-        state.sources = obj.files
-        state.update_new_metadata = True
-        state.update_orig_metadata = True
-
-    return state
-
-
-def _get_metadata_images(state, sources):
-    new_images = set()
-    orig_images = set()
-    for s in sources:
-        if state.update_new_metadata:
-            new_images = new_images.union(s.metadata.images)
-        if state.update_orig_metadata:
-            orig_images = orig_images.union(s.orig_metadata.images)
-    return (new_images, orig_images)
-
-
-def update_metadata_images(obj):
-    """Update the metadata images `obj` based on its children.
-
-    Based on the type of `obj` this will update `obj.metadata.images` to
-    represent the metadata images of all children (`Track` or `File` objects).
-
-    This method will iterate over all children and completely rebuild
-    `obj.metadata.images`. Whenever possible the more specific functions
-    `add_metadata_images` or `remove_metadata_images` should be used.
-
-    Args:
-        obj: A `Cluster`, `Album` or `Track` object with `metadata` property
-    Returns:
-        bool: True, if images where changed, False otherwise
-    """
-    return _update_state(obj, _get_state(obj))
-
-
-def _add_images(metadata, added_images):
-    if not added_images:
-        return False
-
-    current_images = set(metadata.images)
-    if added_images.isdisjoint(current_images):
-        metadata.images = ImageList(current_images.union(added_images))
-        metadata.has_common_images = False
-        return True
-
-    return False
-
-
-def add_metadata_images(obj, added_sources):
-    """Add the images in the metadata of `added_sources` to the metadata of `obj`.
-
-    Args:
-        obj: A `Cluster`, `Album` or `Track` object with `metadata` property
-        added_sources: List of child objects (`Track` or `File`) which's metadata images should be added to `obj`
-    """
-    state = _get_state(obj)
-    (added_new_images, added_orig_images) = _get_metadata_images(state, added_sources)
-    changed = False
-
-    if state.update_new_metadata:
-        changed |= _add_images(obj.metadata, added_new_images)
-    if state.update_orig_metadata:
-        changed |= _add_images(obj.orig_metadata, added_orig_images)
-
-    return changed
-
-
-def _remove_images(metadata, sources, removed_images):
-    """Removes `removed_images` from metadata `images`, but only if they are not included in `sources`.
-
-    Args:
-        metadata: `Metadata` object from which images should be removed
-        sources: List of source `Metadata` objects
-        removed_images: Set of `CoverArt` proposed for removal from `metadata`
-    """
-    if not metadata.images or not removed_images:
-        return
-
-    if not sources:
-        metadata.images = ImageList()
-        metadata.has_common_images = True
-        return
-
-    current_images = set(metadata.images)
-
-    if metadata.has_common_images and current_images == removed_images:
-        return
-
-    common_images = True  # True, if all children share the same images
-    previous_images = None
-
-    # Iterate over all sources and check whether the images proposed to be
-    # removed are used in any sources. Images used in existing sources
-    # must not be removed.
-    for source_metadata in sources:
-        source_images = set(source_metadata.images)
-        if previous_images and common_images and previous_images != source_images:
-            common_images = False
-        previous_images = set(source_metadata.images)  # Remember for next iteration
-        removed_images = removed_images.difference(source_images)
-        if not removed_images and not common_images:
-            return  # No images left to remove, abort immediately
-
-    metadata.images = ImageList(current_images.difference(removed_images))
-    metadata.has_common_images = common_images
-
-
-def remove_metadata_images(obj, removed_sources):
-    """Remove the images in the metadata of `removed_sources` from the metadata of `obj`.
-
-    Args:
-        obj: A `Cluster`, `Album` or `Track` object with `metadata` property
-        removed_sources: List of child objects (`Track` or `File`) which's metadata images should be removed from `obj`
-    """
-    from picard.track import Track
-
-    state = _get_state(obj)
-    (removed_new_images, removed_orig_images) = _get_metadata_images(state, removed_sources)
-
-    if state.update_new_metadata:
-        sources = [s.metadata for s in state.sources]
-        _remove_images(obj.metadata, sources, removed_new_images)
-    if state.update_orig_metadata:
-        sources = [s.orig_metadata for s in state.sources if not isinstance(s, Track)]
-        _remove_images(obj.orig_metadata, sources, removed_orig_images)

test/test_imagelist.py

@@ -31,12 +31,7 @@ from picard.cluster import Cluster
 from picard.coverart.image import CoverArtImage
 from picard.file import File
 from picard.track import Track
-from picard.util.imagelist import (
+from picard.util.imagelist import ImageList
-    ImageList,
-    add_metadata_images,
-    remove_metadata_images,
-    update_metadata_images,
-)
 
 
 def create_test_files():
@@ -67,44 +62,44 @@ class UpdateMetadataImagesTest(PicardTestCase):
     def test_update_cluster_images(self):
         cluster = Cluster('Test')
         cluster.files = list(self.test_files)
-        self.assertTrue(update_metadata_images(cluster))
+        self.assertTrue(cluster.update_metadata_images_from_children())
         self.assertEqual(set(self.test_images), set(cluster.metadata.images))
         self.assertFalse(cluster.metadata.has_common_images)
 
         cluster.files.remove(self.test_files[2])
-        self.assertFalse(update_metadata_images(cluster))
+        self.assertFalse(cluster.update_metadata_images_from_children())
         self.assertEqual(set(self.test_images), set(cluster.metadata.images))
         self.assertFalse(cluster.metadata.has_common_images)
 
         cluster.files.remove(self.test_files[0])
-        self.assertTrue(update_metadata_images(cluster))
+        self.assertTrue(cluster.update_metadata_images_from_children())
         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])
-        self.assertFalse(update_metadata_images(cluster))
+        self.assertFalse(cluster.update_metadata_images_from_children())
         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)
-        self.assertTrue(update_metadata_images(track))
+        self.assertTrue(track.update_metadata_images_from_children())
         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])
-        self.assertFalse(update_metadata_images(track))
+        self.assertFalse(track.update_metadata_images_from_children())
         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])
-        self.assertTrue(update_metadata_images(track))
+        self.assertTrue(track.update_metadata_images_from_children())
         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])
-        self.assertFalse(update_metadata_images(track))
+        self.assertFalse(track.update_metadata_images_from_children())
         self.assertEqual(set(self.test_images[1:]), set(track.orig_metadata.images))
         self.assertTrue(track.orig_metadata.has_common_images)
 
@@ -116,23 +111,22 @@ class UpdateMetadataImagesTest(PicardTestCase):
         track2.files.append(self.test_files[1])
         album.tracks = [track1, track2]
         album.unmatched_files.files.append(self.test_files[2])
-        self.assertTrue(update_metadata_images(album))
+        self.assertTrue(album.update_metadata_images_from_children())
         self.assertEqual(set(self.test_images), set(album.orig_metadata.images))
         self.assertFalse(album.orig_metadata.has_common_images)
 
         album.tracks.remove(track2)
-        self.assertFalse(update_metadata_images(album))
+        self.assertFalse(album.update_metadata_images_from_children())
         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)
-        self.assertTrue(update_metadata_images(album))
+        self.assertTrue(album.update_metadata_images_from_children())
         self.assertEqual(set(self.test_images[1:]), set(album.orig_metadata.images))
         self.assertTrue(album.orig_metadata.has_common_images)
 
         album.tracks.append(track2)
-        self.assertFalse(update_metadata_images(album))
+        self.assertFalse(album.update_metadata_images_from_children())
         self.assertEqual(set(self.test_images[1:]), set(album.orig_metadata.images))
         self.assertTrue(album.orig_metadata.has_common_images)
 
@@ -146,61 +140,61 @@ class RemoveMetadataImagesTest(PicardTestCase):
     def test_remove_from_cluster(self):
         cluster = Cluster('Test')
         cluster.files = list(self.test_files)
-        update_metadata_images(cluster)
+        self.assertTrue(cluster.update_metadata_images_from_children())
         cluster.files.remove(self.test_files[0])
-        remove_metadata_images(cluster, [self.test_files[0]])
+        self.assertTrue(cluster.remove_metadata_images_from_children([self.test_files[0]]))
         self.assertEqual(set(self.test_images[1:]), set(cluster.metadata.images))
         self.assertTrue(cluster.metadata.has_common_images)
 
     def test_remove_from_cluster_with_common_images(self):
         cluster = Cluster('Test')
         cluster.files = list(self.test_files[1:])
-        update_metadata_images(cluster)
+        self.assertTrue(cluster.update_metadata_images_from_children())
         cluster.files.remove(self.test_files[1])
-        remove_metadata_images(cluster, [self.test_files[1]])
+        self.assertFalse(cluster.remove_metadata_images_from_children([self.test_files[1]]))
         self.assertEqual(set(self.test_images[1:]), set(cluster.metadata.images))
         self.assertTrue(cluster.metadata.has_common_images)
 
     def test_remove_from_empty_cluster(self):
         cluster = Cluster('Test')
         cluster.files.append(File('test1.flac'))
-        update_metadata_images(cluster)
+        self.assertFalse(cluster.update_metadata_images_from_children())
-        remove_metadata_images(cluster, [cluster.files[0]])
+        self.assertFalse(cluster.remove_metadata_images_from_children([cluster.files[0]]))
         self.assertEqual(set(), set(cluster.metadata.images))
         self.assertTrue(cluster.metadata.has_common_images)
 
     def test_remove_from_track(self):
         track = Track('00000000-0000-0000-0000-000000000000')
         track.files = list(self.test_files)
-        update_metadata_images(track)
+        self.assertTrue(track.update_metadata_images_from_children())
         track.files.remove(self.test_files[0])
-        remove_metadata_images(track, [self.test_files[0]])
+        self.assertTrue(track.remove_metadata_images_from_children([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.files = list(self.test_files[1:])
-        update_metadata_images(track)
+        self.assertTrue(track.update_metadata_images_from_children())
         track.files.remove(self.test_files[1])
-        remove_metadata_images(track, [self.test_files[1]])
+        self.assertFalse(track.remove_metadata_images_from_children([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.files.append(File('test1.flac'))
-        update_metadata_images(track)
+        self.assertFalse(track.update_metadata_images_from_children())
-        remove_metadata_images(track, [track.files[0]])
+        self.assertFalse(track.remove_metadata_images_from_children([track.files[0]]))
         self.assertEqual(set(), set(track.orig_metadata.images))
         self.assertTrue(track.orig_metadata.has_common_images)
 
     def test_remove_from_album(self):
         album = Album('00000000-0000-0000-0000-000000000000')
         album.unmatched_files.files = list(self.test_files)
-        update_metadata_images(album)
+        self.assertTrue(album.update_metadata_images_from_children())
         album.unmatched_files.files.remove(self.test_files[0])
-        remove_metadata_images(album, [self.test_files[0]])
+        self.assertTrue(album.remove_metadata_images_from_children([self.test_files[0]]))
         self.assertEqual(set(self.test_images[1:]), set(album.metadata.images))
         self.assertEqual(set(self.test_images[1:]), set(album.orig_metadata.images))
         self.assertTrue(album.metadata.has_common_images)
@@ -209,9 +203,9 @@ class RemoveMetadataImagesTest(PicardTestCase):
     def test_remove_from_album_with_common_images(self):
         album = Album('00000000-0000-0000-0000-000000000000')
         album.unmatched_files.files = list(self.test_files[1:])
-        update_metadata_images(album)
+        self.assertTrue(album.update_metadata_images_from_children())
         album.unmatched_files.files.remove(self.test_files[1])
-        remove_metadata_images(album, [self.test_files[1]])
+        self.assertFalse(album.remove_metadata_images_from_children([self.test_files[1]]))
         self.assertEqual(set(self.test_images[1:]), set(album.metadata.images))
         self.assertEqual(set(self.test_images[1:]), set(album.orig_metadata.images))
         self.assertTrue(album.metadata.has_common_images)
@@ -220,8 +214,8 @@ class RemoveMetadataImagesTest(PicardTestCase):
     def test_remove_from_empty_album(self):
         album = Album('00000000-0000-0000-0000-000000000000')
         album.unmatched_files.files.append(File('test1.flac'))
-        update_metadata_images(album)
+        self.assertFalse(album.update_metadata_images_from_children())
-        remove_metadata_images(album, [album.unmatched_files.files[0]])
+        self.assertFalse(album.remove_metadata_images_from_children([album.unmatched_files.files[0]]))
         self.assertEqual(set(), set(album.metadata.images))
         self.assertEqual(set(), set(album.orig_metadata.images))
         self.assertTrue(album.metadata.has_common_images)
@@ -237,27 +231,24 @@ class AddMetadataImagesTest(PicardTestCase):
     def test_add_to_cluster(self):
         cluster = Cluster('Test')
         cluster.files = [self.test_files[0]]
-        update_metadata_images(cluster)
+        self.assertTrue(cluster.update_metadata_images_from_children())
         cluster.files += self.test_files[1:]
-        added = add_metadata_images(cluster, self.test_files[1:])
+        self.assertTrue(cluster.add_metadata_images_from_children(self.test_files[1:]))
-        self.assertTrue(added)
         self.assertEqual(set(self.test_images), set(cluster.metadata.images))
         self.assertFalse(cluster.metadata.has_common_images)
 
     def test_add_no_changes(self):
         cluster = Cluster('Test')
         cluster.files = self.test_files
-        update_metadata_images(cluster)
+        self.assertTrue(cluster.update_metadata_images_from_children())
-        added = add_metadata_images(cluster, [self.test_files[1]])
+        self.assertFalse(cluster.add_metadata_images_from_children([self.test_files[1]]))
-        self.assertFalse(added)
         self.assertEqual(set(self.test_images), set(cluster.metadata.images))
 
     def test_add_nothing(self):
         cluster = Cluster('Test')
         cluster.files = self.test_files
-        update_metadata_images(cluster)
+        self.assertTrue(cluster.update_metadata_images_from_children())
-        added = add_metadata_images(cluster, [])
+        self.assertFalse(cluster.add_metadata_images_from_children([]))
-        self.assertFalse(added)
 
 
 class ImageListTest(PicardTestCase):