From 1212796b4c9cb42a6037e36994b7b3dd3e8b564b Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Mon, 24 Feb 2025 22:17:51 +0100 Subject: [PATCH 1/5] _parse_release(): Move release redirect code to its own method --- picard/album.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/picard/album.py b/picard/album.py index 06dc205e34..7de6100a67 100644 --- a/picard/album.py +++ b/picard/album.py @@ -192,9 +192,8 @@ def _run_album_metadata_processors(self): except BaseException: self.error_append(traceback.format_exc()) - def _parse_release(self, release_node): - log.debug("Loading release %r …", self.id) - self._tracks_loaded = False + def _hande_release_redirect(self, release_node): + """Handle release redirect""" release_id = release_node['id'] if release_id != self.id: self.tagger.mbid_redirects[self.id] = release_id @@ -204,11 +203,19 @@ def _parse_release(self, release_node): album.match_files(self.unmatched_files.files) album.update() self.tagger.remove_album(self) - return ParseResult.REDIRECT + return True else: del self.tagger.albums[self.id] self.tagger.albums[release_id] = self self.id = release_id + return False + + def _parse_release(self, release_node): + """Parse release node from MusicBrainz API data""" + log.debug("Loading release %r …", self.id) + self._tracks_loaded = False + if self._hande_release_redirect(release_node): + return ParseResult.REDIRECT self._release_node = release_node # Make the release artist nodes available, since they may From 077f04208716da95d01755a74eed8810a0beb0fd Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Mon, 24 Feb 2025 22:20:21 +0100 Subject: [PATCH 2/5] _hande_release_redirect(): reduce indentation level --- picard/album.py | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/picard/album.py b/picard/album.py index 7de6100a67..5165a87171 100644 --- a/picard/album.py +++ b/picard/album.py @@ -195,19 +195,21 @@ def _run_album_metadata_processors(self): def _hande_release_redirect(self, release_node): """Handle release redirect""" release_id = release_node['id'] - if release_id != self.id: - self.tagger.mbid_redirects[self.id] = release_id - album = self.tagger.albums.get(release_id) - if album: - log.debug("Release %r already loaded", release_id) - album.match_files(self.unmatched_files.files) - album.update() - self.tagger.remove_album(self) - return True - else: - del self.tagger.albums[self.id] - self.tagger.albums[release_id] = self - self.id = release_id + if release_id == self.id: + return False + + self.tagger.mbid_redirects[self.id] = release_id + album = self.tagger.albums.get(release_id) + if album: + log.debug("Release %r already loaded", release_id) + album.match_files(self.unmatched_files.files) + album.update() + self.tagger.remove_album(self) + return True + + del self.tagger.albums[self.id] + self.tagger.albums[release_id] = self + self.id = release_id return False def _parse_release(self, release_node): From 968bc5779a22a0f10d46f1f2326a3cbc6c57baac Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Mon, 24 Feb 2025 22:27:15 +0100 Subject: [PATCH 3/5] Rename m to metadata, and move customization code to its own method --- picard/album.py | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/picard/album.py b/picard/album.py index 5165a87171..40d33d9051 100644 --- a/picard/album.py +++ b/picard/album.py @@ -212,6 +212,19 @@ def _hande_release_redirect(self, release_node): self.id = release_id return False + def _release_metadata_customization(self, metadata, release_node, config): + """Apply modifications to release metadata""" + + # Custom VA name + if metadata['musicbrainz_albumartistid'] == VARIOUS_ARTISTS_ID: + metadata['albumartistsort'] = metadata['albumartist'] = config.setting['va_name'] + + # Convert Unicode punctuation + if config.setting['convert_punctuation']: + metadata.apply_func(asciipunct) + + metadata['totaldiscs'] = len(release_node['media']) + def _parse_release(self, release_node): """Parse release node from MusicBrainz API data""" log.debug("Loading release %r …", self.id) @@ -229,8 +242,8 @@ def _parse_release(self, release_node): self._release_artist_nodes = _create_artist_node_dict(release_node) # Get release metadata - m = self._new_metadata - m.length = 0 + metadata = self._new_metadata + metadata.length = 0 rg_node = release_node['release-group'] rg = self.release_group = self.tagger.get_release_group_by_id(rg_node['id']) @@ -239,20 +252,11 @@ def _parse_release(self, release_node): _copy_artist_nodes(self._release_artist_nodes, rg_node) release_group_to_metadata(rg_node, rg.metadata, rg) - m.copy(rg.metadata) - release_to_metadata(release_node, m, album=self) + metadata.copy(rg.metadata) + release_to_metadata(release_node, metadata, album=self) config = get_config() - - # Custom VA name - if m['musicbrainz_albumartistid'] == VARIOUS_ARTISTS_ID: - m['albumartistsort'] = m['albumartist'] = config.setting['va_name'] - - # Convert Unicode punctuation - if config.setting['convert_punctuation']: - m.apply_func(asciipunct) - - m['totaldiscs'] = len(release_node['media']) + self._release_metadata_customization(metadata, release_node, config) # Add album to collections add_release_to_user_collections(release_node) From 7d8d8e8a29bd225d2a6ddc0b05fcb4b66362d69f Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Mon, 24 Feb 2025 22:40:19 +0100 Subject: [PATCH 4/5] _parse_release(): move part of the code to new _needs_track_relationships() --- picard/album.py | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/picard/album.py b/picard/album.py index 40d33d9051..a4bc0e28ca 100644 --- a/picard/album.py +++ b/picard/album.py @@ -225,6 +225,19 @@ def _release_metadata_customization(self, metadata, release_node, config): metadata['totaldiscs'] = len(release_node['media']) + def _needs_track_relationships(self, release_node, config): + """Check if track relationships needs to be loaded""" + if not config.setting['track_ars']: + return False + + try: + for medium_node in release_node['media']: + if medium_node['track-count']: + return 'relations' not in medium_node['tracks'][0]['recording'] + except KeyError: + pass + return False + def _parse_release(self, release_node): """Parse release node from MusicBrainz API data""" log.debug("Loading release %r …", self.id) @@ -261,17 +274,8 @@ def _parse_release(self, release_node): # Add album to collections add_release_to_user_collections(release_node) - if config.setting['track_ars']: - # Detect if track relationships did not get loaded - try: - for medium_node in release_node['media']: - if medium_node['track-count']: - if 'relations' in medium_node['tracks'][0]['recording']: - return ParseResult.PARSED - else: - return ParseResult.MISSING_TRACK_RELS - except KeyError: - pass + if self._needs_track_relationships(release_node, config): + return ParseResult.MISSING_TRACK_RELS return ParseResult.PARSED From c230a89cf10e3b9a17696907e66919851662d649 Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Mon, 24 Feb 2025 23:27:42 +0100 Subject: [PATCH 5/5] Explode _parse_release() in smaller bits: - use get_config() wherever needed instead of passing config - introduce smaller methods to split code in logical bits - re-order methods a bit --- picard/album.py | 95 +++++++++++++++++++++++++++---------------------- 1 file changed, 52 insertions(+), 43 deletions(-) diff --git a/picard/album.py b/picard/album.py index a4bc0e28ca..ebdfda5135 100644 --- a/picard/album.py +++ b/picard/album.py @@ -192,6 +192,27 @@ def _run_album_metadata_processors(self): except BaseException: self.error_append(traceback.format_exc()) + def _parse_release(self, release_node): + """Parse release node from MusicBrainz API data""" + log.debug("Loading release %r …", self.id) + self._tracks_loaded = False + + if self._hande_release_redirect(release_node): + return ParseResult.REDIRECT + + self._release_node = release_node + self._setup_release_artist_nodes(release_node) + self._setup_release_group(release_node) + self._setup_release_metadata(release_node) + + # Add album to collections + add_release_to_user_collections(release_node) + + if self._needs_track_relationships(release_node): + return ParseResult.MISSING_TRACK_RELS + + return ParseResult.PARSED + def _hande_release_redirect(self, release_node): """Handle release redirect""" release_id = release_node['id'] @@ -212,8 +233,36 @@ def _hande_release_redirect(self, release_node): self.id = release_id return False - def _release_metadata_customization(self, metadata, release_node, config): + def _setup_release_artist_nodes(self, release_node): + """Setup release artist nodes for supplementary data""" + # Make the release artist nodes available, since they may + # contain supplementary data (aliases, tags, genres, ratings) + # which aren't present in the release group, track, or + # recording artist nodes. We can copy them into those places + # wherever the IDs match, so that the data is shared and + # available for use in mbjson.py and external plugins. + self._release_artist_nodes = _create_artist_node_dict(release_node) + + def _setup_release_group(self, release_node): + """Process and setup release group data""" + rg_node = release_node['release-group'] + rg = self.release_group = self.tagger.get_release_group_by_id(rg_node['id']) + rg.loaded_albums.add(self.id) + rg.refcount += 1 + _copy_artist_nodes(self._release_artist_nodes, rg_node) + release_group_to_metadata(rg_node, rg.metadata, rg) + + def _setup_release_metadata(self, release_node): + """Process and setup release metadata""" + metadata = self._new_metadata + metadata.length = 0 + metadata.copy(self.release_group.metadata) + release_to_metadata(release_node, metadata, album=self) + self._release_metadata_customization(metadata, release_node) + + def _release_metadata_customization(self, metadata, release_node): """Apply modifications to release metadata""" + config = get_config() # Custom VA name if metadata['musicbrainz_albumartistid'] == VARIOUS_ARTISTS_ID: @@ -225,8 +274,9 @@ def _release_metadata_customization(self, metadata, release_node, config): metadata['totaldiscs'] = len(release_node['media']) - def _needs_track_relationships(self, release_node, config): + def _needs_track_relationships(self, release_node): """Check if track relationships needs to be loaded""" + config = get_config() if not config.setting['track_ars']: return False @@ -238,47 +288,6 @@ def _needs_track_relationships(self, release_node, config): pass return False - def _parse_release(self, release_node): - """Parse release node from MusicBrainz API data""" - log.debug("Loading release %r …", self.id) - self._tracks_loaded = False - if self._hande_release_redirect(release_node): - return ParseResult.REDIRECT - - self._release_node = release_node - # Make the release artist nodes available, since they may - # contain supplementary data (aliases, tags, genres, ratings) - # which aren't present in the release group, track, or - # recording artist nodes. We can copy them into those places - # wherever the IDs match, so that the data is shared and - # available for use in mbjson.py and external plugins. - self._release_artist_nodes = _create_artist_node_dict(release_node) - - # Get release metadata - metadata = self._new_metadata - metadata.length = 0 - - rg_node = release_node['release-group'] - rg = self.release_group = self.tagger.get_release_group_by_id(rg_node['id']) - rg.loaded_albums.add(self.id) - rg.refcount += 1 - - _copy_artist_nodes(self._release_artist_nodes, rg_node) - release_group_to_metadata(rg_node, rg.metadata, rg) - metadata.copy(rg.metadata) - release_to_metadata(release_node, metadata, album=self) - - config = get_config() - self._release_metadata_customization(metadata, release_node, config) - - # Add album to collections - add_release_to_user_collections(release_node) - - if self._needs_track_relationships(release_node, config): - return ParseResult.MISSING_TRACK_RELS - - return ParseResult.PARSED - def _release_request_finished(self, document, http, error): if self.load_task is None: return