Improve and simplify show_model_changes

beets/dbcore/db.py

@@ -105,6 +105,8 @@ class FormattedMapping(Mapping[str, str]):
     are replaced.
     """
 
+    model: Model
+
     ALL_KEYS = "*"
 
     def __init__(
@@ -714,7 +716,7 @@ class Model(ABC, Generic[D]):
         self,
         included_keys: str = _formatter.ALL_KEYS,
         for_path: bool = False,
-    ):
+    ) -> FormattedMapping:
         """Get a mapping containing all values on this object formatted
         as human-readable unicode strings.
         """

beets/ui/__init__.py

@@ -43,7 +43,10 @@ from beets.util.deprecation import deprecate_for_maintainers
 from beets.util.functemplate import template
 
 if TYPE_CHECKING:
-    from collections.abc import Callable
+    from collections.abc import Callable, Iterable
+
+    from beets.dbcore.db import FormattedMapping
+
 
 # On Windows platforms, use colorama to support "ANSI" terminal colors.
 if sys.platform == "win32":
@@ -1028,42 +1031,47 @@ def print_newline_layout(
 FLOAT_EPSILON = 0.01
 
 
-def _field_diff(field, old, old_fmt, new, new_fmt):
+def _field_diff(
+    field: str, old: FormattedMapping, new: FormattedMapping
+) -> str | None:
     """Given two Model objects and their formatted views, format their values
     for `field` and highlight changes among them. Return a human-readable
     string. If the value has not changed, return None instead.
     """
-    oldval = old.get(field)
-    newval = new.get(field)
-
     # If no change, abort.
-    if (
+    if (oldval := old.model.get(field)) == (newval := new.model.get(field)) or (
         isinstance(oldval, float)
         and isinstance(newval, float)
         and abs(oldval - newval) < FLOAT_EPSILON
     ):
         return None
-    elif oldval == newval:
-        return None
 
     # Get formatted values for output.
-    oldstr = old_fmt.get(field, "")
-    newstr = new_fmt.get(field, "")
+    oldstr, newstr = old.get(field, ""), new.get(field, "")
+    if field not in new:
+        return colorize("text_diff_removed", f"{field}: {oldstr}")
+
+    if field not in old:
+        return colorize("text_diff_added", f"{field}: {newstr}")
 
     # For strings, highlight changes. For others, colorize the whole
     # thing.
     if isinstance(oldval, str):
-        oldstr, newstr = colordiff(oldval, newstr)
+        oldstr, newstr = colordiff(oldstr, newstr)
     else:
         oldstr = colorize("text_diff_removed", oldstr)
         newstr = colorize("text_diff_added", newstr)
 
-    return f"{oldstr} -> {newstr}"
+    return f"{field}: {oldstr} -> {newstr}"
 
 
 def show_model_changes(
-    new, old=None, fields=None, always=False, print_obj: bool = True
-):
+    new: library.LibModel,
+    old: library.LibModel | None = None,
+    fields: Iterable[str] | None = None,
+    always: bool = False,
+    print_obj: bool = True,
+) -> bool:
     """Given a Model object, print a list of changes from its pristine
     version stored in the database. Return a boolean indicating whether
     any changes were found.
