Skip to content

Commit

Permalink
Stop perpetually writing mb_artistid, mb_albumartistid and `album…
Browse files Browse the repository at this point in the history
…types` fields (#5540)

This PR fixes an issue where the `beet write` command repeatedly shows
differences for certain fields (`mb_artistid`, `mb_albumartistid`,
`albumtype`) even after writing the tags.
This happens because these fields are actually stored as lists in the
media files (`mb_artistids`, `mb_albumartistids`, `albumtypes`), but
beets maintains both single and list versions in its database.

This PR addresses this issue in a non-invasive way: the fix ensures
consistency between single fields and their list counterparts by:
1. When setting a single field value, making it the first element of the
corresponding list
2. When setting a list, using its first element as the single field
value

This resolves long-standing issues #5265, #5371, and #4715 where users
experienced persistent "differences" in these fields despite writing
tags multiple times.

Changes:
- Added `ensure_consistent_list_fields()` function to synchronize field
pairs
- Applied this function during metadata application
- Added tests for all field combinations

Fixes #5265, #5371, #4715

**Note**: your music needs to be reimported with `beet import -LI` or
synchronised with `beet mbsync` in order to fix these fields!
  • Loading branch information
snejus authored Dec 15, 2024
2 parents 22163d7 + 3b0c477 commit 9110a11
Show file tree
Hide file tree
Showing 4 changed files with 97 additions and 2 deletions.
50 changes: 49 additions & 1 deletion beets/autotag/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@
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,
Expand Down Expand Up @@ -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
Expand All @@ -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?
Expand All @@ -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]):
Expand Down Expand Up @@ -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)
6 changes: 6 additions & 0 deletions beets/util/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
Any,
AnyStr,
Callable,
Iterable,
NamedTuple,
TypeVar,
Union,
Expand Down Expand Up @@ -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))
8 changes: 8 additions & 0 deletions docs/changelog.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down
35 changes: 34 additions & 1 deletion test/test_autotag.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
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
Expand Down Expand Up @@ -1040,3 +1040,36 @@ def test_ampersand_expansion(self):
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]

0 comments on commit 9110a11

Please sign in to comment.