From 550a9a82b16844253a035dedf75b33e4a4b5ec74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0ar=C5=ABnas=20Nejus?= Date: Sat, 14 Dec 2024 21:39:47 +0000 Subject: [PATCH 1/3] Fix mb_artistid, mb_albumartistid, albumtype diff issue --- beets/autotag/__init__.py | 39 +++++++++++++++++++++++++++++++++++++++ docs/changelog.rst | 8 ++++++++ test/test_autotag.py | 39 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 85 insertions(+), 1 deletion(-) diff --git a/beets/autotag/__init__.py b/beets/autotag/__init__.py index 70814c182..ca2020470 100644 --- a/beets/autotag/__init__.py +++ b/beets/autotag/__init__.py @@ -146,6 +146,43 @@ def apply_album_metadata(album_info: AlbumInfo, album: Album): _apply_metadata(album_info, album) +def ensure_consistent_list_fields(i: Item) -> None: + """Synchronise single and list values for the list fields that we use. + + That is, ensure the same value in the single field and the first element + in the list. + + For context, the value we set as, say, ``mb_artistid`` is simply ignored: + Under the current :class:`MediaFile` implementation, fields ``albumtype``, + ``mb_artistid`` and ``mb_albumartistid`` are mapped to the first element of + ``albumtypes``, ``mb_artistids`` and ``mb_albumartistids`` respectively. + + This means setting ``mb_artistid`` has no effect. However, beets + functionality still assumes that ``mb_artistid`` is independent and stores + its value in the database. If ``mb_artistid`` != ``mb_artistids[0]``, + ``beet write`` command thinks that ``mb_artistid`` is modified and tries to + update the field in the file. Of course nothing happens, so the same diff + is shown every time the command is run. + + We can avoid this issue by ensuring that ``mb_artistid`` has the same value + as ``mb_artistids[0]``, and that's what this function does. + """ + if aid := i.mb_artistid: + i.mb_artistids = list(dict.fromkeys([aid, *i.mb_artistids])) + elif aids := i.mb_artistids: + i.mb_artistid = aids[0] + + if aaid := i.mb_albumartistid: + i.mb_albumartistids = list(dict.fromkeys([aaid, *i.mb_albumartistids])) + elif aaids := i.mb_albumartistids: + i.mb_albumartistid = aaids[0] + + if atype := i.albumtype: + i.albumtypes = list(dict.fromkeys([atype, *i.albumtypes])) + elif atypes := i.albumtypes: + i.albumtype = atypes[0] + + def apply_metadata(album_info: AlbumInfo, mapping: Mapping[Item, TrackInfo]): """Set the items' metadata to match an AlbumInfo object using a mapping from Items to TrackInfo objects. @@ -250,6 +287,8 @@ def apply_metadata(album_info: AlbumInfo, mapping: Mapping[Item, TrackInfo]): item.mb_albumartistids = album_info.artists_ids item.mb_releasegroupid = album_info.releasegroup_id + ensure_consistent_list_fields(item) + # Compilation flag. item.comp = album_info.va diff --git a/docs/changelog.rst b/docs/changelog.rst index 34c1e6413..9fb3b9e3f 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -19,6 +19,14 @@ Bug fixes: have before the introduction of Poetry. :bug:`5531` :bug:`5526` +* :ref:`write-cmd`: Fix the issue where for certain files differences in + ``mb_artistid``, ``mb_albumartistid`` and ``albumtype`` fields are shown on + every attempt to write tags. Note: your music needs to be reimported with + ``beet import -LI`` or synchronised with ``beet mbsync`` in order to fix + this! + :bug:`5265` + :bug:`5371` + :bug:`4715` For packagers: diff --git a/test/test_autotag.py b/test/test_autotag.py index 6e362f2f4..1854bd654 100644 --- a/test/test_autotag.py +++ b/test/test_autotag.py @@ -20,7 +20,12 @@ import unittest import pytest from beets import autotag, config -from beets.autotag import AlbumInfo, TrackInfo, match +from beets.autotag import ( + AlbumInfo, + TrackInfo, + ensure_consistent_list_fields, + match, +) from beets.autotag.hooks import Distance, string_dist from beets.library import Item from beets.test.helper import BeetsTestCase @@ -1040,3 +1045,35 @@ class StringDistanceTest(unittest.TestCase): def test_accented_characters(self): dist = string_dist("\xe9\xe1\xf1", "ean") assert dist == 0.0 + + +@pytest.mark.parametrize( + "single_field,list_field", + [ + ("mb_artistid", "mb_artistids"), + ("mb_albumartistid", "mb_albumartistids"), + ("albumtype", "albumtypes"), + ], +) +@pytest.mark.parametrize( + "single_value,list_value", + [ + (None, []), + (None, ["1"]), + (None, ["1", "2"]), + ("1", []), + ("1", ["1"]), + ("1", ["1", "2"]), + ("1", ["2", "1"]), + ], +) +def test_ensure_consistent_list_fields( + single_field, list_field, single_value, list_value +): + data = {single_field: single_value, list_field: list_value} + item = Item(**data) + + ensure_consistent_list_fields(item) + + single_val, list_val = item[single_field], item[list_field] + assert (not single_val and not list_val) or single_val == list_val[0] From a091c2eeaef747b898afc64ec3e3fd3c3a92a656 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0ar=C5=ABnas=20Nejus?= Date: Sun, 15 Dec 2024 00:12:36 +0000 Subject: [PATCH 2/3] Ensure that list fields are corrected for album metadata too --- beets/autotag/__init__.py | 95 +++++++++++++++++++++------------------ test/test_autotag.py | 12 ++--- 2 files changed, 56 insertions(+), 51 deletions(-) diff --git a/beets/autotag/__init__.py b/beets/autotag/__init__.py index ca2020470..6bcb34665 100644 --- a/beets/autotag/__init__.py +++ b/beets/autotag/__init__.py @@ -18,7 +18,7 @@ from collections.abc import Mapping, Sequence from typing import Union from beets import config, logging -from beets.library import Album, Item +from beets.library import Album, Item, LibModel # Parts of external interface. from .hooks import AlbumInfo, AlbumMatch, Distance, TrackInfo, TrackMatch @@ -119,34 +119,7 @@ def _apply_metadata( db_obj[field] = value -def apply_item_metadata(item: Item, track_info: TrackInfo): - """Set an item's metadata from its matched TrackInfo object.""" - item.artist = track_info.artist - item.artists = track_info.artists - item.artist_sort = track_info.artist_sort - item.artists_sort = track_info.artists_sort - item.artist_credit = track_info.artist_credit - item.artists_credit = track_info.artists_credit - item.title = track_info.title - item.mb_trackid = track_info.track_id - item.mb_releasetrackid = track_info.release_track_id - if track_info.artist_id: - item.mb_artistid = track_info.artist_id - if track_info.artists_ids: - item.mb_artistids = track_info.artists_ids - - _apply_metadata(track_info, item) - - # At the moment, the other metadata is left intact (including album - # and track number). Perhaps these should be emptied? - - -def apply_album_metadata(album_info: AlbumInfo, album: Album): - """Set the album's metadata to match the AlbumInfo object.""" - _apply_metadata(album_info, album) - - -def ensure_consistent_list_fields(i: Item) -> None: +def correct_list_fields(m: LibModel) -> None: """Synchronise single and list values for the list fields that we use. That is, ensure the same value in the single field and the first element @@ -166,21 +139,57 @@ def ensure_consistent_list_fields(i: Item) -> None: We can avoid this issue by ensuring that ``mb_artistid`` has the same value as ``mb_artistids[0]``, and that's what this function does. + + Note: :class:`Album` model does not have ``mb_artistids`` and + ``mb_albumartistids`` fields therefore we need to check for their presence. """ - if aid := i.mb_artistid: - i.mb_artistids = list(dict.fromkeys([aid, *i.mb_artistids])) - elif aids := i.mb_artistids: - i.mb_artistid = aids[0] + if atype := m.albumtype: + m.albumtypes = list(dict.fromkeys([atype, *m.albumtypes])) + elif atypes := m.albumtypes: + m.albumtype = atypes[0] - if aaid := i.mb_albumartistid: - i.mb_albumartistids = list(dict.fromkeys([aaid, *i.mb_albumartistids])) - elif aaids := i.mb_albumartistids: - i.mb_albumartistid = aaids[0] + if hasattr(m, "mb_artistids"): + if aid := m.mb_artistid: + m.mb_artistids = list(dict.fromkeys([aid, *m.mb_artistids])) + elif aids := m.mb_artistids: + m.mb_artistid = aids[0] - if atype := i.albumtype: - i.albumtypes = list(dict.fromkeys([atype, *i.albumtypes])) - elif atypes := i.albumtypes: - i.albumtype = atypes[0] + if hasattr(m, "mb_albumartistids"): + if aaid := m.mb_albumartistid: + m.mb_albumartistids = list( + dict.fromkeys([aaid, *m.mb_albumartistids]) + ) + elif aaids := m.mb_albumartistids: + m.mb_albumartistid = aaids[0] + + +def apply_item_metadata(item: Item, track_info: TrackInfo): + """Set an item's metadata from its matched TrackInfo object.""" + item.artist = track_info.artist + item.artists = track_info.artists + item.artist_sort = track_info.artist_sort + item.artists_sort = track_info.artists_sort + item.artist_credit = track_info.artist_credit + item.artists_credit = track_info.artists_credit + item.title = track_info.title + item.mb_trackid = track_info.track_id + item.mb_releasetrackid = track_info.release_track_id + if track_info.artist_id: + item.mb_artistid = track_info.artist_id + if track_info.artists_ids: + item.mb_artistids = track_info.artists_ids + + _apply_metadata(track_info, item) + correct_list_fields(item) + + # At the moment, the other metadata is left intact (including album + # and track number). Perhaps these should be emptied? + + +def apply_album_metadata(album_info: AlbumInfo, album: Album): + """Set the album's metadata to match the AlbumInfo object.""" + _apply_metadata(album_info, album) + correct_list_fields(album) def apply_metadata(album_info: AlbumInfo, mapping: Mapping[Item, TrackInfo]): @@ -287,8 +296,6 @@ def apply_metadata(album_info: AlbumInfo, mapping: Mapping[Item, TrackInfo]): item.mb_albumartistids = album_info.artists_ids item.mb_releasegroupid = album_info.releasegroup_id - ensure_consistent_list_fields(item) - # Compilation flag. item.comp = album_info.va @@ -306,3 +313,5 @@ def apply_metadata(album_info: AlbumInfo, mapping: Mapping[Item, TrackInfo]): item, nullable_fields=config["overwrite_null"]["track"].as_str_seq(), ) + + correct_list_fields(item) diff --git a/test/test_autotag.py b/test/test_autotag.py index 1854bd654..e131a6be1 100644 --- a/test/test_autotag.py +++ b/test/test_autotag.py @@ -20,12 +20,7 @@ import unittest import pytest from beets import autotag, config -from beets.autotag import ( - AlbumInfo, - TrackInfo, - ensure_consistent_list_fields, - match, -) +from beets.autotag import AlbumInfo, TrackInfo, correct_list_fields, match from beets.autotag.hooks import Distance, string_dist from beets.library import Item from beets.test.helper import BeetsTestCase @@ -1067,13 +1062,14 @@ class StringDistanceTest(unittest.TestCase): ("1", ["2", "1"]), ], ) -def test_ensure_consistent_list_fields( +def test_correct_list_fields( single_field, list_field, single_value, list_value ): + """Ensure that the first value in a list field matches the single field.""" data = {single_field: single_value, list_field: list_value} item = Item(**data) - ensure_consistent_list_fields(item) + correct_list_fields(item) single_val, list_val = item[single_field], item[list_field] assert (not single_val and not list_val) or single_val == list_val[0] From 3b0c47799b32409ea8e62ecfbe4564c398f1bde2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0ar=C5=ABnas=20Nejus?= Date: Sun, 15 Dec 2024 13:22:07 +0000 Subject: [PATCH 3/3] Define unique_list and dedupe --- beets/autotag/__init__.py | 28 ++++++++++++++-------------- beets/util/__init__.py | 6 ++++++ 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/beets/autotag/__init__.py b/beets/autotag/__init__.py index 6bcb34665..42f957b0d 100644 --- a/beets/autotag/__init__.py +++ b/beets/autotag/__init__.py @@ -21,6 +21,8 @@ from beets import config, logging from beets.library import Album, Item, LibModel # Parts of external interface. +from beets.util import unique_list + from .hooks import AlbumInfo, AlbumMatch, Distance, TrackInfo, TrackMatch from .match import ( Proposal, @@ -143,24 +145,22 @@ def correct_list_fields(m: LibModel) -> None: Note: :class:`Album` model does not have ``mb_artistids`` and ``mb_albumartistids`` fields therefore we need to check for their presence. """ - if atype := m.albumtype: - m.albumtypes = list(dict.fromkeys([atype, *m.albumtypes])) - elif atypes := m.albumtypes: - m.albumtype = atypes[0] + + def ensure_first_value(single_field: str, list_field: str) -> None: + """Ensure the first ``list_field`` item is equal to ``single_field``.""" + single_val, list_val = getattr(m, single_field), getattr(m, list_field) + if single_val: + setattr(m, list_field, unique_list([single_val, *list_val])) + elif list_val: + setattr(m, single_field, list_val[0]) + + ensure_first_value("albumtype", "albumtypes") if hasattr(m, "mb_artistids"): - if aid := m.mb_artistid: - m.mb_artistids = list(dict.fromkeys([aid, *m.mb_artistids])) - elif aids := m.mb_artistids: - m.mb_artistid = aids[0] + ensure_first_value("mb_artistid", "mb_artistids") if hasattr(m, "mb_albumartistids"): - if aaid := m.mb_albumartistid: - m.mb_albumartistids = list( - dict.fromkeys([aaid, *m.mb_albumartistids]) - ) - elif aaids := m.mb_albumartistids: - m.mb_albumartistid = aaids[0] + ensure_first_value("mb_albumartistid", "mb_albumartistids") def apply_item_metadata(item: Item, track_info: TrackInfo): diff --git a/beets/util/__init__.py b/beets/util/__init__.py index 581e5cef7..b5bd859be 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -39,6 +39,7 @@ from typing import ( Any, AnyStr, Callable, + Iterable, NamedTuple, TypeVar, Union, @@ -1126,3 +1127,8 @@ def get_temp_filename( _, filename = tempfile.mkstemp(dir=tempdir, prefix=prefix, suffix=suffix) return bytestring_path(filename) + + +def unique_list(elements: Iterable[T]) -> list[T]: + """Return a list with unique elements in the original order.""" + return list(dict.fromkeys(elements))