diff --git a/beets/autotag/__init__.py b/beets/autotag/__init__.py index 70814c182..42f957b0d 100644 --- a/beets/autotag/__init__.py +++ b/beets/autotag/__init__.py @@ -18,9 +18,11 @@ 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 beets.util import unique_list + from .hooks import AlbumInfo, AlbumMatch, Distance, TrackInfo, TrackMatch from .match import ( Proposal, @@ -119,6 +121,48 @@ def _apply_metadata( db_obj[field] = value +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 + 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. + + Note: :class:`Album` model does not have ``mb_artistids`` and + ``mb_albumartistids`` fields therefore we need to check for their presence. + """ + + 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"): + ensure_first_value("mb_artistid", "mb_artistids") + + if hasattr(m, "mb_albumartistids"): + ensure_first_value("mb_albumartistid", "mb_albumartistids") + + def apply_item_metadata(item: Item, track_info: TrackInfo): """Set an item's metadata from its matched TrackInfo object.""" item.artist = track_info.artist @@ -136,6 +180,7 @@ def apply_item_metadata(item: Item, track_info: TrackInfo): 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? @@ -144,6 +189,7 @@ def apply_item_metadata(item: Item, track_info: TrackInfo): 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]): @@ -267,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/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)) 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..e131a6be1 100644 --- a/test/test_autotag.py +++ b/test/test_autotag.py @@ -20,7 +20,7 @@ import unittest import pytest from beets import autotag, config -from beets.autotag import AlbumInfo, TrackInfo, 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 @@ -1040,3 +1040,36 @@ 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_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) + + 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]