From a1047c634e52510534a7d1e12135563fa60e3639 Mon Sep 17 00:00:00 2001 From: Khoa Nguyen Date: Sat, 20 Sep 2025 06:09:30 -0400 Subject: [PATCH 1/2] fix: cached value and cached sorting issues --- picard/ui/itemviews/__init__.py | 2 + picard/ui/itemviews/basetreeview.py | 26 +++ picard/ui/itemviews/custom_columns/column.py | 13 ++ .../ui/itemviews/custom_columns/protocols.py | 14 ++ .../custom_columns/script_provider.py | 31 +++ .../custom_columns/sorting_adapters.py | 12 ++ test/columns_api/__init__.py | 21 ++ .../test_custom_columns_invalidation.py | 186 ++++++++++++++++++ 8 files changed, 305 insertions(+) create mode 100644 test/columns_api/__init__.py create mode 100644 test/columns_api/test_custom_columns_invalidation.py diff --git a/picard/ui/itemviews/__init__.py b/picard/ui/itemviews/__init__.py index 953421a0ea..abf1d38266 100644 --- a/picard/ui/itemviews/__init__.py +++ b/picard/ui/itemviews/__init__.py @@ -418,6 +418,8 @@ def update_colums_text(self, color=None, bgcolor=None): self.setTextAlignment(i, QtCore.Qt.AlignmentFlag.AlignRight | QtCore.Qt.AlignmentFlag.AlignVCenter) if isinstance(column, CustomColumn): + # Invalidate caches for this object to reflect tag changes + column.invalidate_cache(self.obj) # Hide custom column values for container rows like # "Clusters" and "Unclustered Files". # These represent unrelated collections and should not diff --git a/picard/ui/itemviews/basetreeview.py b/picard/ui/itemviews/basetreeview.py index 90af9d04cb..7e0f1eb035 100644 --- a/picard/ui/itemviews/basetreeview.py +++ b/picard/ui/itemviews/basetreeview.py @@ -462,8 +462,15 @@ def _on_header_updated(self): recognized = set(get_recognized_view_columns().values()) if self.columns not in recognized: return + # Invalidate provider and item sort caches to avoid incompatible keys + self._invalidate_view_caches() self._refresh_header_labels() self._refresh_all_items_data() + # Trigger resort on current column if sorting is enabled + if self.isSortingEnabled(): + current = self.sortColumn() + if current >= 0: + self.sortByColumn(current, self.header().sortIndicatorOrder()) def _init_header(self): # Load any persisted user-defined custom columns before header setup @@ -483,6 +490,25 @@ def _init_header(self): self.restore_default_columns() self.restore_state() + def _invalidate_view_caches(self): + """Clear provider caches and per-item sort keys for this view.""" + # Invalidate provider caches for custom columns in this view + from picard.ui.itemviews.custom_columns import CustomColumn # local import to avoid cycles + + for col in self.columns: + if isinstance(col, CustomColumn): + col.invalidate_cache() + + # Clear cached sort keys on all items + def _clear_item_sort_cache(item): + if hasattr(item, "_sortkeys") and isinstance(item._sortkeys, dict): + item._sortkeys.clear() + for i in range(item.childCount()): + _clear_item_sort_cache(item.child(i)) + + for i in range(self.topLevelItemCount()): + _clear_item_sort_cache(self.topLevelItem(i)) + def supportedDropActions(self): return QtCore.Qt.DropAction.CopyAction | QtCore.Qt.DropAction.MoveAction diff --git a/picard/ui/itemviews/custom_columns/column.py b/picard/ui/itemviews/custom_columns/column.py index 5c17b84ad3..68b08afad0 100644 --- a/picard/ui/itemviews/custom_columns/column.py +++ b/picard/ui/itemviews/custom_columns/column.py @@ -28,6 +28,7 @@ ColumnSortType, ) from picard.ui.itemviews.custom_columns.protocols import ( + CacheInvalidatable, ColumnValueProvider, SortKeyProvider, ) @@ -81,3 +82,15 @@ def __init__( status_icon=False, ) self.provider = provider + + def invalidate_cache(self, obj=None): # pragma: no cover - UI-driven + """Invalidate any caches on the provider if supported. + + Parameters + ---------- + obj + Optional item to invalidate for; if omitted, clears entire cache. + """ + if isinstance(self.provider, CacheInvalidatable): + # type: ignore[attr-defined] + self.provider.invalidate(obj) # noqa: PGH003 diff --git a/picard/ui/itemviews/custom_columns/protocols.py b/picard/ui/itemviews/custom_columns/protocols.py index 6c28b54486..e0c8c49d6c 100644 --- a/picard/ui/itemviews/custom_columns/protocols.py +++ b/picard/ui/itemviews/custom_columns/protocols.py @@ -30,6 +30,20 @@ from picard.item import Item +@runtime_checkable +class CacheInvalidatable(Protocol): + def invalidate(self, obj: Item | None = None) -> None: # pragma: no cover - optional + """Invalidate any cached values. + + Parameters + ---------- + obj + If provided, invalidate cache entries related to this item only. + If ``None``, invalidate the entire cache. + """ + ... + + @runtime_checkable class ColumnValueProvider(Protocol): def evaluate(self, obj: Item) -> str: diff --git a/picard/ui/itemviews/custom_columns/script_provider.py b/picard/ui/itemviews/custom_columns/script_provider.py index f4e3a1ae5e..3aa366a913 100644 --- a/picard/ui/itemviews/custom_columns/script_provider.py +++ b/picard/ui/itemviews/custom_columns/script_provider.py @@ -24,6 +24,7 @@ from collections import deque from collections.abc import Callable +import contextlib import re from time import perf_counter from weakref import WeakKeyDictionary @@ -154,3 +155,33 @@ def __repr__(self) -> str: # pragma: no cover - debug helper f"{cls_name}(script={self._script!r}, max_runtime_ms={self._max_runtime_ms}, " f"cache_size={self._id_cache_max})" ) + + # Optional cache invalidation API (duck-typed via protocol) + def invalidate(self, obj: Item | None = None) -> None: + """Invalidate cached values. + + Parameters + ---------- + obj + If provided, invalidate cache for this specific item; otherwise + clear the entire cache. + """ + if obj is None: + # Clear all caches + self._cache.clear() + self._id_cache.clear() + self._id_order.clear() + return + + # Try to remove from weak cache + with contextlib.suppress(TypeError): # Not weakrefable; fall back to id-cache removal + if obj in self._cache: + del self._cache[obj] + + # Remove from id-based cache if present + obj_id = id(obj) + if obj_id in self._id_cache: + self._id_cache.pop(obj_id, None) + with contextlib.suppress(ValueError): + # Remove one occurrence from order deque + self._id_order.remove(obj_id) diff --git a/picard/ui/itemviews/custom_columns/sorting_adapters.py b/picard/ui/itemviews/custom_columns/sorting_adapters.py index 5a9ef75037..12fcdfc613 100644 --- a/picard/ui/itemviews/custom_columns/sorting_adapters.py +++ b/picard/ui/itemviews/custom_columns/sorting_adapters.py @@ -298,6 +298,18 @@ def sort_key(self, obj: Item): # pragma: no cover - thin wrapper pass return key + # Optional cache invalidation API for adapter + def invalidate(self, obj: Item | None = None) -> None: # pragma: no cover - simple + if obj is None: + self._cache.clear() + else: + # Attempt precise removal; fall back to full clear if not weakrefable + try: + del self._cache[obj] + except (KeyError, TypeError): + # Not present or not weakrefable; best effort clear all to avoid stale keys + self._cache.clear() + class ReverseAdapter(_AdapterBase): """Provide reversed sorting for string or numeric sort keys.""" diff --git a/test/columns_api/__init__.py b/test/columns_api/__init__.py new file mode 100644 index 0000000000..cd2f27c257 --- /dev/null +++ b/test/columns_api/__init__.py @@ -0,0 +1,21 @@ +# -*- coding: utf-8 -*- +# +# Picard, the next-generation MusicBrainz tagger +# +# Copyright (C) 2025 The MusicBrainz Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + +"""Tests for public API for custom columns: types, factories and registry.""" diff --git a/test/columns_api/test_custom_columns_invalidation.py b/test/columns_api/test_custom_columns_invalidation.py new file mode 100644 index 0000000000..36ab811491 --- /dev/null +++ b/test/columns_api/test_custom_columns_invalidation.py @@ -0,0 +1,186 @@ +# -*- coding: utf-8 -*- +# +# Picard, the next-generation MusicBrainz tagger +# +# Copyright (C) 2025 The MusicBrainz Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + +from __future__ import annotations + +from collections.abc import Callable +from dataclasses import dataclass + +from picard.item import Item +from picard.metadata import Metadata + +import pytest + +from picard.ui.columns import ( + ColumnAlign, + ColumnSortType, +) +from picard.ui.itemviews.custom_columns import CustomColumn +from picard.ui.itemviews.custom_columns.protocols import ColumnValueProvider +from picard.ui.itemviews.custom_columns.script_provider import ChainedValueProvider +from picard.ui.itemviews.custom_columns.sorting_adapters import CachedSortAdapter + + +# --- Fixtures and helpers ---------------------------------------------------- + + +@dataclass +class _FakeItem: + values: dict[str, str] + + def column(self, key: str) -> str: + return self.values.get(key, "") + + @property + def metadata(self) -> Metadata: + md = Metadata() + for k, v in self.values.items(): + md[k] = v + return md + + +class _NonWeakRefable: + def __init__(self, artist: str) -> None: + self.artist = artist + + def column(self, key: str) -> str: + return self.artist if key == "artist" else "" + + @property + def metadata(self) -> Metadata: + md = Metadata() + md["artist"] = self.artist + return md + + +@pytest.fixture(params=["weak", "nonweak"]) +def item_factory(request: pytest.FixtureRequest) -> Callable[[str], object]: + def _make(initial_value: str) -> object: + if request.param == "weak": + return _FakeItem(values={"artist": initial_value}) + return _NonWeakRefable(initial_value) + + return _make + + +# --- Tests: provider invalidation -------------------------------------------- + + +def test_chained_provider_invalidate_single_and_all(item_factory: Callable[[str], Item]) -> None: + provider = ChainedValueProvider("%artist%", max_runtime_ms=1000) + obj = item_factory("Artist A") + + # Initial evaluation and cache + assert provider.evaluate(obj) == "Artist A" + + # Change underlying value; still cached until invalidated + if isinstance(obj, _FakeItem): + obj.values["artist"] = "Artist B" + else: + obj.artist = "Artist B" # type: ignore[attr-defined] + assert provider.evaluate(obj) == "Artist A" + + # Invalidate for this object only + provider.invalidate(obj) + assert provider.evaluate(obj) == "Artist B" + + # Cache again; change value to C + if isinstance(obj, _FakeItem): + obj.values["artist"] = "Artist C" + else: + obj.artist = "Artist C" # type: ignore[attr-defined] + assert provider.evaluate(obj) == "Artist B" + + # Invalidate all and confirm new value + provider.invalidate(None) + assert provider.evaluate(obj) == "Artist C" + + +# --- Tests: CustomColumn.invalidate_cache delegation ------------------------- + + +class _SpyProvider(ChainedValueProvider): + def __init__(self, script: str) -> None: + super().__init__(script) + self.calls: list[object | None] = [] + + def invalidate(self, obj: object | None = None) -> None: # type: ignore[override] + self.calls.append(obj) + super().invalidate(obj) # keep behavior intact + + +def test_custom_column_invalidate_cache_delegates() -> None: + spy = _SpyProvider("%artist%") + col = CustomColumn( + title="T", + key="k", + provider=spy, + width=None, + align=ColumnAlign.LEFT, + sort_type=ColumnSortType.TEXT, + always_visible=False, + ) + + # Call without obj and with obj + col.invalidate_cache(None) + fake = _FakeItem(values={"artist": "A"}) + col.invalidate_cache(fake) + + assert spy.calls == [None, fake] + + +# --- Tests: CachedSortAdapter invalidation ----------------------------------- + + +class _ValueItem: + def __init__(self, value: str) -> None: + self.value = value + + +class _ValueProvider(ColumnValueProvider): + def evaluate(self, obj: _ValueItem) -> str: + return obj.value + + +def test_cached_sort_adapter_invalidate_single_and_all() -> None: + provider = _ValueProvider() + adapter = CachedSortAdapter(provider) + a_item = _ValueItem("B") + b_item = _ValueItem("a") + + # Populate cache + assert adapter.sort_key(a_item) == "b" + assert adapter.sort_key(b_item) == "a" + + # Change values; still cached + a_item.value = "Z" + b_item.value = "y" + assert adapter.sort_key(a_item) == "b" + assert adapter.sort_key(b_item) == "a" + + # Invalidate single + adapter.invalidate(a_item) + assert adapter.sort_key(a_item) == "z" + # b still cached + assert adapter.sort_key(b_item) == "a" + + # Invalidate all + adapter.invalidate(None) + assert adapter.sort_key(b_item) == "y" From c38df5939dc903cff2bc9e428be0f39ef969cfe8 Mon Sep 17 00:00:00 2001 From: Khoa Nguyen Date: Sun, 28 Sep 2025 10:09:11 -0400 Subject: [PATCH 2/2] fix: Add `invalidate` to `_AdapterBase` --- .../custom_columns/sorting_adapters.py | 15 +++ ...orting_adapters_invalidation_forwarding.py | 119 ++++++++++++++++++ 2 files changed, 134 insertions(+) create mode 100644 test/columns_api/test_sorting_adapters_invalidation_forwarding.py diff --git a/picard/ui/itemviews/custom_columns/sorting_adapters.py b/picard/ui/itemviews/custom_columns/sorting_adapters.py index 12fcdfc613..686436f400 100644 --- a/picard/ui/itemviews/custom_columns/sorting_adapters.py +++ b/picard/ui/itemviews/custom_columns/sorting_adapters.py @@ -28,6 +28,7 @@ from picard.item import Item from picard.ui.itemviews.custom_columns.protocols import ( + CacheInvalidatable, ColumnValueProvider, SortKeyProvider, ) @@ -62,6 +63,18 @@ def evaluate(self, obj: Item) -> str: """Return evaluated text value for item.""" return self._base.evaluate(obj) + def invalidate(self, obj: Item | None = None) -> None: # pragma: no cover - simple delegation + """Forward cache invalidation to the wrapped provider if supported. + + Parameters + ---------- + obj + Optional item to invalidate for; if omitted, clears entire cache. + """ + # Not all providers implement `invalidate`; the check avoids `AttributeError` + if isinstance(self._base, CacheInvalidatable): + self._base.invalidate(obj) + def __repr__(self) -> str: # pragma: no cover - debug helper return f"{self.__class__.__name__}(base={self._base!r})" @@ -309,6 +322,8 @@ def invalidate(self, obj: Item | None = None) -> None: # pragma: no cover - sim except (KeyError, TypeError): # Not present or not weakrefable; best effort clear all to avoid stale keys self._cache.clear() + # Also forward invalidation to the wrapped provider if it supports it + super().invalidate(obj) class ReverseAdapter(_AdapterBase): diff --git a/test/columns_api/test_sorting_adapters_invalidation_forwarding.py b/test/columns_api/test_sorting_adapters_invalidation_forwarding.py new file mode 100644 index 0000000000..ceaa1324a4 --- /dev/null +++ b/test/columns_api/test_sorting_adapters_invalidation_forwarding.py @@ -0,0 +1,119 @@ +# -*- coding: utf-8 -*- +# +# Picard, the next-generation MusicBrainz tagger +# +# Copyright (C) 2025 The MusicBrainz Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + +"""Tests for sorting adapters' invalidation forwarding.""" + +from __future__ import annotations + +from typing import Protocol + +import pytest + +from picard.ui.columns import ColumnAlign, ColumnSortType +from picard.ui.itemviews.custom_columns import ( + CachedSortAdapter, + CasefoldSortAdapter, + CustomColumn, +) + + +class _CacheInvalidatable(Protocol): + def invalidate(self, obj: object | None = None) -> None: # pragma: no cover - protocol + ... + + +class _ValueItem: + def __init__(self, value: str) -> None: + self.value = value + + +class _SpyProvider: + def __init__(self) -> None: + self.calls: list[object | None] = [] + + def evaluate(self, obj: _ValueItem) -> str: + return obj.value + + # CacheInvalidatable-compatible API + def invalidate(self, obj: object | None = None) -> None: # type: ignore[override] + self.calls.append(obj) + + +@pytest.mark.parametrize( + "adapter_factory", + [ + lambda base: CasefoldSortAdapter(base), + lambda base: CachedSortAdapter(base), + ], +) +def test_adapter_invalidate_forwards_to_base(adapter_factory) -> None: + spy = _SpyProvider() + adapter = adapter_factory(spy) + + a = _ValueItem("A") + + # Forward single-item invalidation + # type: ignore[attr-defined] + adapter.invalidate(a) # noqa: PGH003 + # Forward full invalidation + # type: ignore[attr-defined] + adapter.invalidate(None) # noqa: PGH003 + + assert spy.calls == [a, None] + + +def test_cached_sort_adapter_invalidate_forwards_and_recomputes() -> None: + spy = _SpyProvider() + adapter = CachedSortAdapter(spy) + + it = _ValueItem("B") + # Populate cache + key1 = adapter.sort_key(it) + assert key1 == "b" + + # Change value; still cached until invalidated + it.value = "Z" + assert adapter.sort_key(it) == "b" + + # Invalidate and ensure base received invalidation, and key recomputed + # type: ignore[attr-defined] + adapter.invalidate(it) # noqa: PGH003 + assert spy.calls == [it] + assert adapter.sort_key(it) == "z" + + +def test_custom_column_invalidate_cache_delegates_through_adapter() -> None: + spy = _SpyProvider() + adapter = CasefoldSortAdapter(spy) + col = CustomColumn( + title="T", + key="k", + provider=adapter, + width=None, + align=ColumnAlign.LEFT, + sort_type=ColumnSortType.SORTKEY, + always_visible=False, + ) + + a = _ValueItem("a") + col.invalidate_cache(None) + col.invalidate_cache(a) + + assert spy.calls == [None, a]