@@ -1081,31 +1089,21 @@ def show_model_changes(
     new_fmt = new.formatted()
 
     # Build up lines showing changed fields.
-    changes = []
-    for field in old:
-        # Subset of the fields. Never show mtime.
-        if field == "mtime" or (fields and field not in fields):
-            continue
+    diff_fields = (set(old) | set(new)) - {"mtime"}
+    if allowed_fields := set(fields or {}):
+        diff_fields &= allowed_fields
 
-        # Detect and show difference for this field.
-        line = _field_diff(field, old, old_fmt, new, new_fmt)
-        if line:
-            changes.append(f"  {field}: {line}")
-
-    # New fields.
-    for field in set(new) - set(old):
-        if fields and field not in fields:
-            continue
-
-        changes.append(
-            f"  {field}: {colorize('text_highlight', new_fmt[field])}"
-        )
+    changes = [
+        d
+        for f in sorted(diff_fields)
+        if (d := _field_diff(f, old_fmt, new_fmt))
+    ]
 
     # Print changes.
     if print_obj and (changes or always):
         print_(format(old))
     if changes:
-        print_("\n".join(changes))
+        print_(textwrap.indent("\n".join(changes), "  "))
 
     return bool(changes)
 

docs/changelog.rst

@@ -70,6 +70,8 @@ Bug fixes:
 - When using :doc:`plugins/fromfilename` together with :doc:`plugins/edit`,
   temporary tags extracted from filenames are no longer lost when discarding or
   cancelling an edit session during import. :bug:`6104`
+- :ref:`update-cmd` :doc:`plugins/edit` fix display formatting of field changes
+  to clearly show added and removed flexible fields.
 
 For plugin developers:
 

pyproject.toml

@@ -322,6 +322,7 @@ ignore = [
 [tool.ruff.lint.per-file-ignores]
 "beets/**" = ["PT"]
 "test/test_util.py" = ["E501"]
+"test/ui/test_field_diff.py" = ["E501"]
 
 [tool.ruff.lint.isort]
 split-on-trailing-comma = false

test/ui/test_field_diff.py

@@ -0,0 +1,64 @@
+import pytest
+
+from beets.library import Item
+from beets.test.helper import ConfigMixin
+from beets.ui import _field_diff
+
+p = pytest.param
+
+
+class TestFieldDiff:
+    @pytest.fixture(scope="class", autouse=True)
+    def config(self):
+        return ConfigMixin().config
+
+    @pytest.fixture(autouse=True)
+    def configure_color(self, config, color):
+        config["ui"]["color"] = color
+
+    @pytest.fixture(autouse=True)
+    def patch_colorize(self, monkeypatch):
+        """Patch to return a deterministic string format instead of ANSI codes."""
+        monkeypatch.setattr(
+            "beets.ui.colorize",
+            lambda color_name, text: f"[{color_name}]{text}[/]",
+        )
+
+    @staticmethod
+    def diff_fmt(old, new):
+        return f"[text_diff_removed]{old}[/] -> [text_diff_added]{new}[/]"
+
+    @pytest.mark.parametrize(
+        "old_data, new_data, field, expected_diff",
+        [
+            p({"title": "foo"}, {"title": "foo"}, "title", None, id="no_change"),
+            p({"bpm": 120.0}, {"bpm": 120.005}, "bpm", None, id="float_close_enough"),
+            p({"bpm": 120.0}, {"bpm": 121.0}, "bpm", f"bpm: {diff_fmt('120', '121')}", id="float_changed"),
+            p({"title": "foo"}, {"title": "bar"}, "title", f"title: {diff_fmt('foo', 'bar')}", id="string_full_replace"),
+            p({"title": "prefix foo"}, {"title": "prefix bar"}, "title", "title: prefix [text_diff_removed]foo[/] -> prefix [text_diff_added]bar[/]", id="string_partial_change"),
+            p({"year": 2000}, {"year": 2001}, "year", f"year: {diff_fmt('2000', '2001')}", id="int_changed"),
+            p({}, {"genre": "Rock"}, "genre", "genre:  -> [text_diff_added]Rock[/]", id="field_added"),
+            p({"genre": "Rock"}, {}, "genre", "genre: [text_diff_removed]Rock[/] -> ", id="field_removed"),
+            p({"track": 1}, {"track": 2}, "track", f"track: {diff_fmt('01', '02')}", id="formatted_value_changed"),
+            p({"mb_trackid": None}, {"mb_trackid": "1234"}, "mb_trackid", "mb_trackid:  -> [text_diff_added]1234[/]", id="none_to_value"),
+            p({}, {"new_flex": "foo"}, "new_flex", "[text_diff_added]new_flex: foo[/]", id="flex_field_added"),
+            p({"old_flex": "foo"}, {}, "old_flex", "[text_diff_removed]old_flex: foo[/]", id="flex_field_removed"),
+        ],
+    )  # fmt: skip
+    @pytest.mark.parametrize("color", [True], ids=["color_enabled"])
+    def test_field_diff_colors(self, old_data, new_data, field, expected_diff):
+        old_item = Item(**old_data)
+        new_item = Item(**new_data)
+
+        diff = _field_diff(field, old_item.formatted(), new_item.formatted())
+
+        assert diff == expected_diff
+
+    @pytest.mark.parametrize("color", [False], ids=["color_disabled"])
+    def test_field_diff_no_color(self):
+        old_item = Item(title="foo")
+        new_item = Item(title="bar")
+
+        diff = _field_diff("title", old_item.formatted(), new_item.formatted())
+
+        assert diff == "title: foo -> bar"