From e4763e86513e73e5570053d804ad221d1c005ccf Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Wed, 19 Sep 2018 22:59:45 +0200 Subject: [PATCH 1/9] PICARD-1339: Optimize update_metadata_images performance Greatly improves the time it takes to remove a huge amount of unclustered files from the tagger. --- picard/util/imagelist.py | 74 ++++++++++++++++++---------------------- 1 file changed, 34 insertions(+), 40 deletions(-) diff --git a/picard/util/imagelist.py b/picard/util/imagelist.py index 1b3cba11c..e55a2a769 100644 --- a/picard/util/imagelist.py +++ b/picard/util/imagelist.py @@ -35,48 +35,51 @@ class ImageList(list): return result -def _process_images(state, src_obj): - from picard.track import Track - - # Check new images - if state.update_new_metadata: - if state.first_new_obj: - state.new_images = src_obj.metadata.images[:] - state.first_new_obj = False - else: - if state.new_images != src_obj.metadata.images: - state.has_common_new_images = False - state.new_images.extend([image for image in src_obj.metadata.images if image not in state.new_images]) - - 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) - if state.first_orig_obj: - state.orig_images = src_obj.orig_metadata.images[:] - state.first_orig_obj = False - else: - if state.orig_images != src_obj.orig_metadata.images: - state.has_common_orig_images = False - state.orig_images.extend([image for image in src_obj.orig_metadata.images if image not in state.orig_images]) - - class State: def __init__(self): - self.new_images = ImageList() - self.orig_images = ImageList() + self.new_images = set() + self.orig_images = set() 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): + from picard.track import Track + + # Check new images + if state.update_new_metadata: + if state.new_images != set(src_obj.metadata.images): + state.has_common_new_images = False + state.new_images = state.new_images.union(src_obj.metadata.images) + + 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) + if state.orig_images != set(src_obj.orig_metadata.images): + state.has_common_orig_images = False + state.orig_images = state.orig_images.union(src_obj.orig_metadata.images) + + +def _update_state(obj, state, sources): + for src_obj in sources: + _process_images(state, src_obj) + + if state.update_new_metadata: + obj.metadata.images = ImageList(state.new_images) + obj.metadata.has_common_images = state.has_common_new_images + + if state.update_orig_metadata: + obj.orig_metadata.images = ImageList(state.orig_images) + obj.orig_metadata.has_common_images = state.has_common_orig_images + + # TODO: use functools.singledispatch when py3 is supported def update_metadata_images(obj): - from picard.track import Track - from picard.cluster import Cluster from picard.album import Album + from picard.cluster import Cluster + from picard.track import Track state = State() @@ -95,13 +98,4 @@ def update_metadata_images(obj): sources = obj.files state.update_new_metadata = True - for src_obj in sources: - _process_images(state, src_obj) - - if state.update_new_metadata: - obj.metadata.images = state.new_images - obj.metadata.has_common_images = state.has_common_new_images - - if state.update_orig_metadata: - obj.orig_metadata.images = state.orig_images - obj.orig_metadata.has_common_images = state.has_common_orig_images + _update_state(obj, state, sources) From 0e550c07a88e42d9519348fc944fffc39d8bf9e3 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Sun, 23 Sep 2018 11:09:54 +0200 Subject: [PATCH 2/9] Renamed imagelist.State to imagelist.ImageListState --- picard/util/imagelist.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/picard/util/imagelist.py b/picard/util/imagelist.py index e55a2a769..2c7e1bf9c 100644 --- a/picard/util/imagelist.py +++ b/picard/util/imagelist.py @@ -35,7 +35,7 @@ class ImageList(list): return result -class State: +class ImageListState: def __init__(self): self.new_images = set() self.orig_images = set() @@ -81,7 +81,7 @@ def update_metadata_images(obj): from picard.cluster import Cluster from picard.track import Track - state = State() + state = ImageListState() if isinstance(obj, Album): sources = [] From a68e76a4b78829b3276fc6c1f1ed37739ad089d3 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Mon, 24 Sep 2018 07:11:31 +0200 Subject: [PATCH 3/9] PICARD-1339: Added unitttest for update_metadata_images --- picard/util/imagelist.py | 14 ++++- test/test_imagelist.py | 111 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 2 deletions(-) create mode 100644 test/test_imagelist.py diff --git a/picard/util/imagelist.py b/picard/util/imagelist.py index 2c7e1bf9c..83a479f14 100644 --- a/picard/util/imagelist.py +++ b/picard/util/imagelist.py @@ -2,6 +2,7 @@ # # Picard, the next-generation MusicBrainz tagger # Copyright (C) 2017 Antonio Larrosa +# Copyright (C) 2018 Philipp Wolfer # # This program is free software; you can redistribute it and/or # modify it under the terms of the GNU General Public License @@ -41,6 +42,8 @@ class ImageListState: self.orig_images = set() 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 @@ -52,17 +55,24 @@ def _process_images(state, src_obj): # Check new images if state.update_new_metadata: if state.new_images != set(src_obj.metadata.images): - state.has_common_new_images = False + if not state.first_new_obj: + state.has_common_new_images = False state.new_images = state.new_images.union(src_obj.metadata.images) + 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) if state.orig_images != set(src_obj.orig_metadata.images): - state.has_common_orig_images = False + if not state.first_orig_obj: + state.has_common_orig_images = False state.orig_images = state.orig_images.union(src_obj.orig_metadata.images) + if state.first_orig_obj: + state.first_orig_obj = False def _update_state(obj, state, sources): + is_first = True for src_obj in sources: _process_images(state, src_obj) diff --git a/test/test_imagelist.py b/test/test_imagelist.py new file mode 100644 index 000000000..1a5e5c647 --- /dev/null +++ b/test/test_imagelist.py @@ -0,0 +1,111 @@ +# -*- coding: utf-8 -*- + +import os +import struct +import unittest + +from picard.album import Album +from picard.cluster import Cluster +from picard.coverart.image import CoverArtImage +from picard.track import Track +from picard.file import File +from picard.util.imagelist import ( + update_metadata_images +) + + +def create_fake_png(extra): + """Creates fake PNG data that satisfies Picard's internal image type detection""" + return b'\x89PNG\x0D\x0A\x1A\x0A' + (b'a' * 4) + b'IHDR' + struct.pack('>LL', 100, 100) + extra + + +class UpdateMetadataImagesTest(unittest.TestCase): + + def setUp(self): + self.test_images = [ + CoverArtImage(data=create_fake_png(b'a')), + CoverArtImage(data=create_fake_png(b'b')), + ] + self.test_files = [ + File('test1.flac'), + File('test2.flac'), + File('test2.flac') + ] + self.test_files[0].metadata.append_image(self.test_images[0]) + self.test_files[1].metadata.append_image(self.test_images[1]) + self.test_files[2].metadata.append_image(self.test_images[1]) + self.test_files[0].orig_metadata.append_image(self.test_images[0]) + self.test_files[1].orig_metadata.append_image(self.test_images[1]) + self.test_files[2].orig_metadata.append_image(self.test_images[1]) + + def test_update_cluster_images(self): + cluster = Cluster('Test') + cluster.files = list(self.test_files) + 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.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.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.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.linked_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]) + 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]) + 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]) + update_metadata_images(track) + self.assertEqual(set(self.test_images[1:]), set(track.orig_metadata.images)) + self.assertTrue(track.orig_metadata.has_common_images) + + 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]) + track2 = Track('00000000-0000-0000-0000-000000000002') + track2.linked_files.append(self.test_files[1]) + album.tracks = [track1, track2] + album.unmatched_files.files.append(self.test_files[2]) + 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.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.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.assertEqual(set(self.test_images[1:]), set(album.orig_metadata.images)) + self.assertTrue(album.orig_metadata.has_common_images) From bc9c9ba602f3c381401d1936732daa36c17964f3 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Tue, 25 Sep 2018 15:06:13 +0200 Subject: [PATCH 4/9] PICARD-1339: WIP Added optimized imagelist.remove_metadata_images --- picard/cluster.py | 7 +++-- picard/metadata.py | 1 + picard/util/imagelist.py | 65 +++++++++++++++++++++++++++++++++------- test/test_imagelist.py | 47 +++++++++++++++++++++++++++++ 4 files changed, 108 insertions(+), 12 deletions(-) diff --git a/picard/cluster.py b/picard/cluster.py index 3d2cd3bf0..c4183119a 100644 --- a/picard/cluster.py +++ b/picard/cluster.py @@ -37,7 +37,10 @@ from picard.util import ( album_artist_from_path, format_time, ) -from picard.util.imagelist import update_metadata_images +from picard.util.imagelist import ( + remove_metadata_images, + update_metadata_images +) from picard.ui.item import Item @@ -114,7 +117,7 @@ class Cluster(QtCore.QObject, Item): self.item.remove_file(file) if not self.special and self.get_num_files() == 0: self.tagger.remove_cluster(self) - self.update_metadata_images() + remove_metadata_images(self, [file]) self._update_related_album() def update(self): diff --git a/picard/metadata.py b/picard/metadata.py index 134551af5..f19481fdb 100644 --- a/picard/metadata.py +++ b/picard/metadata.py @@ -62,6 +62,7 @@ class Metadata(dict): def __init__(self): super().__init__() self.images = ImageList() + self.has_common_images = True self.deleted_tags = set() self.length = 0 diff --git a/picard/util/imagelist.py b/picard/util/imagelist.py index 83a479f14..bf2dde2e2 100644 --- a/picard/util/imagelist.py +++ b/picard/util/imagelist.py @@ -40,6 +40,7 @@ class ImageListState: def __init__(self): self.new_images = set() self.orig_images = set() + self.sources = [] self.has_common_new_images = True self.has_common_orig_images = True self.first_new_obj = True @@ -71,9 +72,9 @@ def _process_images(state, src_obj): state.first_orig_obj = False -def _update_state(obj, state, sources): +def _update_state(obj, state): is_first = True - for src_obj in sources: + for src_obj in state.sources: _process_images(state, src_obj) if state.update_new_metadata: @@ -86,7 +87,7 @@ def _update_state(obj, state, sources): # TODO: use functools.singledispatch when py3 is supported -def update_metadata_images(obj): +def _get_state(obj): from picard.album import Album from picard.cluster import Cluster from picard.track import Track @@ -94,18 +95,62 @@ def update_metadata_images(obj): state = ImageListState() if isinstance(obj, Album): - sources = [] for track in obj.tracks: - sources.append(track) - sources += track.linked_files - sources += obj.unmatched_files.files + state.sources.append(track) + state.sources += track.linked_files + state.sources += obj.unmatched_files.files state.update_new_metadata = True state.update_orig_metadata = True elif isinstance(obj, Track): - sources = obj.linked_files + state.sources = obj.linked_files state.update_orig_metadata = True elif isinstance(obj, Cluster): - sources = obj.files + state.sources = obj.files state.update_new_metadata = True - _update_state(obj, state, sources) + return state + + +def update_metadata_images(obj): + state = _get_state(obj) + + _update_state(obj, state) + + +def _remove_images(metadata, sources, removed_images): + if len(metadata.images) == 0 or len(removed_images) == 0: + return + + if len(sources) == 0: + 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 + previous_images = None + for source 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) + removed_images = removed_images.difference(source_images) + if len(removed_images) == 0 and common_images == False: + return + + metadata.images = ImageList(current_images.difference(removed_images)) + metadata.has_common_images = common_images + + +def remove_metadata_images(obj, removed_sources): + state = _get_state(obj) + + if state.update_new_metadata: + removed_images = set() + for s in removed_sources: + removed_images = removed_images.union(s.metadata.images) + _remove_images(obj.metadata, state.sources, removed_images) diff --git a/test/test_imagelist.py b/test/test_imagelist.py index 1a5e5c647..e989550b5 100644 --- a/test/test_imagelist.py +++ b/test/test_imagelist.py @@ -10,6 +10,7 @@ from picard.coverart.image import CoverArtImage from picard.track import Track from picard.file import File from picard.util.imagelist import ( + remove_metadata_images, update_metadata_images ) @@ -109,3 +110,49 @@ class UpdateMetadataImagesTest(unittest.TestCase): update_metadata_images(album) self.assertEqual(set(self.test_images[1:]), set(album.orig_metadata.images)) self.assertTrue(album.orig_metadata.has_common_images) + + +class RemoveMetadataImagesTest(unittest.TestCase): + + def setUp(self): + self.test_images = [ + CoverArtImage(url='file://file1', data=create_fake_png(b'a')), + CoverArtImage(url='file://file2', data=create_fake_png(b'b')), + ] + self.test_files = [ + File('test1.flac'), + File('test2.flac'), + File('test2.flac') + ] + self.test_files[0].metadata.append_image(self.test_images[0]) + self.test_files[1].metadata.append_image(self.test_images[1]) + self.test_files[2].metadata.append_image(self.test_images[1]) + self.test_files[0].orig_metadata.append_image(self.test_images[0]) + self.test_files[1].orig_metadata.append_image(self.test_images[1]) + self.test_files[2].orig_metadata.append_image(self.test_images[1]) + + def test_remove_from_cluster(self): + cluster = Cluster('Test') + cluster.files = list(self.test_files) + update_metadata_images(cluster) + cluster.files.remove(self.test_files[0]) + remove_metadata_images(cluster, [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) + cluster.files.remove(self.test_files[1]) + remove_metadata_images(cluster, [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) + remove_metadata_images(cluster, [cluster.files[0]]) + self.assertEqual(set(), set(cluster.metadata.images)) + self.assertTrue(cluster.metadata.has_common_images) From e9a8c3abee62f84ce7ab4e56aebe0f8be167bc5f Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Fri, 5 Oct 2018 17:12:43 +0200 Subject: [PATCH 5/9] PICARD-1339: Make conditionals more pythonic --- picard/util/imagelist.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/picard/util/imagelist.py b/picard/util/imagelist.py index bf2dde2e2..5ddca62b1 100644 --- a/picard/util/imagelist.py +++ b/picard/util/imagelist.py @@ -118,10 +118,10 @@ def update_metadata_images(obj): def _remove_images(metadata, sources, removed_images): - if len(metadata.images) == 0 or len(removed_images) == 0: + if not metadata.images or not removed_images: return - if len(sources) == 0: + if not sources: metadata.images = ImageList() metadata.has_common_images = True return @@ -139,7 +139,7 @@ def _remove_images(metadata, sources, removed_images): common_images = False previous_images = set(source.metadata.images) removed_images = removed_images.difference(source_images) - if len(removed_images) == 0 and common_images == False: + if not removed_images and not common_images: return metadata.images = ImageList(current_images.difference(removed_images)) From ab2778a521b4202850f608d0f762c045369e1874 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Fri, 5 Oct 2018 17:53:13 +0200 Subject: [PATCH 6/9] PICARD-1339: Use remove_metadata_images for Album and Track --- picard/album.py | 7 +++-- picard/track.py | 11 +++++--- picard/util/imagelist.py | 26 +++++++++++------- test/test_imagelist.py | 58 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 87 insertions(+), 15 deletions(-) diff --git a/picard/album.py b/picard/album.py index bc412fe6b..323069a59 100644 --- a/picard/album.py +++ b/picard/album.py @@ -58,7 +58,10 @@ from picard.util import ( format_time, mbid_validate, ) -from picard.util.imagelist import update_metadata_images +from picard.util.imagelist import ( + remove_metadata_images, + update_metadata_images +) from picard.util.textencoding import asciipunct from picard.ui.item import Item @@ -413,7 +416,7 @@ class Album(DataObject, Item): self._files -= 1 self.update(update_tracks=False) file.metadata_images_changed.disconnect(self.update_metadata_images) - self.update_metadata_images() + remove_metadata_images(self, [file]) def match_files(self, files, use_recordingid=True): """Match files to tracks on this album, based on metadata similarity or recordingid.""" diff --git a/picard/track.py b/picard/track.py index 4baac7c78..74f9bcc00 100644 --- a/picard/track.py +++ b/picard/track.py @@ -43,7 +43,10 @@ from picard.script import ( ScriptParser, enabled_tagger_scripts_texts, ) -from picard.util.imagelist import update_metadata_images +from picard.util.imagelist import ( + remove_metadata_images, + update_metadata_images +) from picard.util.textencoding import asciipunct from picard.ui.item import Item @@ -91,6 +94,7 @@ class Track(DataObject, Item): file.metadata['~extension'] = file.orig_metadata['~extension'] file.update(signal=False) self.update() + self.update_orig_metadata_images() def remove_file(self, file): if file not in self.linked_files: @@ -98,14 +102,14 @@ class Track(DataObject, Item): self.linked_files.remove(file) self.num_linked_files -= 1 file.copy_metadata(file.orig_metadata, preserve_deleted=False) - self.album._remove_file(self, file) file.metadata_images_changed.disconnect(self.update_orig_metadata_images) + self.album._remove_file(self, file) + remove_metadata_images(self, [file]) self.update() def update(self): if self.item: self.item.update() - self.update_orig_metadata_images() def iterfiles(self, save=False): for file in self.linked_files: @@ -252,6 +256,7 @@ class Track(DataObject, Item): else: self.metadata.images = [] self.update() + self.update_orig_metadata_images() class NonAlbumTrack(Track): diff --git a/picard/util/imagelist.py b/picard/util/imagelist.py index 5ddca62b1..585eff5a8 100644 --- a/picard/util/imagelist.py +++ b/picard/util/imagelist.py @@ -112,9 +112,7 @@ def _get_state(obj): def update_metadata_images(obj): - state = _get_state(obj) - - _update_state(obj, state) + _update_state(obj, _get_state(obj)) def _remove_images(metadata, sources, removed_images): @@ -133,11 +131,11 @@ def _remove_images(metadata, sources, removed_images): common_images = True previous_images = None - for source in sources: - source_images = set(source.metadata.images) + 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) + previous_images = set(source_metadata.images) removed_images = removed_images.difference(source_images) if not removed_images and not common_images: return @@ -147,10 +145,18 @@ def _remove_images(metadata, sources, removed_images): def remove_metadata_images(obj, removed_sources): + from picard.track import Track + state = _get_state(obj) + removed_new_images = set() + removed_orig_images = set() + for s in removed_sources: + removed_new_images = removed_new_images.union(s.metadata.images) + removed_orig_images = removed_orig_images.union(s.orig_metadata.images) if state.update_new_metadata: - removed_images = set() - for s in removed_sources: - removed_images = removed_images.union(s.metadata.images) - _remove_images(obj.metadata, state.sources, removed_images) + 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) diff --git a/test/test_imagelist.py b/test/test_imagelist.py index e989550b5..0c5e5586c 100644 --- a/test/test_imagelist.py +++ b/test/test_imagelist.py @@ -156,3 +156,61 @@ class RemoveMetadataImagesTest(unittest.TestCase): remove_metadata_images(cluster, [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.linked_files = list(self.test_files) + update_metadata_images(track) + track.linked_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:]) + update_metadata_images(track) + track.linked_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')) + update_metadata_images(track) + remove_metadata_images(track, [track.linked_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) + album.unmatched_files.files.remove(self.test_files[0]) + remove_metadata_images(album, [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) + self.assertTrue(album.orig_metadata.has_common_images) + + 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) + album.unmatched_files.files.remove(self.test_files[1]) + remove_metadata_images(album, [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) + self.assertTrue(album.orig_metadata.has_common_images) + + 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) + remove_metadata_images(album, [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) + self.assertTrue(album.orig_metadata.has_common_images) From a4f2cd8a8931828c8abbf6dda46c343978ff8c11 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Fri, 5 Oct 2018 21:12:37 +0200 Subject: [PATCH 7/9] PICARD-1339: Added add_metadata_images function --- picard/album.py | 3 +- picard/track.py | 8 ++--- picard/util/imagelist.py | 39 ++++++++++++++++++++--- test/test_imagelist.py | 67 ++++++++++++++++++++++------------------ 4 files changed, 77 insertions(+), 40 deletions(-) diff --git a/picard/album.py b/picard/album.py index 323069a59..03ecf0eb3 100644 --- a/picard/album.py +++ b/picard/album.py @@ -59,6 +59,7 @@ from picard.util import ( mbid_validate, ) from picard.util.imagelist import ( + add_metadata_images, remove_metadata_images, update_metadata_images ) @@ -409,8 +410,8 @@ class Album(DataObject, Item): def _add_file(self, track, file): self._files += 1 self.update(update_tracks=False) + add_metadata_images(self, [file]) file.metadata_images_changed.connect(self.update_metadata_images) - self.update_metadata_images() def _remove_file(self, track, file): self._files -= 1 diff --git a/picard/track.py b/picard/track.py index 74f9bcc00..0868dd5e6 100644 --- a/picard/track.py +++ b/picard/track.py @@ -44,6 +44,7 @@ from picard.script import ( enabled_tagger_scripts_texts, ) from picard.util.imagelist import ( + add_metadata_images, remove_metadata_images, update_metadata_images ) @@ -83,8 +84,9 @@ class Track(DataObject, Item): if file not in self.linked_files: self.linked_files.append(file) self.num_linked_files += 1 - self.album._add_file(self, file) 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) def update_file_metadata(self, file): @@ -94,7 +96,6 @@ class Track(DataObject, Item): file.metadata['~extension'] = file.orig_metadata['~extension'] file.update(signal=False) self.update() - self.update_orig_metadata_images() def remove_file(self, file): if file not in self.linked_files: @@ -250,13 +251,12 @@ class Track(DataObject, Item): def keep_original_images(self): for file in self.linked_files: file.keep_original_images() + self.update_orig_metadata_images() if self.linked_files: - self.update_orig_metadata_images() self.metadata.images = self.orig_metadata.images[:] else: self.metadata.images = [] self.update() - self.update_orig_metadata_images() class NonAlbumTrack(Track): diff --git a/picard/util/imagelist.py b/picard/util/imagelist.py index 585eff5a8..7cadcbb32 100644 --- a/picard/util/imagelist.py +++ b/picard/util/imagelist.py @@ -111,10 +111,43 @@ def _get_state(obj): 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_state(obj, _get_state(obj)) +def _add_images(metadata, added_images): + if not added_images: + return + + current_images = set(metadata.images) + if added_images == current_images: + return + else: + metadata.images = ImageList(current_images.union(added_images)) + metadata.has_common_images = False + + +def add_metadata_images(obj, added_sources): + state = _get_state(obj) + (added_new_images, added_orig_images) = _get_metadata_images(state, added_sources) + + if state.update_new_metadata: + _add_images(obj.metadata, added_new_images) + if state.update_orig_metadata: + _add_images(obj.orig_metadata, added_orig_images) + + def _remove_images(metadata, sources, removed_images): if not metadata.images or not removed_images: return @@ -148,12 +181,8 @@ def remove_metadata_images(obj, removed_sources): from picard.track import Track state = _get_state(obj) + (removed_new_images, removed_orig_images) = _get_metadata_images(state, removed_sources) - removed_new_images = set() - removed_orig_images = set() - for s in removed_sources: - removed_new_images = removed_new_images.union(s.metadata.images) - removed_orig_images = removed_orig_images.union(s.orig_metadata.images) if state.update_new_metadata: sources = [s.metadata for s in state.sources] _remove_images(obj.metadata, sources, removed_new_images) diff --git a/test/test_imagelist.py b/test/test_imagelist.py index 0c5e5586c..a2026c0b8 100644 --- a/test/test_imagelist.py +++ b/test/test_imagelist.py @@ -10,6 +10,7 @@ from picard.coverart.image import CoverArtImage from picard.track import Track from picard.file import File from picard.util.imagelist import ( + add_metadata_images, remove_metadata_images, update_metadata_images ) @@ -20,24 +21,29 @@ def create_fake_png(extra): return b'\x89PNG\x0D\x0A\x1A\x0A' + (b'a' * 4) + b'IHDR' + struct.pack('>LL', 100, 100) + extra +def create_test_files(): + test_images = [ + CoverArtImage(url='file://file1', data=create_fake_png(b'a')), + CoverArtImage(url='file://file2', data=create_fake_png(b'b')), + ] + test_files = [ + File('test1.flac'), + File('test2.flac'), + File('test2.flac') + ] + test_files[0].metadata.append_image(test_images[0]) + test_files[1].metadata.append_image(test_images[1]) + test_files[2].metadata.append_image(test_images[1]) + test_files[0].orig_metadata.append_image(test_images[0]) + test_files[1].orig_metadata.append_image(test_images[1]) + test_files[2].orig_metadata.append_image(test_images[1]) + return (test_images, test_files) + + class UpdateMetadataImagesTest(unittest.TestCase): def setUp(self): - self.test_images = [ - CoverArtImage(data=create_fake_png(b'a')), - CoverArtImage(data=create_fake_png(b'b')), - ] - self.test_files = [ - File('test1.flac'), - File('test2.flac'), - File('test2.flac') - ] - self.test_files[0].metadata.append_image(self.test_images[0]) - self.test_files[1].metadata.append_image(self.test_images[1]) - self.test_files[2].metadata.append_image(self.test_images[1]) - self.test_files[0].orig_metadata.append_image(self.test_images[0]) - self.test_files[1].orig_metadata.append_image(self.test_images[1]) - self.test_files[2].orig_metadata.append_image(self.test_images[1]) + (self.test_images, self.test_files) = create_test_files() def test_update_cluster_images(self): cluster = Cluster('Test') @@ -115,21 +121,7 @@ class UpdateMetadataImagesTest(unittest.TestCase): class RemoveMetadataImagesTest(unittest.TestCase): def setUp(self): - self.test_images = [ - CoverArtImage(url='file://file1', data=create_fake_png(b'a')), - CoverArtImage(url='file://file2', data=create_fake_png(b'b')), - ] - self.test_files = [ - File('test1.flac'), - File('test2.flac'), - File('test2.flac') - ] - self.test_files[0].metadata.append_image(self.test_images[0]) - self.test_files[1].metadata.append_image(self.test_images[1]) - self.test_files[2].metadata.append_image(self.test_images[1]) - self.test_files[0].orig_metadata.append_image(self.test_images[0]) - self.test_files[1].orig_metadata.append_image(self.test_images[1]) - self.test_files[2].orig_metadata.append_image(self.test_images[1]) + (self.test_images, self.test_files) = create_test_files() def test_remove_from_cluster(self): cluster = Cluster('Test') @@ -214,3 +206,18 @@ class RemoveMetadataImagesTest(unittest.TestCase): self.assertEqual(set(), set(album.orig_metadata.images)) self.assertTrue(album.metadata.has_common_images) self.assertTrue(album.orig_metadata.has_common_images) + + +class AddMetadataImagesTest(unittest.TestCase): + + def setUp(self): + (self.test_images, self.test_files) = create_test_files() + + def test_add_to_cluster(self): + cluster = Cluster('Test') + cluster.files = [self.test_files[0]] + update_metadata_images(cluster) + cluster.files += self.test_files[1:] + add_metadata_images(cluster, self.test_files[1:]) + self.assertEqual(set(self.test_images), set(cluster.metadata.images)) + self.assertFalse(cluster.metadata.has_common_images) From 7925e6567a4e9c5f0d70b6e0d461328728e2cdc0 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Sat, 6 Oct 2018 09:56:19 +0200 Subject: [PATCH 8/9] Code cleanup after review --- picard/util/imagelist.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/picard/util/imagelist.py b/picard/util/imagelist.py index 7cadcbb32..61cd42996 100644 --- a/picard/util/imagelist.py +++ b/picard/util/imagelist.py @@ -73,7 +73,6 @@ def _process_images(state, src_obj): def _update_state(obj, state): - is_first = True for src_obj in state.sources: _process_images(state, src_obj) @@ -131,9 +130,7 @@ def _add_images(metadata, added_images): return current_images = set(metadata.images) - if added_images == current_images: - return - else: + if added_images != current_images: metadata.images = ImageList(current_images.union(added_images)) metadata.has_common_images = False From acdc31688cec745a7750e88c0bddb19eb8b7c911 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Mon, 8 Oct 2018 20:46:15 +0200 Subject: [PATCH 9/9] Add some documentation to util.imagelist --- picard/util/imagelist.py | 41 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 3 deletions(-) diff --git a/picard/util/imagelist.py b/picard/util/imagelist.py index 61cd42996..6043d201f 100644 --- a/picard/util/imagelist.py +++ b/picard/util/imagelist.py @@ -122,6 +122,18 @@ def _get_metadata_images(state, sources): 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 + """ _update_state(obj, _get_state(obj)) @@ -136,6 +148,12 @@ def _add_images(metadata, added_images): 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) @@ -146,6 +164,13 @@ def add_metadata_images(obj, added_sources): 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 @@ -159,22 +184,32 @@ def _remove_images(metadata, sources, removed_images): if metadata.has_common_images and current_images == removed_images: return - common_images = True + 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) + 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 + return # No images left to remove, abort immediatelly 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)