From cde03a1d6a508bfee3726beeec5146b52556b09f Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Thu, 18 Sep 2025 13:50:38 +0200 Subject: [PATCH 1/7] Remove descending sort adapters --- .../ui/itemviews/custom_columns/__init__.py | 6 -- picard/ui/itemviews/custom_columns/shared.py | 3 - .../custom_columns/sorting_adapters.py | 57 ------------- picard/ui/itemviews/custom_columns/storage.py | 6 -- test/test_custom_columns_sorting_adapters.py | 81 ------------------- ...test_sorting_adapters_key_stabilisation.py | 16 ---- 6 files changed, 169 deletions(-) diff --git a/picard/ui/itemviews/custom_columns/__init__.py b/picard/ui/itemviews/custom_columns/__init__.py index 9d9e1e577c..9b7d5d6a21 100644 --- a/picard/ui/itemviews/custom_columns/__init__.py +++ b/picard/ui/itemviews/custom_columns/__init__.py @@ -41,9 +41,6 @@ CachedSortAdapter, CasefoldSortAdapter, CompositeSortAdapter, - DescendingCasefoldSortAdapter, - DescendingNaturalSortAdapter, - DescendingNumericSortAdapter, LengthSortAdapter, NaturalSortAdapter, NullsFirstAdapter, @@ -66,11 +63,8 @@ "_create_custom_column", # Sorting adapters "CasefoldSortAdapter", - "DescendingCasefoldSortAdapter", "NumericSortAdapter", - "DescendingNumericSortAdapter", "NaturalSortAdapter", - "DescendingNaturalSortAdapter", "LengthSortAdapter", "ArticleInsensitiveAdapter", "CompositeSortAdapter", diff --git a/picard/ui/itemviews/custom_columns/shared.py b/picard/ui/itemviews/custom_columns/shared.py index 4396adc5b6..fea5b35659 100644 --- a/picard/ui/itemviews/custom_columns/shared.py +++ b/picard/ui/itemviews/custom_columns/shared.py @@ -298,11 +298,8 @@ def generate_new_key() -> str: SORTING_ADAPTER_NAMES: dict[str, str] = { N_("Default"): "", # No adapter (use default sorting) N_("Case Insensitive"): "CasefoldSortAdapter", - N_("Case Insensitive - Descending"): "DescendingCasefoldSortAdapter", N_("Numeric"): "NumericSortAdapter", - N_("Numeric - Descending"): "DescendingNumericSortAdapter", N_("Natural"): "NaturalSortAdapter", - N_("Natural - Descending"): "DescendingNaturalSortAdapter", N_("By Value Length"): "LengthSortAdapter", N_("Article Insensitive"): "ArticleInsensitiveAdapter", N_("Empty Values Last"): "NullsLastAdapter", diff --git a/picard/ui/itemviews/custom_columns/sorting_adapters.py b/picard/ui/itemviews/custom_columns/sorting_adapters.py index 5a9ef75037..4419105195 100644 --- a/picard/ui/itemviews/custom_columns/sorting_adapters.py +++ b/picard/ui/itemviews/custom_columns/sorting_adapters.py @@ -74,21 +74,6 @@ def sort_key(self, obj: Item): # pragma: no cover - thin wrapper return (self._base.evaluate(obj) or "").casefold() -class DescendingCasefoldSortAdapter(_AdapterBase): - """Provide descending case-insensitive sort by inverting code points.""" - - @staticmethod - def _invert_string(s: str) -> str: - # Map code points to inverted order to simulate descending in ascending sort - # Use BMP range for practicality; characters outside will still order consistently - return "".join(chr(0x10FFFF - ord(c)) for c in s) - - def sort_key(self, obj: Item): # pragma: no cover - thin wrapper - """Return descending case-insensitive sort key for item.""" - v = (self._base.evaluate(obj) or "").casefold() - return self._invert_string(v) - - class NumericSortAdapter(_AdapterBase): """Provide numeric sort using a parser (default: float) on evaluated value.""" @@ -116,26 +101,6 @@ def sort_key(self, obj: Item): # pragma: no cover - thin wrapper return (0, parsed_value) -class DescendingNumericSortAdapter(NumericSortAdapter): - """Provide descending numeric sort by negating parsed value.""" - - def sort_key(self, obj: Item): # pragma: no cover - thin wrapper - """Return descending numeric-first composite sort key for item. - - - (0, -number) when numeric (numbers first, descending) - - (1, inverted_natural_key) when non-numeric (fallback, descending) - """ - value_str: str = self._base.evaluate(obj) or "" - try: - parsed_value = self._parser(value_str) - except (ValueError, TypeError): - natural_key = _sort_key(value_str, numeric=True) - inverted = DescendingNaturalSortAdapter._invert_string(str(natural_key)) - return (1, inverted) - else: - return (0, -parsed_value) - - class LengthSortAdapter(_AdapterBase): """Provide sort by string length of evaluated value.""" @@ -340,25 +305,3 @@ def sort_key(self, obj: Item): # pragma: no cover - thin wrapper For example: "Track 1", "Track 2", "Track 10" instead of "Track 1", "Track 10", "Track 2". """ return _sort_key(self._base.evaluate(obj) or "", numeric=True) - - -class DescendingNaturalSortAdapter(_AdapterBase): - """Provide descending natural (alphanumeric) sort using string inversion.""" - - @staticmethod - def _invert_string(s: str) -> str: - """Invert string for descending order (same as ReverseAdapter).""" - return "".join(chr(0x10FFFF - ord(c)) for c in s) - - def sort_key(self, obj: Item): # pragma: no cover - thin wrapper - """Return descending natural sort key for item. - - Gets the natural sort key, converts to string, then inverts character order. - """ - - # Get the natural sort key and convert to string - natural_key = _sort_key(self._base.evaluate(obj) or "", numeric=True) - key_str = str(natural_key) - - # Apply string inversion for proper descending order - return self._invert_string(key_str) diff --git a/picard/ui/itemviews/custom_columns/storage.py b/picard/ui/itemviews/custom_columns/storage.py index d60e34e6d2..39c76cfb14 100644 --- a/picard/ui/itemviews/custom_columns/storage.py +++ b/picard/ui/itemviews/custom_columns/storage.py @@ -263,9 +263,6 @@ def _apply_sorting_adapter(base_provider: ColumnValueProvider, sorting_adapter_n from picard.ui.itemviews.custom_columns.sorting_adapters import ( ArticleInsensitiveAdapter, CasefoldSortAdapter, - DescendingCasefoldSortAdapter, - DescendingNaturalSortAdapter, - DescendingNumericSortAdapter, LengthSortAdapter, NaturalSortAdapter, NullsFirstAdapter, @@ -277,11 +274,8 @@ def _apply_sorting_adapter(base_provider: ColumnValueProvider, sorting_adapter_n # Mapping of class names to actual classes adapter_classes = { 'CasefoldSortAdapter': CasefoldSortAdapter, - 'DescendingCasefoldSortAdapter': DescendingCasefoldSortAdapter, 'NumericSortAdapter': NumericSortAdapter, - 'DescendingNumericSortAdapter': DescendingNumericSortAdapter, 'NaturalSortAdapter': NaturalSortAdapter, - 'DescendingNaturalSortAdapter': DescendingNaturalSortAdapter, 'LengthSortAdapter': LengthSortAdapter, 'ArticleInsensitiveAdapter': ArticleInsensitiveAdapter, 'NullsLastAdapter': NullsLastAdapter, diff --git a/test/test_custom_columns_sorting_adapters.py b/test/test_custom_columns_sorting_adapters.py index f064a1bb79..b384c17415 100644 --- a/test/test_custom_columns_sorting_adapters.py +++ b/test/test_custom_columns_sorting_adapters.py @@ -37,9 +37,6 @@ CasefoldSortAdapter, CompositeSortAdapter, CustomColumn, - DescendingCasefoldSortAdapter, - DescendingNaturalSortAdapter, - DescendingNumericSortAdapter, LengthSortAdapter, NaturalSortAdapter, NullsFirstAdapter, @@ -94,18 +91,6 @@ def test_casefold_sort_adapter(values: list[str], expected: list[str]) -> None: assert result == expected -@pytest.mark.parametrize( - ("values", "expected"), - [ - (["b", "A", "c"], ["c", "b", "A"]), - (["X", "x", "Y", "y"], ["Y", "y", "X", "x"]), - ], -) -def test_descending_casefold_sort_adapter(values: list[str], expected: list[str]) -> None: - result = _sorted_values(DescendingCasefoldSortAdapter, values) - assert result == expected - - @pytest.mark.parametrize( ("values", "expected"), [ @@ -137,39 +122,6 @@ def adapter(base): assert result == expected -@pytest.mark.parametrize( - ("values", "expected"), - [ - (["10", "2", "3.5", "x", "-1"], ["10", "3.5", "2", "-1", "x"]), - ], -) -def test_descending_numeric_sort_adapter(values: list[str], expected: list[str]) -> None: - result = _sorted_values(DescendingNumericSortAdapter, values) - assert result == expected - - -@pytest.mark.parametrize( - ("values", "expected"), - [ - (['-2', '-10', '3', '0'], ['3', '0', '-2', '-10']), - (['1.2', '1.10', '-0.5', 'x'], ['1.2', '1.10', '-0.5', 'x']), - ], -) -def test_descending_numeric_sort_adapter_extended(values: list[str], expected: list[str]) -> None: - result = _sorted_values(DescendingNumericSortAdapter, values) - assert result == expected - - -def test_descending_numeric_sort_adapter_with_custom_parser() -> None: - def adapter(base): - return DescendingNumericSortAdapter(base, parser=_mmss_parser) - - values = ['3:30', '2:05', '1:00', 'x'] - expected = ['3:30', '2:05', '1:00', 'x'] - result = _sorted_values(adapter, values) - assert result == expected - - def test_length_sort_adapter() -> None: values = ["aaa", "b", "cccc", "dd"] expected = ["b", "dd", "aaa", "cccc"] @@ -295,21 +247,6 @@ def test_natural_sort_adapter_basic_functionality() -> None: assert result == expected -@pytest.mark.skipif(IS_LINUX, reason="Natural sorting behaviour is different on Linux") -@pytest.mark.skipif(IS_MACOS, reason="QCollator doesn't support sort keys for the C locale on macOS") -def test_descending_natural_sort_adapter_basic_functionality() -> None: - """Test that descending natural sorting produces reasonable results.""" - values = ["item1", "item10", "item2"] - result = _sorted_values(DescendingNaturalSortAdapter, values) - - # Should produce some reasonable descending order - # The exact order depends on implementation, but should be different from ascending - natural_result = _sorted_values(NaturalSortAdapter, values) - assert result != natural_result # Should be different from ascending - assert len(result) == len(values) # Should have all items - assert set(result) == set(values) # Should have same items - - @pytest.mark.skipif(IS_LINUX, reason="Natural sorting behaviour is different on Linux") @pytest.mark.skipif(IS_MACOS, reason="QCollator doesn't support sort keys for the C locale on macOS") def test_natural_sort_adapter_vs_regular_sorting() -> None: @@ -337,24 +274,6 @@ def test_natural_sort_adapter_empty_handling() -> None: assert len(result) == 3 -@pytest.mark.skipif(IS_LINUX, reason="Natural sorting behaviour is different on Linux") -@pytest.mark.skipif(IS_MACOS, reason="QCollator doesn't support sort keys for the C locale on macOS") -def test_descending_natural_sort_produces_different_order() -> None: - """Test that descending natural sort produces different order than ascending.""" - values = ["track1", "track10", "track2", "track20", "track3"] - - # Get ascending natural sort - ascending = _sorted_values(NaturalSortAdapter, values) - - # Get descending natural sort - descending = _sorted_values(DescendingNaturalSortAdapter, values) - - # Should be different orders (implementation may vary) - assert ascending != descending - assert len(ascending) == len(descending) - assert set(ascending) == set(descending) - - def test_natural_sort_adapter_unicode_handling() -> None: """Test natural sorting with unicode characters.""" values = ["ñ1", "n10", "ñ2", "n1"] diff --git a/test/test_sorting_adapters_key_stabilisation.py b/test/test_sorting_adapters_key_stabilisation.py index 46c86a96ed..e7fd85a320 100644 --- a/test/test_sorting_adapters_key_stabilisation.py +++ b/test/test_sorting_adapters_key_stabilisation.py @@ -53,7 +53,6 @@ from picard.ui.itemviews.custom_columns.sorting_adapters import ( CachedSortAdapter, CompositeSortAdapter, - DescendingNumericSortAdapter, NumericSortAdapter, ReverseAdapter, ) @@ -106,21 +105,6 @@ class Dummy(Item): assert sorted_values[:3] == ["-3.5", "2", "10"] -def test_desc_numeric_sort_adapter_mixed_values_no_typeerror_and_ordering() -> None: - values = ["10", "abc", "2", "", "7a", "-3.5"] - - class Dummy(Item): - pass - - adapters = [DescendingNumericSortAdapter(StubProvider(v)) for v in values] - keys = [a.sort_key(Dummy()) for a in adapters] - # Should not raise TypeError - sorted_pairs = sorted(zip(values, keys, strict=True), key=lambda p: p[1]) - sorted_values = [v for v, _ in sorted_pairs] - # Numbers first, descending - assert sorted_values[:3] == ["10", "2", "-3.5"] - - class WeirdKeyProvider(ColumnValueProvider): def __init__(self, value: str): self._value = value From b4f79c194eac60bcb6c60193c639f5f7b542e287 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Thu, 18 Sep 2025 13:52:37 +0200 Subject: [PATCH 2/7] Do not skip natural sort order tests on Linux and macOS --- test/test_custom_columns_sorting_adapters.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/test/test_custom_columns_sorting_adapters.py b/test/test_custom_columns_sorting_adapters.py index b384c17415..10b868defa 100644 --- a/test/test_custom_columns_sorting_adapters.py +++ b/test/test_custom_columns_sorting_adapters.py @@ -23,7 +23,8 @@ from collections.abc import Callable import dataclasses -from picard.const.sys import IS_LINUX, IS_MACOS +from picard.const.sys import IS_MACOS +from picard.i18n import setup_gettext import pytest @@ -87,6 +88,7 @@ def _sorted_values(adapter_factory: Callable[[object], object], values: list[str ], ) def test_casefold_sort_adapter(values: list[str], expected: list[str]) -> None: + setup_gettext(None, 'en') result = _sorted_values(CasefoldSortAdapter, values) assert result == expected @@ -130,6 +132,7 @@ def test_length_sort_adapter() -> None: def test_article_insensitive_adapter() -> None: + setup_gettext(None, 'en') values = ["The Beatles", "Beatles", "An Artist", "Artist"] expected = ["An Artist", "Artist", "Beatles", "The Beatles"] result = _sorted_values(ArticleInsensitiveAdapter, values) @@ -216,7 +219,6 @@ def factory(base): assert desc == ["c", "B", "a"] -@pytest.mark.skipif(IS_LINUX, reason="Natural sorting behaviour is different on Linux") @pytest.mark.skipif(IS_MACOS, reason="QCollator doesn't support sort keys for the C locale on macOS") @pytest.mark.parametrize( ("values", "expected"), @@ -225,7 +227,7 @@ def factory(base): (["item10", "item2", "item1"], ["item1", "item2", "item10"]), (["track1", "track10", "track2"], ["track1", "track2", "track10"]), # Mixed text and pure numbers - (["10", "2", "item1", "item10"], ["2", "10", "item1", "item10"]), + (["10", "2", "item3", "09 item", "item10"], ["2", "09 item", "10", "item3", "item10"]), # Same text, different numbers (["version 1.10", "version 1.2", "version 1.1"], ["version 1.1", "version 1.2", "version 1.10"]), # Pure text (fallback to regular sorting) @@ -233,24 +235,23 @@ def factory(base): ], ) def test_natural_sort_adapter(values: list[str], expected: list[str]) -> None: + setup_gettext(None, 'en') result = _sorted_values(NaturalSortAdapter, values) assert result == expected -@pytest.mark.skipif(IS_LINUX, reason="Natural sorting behaviour is different on Linux") -@pytest.mark.skipif(IS_MACOS, reason="QCollator doesn't support sort keys for the C locale on macOS") def test_natural_sort_adapter_basic_functionality() -> None: """Test that natural sorting works for basic cases.""" + setup_gettext(None, 'en') values = ["item1", "item10", "item2"] result = _sorted_values(NaturalSortAdapter, values) expected = ["item1", "item2", "item10"] assert result == expected -@pytest.mark.skipif(IS_LINUX, reason="Natural sorting behaviour is different on Linux") -@pytest.mark.skipif(IS_MACOS, reason="QCollator doesn't support sort keys for the C locale on macOS") def test_natural_sort_adapter_vs_regular_sorting() -> None: """Test that natural sorting differs from regular text sorting for numeric content.""" + setup_gettext(None, 'en') values = ["file1.txt", "file10.txt", "file2.txt", "file20.txt"] # Regular casefold sorting (lexicographic) From 75e3590fb09e3e3640af53cfc7d051576f33ef47 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Thu, 18 Sep 2025 15:19:01 +0200 Subject: [PATCH 3/7] Sort custom columns locale aware by default --- picard/ui/itemviews/custom_columns/__init__.py | 2 ++ picard/ui/itemviews/custom_columns/shared.py | 2 +- .../itemviews/custom_columns/sorting_adapters.py | 14 +++++++++++--- picard/ui/itemviews/custom_columns/storage.py | 2 ++ test/test_custom_columns_sorting_adapters.py | 2 +- 5 files changed, 17 insertions(+), 5 deletions(-) diff --git a/picard/ui/itemviews/custom_columns/__init__.py b/picard/ui/itemviews/custom_columns/__init__.py index 9b7d5d6a21..5ee7c3bbc5 100644 --- a/picard/ui/itemviews/custom_columns/__init__.py +++ b/picard/ui/itemviews/custom_columns/__init__.py @@ -42,6 +42,7 @@ CasefoldSortAdapter, CompositeSortAdapter, LengthSortAdapter, + LocaleAwareSortAdapter, NaturalSortAdapter, NullsFirstAdapter, NullsLastAdapter, @@ -66,6 +67,7 @@ "NumericSortAdapter", "NaturalSortAdapter", "LengthSortAdapter", + "LocaleAwareSortAdapter", "ArticleInsensitiveAdapter", "CompositeSortAdapter", "NullsLastAdapter", diff --git a/picard/ui/itemviews/custom_columns/shared.py b/picard/ui/itemviews/custom_columns/shared.py index fea5b35659..e394a12d4a 100644 --- a/picard/ui/itemviews/custom_columns/shared.py +++ b/picard/ui/itemviews/custom_columns/shared.py @@ -296,7 +296,7 @@ def generate_new_key() -> str: # Mapping of user-friendly names to sorting adapter class names SORTING_ADAPTER_NAMES: dict[str, str] = { - N_("Default"): "", # No adapter (use default sorting) + N_("Default"): "LocaleAwareSortAdapter", N_("Case Insensitive"): "CasefoldSortAdapter", N_("Numeric"): "NumericSortAdapter", N_("Natural"): "NaturalSortAdapter", diff --git a/picard/ui/itemviews/custom_columns/sorting_adapters.py b/picard/ui/itemviews/custom_columns/sorting_adapters.py index 4419105195..03816c2930 100644 --- a/picard/ui/itemviews/custom_columns/sorting_adapters.py +++ b/picard/ui/itemviews/custom_columns/sorting_adapters.py @@ -66,12 +66,20 @@ def __repr__(self) -> str: # pragma: no cover - debug helper return f"{self.__class__.__name__}(base={self._base!r})" +class LocaleAwareSortAdapter(_AdapterBase): + """Provide case-insensitive sort using `str.casefold()` on evaluated value.""" + + def sort_key(self, obj: Item): # pragma: no cover - thin wrapper + """Return case-insensitive sort key for item.""" + return _sort_key(self._base.evaluate(obj) or "") + + class CasefoldSortAdapter(_AdapterBase): """Provide case-insensitive sort using `str.casefold()` on evaluated value.""" def sort_key(self, obj: Item): # pragma: no cover - thin wrapper """Return case-insensitive sort key for item.""" - return (self._base.evaluate(obj) or "").casefold() + return _sort_key((self._base.evaluate(obj) or "").casefold()) class NumericSortAdapter(_AdapterBase): @@ -139,8 +147,8 @@ def sort_key(self, obj: Item): # pragma: no cover - thin wrapper for art in self._articles: prefix = f"{art} " if lv.startswith(prefix): - return (lv[len(prefix) :], lv) - return (lv, lv) + return (_sort_key(lv[len(prefix) :]), _sort_key(prefix)) + return (_sort_key(lv), _sort_key("")) class CompositeSortAdapter(_AdapterBase): diff --git a/picard/ui/itemviews/custom_columns/storage.py b/picard/ui/itemviews/custom_columns/storage.py index 39c76cfb14..ef2ca93f87 100644 --- a/picard/ui/itemviews/custom_columns/storage.py +++ b/picard/ui/itemviews/custom_columns/storage.py @@ -264,6 +264,7 @@ def _apply_sorting_adapter(base_provider: ColumnValueProvider, sorting_adapter_n ArticleInsensitiveAdapter, CasefoldSortAdapter, LengthSortAdapter, + LocaleAwareSortAdapter, NaturalSortAdapter, NullsFirstAdapter, NullsLastAdapter, @@ -277,6 +278,7 @@ def _apply_sorting_adapter(base_provider: ColumnValueProvider, sorting_adapter_n 'NumericSortAdapter': NumericSortAdapter, 'NaturalSortAdapter': NaturalSortAdapter, 'LengthSortAdapter': LengthSortAdapter, + 'LocaleAwareSortAdapter': LocaleAwareSortAdapter, 'ArticleInsensitiveAdapter': ArticleInsensitiveAdapter, 'NullsLastAdapter': NullsLastAdapter, 'NullsFirstAdapter': NullsFirstAdapter, diff --git a/test/test_custom_columns_sorting_adapters.py b/test/test_custom_columns_sorting_adapters.py index 10b868defa..c52ef36769 100644 --- a/test/test_custom_columns_sorting_adapters.py +++ b/test/test_custom_columns_sorting_adapters.py @@ -134,7 +134,7 @@ def test_length_sort_adapter() -> None: def test_article_insensitive_adapter() -> None: setup_gettext(None, 'en') values = ["The Beatles", "Beatles", "An Artist", "Artist"] - expected = ["An Artist", "Artist", "Beatles", "The Beatles"] + expected = ["An Artist", "Artist", "The Beatles", "Beatles"] result = _sorted_values(ArticleInsensitiveAdapter, values) assert result == expected From adb71b4ca53822c95ea72e6b55bad9c8567d92d1 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Thu, 18 Sep 2025 15:22:28 +0200 Subject: [PATCH 4/7] Remove unused sorting ReverseAdapater This is unused and doesn't work with localized sorting anyway --- .../ui/itemviews/custom_columns/__init__.py | 2 - .../custom_columns/sorting_adapters.py | 31 ---------- picard/ui/itemviews/custom_columns/storage.py | 2 - test/test_custom_columns_sorting_adapters.py | 21 +------ ...test_sorting_adapters_key_stabilisation.py | 57 ------------------- 5 files changed, 2 insertions(+), 111 deletions(-) diff --git a/picard/ui/itemviews/custom_columns/__init__.py b/picard/ui/itemviews/custom_columns/__init__.py index 5ee7c3bbc5..671c220433 100644 --- a/picard/ui/itemviews/custom_columns/__init__.py +++ b/picard/ui/itemviews/custom_columns/__init__.py @@ -47,7 +47,6 @@ NullsFirstAdapter, NullsLastAdapter, NumericSortAdapter, - ReverseAdapter, ) @@ -73,5 +72,4 @@ "NullsLastAdapter", "NullsFirstAdapter", "CachedSortAdapter", - "ReverseAdapter", ] diff --git a/picard/ui/itemviews/custom_columns/sorting_adapters.py b/picard/ui/itemviews/custom_columns/sorting_adapters.py index 03816c2930..767881c5f9 100644 --- a/picard/ui/itemviews/custom_columns/sorting_adapters.py +++ b/picard/ui/itemviews/custom_columns/sorting_adapters.py @@ -272,37 +272,6 @@ def sort_key(self, obj: Item): # pragma: no cover - thin wrapper return key -class ReverseAdapter(_AdapterBase): - """Provide reversed sorting for string or numeric sort keys.""" - - @staticmethod - def _invert_string(s: str) -> str: - return "".join(chr(0x10FFFF - ord(c)) for c in s) - - def sort_key(self, obj: Item): # pragma: no cover - thin wrapper - """Return reversed sort key for item. - - Always return a normalized tuple key to avoid cross-type comparisons: - - Numbers -> (0, -float(value)) - - Strings -> (1, inverted string) - - Tuples -> keep as-is (assumed already normalized) - - Other -> (1, lowercased string) - """ - if isinstance(self._base, SortKeyProvider): - base_key = self._base.sort_key(obj) - else: - base_key = self._base.evaluate(obj) or "" - - if isinstance(base_key, tuple): - return base_key - if isinstance(base_key, (int, float)): - return (0, -float(base_key)) - if isinstance(base_key, str): - return (1, self._invert_string(base_key)) - text = "" if base_key is None else str(base_key) - return (1, text.casefold()) - - class NaturalSortAdapter(_AdapterBase): """Provide natural (alphanumeric) sort using locale-aware natural ordering.""" diff --git a/picard/ui/itemviews/custom_columns/storage.py b/picard/ui/itemviews/custom_columns/storage.py index ef2ca93f87..68fee5056d 100644 --- a/picard/ui/itemviews/custom_columns/storage.py +++ b/picard/ui/itemviews/custom_columns/storage.py @@ -269,7 +269,6 @@ def _apply_sorting_adapter(base_provider: ColumnValueProvider, sorting_adapter_n NullsFirstAdapter, NullsLastAdapter, NumericSortAdapter, - ReverseAdapter, ) # Mapping of class names to actual classes @@ -282,7 +281,6 @@ def _apply_sorting_adapter(base_provider: ColumnValueProvider, sorting_adapter_n 'ArticleInsensitiveAdapter': ArticleInsensitiveAdapter, 'NullsLastAdapter': NullsLastAdapter, 'NullsFirstAdapter': NullsFirstAdapter, - 'ReverseAdapter': ReverseAdapter, } adapter_class = adapter_classes.get(sorting_adapter_name) diff --git a/test/test_custom_columns_sorting_adapters.py b/test/test_custom_columns_sorting_adapters.py index c52ef36769..2ffb783e1f 100644 --- a/test/test_custom_columns_sorting_adapters.py +++ b/test/test_custom_columns_sorting_adapters.py @@ -23,7 +23,7 @@ from collections.abc import Callable import dataclasses -from picard.const.sys import IS_MACOS +from picard.const.sys import IS_WIN from picard.i18n import setup_gettext import pytest @@ -43,7 +43,6 @@ NullsFirstAdapter, NullsLastAdapter, NumericSortAdapter, - ReverseAdapter, make_callable_column, ) @@ -131,6 +130,7 @@ def test_length_sort_adapter() -> None: assert result == expected +@pytest.mark.skipif(IS_WIN, reason="QCollator not used on Windows") def test_article_insensitive_adapter() -> None: setup_gettext(None, 'en') values = ["The Beatles", "Beatles", "An Artist", "Artist"] @@ -203,23 +203,6 @@ def factory(base): assert all(count >= 1 for count in calls.values()) -def test_reverse_adapter() -> None: - values = ["a", "B", "c"] - asc = _sorted_values(CasefoldSortAdapter, values) - - def factory(base): - # Reverse the underlying casefold sort; ties resolved by original case - return ReverseAdapter(CasefoldSortAdapter(base)) - - desc = _sorted_values(factory, values) - assert asc == ["a", "B", "c"] - # Reverse order of case-insensitive ascending becomes descending - # Case-insensitive compare: 'a' == 'A', but we reverse based on key, - # so 'B' (from 'B') will come before 'a' - assert desc == ["c", "B", "a"] - - -@pytest.mark.skipif(IS_MACOS, reason="QCollator doesn't support sort keys for the C locale on macOS") @pytest.mark.parametrize( ("values", "expected"), [ diff --git a/test/test_sorting_adapters_key_stabilisation.py b/test/test_sorting_adapters_key_stabilisation.py index e7fd85a320..42ff1bd165 100644 --- a/test/test_sorting_adapters_key_stabilisation.py +++ b/test/test_sorting_adapters_key_stabilisation.py @@ -36,9 +36,6 @@ across items. - CachedSortAdapter normalizes keys when a custom ``key_func`` returns mixed types, ensuring stable ordering and no cross-type errors. - - ReverseAdapter normalizes and inverts keys deterministically; it also - preserves tuple keys (e.g. when wrapping NumericSortAdapter) to avoid - introducing new mixed-type comparisons or semantic changes. These tests are focused on the normalization behavior introduced to fix the crash and complement (not duplicate) the broader behavioral tests in @@ -54,7 +51,6 @@ CachedSortAdapter, CompositeSortAdapter, NumericSortAdapter, - ReverseAdapter, ) @@ -105,17 +101,6 @@ class Dummy(Item): assert sorted_values[:3] == ["-3.5", "2", "10"] -class WeirdKeyProvider(ColumnValueProvider): - def __init__(self, value: str): - self._value = value - - def evaluate(self, _obj) -> str: - return self._value - - # Simulate an inconsistent sort_key source for ReverseAdapter tests by - # attaching a dynamic method later when needed - - def test_composite_sort_adapter_mixed_component_types_no_typeerror() -> None: class Dummy(Item): def __init__(self, s: str): @@ -160,45 +145,3 @@ class Dummy(Item): sorted_values = [v for v, _ in sorted_pairs] # Numbers first (ascending), then strings assert sorted_values == ["2", "10", "abc"] - - -def test_reverse_adapter_normalizes_mixed_types_from_custom_sort_key() -> None: - # Provide a SortKeyProvider-like object with mixed numeric / string outputs - class MixedSortKeyProvider(WeirdKeyProvider): - def sort_key(self, _obj): # type: ignore[override] - s = self._value - try: - return float(s) - except Exception: - return s - - providers = [MixedSortKeyProvider(v) for v in ["10", "abc", "2"]] - adaptered = [ReverseAdapter(p) for p in providers] - - class Dummy(Item): - pass - - keys = [a.sort_key(Dummy()) for a in adaptered] - # Should not raise TypeError when sorting - sorted_pairs = sorted(zip(["10", "abc", "2"], keys, strict=True), key=lambda p: p[1]) - sorted_values = [v for v, _ in sorted_pairs] - # Numbers first in descending due to reverse: 10, 2, then strings - assert sorted_values == ["10", "2", "abc"] - - -def test_reverse_adapter_over_numeric_adapter_leaves_order_intact() -> None: - values = ["10", "abc", "2"] - - class Dummy(Item): - pass - - base_adapters = [NumericSortAdapter(StubProvider(v)) for v in values] - base_keys = [a.sort_key(Dummy()) for a in base_adapters] - base_sorted = [v for v, _ in sorted(zip(values, base_keys, strict=True), key=lambda p: p[1])] - - rev_adapters = [ReverseAdapter(a) for a in base_adapters] - rev_keys = [a.sort_key(Dummy()) for a in rev_adapters] - rev_sorted = [v for v, _ in sorted(zip(values, rev_keys, strict=True), key=lambda p: p[1])] - - # ReverseAdapter keeps tuple keys from NumericSortAdapter as-is; ordering remains identical - assert rev_sorted == base_sorted From b31023c42b79e0b61ce20cf1ff9707ab1b3ae04f Mon Sep 17 00:00:00 2001 From: Khoa Nguyen Date: Fri, 19 Sep 2025 10:13:19 +0200 Subject: [PATCH 5/7] Generic implementation of _apply_sorting_adapter --- picard/ui/itemviews/custom_columns/storage.py | 43 +++++++++---------- 1 file changed, 20 insertions(+), 23 deletions(-) diff --git a/picard/ui/itemviews/custom_columns/storage.py b/picard/ui/itemviews/custom_columns/storage.py index 68fee5056d..8c1117eefb 100644 --- a/picard/ui/itemviews/custom_columns/storage.py +++ b/picard/ui/itemviews/custom_columns/storage.py @@ -259,29 +259,26 @@ def _apply_sorting_adapter(base_provider: ColumnValueProvider, sorting_adapter_n if not sorting_adapter_name: return base_provider - # Import adapters here to avoid circular imports - from picard.ui.itemviews.custom_columns.sorting_adapters import ( - ArticleInsensitiveAdapter, - CasefoldSortAdapter, - LengthSortAdapter, - LocaleAwareSortAdapter, - NaturalSortAdapter, - NullsFirstAdapter, - NullsLastAdapter, - NumericSortAdapter, - ) - - # Mapping of class names to actual classes - adapter_classes = { - 'CasefoldSortAdapter': CasefoldSortAdapter, - 'NumericSortAdapter': NumericSortAdapter, - 'NaturalSortAdapter': NaturalSortAdapter, - 'LengthSortAdapter': LengthSortAdapter, - 'LocaleAwareSortAdapter': LocaleAwareSortAdapter, - 'ArticleInsensitiveAdapter': ArticleInsensitiveAdapter, - 'NullsLastAdapter': NullsLastAdapter, - 'NullsFirstAdapter': NullsFirstAdapter, - } + # Import the shared mapping to get all available class names + from picard.ui.itemviews.custom_columns import sorting_adapters + from picard.ui.itemviews.custom_columns.shared import SORTING_ADAPTER_NAMES + + # Build mapping dynamically from available class names in SORTING_ADAPTER_NAMES + # This ensures we only try to map classes that are actually defined in the UI + available_class_names = {name for name in SORTING_ADAPTER_NAMES.values() if name} + + # Create a mapping of class name strings to actual classes + # Only include classes that are referenced in SORTING_ADAPTER_NAMES + adapter_classes = {} + for class_name in available_class_names: + # Dynamically import and get the class from the sorting_adapters module + try: + adapter_class = getattr(sorting_adapters, class_name, None) + if adapter_class is not None: + adapter_classes[class_name] = adapter_class + except (ImportError, AttributeError): + # Skip classes that don't exist or can't be imported + continue adapter_class = adapter_classes.get(sorting_adapter_name) if adapter_class is None: From 1a922c51d98b2d80cb3e6341487a57004c63768c Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Fri, 19 Sep 2025 10:28:38 +0200 Subject: [PATCH 6/7] Locale aware sorting also for NullsLastAdapter and NullsFirstAdapter --- picard/ui/itemviews/custom_columns/sorting_adapters.py | 4 ++-- test/test_custom_columns_sorting_adapters.py | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/picard/ui/itemviews/custom_columns/sorting_adapters.py b/picard/ui/itemviews/custom_columns/sorting_adapters.py index 767881c5f9..5b6375d3ae 100644 --- a/picard/ui/itemviews/custom_columns/sorting_adapters.py +++ b/picard/ui/itemviews/custom_columns/sorting_adapters.py @@ -201,7 +201,7 @@ def sort_key(self, obj: Item): # pragma: no cover - thin wrapper cleaned = _clean_invisible_and_whitespace(v) is_empty = cleaned == "" key = cleaned.casefold() - return (is_empty, key) + return (is_empty, _sort_key(key)) class NullsFirstAdapter(_AdapterBase): @@ -213,7 +213,7 @@ def sort_key(self, obj: Item): # pragma: no cover - thin wrapper cleaned = _clean_invisible_and_whitespace(v) is_empty = cleaned == "" key = cleaned.casefold() - return (not is_empty, key) + return (not is_empty, _sort_key(key)) class CachedSortAdapter(_AdapterBase): diff --git a/test/test_custom_columns_sorting_adapters.py b/test/test_custom_columns_sorting_adapters.py index 2ffb783e1f..c9a30799ee 100644 --- a/test/test_custom_columns_sorting_adapters.py +++ b/test/test_custom_columns_sorting_adapters.py @@ -161,6 +161,7 @@ def factory(base): ], ) def test_nulls_last_adapter(values: list[str], expected: list[str]) -> None: + setup_gettext(None, 'en') result = _sorted_values(NullsLastAdapter, values) assert result == expected @@ -176,6 +177,7 @@ def test_nulls_last_adapter(values: list[str], expected: list[str]) -> None: ], ) def test_nulls_first_adapter(values: list[str], expected: list[str]) -> None: + setup_gettext(None, 'en') result = _sorted_values(NullsFirstAdapter, values) assert result == expected From f6d5fb150c523b5c70843a26235b58bdbc5db9fc Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Fri, 19 Sep 2025 12:24:05 +0200 Subject: [PATCH 7/7] Add tooltips for sorting adapters --- .../custom_columns/manager_dialog.py | 5 +- picard/ui/itemviews/custom_columns/shared.py | 60 ++++++++++++++----- picard/ui/itemviews/custom_columns/storage.py | 2 +- 3 files changed, 49 insertions(+), 18 deletions(-) diff --git a/picard/ui/itemviews/custom_columns/manager_dialog.py b/picard/ui/itemviews/custom_columns/manager_dialog.py index 1f229317a0..2c87040be3 100644 --- a/picard/ui/itemviews/custom_columns/manager_dialog.py +++ b/picard/ui/itemviews/custom_columns/manager_dialog.py @@ -154,8 +154,9 @@ def __init__(self, parent: QtWidgets.QWidget | None = None) -> None: # Sorting adapter dropdown self._sorting_adapter = QtWidgets.QComboBox(self._editor_panel) - for display_name, class_name in get_sorting_adapter_options(): - self._sorting_adapter.addItem(_(display_name), class_name) + for i, (class_name, display_info) in enumerate(get_sorting_adapter_options()): + self._sorting_adapter.addItem(_(display_info.display_name), class_name) + self._sorting_adapter.setItemData(i, _(display_info.tooltip), QtCore.Qt.ItemDataRole.ToolTipRole) self._sorting_adapter.setMaximumWidth(200) self._view_selector = ViewSelector(self._editor_panel) diff --git a/picard/ui/itemviews/custom_columns/shared.py b/picard/ui/itemviews/custom_columns/shared.py index e394a12d4a..3ad6bcb37a 100644 --- a/picard/ui/itemviews/custom_columns/shared.py +++ b/picard/ui/itemviews/custom_columns/shared.py @@ -294,16 +294,46 @@ def generate_new_key() -> str: return str(uuid.uuid4()) +@dataclass(frozen=True) +class SortingAdapterDisplayInfo: + display_name: str + tooltip: str + + # Mapping of user-friendly names to sorting adapter class names -SORTING_ADAPTER_NAMES: dict[str, str] = { - N_("Default"): "LocaleAwareSortAdapter", - N_("Case Insensitive"): "CasefoldSortAdapter", - N_("Numeric"): "NumericSortAdapter", - N_("Natural"): "NaturalSortAdapter", - N_("By Value Length"): "LengthSortAdapter", - N_("Article Insensitive"): "ArticleInsensitiveAdapter", - N_("Empty Values Last"): "NullsLastAdapter", - N_("Empty Values First"): "NullsFirstAdapter", +SORTING_ADAPTER_NAMES: dict[str, SortingAdapterDisplayInfo] = { + "LocaleAwareSortAdapter": SortingAdapterDisplayInfo( + display_name=N_("Default"), + tooltip=N_("Default alphabetical text sorting."), + ), + "CasefoldSortAdapter": SortingAdapterDisplayInfo( + display_name=N_("Case insensitive"), + tooltip=N_("Case insensitive alphabetical text sorting."), + ), + "NumericSortAdapter": SortingAdapterDisplayInfo( + display_name=N_("Numeric"), + tooltip=N_("Numeric sorting if the column content is numeric. Falls back to natural number sorting."), + ), + "NaturalSortAdapter": SortingAdapterDisplayInfo( + display_name=N_("Natural number sorting"), + tooltip=N_('Alphabetical text sorting, but consider numbers (e.g. "Track 2" before "Track 10").'), + ), + "LengthSortAdapter": SortingAdapterDisplayInfo( + display_name=N_("By value length"), + tooltip=N_("Sort by string length."), + ), + "ArticleInsensitiveAdapter": SortingAdapterDisplayInfo( + display_name=N_("Article insensitive"), + tooltip=N_("Sort ignoring leading articles (e.g. a, an, the)."), + ), + "NullsLastAdapter": SortingAdapterDisplayInfo( + display_name=N_("Empty values first"), + tooltip=N_("Empty/whitespace values sort first."), + ), + "NullsFirstAdapter": SortingAdapterDisplayInfo( + display_name=N_("Empty values last"), + tooltip=N_("Empty/whitespace values sort last."), + ), } @@ -312,18 +342,18 @@ def get_sorting_adapter_options() -> tuple[tuple[str, str], ...]: Returns ------- - tuple[tuple[str, str], ...] - Sorted pairs of user-friendly display names and corresponding adapter class names. + tuple[tuple[str, SortingAdapterDisplayInfo], ...] + Sorted pairs of adapter class names and corresponding display info. The "Default" option always appears first. """ # Get the Default item directly - default_name = N_("Default") - default_item = (default_name, SORTING_ADAPTER_NAMES[default_name]) + default_class = "LocaleAwareSortAdapter" + default_item = (default_class, SORTING_ADAPTER_NAMES[default_class]) # Get other items (excluding Default) and sort them other_items = sorted( - [(name, class_name) for name, class_name in SORTING_ADAPTER_NAMES.items() if name != default_name], - key=lambda x: x[0], + [(class_name, info) for class_name, info in SORTING_ADAPTER_NAMES.items() if class_name != default_class], + key=lambda x: _(x[1].display_name), ) # Return tuple with Default first, then sorted others diff --git a/picard/ui/itemviews/custom_columns/storage.py b/picard/ui/itemviews/custom_columns/storage.py index 8c1117eefb..7b1328ef41 100644 --- a/picard/ui/itemviews/custom_columns/storage.py +++ b/picard/ui/itemviews/custom_columns/storage.py @@ -265,7 +265,7 @@ def _apply_sorting_adapter(base_provider: ColumnValueProvider, sorting_adapter_n # Build mapping dynamically from available class names in SORTING_ADAPTER_NAMES # This ensures we only try to map classes that are actually defined in the UI - available_class_names = {name for name in SORTING_ADAPTER_NAMES.values() if name} + available_class_names = {name for name in SORTING_ADAPTER_NAMES.keys() if name} # Create a mapping of class name strings to actual classes # Only include classes that are referenced in SORTING_ADAPTER_NAMES