From 676536efa7367049d8cacdc384572f35ed47d852 Mon Sep 17 00:00:00 2001 From: Dmitry Bogdanov Date: Fri, 27 Apr 2018 19:28:35 +0200 Subject: [PATCH 1/8] Fix incorrect indexing of two-sided mediums Fix incorrect split of a tracklist by medium for the case of two-sided mediums (#2887). Following the discussion in #2887, the 'medium_total' value should contain the number of tracks on the medium to which each particular track belongs, not the total number of different mediums present on a release. Fix unit tests accordingly. --- beetsplug/discogs.py | 8 ++++---- test/test_discogs.py | 22 +++++++++++----------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/beetsplug/discogs.py b/beetsplug/discogs.py index 0957b3403..515d1af8e 100644 --- a/beetsplug/discogs.py +++ b/beetsplug/discogs.py @@ -246,7 +246,7 @@ class DiscogsPlugin(BeetsPlugin): # Extract information for the optional AlbumInfo fields, if possible. va = result.data['artists'][0].get('name', '').lower() == 'various' year = result.data.get('year') - mediums = len(set(t.medium for t in tracks)) + mediums = [t.medium for t in tracks] country = result.data.get('country') data_url = result.data.get('uri') @@ -270,11 +270,11 @@ class DiscogsPlugin(BeetsPlugin): # `autotag.apply_metadata`, and set `medium_total`. for track in tracks: track.media = media - track.medium_total = mediums + track.medium_total = mediums.count(track.medium) return AlbumInfo(album, album_id, artist, artist_id, tracks, asin=None, albumtype=albumtype, va=va, year=year, month=None, - day=None, label=label, mediums=mediums, + day=None, label=label, mediums=len(set(mediums)), artist_sort=None, releasegroup_id=None, catalognum=catalogno, script=None, language=None, country=country, albumstatus=None, media=media, @@ -351,7 +351,7 @@ class DiscogsPlugin(BeetsPlugin): # are the track index, not the medium. medium_is_index = track.medium and not track.medium_index and ( len(track.medium) != 1 or - ord(track.medium) - 64 != medium_count + 1 + ord(track.medium) - 64 != medium_count * sides_per_medium + side_count ) if not medium_is_index and medium != track.medium: diff --git a/test/test_discogs.py b/test/test_discogs.py index 74f9a5656..8b2eff9f1 100644 --- a/test/test_discogs.py +++ b/test/test_discogs.py @@ -101,9 +101,9 @@ class DGAlbumInfoTest(_common.TestCase): self.assertEqual(d.mediums, 1) self.assertEqual(t[0].medium, 1) - self.assertEqual(t[0].medium_total, 1) + self.assertEqual(t[0].medium_total, 2) self.assertEqual(t[1].medium, 1) - self.assertEqual(t[0].medium_total, 1) + self.assertEqual(t[0].medium_total, 2) def test_parse_medium_numbers_two_mediums(self): release = self._make_release_from_positions(['1-1', '2-1']) @@ -112,9 +112,9 @@ class DGAlbumInfoTest(_common.TestCase): self.assertEqual(d.mediums, 2) self.assertEqual(t[0].medium, 1) - self.assertEqual(t[0].medium_total, 2) + self.assertEqual(t[0].medium_total, 1) self.assertEqual(t[1].medium, 2) - self.assertEqual(t[1].medium_total, 2) + self.assertEqual(t[1].medium_total, 1) def test_parse_medium_numbers_two_mediums_two_sided(self): release = self._make_release_from_positions(['A1', 'B1', 'C1']) @@ -129,7 +129,7 @@ class DGAlbumInfoTest(_common.TestCase): self.assertEqual(t[1].medium_total, 2) self.assertEqual(t[1].medium_index, 2) self.assertEqual(t[2].medium, 2) - self.assertEqual(t[2].medium_total, 2) + self.assertEqual(t[2].medium_total, 1) self.assertEqual(t[2].medium_index, 1) def test_parse_track_indices(self): @@ -139,10 +139,10 @@ class DGAlbumInfoTest(_common.TestCase): self.assertEqual(t[0].medium_index, 1) self.assertEqual(t[0].index, 1) - self.assertEqual(t[0].medium_total, 1) + self.assertEqual(t[0].medium_total, 2) self.assertEqual(t[1].medium_index, 2) self.assertEqual(t[1].index, 2) - self.assertEqual(t[1].medium_total, 1) + self.assertEqual(t[1].medium_total, 2) def test_parse_track_indices_several_media(self): release = self._make_release_from_positions(['1-1', '1-2', '2-1', @@ -153,16 +153,16 @@ class DGAlbumInfoTest(_common.TestCase): self.assertEqual(d.mediums, 3) self.assertEqual(t[0].medium_index, 1) self.assertEqual(t[0].index, 1) - self.assertEqual(t[0].medium_total, 3) + self.assertEqual(t[0].medium_total, 2) self.assertEqual(t[1].medium_index, 2) self.assertEqual(t[1].index, 2) - self.assertEqual(t[1].medium_total, 3) + self.assertEqual(t[1].medium_total, 2) self.assertEqual(t[2].medium_index, 1) self.assertEqual(t[2].index, 3) - self.assertEqual(t[2].medium_total, 3) + self.assertEqual(t[2].medium_total, 1) self.assertEqual(t[3].medium_index, 1) self.assertEqual(t[3].index, 4) - self.assertEqual(t[3].medium_total, 3) + self.assertEqual(t[3].medium_total, 1) def test_parse_position(self): """Test the conversion of discogs `position` to medium, medium_index From 2e422122b3f08ce9ce7c69e018094472084ce378 Mon Sep 17 00:00:00 2001 From: Dmitry Bogdanov Date: Fri, 27 Apr 2018 20:30:35 +0200 Subject: [PATCH 2/8] Invent our own track IDs for Discogs Discogs does not provide track IDs. As a workaround, invent our own IDs by combining release ID with original track position strings returned by Discogs (#2336). --- beetsplug/discogs.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/beetsplug/discogs.py b/beetsplug/discogs.py index 515d1af8e..cc404a326 100644 --- a/beetsplug/discogs.py +++ b/beetsplug/discogs.py @@ -271,6 +271,8 @@ class DiscogsPlugin(BeetsPlugin): for track in tracks: track.media = media track.medium_total = mediums.count(track.medium) + # Invent our own track IDs as Discogs doesn't have them + track.track_id = str(album_id) + "-" + track.track_alt return AlbumInfo(album, album_id, artist, artist_id, tracks, asin=None, albumtype=albumtype, va=va, year=year, month=None, From 48140f11e7b86168133b1e9ad794c4ef1ef63da8 Mon Sep 17 00:00:00 2001 From: Dmitry Bogdanov Date: Fri, 27 Apr 2018 21:56:51 +0200 Subject: [PATCH 3/8] Use releasegroup_id to store master release IDs from Discogs Master releases roughly correspond to MusicBrainz' release groups. It will be usefull to store master IDs, for example to retrieve original release dates (#1122). --- beetsplug/discogs.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/beetsplug/discogs.py b/beetsplug/discogs.py index cc404a326..2d79ca754 100644 --- a/beetsplug/discogs.py +++ b/beetsplug/discogs.py @@ -274,10 +274,13 @@ class DiscogsPlugin(BeetsPlugin): # Invent our own track IDs as Discogs doesn't have them track.track_id = str(album_id) + "-" + track.track_alt + # Retrieve master release id (returns None if there isn't one) + master_id = result.data.get('master_id') + return AlbumInfo(album, album_id, artist, artist_id, tracks, asin=None, albumtype=albumtype, va=va, year=year, month=None, day=None, label=label, mediums=len(set(mediums)), - artist_sort=None, releasegroup_id=None, + artist_sort=None, releasegroup_id=master_id, catalognum=catalogno, script=None, language=None, country=country, albumstatus=None, media=media, albumdisambig=None, artist_credit=None, From 09ee194142bd2fdffb7a6885a402c334b62fe0c4 Mon Sep 17 00:00:00 2001 From: Dmitry Bogdanov Date: Sat, 28 Apr 2018 02:23:46 +0200 Subject: [PATCH 4/8] Make Discogs medium indexing code easier to understand --- beetsplug/discogs.py | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/beetsplug/discogs.py b/beetsplug/discogs.py index 2d79ca754..cdc80ff83 100644 --- a/beetsplug/discogs.py +++ b/beetsplug/discogs.py @@ -347,30 +347,34 @@ class DiscogsPlugin(BeetsPlugin): # a 2-sided medium. if ''.join(m) in ascii_lowercase: sides_per_medium = 2 - side_count = 1 # Force for first item, where medium == None for track in tracks: # Handle special case where a different medium does not indicate a # new disc, when there is no medium_index and the ordinal of medium # is not sequential. For example, I, II, III, IV, V. Assume these # are the track index, not the medium. + + # side_count is the number of mediums or medium sides (in the case + # of two-sided mediums) that were seen before medium_is_index = track.medium and not track.medium_index and ( len(track.medium) != 1 or - ord(track.medium) - 64 != medium_count * sides_per_medium + side_count + # Not witin standard incremental medium values (A, B, C, ...) + ord(track.medium) - 64 != side_count + 1 ) if not medium_is_index and medium != track.medium: - if side_count < (sides_per_medium - 1): - # Increment side count: side changed, but not medium. - side_count += 1 - medium = track.medium + side_count += 1 + if sides_per_medium == 2: + if side_count % sides_per_medium: + # Two-sided medium changed. Reset index_count + index_count = 0 + medium_count += 1 else: - # Increment medium_count and reset index_count and side - # count when medium changes. - medium = track.medium + # Medium changed. Reset index_count medium_count += 1 index_count = 0 - side_count = 0 + medium = track.medium + index_count += 1 medium_count = 1 if medium_count == 0 else medium_count track.medium, track.medium_index = medium_count, index_count From 0ea5882bc03d8de798e56b8cd55b916e268d43a1 Mon Sep 17 00:00:00 2001 From: Dmitry Bogdanov Date: Sat, 28 Apr 2018 02:32:42 +0200 Subject: [PATCH 5/8] Better comments in code explaining Discogs track IDs (#2336) --- beetsplug/discogs.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/beetsplug/discogs.py b/beetsplug/discogs.py index cdc80ff83..740948348 100644 --- a/beetsplug/discogs.py +++ b/beetsplug/discogs.py @@ -271,7 +271,8 @@ class DiscogsPlugin(BeetsPlugin): for track in tracks: track.media = media track.medium_total = mediums.count(track.medium) - # Invent our own track IDs as Discogs doesn't have them + # Discogs does not have track IDs. Invent our own IDs as proposed + # in #2336. track.track_id = str(album_id) + "-" + track.track_alt # Retrieve master release id (returns None if there isn't one) From 8c42c458a71bbe8fcd77e8ed5348cc3268aadde8 Mon Sep 17 00:00:00 2001 From: Dmitry Bogdanov Date: Sat, 28 Apr 2018 03:34:34 +0200 Subject: [PATCH 6/8] Update changelog --- docs/changelog.rst | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index b1c0d4c29..fafbbbda6 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -24,6 +24,10 @@ New features: * A new interoperability plugin to automatically notify Sonos controllers to update the music library once the beets library got updated. Thanks to :user:`cgtobi`. +* :doc:`/plugins/discogs`: The plugin now stores master release ids into + ``mb_releasegroupid`` as well as simulates track ids using release id + and tracklist positions. Track ids are stored in ``mb_trackid``. :bug:`#2336` + Thanks to :user:`dbogdanov`. Fixes: @@ -35,7 +39,9 @@ Fixes: autotagger. Data tracks will always be ignored, but a new option ``ignore_video_tracks`` has been added to control if video tracks should be ignored or not. :bug:`1210` -* :doc:`/plugins/replaygain`: Fix a corner-case with the ``bs1770gain`` backend where ReplayGain values were assigned to the wrong files. Now ``bs1770gain`` version 0.4.6 or later is required. :bug:`2777` +* :doc:`/plugins/replaygain`: Fix a corner-case with the ``bs1770gain`` backend + where ReplayGain values were assigned to the wrong files. Now ``bs1770gain`` + version 0.4.6 or later is required. :bug:`2777` * :doc:`/plugins/lyrics`: The plugin no longer crashes in the Genius source when BeautifulSoup is not found. Instead, it just logs a message and disables the source. @@ -91,6 +97,13 @@ Fixes: * Really fix album replaygain calculation with gstreamer backend. :bug:`2846` * Avoid an error when doing a "no-op" move on non-existent files (i.e., moving a file onto itself). :bug:`2863` +* :doc:`/plugins/discogs`: Fix ``medium`` and ``medium_index`` values which + were occasionally incorrect for releases with two-sided mediums. Fix + 'medium_total' value. It now contains total number of tracks on the medium + to which a track belongs, not the total number of different mediums present + on the release. :bug:`2887` + Thanks to :user:`dbogdanov`. + For developers: From 7ac2aff50c9ed9a02ebcfb529ccec9b677403e9c Mon Sep 17 00:00:00 2001 From: Dmitry Bogdanov Date: Sat, 28 Apr 2018 14:06:29 +0200 Subject: [PATCH 7/8] Fix a few code style issues --- beetsplug/discogs.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/beetsplug/discogs.py b/beetsplug/discogs.py index 740948348..afb054081 100644 --- a/beetsplug/discogs.py +++ b/beetsplug/discogs.py @@ -354,8 +354,7 @@ class DiscogsPlugin(BeetsPlugin): # new disc, when there is no medium_index and the ordinal of medium # is not sequential. For example, I, II, III, IV, V. Assume these # are the track index, not the medium. - - # side_count is the number of mediums or medium sides (in the case + # side_count is the number of mediums or medium sides (in the case # of two-sided mediums) that were seen before medium_is_index = track.medium and not track.medium_index and ( len(track.medium) != 1 or From f9b6473893f11ef278e0157d4f8b017e7e6f3b4e Mon Sep 17 00:00:00 2001 From: Dmitry Bogdanov Date: Sat, 28 Apr 2018 19:03:32 +0200 Subject: [PATCH 8/8] Some more code style fixes --- beetsplug/discogs.py | 10 +++++----- docs/changelog.rst | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/beetsplug/discogs.py b/beetsplug/discogs.py index afb054081..82c79a023 100644 --- a/beetsplug/discogs.py +++ b/beetsplug/discogs.py @@ -354,11 +354,11 @@ class DiscogsPlugin(BeetsPlugin): # new disc, when there is no medium_index and the ordinal of medium # is not sequential. For example, I, II, III, IV, V. Assume these # are the track index, not the medium. - # side_count is the number of mediums or medium sides (in the case - # of two-sided mediums) that were seen before + # side_count is the number of mediums or medium sides (in the case + # of two-sided mediums) that were seen before. medium_is_index = track.medium and not track.medium_index and ( len(track.medium) != 1 or - # Not witin standard incremental medium values (A, B, C, ...) + # Not within standard incremental medium values (A, B, C, ...). ord(track.medium) - 64 != side_count + 1 ) @@ -366,11 +366,11 @@ class DiscogsPlugin(BeetsPlugin): side_count += 1 if sides_per_medium == 2: if side_count % sides_per_medium: - # Two-sided medium changed. Reset index_count + # Two-sided medium changed. Reset index_count. index_count = 0 medium_count += 1 else: - # Medium changed. Reset index_count + # Medium changed. Reset index_count. medium_count += 1 index_count = 0 medium = track.medium diff --git a/docs/changelog.rst b/docs/changelog.rst index fafbbbda6..8f45aa69c 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -99,7 +99,7 @@ Fixes: a file onto itself). :bug:`2863` * :doc:`/plugins/discogs`: Fix ``medium`` and ``medium_index`` values which were occasionally incorrect for releases with two-sided mediums. Fix - 'medium_total' value. It now contains total number of tracks on the medium + ``medium_total`` value. It now contains total number of tracks on the medium to which a track belongs, not the total number of different mediums present on the release. :bug:`2887` Thanks to :user:`dbogdanov`.