From 2e0eefa24e554ec3010dda4ab31037460e2a0e98 Mon Sep 17 00:00:00 2001 From: Khoa Nguyen Date: Mon, 22 Sep 2025 10:00:43 -0400 Subject: [PATCH 1/5] refactor: standard columns to use new API - Refactor standard columns to use new provider architecture - Remove delegate adapter wrapper for cleaner implementation - Fix size, bitrate, length columns to be field columns - General cleanup and code improvements - Update tests to reflect new column architecture --- picard/ui/itemviews/__init__.py | 13 +- picard/ui/itemviews/basetreeview.py | 13 +- picard/ui/itemviews/columns.py | 144 +---- .../ui/itemviews/custom_columns/__init__.py | 12 +- picard/ui/itemviews/custom_columns/column.py | 151 +++++- .../custom_columns/common_columns.py | 183 +++++++ picard/ui/itemviews/custom_columns/factory.py | 118 +++- .../ui/itemviews/custom_columns/protocols.py | 30 + .../ui/itemviews/custom_columns/providers.py | 24 +- picard/ui/itemviews/custom_columns/utils.py | 120 ++++ picard/ui/itemviews/match_quality_column.py | 224 ++++++-- test/test_album_loading_sorting.py | 92 ---- test/test_itemviews_columns_split.py | 8 +- test/test_itemviews_match_quality.py | 513 ++++++++++-------- test/test_ui_columns.py | 15 +- 15 files changed, 1124 insertions(+), 536 deletions(-) create mode 100644 picard/ui/itemviews/custom_columns/common_columns.py create mode 100644 picard/ui/itemviews/custom_columns/utils.py delete mode 100644 test/test_album_loading_sorting.py diff --git a/picard/ui/itemviews/__init__.py b/picard/ui/itemviews/__init__.py index abf1d38266..12365f7cf1 100644 --- a/picard/ui/itemviews/__init__.py +++ b/picard/ui/itemviews/__init__.py @@ -73,14 +73,14 @@ from picard.ui.columns import ( ColumnAlign, ColumnSortType, + ImageColumn, ) from picard.ui.itemviews.basetreeview import BaseTreeView from picard.ui.itemviews.columns import ( ALBUMVIEW_COLUMNS, FILEVIEW_COLUMNS, - IconColumn, ) -from picard.ui.itemviews.match_quality_column import MatchQualityColumn +from picard.ui.itemviews.custom_columns import DelegateColumn def get_match_color(similarity, basecolor): @@ -408,11 +408,12 @@ def update_colums_text(self, color=None, bgcolor=None): self.setForeground(i, color) if bgcolor is not None: self.setBackground(i, bgcolor) - if isinstance(column, IconColumn): - self.setSizeHint(i, column.size) - elif isinstance(column, MatchQualityColumn): - # Progress columns are handled by delegate, just set size hint + if isinstance(column, ImageColumn): self.setSizeHint(i, column.size) + elif isinstance(column, DelegateColumn): + # Delegate columns are handled by delegate, just set size hint + if hasattr(column, 'size'): + self.setSizeHint(i, column.size) else: if column.align == ColumnAlign.RIGHT: self.setTextAlignment(i, QtCore.Qt.AlignmentFlag.AlignRight | QtCore.Qt.AlignmentFlag.AlignVCenter) diff --git a/picard/ui/itemviews/basetreeview.py b/picard/ui/itemviews/basetreeview.py index 7e0f1eb035..c493c0006d 100644 --- a/picard/ui/itemviews/basetreeview.py +++ b/picard/ui/itemviews/basetreeview.py @@ -91,8 +91,8 @@ from picard.ui.collectionmenu import CollectionMenu from picard.ui.enums import MainAction from picard.ui.filter import Filter +from picard.ui.itemviews.custom_columns import DelegateColumn from picard.ui.itemviews.events import header_events -from picard.ui.itemviews.match_quality_column import MatchQualityColumn, MatchQualityColumnDelegate from picard.ui.ratingwidget import RatingWidget from picard.ui.scriptsmenu import ScriptsMenu from picard.ui.util import menu_builder @@ -481,11 +481,14 @@ def _init_header(self): header = ConfigurableColumnsHeader(self.columns, parent=self) self.setHeader(header) - # Set up delegate for progress columns - progress_delegate = MatchQualityColumnDelegate(self) + # Set up delegates for delegate columns + delegate_instances = {} for i, column in enumerate(self.columns): - if isinstance(column, MatchQualityColumn): - self.setItemDelegateForColumn(i, progress_delegate) + if isinstance(column, DelegateColumn): + delegate_class = column.delegate_class + if delegate_class not in delegate_instances: + delegate_instances[delegate_class] = delegate_class(self) + self.setItemDelegateForColumn(i, delegate_instances[delegate_class]) self.restore_default_columns() self.restore_state() diff --git a/picard/ui/itemviews/columns.py b/picard/ui/itemviews/columns.py index 3383cf9a5d..987ca26ab3 100644 --- a/picard/ui/itemviews/columns.py +++ b/picard/ui/itemviews/columns.py @@ -43,151 +43,18 @@ # along with this program; if not, write to the Free Software # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -from PyQt6 import QtCore - -from picard.album import AlbumStatus -from picard.const.sys import IS_LINUX -from picard.i18n import N_ -from picard.util import icontheme from picard.ui.columns import ( - Column, - ColumnAlign, Columns, - ColumnSortType, - DefaultColumn, - ImageColumn, ) -from picard.ui.itemviews.match_quality_column import MatchQualityColumn - - -def _sortkey_length(obj): - return obj.metadata.length or 0 - - -def _sortkey_filesize(obj): - try: - return int(obj.metadata['~filesize'] or obj.orig_metadata['~filesize']) - except ValueError: - return 0 - - -def _sortkey_bitrate(obj): - try: - return float(obj.metadata['~bitrate'] or obj.orig_metadata['~bitrate'] or 0) - except (ValueError, TypeError): - return 0 - - -def _sortkey_match_quality(obj): - """Sort key for match quality column - sort by completion percentage.""" - if hasattr(obj, 'get_num_matched_tracks') and hasattr(obj, 'tracks'): - # Album object - # Check if album is still loading - if so, return 0 to avoid premature sorting - if hasattr(obj, 'status') and obj.status == AlbumStatus.LOADING: - return 0.0 - - total = len(obj.tracks) if obj.tracks else 0 - if total > 0: - # Column sorting is reversed on Linux - multiplier = -1 if IS_LINUX else 1 - matched = obj.get_num_matched_tracks() - return matched / total * multiplier - return 0.0 - # For track objects, return 0 since we don't show icons at track level - return 0.0 - - -class IconColumn(ImageColumn): - _header_icon = None - header_icon_func = None - header_icon_size = QtCore.QSize(0, 0) - header_icon_border = 0 - - @property - def header_icon(self): - # icon cannot be set before QApplication is created - # so create it during runtime and cache it - # Avoid error: QPixmap: Must construct a QGuiApplication before a QPixmap - if self._header_icon is None: - self._header_icon = self.header_icon_func() - return self._header_icon - - def set_header_icon_size(self, width, height, border): - self.header_icon_size = QtCore.QSize(width, height) - self.header_icon_border = border - self.size = QtCore.QSize(width + 2 * border, height + 2 * border) - self.width = self.size.width() - - def paint(self, painter, rect): - icon = self.header_icon - if not icon: - return - h = self.header_icon_size.height() - w = self.header_icon_size.width() - border = self.header_icon_border - padding_v = (rect.height() - h) // 2 - target_rect = QtCore.QRect(rect.x() + border, rect.y() + padding_v, w, h) - painter.drawPixmap(target_rect, icon.pixmap(self.header_icon_size)) - - -_fingerprint_column = IconColumn(N_("Fingerprint status"), '~fingerprint') -_fingerprint_column.header_icon_func = lambda: icontheme.lookup('fingerprint-gray', icontheme.ICON_SIZE_MENU) -_fingerprint_column.set_header_icon_size(16, 16, 1) - -_match_quality_column = MatchQualityColumn(N_("Match"), '~match_quality', width=57) -_match_quality_column.sortable = True -_match_quality_column.sort_type = ColumnSortType.SORTKEY -_match_quality_column.sortkey = _sortkey_match_quality -_match_quality_column.is_default = True +from picard.ui.itemviews.custom_columns.common_columns import ( + create_common_columns, + create_match_quality_column, +) # Common columns used by both views -_common_columns = ( - DefaultColumn(N_("Title"), 'title', sort_type=ColumnSortType.NAT, width=250, always_visible=True, status_icon=True), - DefaultColumn( - N_("Length"), - '~length', - align=ColumnAlign.RIGHT, - sort_type=ColumnSortType.SORTKEY, - sortkey=_sortkey_length, - width=50, - ), - DefaultColumn(N_("Artist"), 'artist', width=200), - Column(N_("Album Artist"), 'albumartist'), - Column(N_("Composer"), 'composer'), - Column(N_("Album"), 'album', sort_type=ColumnSortType.NAT), - Column(N_("Disc Subtitle"), 'discsubtitle', sort_type=ColumnSortType.NAT), - Column(N_("Track No."), 'tracknumber', align=ColumnAlign.RIGHT, sort_type=ColumnSortType.NAT), - Column(N_("Disc No."), 'discnumber', align=ColumnAlign.RIGHT, sort_type=ColumnSortType.NAT), - Column(N_("Catalog No."), 'catalognumber', sort_type=ColumnSortType.NAT), - Column(N_("Barcode"), 'barcode'), - Column(N_("Media"), 'media'), - Column( - N_("Size"), - '~filesize', - align=ColumnAlign.RIGHT, - sort_type=ColumnSortType.SORTKEY, - sortkey=_sortkey_filesize, - ), - Column(N_("File Type"), '~format', width=120), - Column( - N_("Bitrate"), - '~bitrate', - align=ColumnAlign.RIGHT, - sort_type=ColumnSortType.SORTKEY, - sortkey=_sortkey_bitrate, - width=80, - ), - Column(N_("Genre"), 'genre'), - _fingerprint_column, - Column(N_("Date"), 'date'), - Column(N_("Original Release Date"), 'originaldate'), - Column(N_("Release Date"), 'releasedate'), - Column(N_("Cover"), 'covercount'), - Column(N_("Cover Dimensions"), 'coverdimensions'), -) - +_common_columns = create_common_columns() # File view columns (without match quality column) FILEVIEW_COLUMNS = Columns(_common_columns, default_width=100) @@ -195,4 +62,5 @@ def paint(self, painter, rect): # Album view columns (with match quality column) # Insert `_match_quality_column` after Title, Length, Artist, Album Artist ALBUMVIEW_COLUMNS = Columns(_common_columns, default_width=100) +_match_quality_column = create_match_quality_column() ALBUMVIEW_COLUMNS.insert(ALBUMVIEW_COLUMNS.pos('albumartist') + 1, _match_quality_column) diff --git a/picard/ui/itemviews/custom_columns/__init__.py b/picard/ui/itemviews/custom_columns/__init__.py index 671c220433..cc54dfa665 100644 --- a/picard/ui/itemviews/custom_columns/__init__.py +++ b/picard/ui/itemviews/custom_columns/__init__.py @@ -22,17 +22,21 @@ from __future__ import annotations -from picard.ui.itemviews.custom_columns.column import CustomColumn +from picard.ui.itemviews.custom_columns.column import CustomColumn, DelegateColumn, IconColumn from picard.ui.itemviews.custom_columns.factory import ( _create_custom_column, make_callable_column, + make_delegate_column, make_field_column, + make_numeric_field_column, make_provider_column, make_script_column, make_transformed_column, ) from picard.ui.itemviews.custom_columns.protocols import ( ColumnValueProvider, + DelegateProvider, + HeaderIconProvider, SortKeyProvider, ) from picard.ui.itemviews.custom_columns.registry import registry @@ -52,13 +56,19 @@ __all__ = [ "ColumnValueProvider", + "DelegateProvider", "SortKeyProvider", + "HeaderIconProvider", "CustomColumn", + "DelegateColumn", + "IconColumn", "make_field_column", + "make_numeric_field_column", "make_provider_column", "make_script_column", "make_callable_column", "make_transformed_column", + "make_delegate_column", "registry", "_create_custom_column", # Sorting adapters diff --git a/picard/ui/itemviews/custom_columns/column.py b/picard/ui/itemviews/custom_columns/column.py index 68b08afad0..a4773546d4 100644 --- a/picard/ui/itemviews/custom_columns/column.py +++ b/picard/ui/itemviews/custom_columns/column.py @@ -22,14 +22,19 @@ from __future__ import annotations +from PyQt6 import QtCore, QtGui + from picard.ui.columns import ( Column, ColumnAlign, ColumnSortType, + ImageColumn, ) from picard.ui.itemviews.custom_columns.protocols import ( CacheInvalidatable, ColumnValueProvider, + DelegateProvider, + HeaderIconProvider, SortKeyProvider, ) @@ -46,6 +51,8 @@ def __init__( align: ColumnAlign = ColumnAlign.LEFT, sort_type: ColumnSortType = ColumnSortType.TEXT, always_visible: bool = False, + *, + status_icon: bool = False, ): """Create custom column. @@ -79,10 +86,9 @@ def __init__( sort_type=sort_type, sortkey=sortkey_fn, always_visible=always_visible, - status_icon=False, + status_icon=status_icon, ) self.provider = provider - def invalidate_cache(self, obj=None): # pragma: no cover - UI-driven """Invalidate any caches on the provider if supported. @@ -94,3 +100,144 @@ def invalidate_cache(self, obj=None): # pragma: no cover - UI-driven if isinstance(self.provider, CacheInvalidatable): # type: ignore[attr-defined] self.provider.invalidate(obj) # noqa: PGH003 + +class DelegateColumn(Column): + """A column that uses a delegate for custom rendering and optional sorting.""" + + def __init__( + self, + title: str, + key: str, + provider: DelegateProvider, + width: int | None = None, + align: ColumnAlign = ColumnAlign.LEFT, + sort_type: ColumnSortType = ColumnSortType.TEXT, + always_visible: bool = False, + size: QtCore.QSize | None = None, + *, + status_icon: bool = False, + sort_provider: SortKeyProvider | None = None, + ): + """Create delegate column. + + Parameters + ---------- + title + Display title of the column. + key + Internal key used to identify the column. + provider + Provide delegate class and data for rendering. + width + Optional fixed width in pixels. + align + Set text alignment. + sort_type + Set sorting behavior of the column. + always_visible + If True, hide the column visibility toggle. + size + Preferred delegate size. + """ + + sortkey_fn = None + if sort_type == ColumnSortType.SORTKEY: + if isinstance(sort_provider, SortKeyProvider): + sortkey_fn = sort_provider.sort_key # type: ignore[assignment] + elif isinstance(provider, SortKeyProvider): + # Fallback for backward compatibility if provider implements sorting + sortkey_fn = provider.sort_key # type: ignore[assignment] + + super().__init__( + title, + key, + width=width, + align=align, + sort_type=sort_type, + sortkey=sortkey_fn, + always_visible=always_visible, + status_icon=status_icon, + ) + self.delegate_provider = provider + self.size = size if size is not None else QtCore.QSize(16, 16) # Default icon size + + @property + def delegate_class(self): + """Get the delegate class for this column.""" + return self.delegate_provider.get_delegate_class() + + +class IconColumn(ImageColumn): + """A column that paints a header icon provided by a `HeaderIconProvider`. + + The header icon is constructed lazily via the provider to avoid creating + Qt objects before the application is initialized. + """ + + _header_icon: QtGui.QIcon | None = None + + def __init__(self, title: str, key: str, provider: HeaderIconProvider, *, width: int | None = None) -> None: + super().__init__( + title, + key, + width=width, + align=ColumnAlign.LEFT, + sort_type=ColumnSortType.TEXT, + sortkey=None, + always_visible=False, + status_icon=False, + ) + self._provider = provider + self.header_icon_size = QtCore.QSize(0, 0) + self.header_icon_border = 0 + self.size = QtCore.QSize(0, 0) + + @property + def header_icon(self) -> QtGui.QIcon | None: + """Get the header icon for this column. + + Returns + ------- + QtGui.QIcon | None + The header icon, or None if no icon is available. + """ + if self._header_icon is None: + self._header_icon = self._provider.get_icon() + return self._header_icon + + def set_header_icon_size(self, width: int, height: int, border: int) -> None: + """Set the header icon size and border. + + Parameters + ---------- + width : int + Width of the icon in pixels. + height : int + Height of the icon in pixels. + border : int + Border size around the icon in pixels. + """ + self.header_icon_size = QtCore.QSize(width, height) + self.header_icon_border = border + self.size = QtCore.QSize(width + 2 * border, height + 2 * border) + self.width = self.size.width() + + def paint(self, painter: QtGui.QPainter, rect: QtCore.QRect) -> None: # pragma: no cover - GUI rendering + """Paint the header icon in the specified rectangle. + + Parameters + ---------- + painter : QtGui.QPainter + The painter to use for drawing. + rect : QtCore.QRect + The rectangle where the icon should be painted. + """ + icon = self.header_icon + if not icon: + return + h = self.header_icon_size.height() + w = self.header_icon_size.width() + border = self.header_icon_border + padding_v = (rect.height() - h) // 2 + target_rect = QtCore.QRect(rect.x() + border, rect.y() + padding_v, w, h) + painter.drawPixmap(target_rect, icon.pixmap(self.header_icon_size)) diff --git a/picard/ui/itemviews/custom_columns/common_columns.py b/picard/ui/itemviews/custom_columns/common_columns.py new file mode 100644 index 0000000000..d75496e731 --- /dev/null +++ b/picard/ui/itemviews/custom_columns/common_columns.py @@ -0,0 +1,183 @@ +# -*- 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 GNU General Public License 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. + +"""Factories for creating standard built-in columns (match, file, etc.).""" + +from __future__ import annotations + +from PyQt6 import QtCore + +from picard.i18n import N_ +from picard.util import icontheme + +from picard.ui.columns import ColumnAlign, ColumnSortType +from picard.ui.itemviews.custom_columns import make_delegate_column, make_field_column, make_numeric_field_column +from picard.ui.itemviews.custom_columns.factory import make_icon_header_column +from picard.ui.itemviews.custom_columns.providers import LazyHeaderIconProvider +from picard.ui.itemviews.custom_columns.sorting_adapters import NumericSortAdapter +from picard.ui.itemviews.custom_columns.utils import ( + parse_bitrate, + parse_file_size, + parse_time_format, +) +from picard.ui.itemviews.match_quality_column import MatchQualityProvider + + +def create_match_quality_column(): + """Create a match quality delegate column with proper sorting. + + Returns + ------- + DelegateColumn + The configured match quality column. + """ + base_provider = MatchQualityProvider() + sorter = NumericSortAdapter(base_provider) + column = make_delegate_column( + N_("Match"), + '~match_quality', + base_provider, + width=57, + sort_type=ColumnSortType.SORTKEY, + size=QtCore.QSize(16, 16), + sort_provider=sorter, + ) + column.is_default = True + return column + + +def create_fingerprint_status_column(): + """Create the fingerprint status icon header column. + + Returns + ------- + IconColumn + The configured fingerprint status column. + """ + provider = LazyHeaderIconProvider(lambda: icontheme.lookup('fingerprint-gray', icontheme.ICON_SIZE_MENU)) + column = make_icon_header_column( + N_("Fingerprint status"), + '~fingerprint', + provider, + icon_width=16, + icon_height=16, + border=1, + ) + return column + + +def create_common_columns(): + """Create the built-in common columns using factories. + + Returns + ------- + tuple + Tuple of configured column objects for both views. + """ + # Title (status icon column) + title_col = make_field_column( + N_("Title"), + 'title', + sort_type=ColumnSortType.NAT, + width=250, + always_visible=True, + status_icon=True, + ) + title_col.is_default = True + + # Length with numeric sort key from metadata.length + length_col = make_numeric_field_column( + N_("Length"), + '~length', + parse_time_format, + width=50, + align=ColumnAlign.RIGHT, + ) + length_col.is_default = True + + # Artist + artist_col = make_field_column(N_("Artist"), 'artist', width=200) + artist_col.is_default = True + + # Others (mostly field columns) + album_artist = make_field_column(N_("Album Artist"), 'albumartist') + composer = make_field_column(N_("Composer"), 'composer') + album = make_field_column(N_("Album"), 'album', sort_type=ColumnSortType.NAT) + discsubtitle = make_field_column(N_("Disc Subtitle"), 'discsubtitle', sort_type=ColumnSortType.NAT) + trackno = make_field_column(N_("Track No."), 'tracknumber', align=ColumnAlign.RIGHT, sort_type=ColumnSortType.NAT) + discno = make_field_column(N_("Disc No."), 'discnumber', align=ColumnAlign.RIGHT, sort_type=ColumnSortType.NAT) + catalognumber = make_field_column(N_("Catalog No."), 'catalognumber', sort_type=ColumnSortType.NAT) + barcode = make_field_column(N_("Barcode"), 'barcode') + media = make_field_column(N_("Media"), 'media') + + # Size with numeric sort key + size_col = make_numeric_field_column( + N_("Size"), + '~filesize', + parse_file_size, + align=ColumnAlign.RIGHT, + ) + + # File Type + filetype = make_field_column(N_("File Type"), '~format', width=120) + + # Bitrate + bitrate = make_numeric_field_column( + N_("Bitrate"), + '~bitrate', + parse_bitrate, + width=80, + align=ColumnAlign.RIGHT, + ) + + genre = make_field_column(N_("Genre"), 'genre') + + fingerprint = create_fingerprint_status_column() + + date = make_field_column(N_("Date"), 'date') + originaldate = make_field_column(N_("Original Release Date"), 'originaldate') + releasedate = make_field_column(N_("Release Date"), 'releasedate') + cover = make_field_column(N_("Cover"), 'covercount') + coverdims = make_field_column(N_("Cover Dimensions"), 'coverdimensions') + + return ( + title_col, + length_col, + artist_col, + album_artist, + composer, + album, + discsubtitle, + trackno, + discno, + catalognumber, + barcode, + media, + size_col, + filetype, + bitrate, + genre, + fingerprint, + date, + originaldate, + releasedate, + cover, + coverdims, + ) diff --git a/picard/ui/itemviews/custom_columns/factory.py b/picard/ui/itemviews/custom_columns/factory.py index 612723baf3..56a35ba5ef 100644 --- a/picard/ui/itemviews/custom_columns/factory.py +++ b/picard/ui/itemviews/custom_columns/factory.py @@ -24,6 +24,8 @@ from collections.abc import Callable +from PyQt6 import QtCore + from picard.item import Item from picard.script import ScriptParser @@ -31,9 +33,11 @@ ColumnAlign, ColumnSortType, ) -from picard.ui.itemviews.custom_columns.column import CustomColumn +from picard.ui.itemviews.custom_columns.column import CustomColumn, DelegateColumn, IconColumn from picard.ui.itemviews.custom_columns.protocols import ( ColumnValueProvider, + DelegateProvider, + HeaderIconProvider, SortKeyProvider, ) from picard.ui.itemviews.custom_columns.providers import ( @@ -42,6 +46,7 @@ TransformProvider, ) from picard.ui.itemviews.custom_columns.script_provider import ChainedValueProvider +from picard.ui.itemviews.custom_columns.sorting_adapters import NumericSortAdapter def _infer_sort_type(provider: ColumnValueProvider, sort_type: ColumnSortType | None) -> ColumnSortType: @@ -75,6 +80,7 @@ def _create_custom_column( align: ColumnAlign = ColumnAlign.LEFT, always_visible: bool = False, sort_type: ColumnSortType | None = None, + status_icon: bool = False, ) -> CustomColumn: """Create `CustomColumn`. @@ -101,6 +107,7 @@ def _create_custom_column( align=align, sort_type=inferred_sort_type, always_visible=always_visible, + status_icon=status_icon, ) @@ -111,6 +118,8 @@ def make_field_column( width: int | None = None, align: ColumnAlign = ColumnAlign.LEFT, always_visible: bool = False, + sort_type: ColumnSortType = ColumnSortType.TEXT, + status_icon: bool = False, ) -> CustomColumn: """Create column that displays a field via `obj.column(key)`. @@ -132,7 +141,47 @@ def make_field_column( width=width, align=align, always_visible=always_visible, - sort_type=ColumnSortType.TEXT, + sort_type=sort_type, + status_icon=status_icon, + ) + + +def make_numeric_field_column( + title: str, + key: str, + parser: Callable[[str], float] | None = None, + *, + width: int | None = None, + align: ColumnAlign = ColumnAlign.LEFT, + always_visible: bool = False, + status_icon: bool = False, +) -> CustomColumn: + """Create column that displays a field with numeric sorting. + + Parameters + ---------- + title, key, width, align, always_visible, status_icon + Column configuration. + parser : Callable[[str], float] | None, optional + Function to parse the field value to a numeric value for sorting. + If None, uses the default float() parser. + + Returns + ------- + CustomColumn + The numeric field column with proper sorting. + """ + base_provider = FieldReferenceProvider(key) + numeric_provider = NumericSortAdapter(base_provider, parser=parser) + return _create_custom_column( + title, + key, + numeric_provider, + width=width, + align=align, + always_visible=always_visible, + sort_type=ColumnSortType.SORTKEY, + status_icon=status_icon, ) @@ -188,6 +237,7 @@ def make_callable_column( align: ColumnAlign = ColumnAlign.LEFT, always_visible: bool = False, sort_type: ColumnSortType | None = None, + status_icon: bool = False, ) -> CustomColumn: """Create column backed by a Python callable. @@ -210,6 +260,7 @@ def make_callable_column( align=align, always_visible=always_visible, sort_type=sort_type, + status_icon=status_icon, ) @@ -281,3 +332,66 @@ def make_provider_column( always_visible=always_visible, sort_type=sort_type, ) + + +def make_delegate_column( + title: str, + key: str, + provider: DelegateProvider, + *, + width: int | None = None, + align: ColumnAlign = ColumnAlign.LEFT, + always_visible: bool = False, + sort_type: ColumnSortType | None = None, + size: QtCore.QSize | None = None, + sort_provider: SortKeyProvider | None = None, +) -> DelegateColumn: + """Create column that uses a delegate for custom rendering. + + Parameters + ---------- + title, key, provider, width, align, always_visible, sort_type + Column configuration. + + Returns + ------- + DelegateColumn + The delegate column. + """ + resolved_sort_type = sort_type or ColumnSortType.TEXT + return DelegateColumn( + title, + key, + provider, + width=width, + align=align, + sort_type=resolved_sort_type, + always_visible=always_visible, + size=size, + sort_provider=sort_provider, + ) + + +def make_icon_header_column( + title: str, + key: str, + provider: HeaderIconProvider, + *, + icon_width: int, + icon_height: int, + border: int = 0, +) -> IconColumn: + """Create an icon header column from a header icon provider. + + Parameters + ---------- + title, key + Column configuration. + provider + Object implementing `HeaderIconProvider`. + icon_width, icon_height, border + Header icon sizing and border. + """ + column = IconColumn(title, key, provider, width=None) + column.set_header_icon_size(icon_width, icon_height, border) + return column diff --git a/picard/ui/itemviews/custom_columns/protocols.py b/picard/ui/itemviews/custom_columns/protocols.py index e0c8c49d6c..236e86aad3 100644 --- a/picard/ui/itemviews/custom_columns/protocols.py +++ b/picard/ui/itemviews/custom_columns/protocols.py @@ -27,6 +27,8 @@ runtime_checkable, ) +from PyQt6 import QtGui + from picard.item import Item @@ -78,3 +80,31 @@ def sort_key(self, obj: Item): # pragma: no cover - optional Sort key. """ ... + + +@runtime_checkable +class DelegateProvider(Protocol): + """Protocol for columns that require custom rendering via delegates.""" + + def get_delegate_class(self) -> type: + """Return the delegate class for custom rendering. + + Returns + ------- + type + The delegate class (subclass of QStyledItemDelegate). + """ + ... + + +@runtime_checkable +class HeaderIconProvider(Protocol): + """Protocol for providing a header icon for an icon column. + + Implementations should construct icons lazily (after QApplication exists) + and can internally cache the icon. + """ + + def get_icon(self) -> QtGui.QIcon: # pragma: no cover - QtGui.QIcon + """Return a QIcon used for the column header.""" + ... diff --git a/picard/ui/itemviews/custom_columns/providers.py b/picard/ui/itemviews/custom_columns/providers.py index 18027179d9..3d6b84a828 100644 --- a/picard/ui/itemviews/custom_columns/providers.py +++ b/picard/ui/itemviews/custom_columns/providers.py @@ -26,11 +26,13 @@ from dataclasses import dataclass import re +from PyQt6 import QtGui + from picard import log from picard.item import Item from picard.script.parser import normalize_tagname -from picard.ui.itemviews.custom_columns.protocols import ColumnValueProvider +from picard.ui.itemviews.custom_columns.protocols import ColumnValueProvider, HeaderIconProvider @dataclass(frozen=True) @@ -105,3 +107,23 @@ def evaluate(self, obj: Item) -> str: def __repr__(self) -> str: # pragma: no cover - debug helper func_name = getattr(self._func, "__name__", repr(self._func)) return f"{self.__class__.__name__}(func={func_name})" + + +class LazyHeaderIconProvider(HeaderIconProvider): + """Provide a header icon lazily via a callable. + + Parameters + ---------- + factory + A zero-argument callable that returns a `QtGui.QIcon` when invoked. + The icon is created only once and cached. + """ + + def __init__(self, factory: Callable[[], QtGui.QIcon]): + self._factory = factory + self._icon: QtGui.QIcon | None = None + + def get_icon(self) -> QtGui.QIcon: # pragma: no cover - Qt object + if self._icon is None: + self._icon = self._factory() + return self._icon diff --git a/picard/ui/itemviews/custom_columns/utils.py b/picard/ui/itemviews/custom_columns/utils.py new file mode 100644 index 0000000000..38816251f4 --- /dev/null +++ b/picard/ui/itemviews/custom_columns/utils.py @@ -0,0 +1,120 @@ +# -*- 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 GNU General Public License 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. + +"""Utility functions for custom column parsing and formatting.""" + +from __future__ import annotations + +import re + + +def parse_time_format(time_str: str) -> float: + """Parse time format (e.g., '3:00', '1:02:59') to seconds. + + Parameters + ---------- + time_str : str + Time string in format MM:SS or HH:MM:SS. + + Returns + ------- + float + Time in seconds. + + Raises + ------ + ValueError + If the time format is invalid. + """ + if not time_str or time_str == "?:??": + return 0.0 + parts = time_str.split(':') + if len(parts) == 2: # MM:SS + minutes, seconds = map(float, parts) + return minutes * 60 + seconds + elif len(parts) == 3: # HH:MM:SS + hours, minutes, seconds = map(float, parts) + return hours * 3600 + minutes * 60 + seconds + else: + raise ValueError(f"Invalid time format: {time_str}") + + +def parse_file_size(size_str: str) -> float: + """Parse file size format (e.g., '1.0 MB', '1.0 MiB') to bytes. + + Parameters + ---------- + size_str : str + File size string with optional unit (B, KB, MB, GB, TB). + + Returns + ------- + float + Size in bytes. + + Raises + ------ + ValueError + If the size format is invalid. + """ + if not size_str: + return 0.0 + # Remove any parentheses content like "(1.0 MiB)" + size_str = size_str.split('(')[0].strip() + size_str = size_str.upper() + + # Extract number and unit + match = re.match(r'^([\d.]+)\s*([KMGT]?B?)$', size_str) + if not match: + raise ValueError(f"Invalid size format: {size_str}") + + number, unit = match.groups() + number = float(number) + + # Convert to bytes + multipliers = {'B': 1, 'KB': 1024, 'MB': 1024**2, 'GB': 1024**3, 'TB': 1024**4} + return number * multipliers.get(unit, 1) + + +def parse_bitrate(bitrate_str: str) -> float: + """Parse bitrate format (e.g., '320 kbps', '128 kbps') to kbps. + + Parameters + ---------- + bitrate_str : str + Bitrate string with numeric value. + + Returns + ------- + float + Bitrate in kbps. + + Raises + ------ + ValueError + If the bitrate format is invalid. + """ + if not bitrate_str: + return 0.0 + # Extract number from bitrate string + match = re.search(r'([\d.]+)', bitrate_str) + if not match: + raise ValueError(f"Invalid bitrate format: {bitrate_str}") + return float(match.group(1)) diff --git a/picard/ui/itemviews/match_quality_column.py b/picard/ui/itemviews/match_quality_column.py index 268257eb6e..da8450c485 100644 --- a/picard/ui/itemviews/match_quality_column.py +++ b/picard/ui/itemviews/match_quality_column.py @@ -21,11 +21,13 @@ from collections import namedtuple -from PyQt6 import QtCore, QtWidgets +from PyQt6 import QtCore, QtGui, QtWidgets from picard.i18n import gettext as _ +from picard.item import Item -from picard.ui.columns import ImageColumn +from picard.ui.itemviews.custom_columns import DelegateColumn +from picard.ui.itemviews.custom_columns.protocols import ColumnValueProvider, DelegateProvider # Structured container for delegate item data @@ -45,57 +47,58 @@ } -class MatchQualityColumn(ImageColumn): +class MatchQualityProvider(ColumnValueProvider, DelegateProvider): """Column that displays match quality using match icons at the release level.""" - def __init__(self, title, key, width=120): - super().__init__(title, key, width=width) - self.size = QtCore.QSize(16, 16) # Icon size - self.width = width + def __init__(self) -> None: + """Initialize the match quality provider.""" + self._delegate_class = MatchQualityColumnDelegate - def paint(self, painter, rect): - """Override paint method to prevent NotImplementedError from base class.""" - # This column is painted by the delegate, not the header - # So we just do nothing here to prevent the error - pass + def evaluate(self, obj: Item) -> str: + """Return the match percentage as a string for sorting. - def get_match_icon(self, obj): - """Get the appropriate match icon for the given object.""" - # Only show icons at the release (album) level, not track level + Parameters + ---------- + obj + The item to evaluate. + + Returns + ------- + str + The match percentage as a string (e.g., "0.75" for 75%). + """ if not hasattr(obj, 'get_num_matched_tracks') or not hasattr(obj, 'tracks'): - return None + return "0.0" # Album object total = len(obj.tracks) if obj.tracks else 0 if total == 0: - # Use pending icon for zero tracks - from picard.ui.itemviews import FileItem + return "0.0" - if hasattr(FileItem, 'match_pending_icons') and len(FileItem.match_pending_icons) > 5: - return FileItem.match_pending_icons[5] # match-pending-100.png - return None + get_num_matched_tracks = getattr(obj, 'get_num_matched_tracks', None) + if not callable(get_num_matched_tracks): + return "0.0" - # Calculate match percentage - matched = obj.get_num_matched_tracks() + matched = int(get_num_matched_tracks()) percentage = matched / total - - # Determine which icon to use based on percentage using thresholds - for threshold in sorted(THRESHOLD_TO_ICON_INDEX.keys(), reverse=True): - if percentage >= threshold: - icon_index = THRESHOLD_TO_ICON_INDEX[threshold] - break - - # Get the match icons from FileItem - from picard.ui.itemviews import FileItem - - if hasattr(FileItem, 'match_icons') and icon_index < len(FileItem.match_icons): - return FileItem.match_icons[icon_index] - - return None - - def get_match_stats(self, obj): - """Get comprehensive match statistics for the given object.""" + return str(percentage) + + def get_match_stats(self, obj: Item) -> dict[str, int] | None: + """Get comprehensive match statistics for the given object. + + Parameters + ---------- + obj + The item to evaluate. + + Returns + ------- + dict[str, int] | None + A dictionary with keys ``matched``, ``total``, ``unmatched``, + ``duplicates``, ``extra``, and ``missing``. Returns ``None`` if + stats are not available for the given object. + """ # Only show stats at the release (album) level, not track level if not hasattr(obj, 'get_num_matched_tracks') or not hasattr(obj, 'tracks'): return None @@ -141,11 +144,21 @@ def get_match_stats(self, obj): 'missing': missing, } + def get_delegate_class(self) -> type[QtWidgets.QStyledItemDelegate]: + """Return the delegate class for custom rendering. + + Returns + ------- + type[QtWidgets.QStyledItemDelegate] + The delegate class (subclass of QStyledItemDelegate). + """ + return self._delegate_class + class MatchQualityColumnDelegate(QtWidgets.QStyledItemDelegate): """Delegate for rendering match quality icons in tree items.""" - def __init__(self, parent=None): + def __init__(self, parent: QtCore.QObject | None = None) -> None: super().__init__(parent) def _get_item_data(self, index: QtCore.QModelIndex) -> MatchItemData | None: @@ -172,23 +185,67 @@ def _get_item_data(self, index: QtCore.QModelIndex) -> MatchItemData | None: return None column = columns[column_index] - if not isinstance(column, MatchQualityColumn): + if not isinstance(column, DelegateColumn): return None - stats = column.get_match_stats(obj) + # Get stats from the provider (not from delegate) + provider = column.delegate_provider + if hasattr(provider, '_base'): + provider = provider._base # Unwrap adapter + + stats = provider.get_match_stats(obj) if not stats: return None return MatchItemData(obj=obj, column=column, stats=stats) - def _format_tooltip_text(self, stats): - """Format stats into detailed tooltip text. + def _get_match_icon(self, obj: Item, column: DelegateColumn) -> QtGui.QIcon | None: + """Select the appropriate match icon using delegate-side logic.""" + # Compute stats using provider, but map to icons here (UI concern) + provider = column.delegate_provider + if hasattr(provider, '_base'): + provider = provider._base # Unwrap adapter - Args: - stats: Dictionary containing match statistics + stats = provider.get_match_stats(obj) + if not stats: + return None - Returns: - str: Formatted tooltip text + total = stats.get('total', 0) + if total == 0: + from picard.ui.itemviews import FileItem + + if hasattr(FileItem, 'match_pending_icons') and len(FileItem.match_pending_icons) > 5: + return FileItem.match_pending_icons[5] + return None + + matched = stats.get('matched', 0) + percentage = matched / total if total else 0.0 + + # Determine which icon index to use based on percentage using thresholds + icon_index = 0 + for threshold in sorted(THRESHOLD_TO_ICON_INDEX.keys(), reverse=True): + if percentage >= threshold: + icon_index = THRESHOLD_TO_ICON_INDEX[threshold] + break + + from picard.ui.itemviews import FileItem + + if hasattr(FileItem, 'match_icons') and icon_index < len(FileItem.match_icons): + return FileItem.match_icons[icon_index] + return None + + def _format_tooltip_text(self, stats: dict[str, int]) -> str: + """Format stats into a detailed tooltip string. + + Parameters + ---------- + stats + Dictionary containing integer match statistics. + + Returns + ------- + str + The formatted tooltip text. """ tooltip_parts = [] @@ -214,7 +271,23 @@ def _format_tooltip_text(self, stats): return "\n".join(tooltip_parts) - def paint(self, painter, option, index): + def paint( + self, + painter: QtGui.QPainter, + option: QtWidgets.QStyleOptionViewItem, + index: QtCore.QModelIndex, + ) -> None: + """Paint the match quality icon in the item cell. + + Parameters + ---------- + painter + The painter used for drawing. + option + The style options for the item. + index + The model index identifying the item. + """ # Initialize the style option self.initStyleOption(option, index) @@ -230,8 +303,8 @@ def paint(self, painter, option, index): if not item_data: return - # Get the match icon - icon = item_data.column.get_match_icon(item_data.obj) + # Get the match icon from the provider + icon = self._get_match_icon(item_data.obj, item_data.column) # Calculate layout icon_size = item_data.column.size @@ -243,8 +316,31 @@ def paint(self, painter, option, index): y = option.rect.y() + (option.rect.height() - icon_size.height()) // 2 icon.paint(painter, QtCore.QRect(x, y, icon_size.width(), icon_size.height())) - def helpEvent(self, event, view, option, index): - """Show tooltip with explanation of the stats.""" + def helpEvent( + self, + event: QtGui.QHelpEvent, + view: QtWidgets.QWidget, + option: QtWidgets.QStyleOptionViewItem, + index: QtCore.QModelIndex, + ) -> bool: + """Show a tooltip with an explanation of the stats. + + Parameters + ---------- + event + The help event providing cursor position. + view + The view showing the items. + option + The style options for the item. + index + The model index identifying the item. + + Returns + ------- + bool + ``True`` if the event was handled, else ``False``. + """ # Get item data item_data = self._get_item_data(index) if not item_data: @@ -257,6 +353,24 @@ def helpEvent(self, event, view, option, index): QtWidgets.QToolTip.showText(event.globalPos(), tooltip_text, view) return True - def sizeHint(self, option, index): + def sizeHint( + self, + option: QtWidgets.QStyleOptionViewItem, + index: QtCore.QModelIndex, + ) -> QtCore.QSize: + """Return the recommended size for the item cell. + + Parameters + ---------- + option + The style options for the item. + index + The model index identifying the item. + + Returns + ------- + QtCore.QSize + A size that accommodates both icon and text. + """ # Return a size that accommodates both icon and text return QtCore.QSize(57, 16) diff --git a/test/test_album_loading_sorting.py b/test/test_album_loading_sorting.py deleted file mode 100644 index 3612cabe01..0000000000 --- a/test/test_album_loading_sorting.py +++ /dev/null @@ -1,92 +0,0 @@ -# -*- 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 GNU General Public License 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 unittest.mock import Mock - -from picard.album import AlbumStatus -from picard.const.sys import IS_LINUX - -import pytest - -from picard.ui.itemviews.columns import _sortkey_match_quality - - -def _apply_platform_multiplier(value): - """Apply the same platform-specific multiplier logic as in _sortkey_match_quality.""" - multiplier = -1 if IS_LINUX else 1 - return value * multiplier - - -class TestAlbumLoadingSorting: - """Test that sorting works correctly during album loading.""" - - def test_sortkey_during_loading_returns_zero(self): - """Test that sort key returns 0.0 when album is still loading.""" - album = Mock() - album.status = AlbumStatus.LOADING - album.get_num_matched_tracks.return_value = 3 - album.tracks = [Mock(), Mock(), Mock(), Mock(), Mock()] # 5 tracks - - result = _sortkey_match_quality(album) - assert result == 0.0 - - def test_sortkey_after_loading_returns_correct_percentage(self): - """Test that sort key returns correct percentage after album is loaded.""" - album = Mock() - album.status = AlbumStatus.LOADED - album.get_num_matched_tracks.return_value = 3 - album.tracks = [Mock(), Mock(), Mock(), Mock(), Mock()] # 5 tracks - - result = _sortkey_match_quality(album) - expected = _apply_platform_multiplier(0.6) - assert result == expected - - def test_sortkey_no_status_attribute_works_normally(self): - """Test that objects without status attribute work normally.""" - album = Mock() - # No status attribute - album.get_num_matched_tracks.return_value = 2 - album.tracks = [Mock(), Mock(), Mock()] # 3 tracks - - result = _sortkey_match_quality(album) - expected = _apply_platform_multiplier(0.6666666666666666) - assert result == pytest.approx(expected, rel=1e-10) # 2/3 ≈ 0.666 - - def test_sortkey_error_status_works_normally(self): - """Test that albums with error status work normally.""" - album = Mock() - album.status = AlbumStatus.ERROR - album.get_num_matched_tracks.return_value = 1 - album.tracks = [Mock(), Mock()] # 2 tracks - - result = _sortkey_match_quality(album) - expected = _apply_platform_multiplier(0.5) - assert result == expected - - def test_sortkey_none_status_works_normally(self): - """Test that albums with None status work normally.""" - album = Mock() - album.status = None - album.get_num_matched_tracks.return_value = 4 - album.tracks = [Mock(), Mock(), Mock(), Mock(), Mock()] # 5 tracks - - result = _sortkey_match_quality(album) - expected = _apply_platform_multiplier(0.8) - assert result == expected diff --git a/test/test_itemviews_columns_split.py b/test/test_itemviews_columns_split.py index a01963a233..4f2d51947e 100644 --- a/test/test_itemviews_columns_split.py +++ b/test/test_itemviews_columns_split.py @@ -27,7 +27,7 @@ ALBUMVIEW_COLUMNS, FILEVIEW_COLUMNS, ) -from picard.ui.itemviews.match_quality_column import MatchQualityColumn +from picard.ui.itemviews.custom_columns import DelegateColumn @pytest.fixture @@ -74,11 +74,11 @@ def test_album_view_match_after_albumartist(album_keys: list[str]) -> None: assert idx_match == idx_albumartist + 1 -def test_album_view_match_is_match_quality_column_type() -> None: - # Ensure the actual column instance at the expected index is MatchQualityColumn +def test_album_view_match_is_delegate_column_type() -> None: + # Ensure the actual column instance at the expected index is DelegateColumn keys: list[str] = [c.key for c in ALBUMVIEW_COLUMNS] idx_match: int = _index_of(keys, "~match_quality") - assert isinstance(ALBUMVIEW_COLUMNS[idx_match], MatchQualityColumn) + assert isinstance(ALBUMVIEW_COLUMNS[idx_match], DelegateColumn) class _DummyObj: diff --git a/test/test_itemviews_match_quality.py b/test/test_itemviews_match_quality.py index a74aa2a0c8..b5dab24ec2 100644 --- a/test/test_itemviews_match_quality.py +++ b/test/test_itemviews_match_quality.py @@ -23,33 +23,24 @@ from PyQt6 import QtCore, QtGui, QtWidgets -from picard.album import Album, AlbumStatus -from picard.const.sys import IS_LINUX +from picard.album import Album from picard.track import Track import pytest from picard.ui.itemviews import TreeItem -from picard.ui.itemviews.columns import _sortkey_match_quality -from picard.ui.itemviews.match_quality_column import ( - MatchQualityColumn, - MatchQualityColumnDelegate, -) +from picard.ui.itemviews.custom_columns import DelegateColumn +from picard.ui.itemviews.custom_columns.common_columns import create_match_quality_column +from picard.ui.itemviews.match_quality_column import MatchQualityColumnDelegate, MatchQualityProvider -def _apply_platform_multiplier(value): - """Apply the same platform-specific multiplier logic as in _sortkey_match_quality.""" - multiplier = -1 if IS_LINUX else 1 - return value * multiplier - - -class TestMatchQualityColumn: - """Test the MatchQualityColumn class.""" +class TestMatchQualityProvider: + """Test the MatchQualityProvider class.""" @pytest.fixture - def match_quality_column(self) -> MatchQualityColumn: - """Create a MatchQualityColumn instance for testing.""" - return MatchQualityColumn("Match Quality", "~match_quality", width=120) + def provider(self) -> MatchQualityProvider: + """Create a MatchQualityProvider instance for testing.""" + return MatchQualityProvider() @pytest.fixture def mock_album(self) -> Mock: @@ -62,88 +53,49 @@ def mock_album(self) -> Mock: album.unmatched_files.files = [] return album - @pytest.fixture - def mock_track(self) -> Mock: - """Create a mock track.""" - track = Mock(spec=Track) - track.files = [] - return track - - def test_init(self, match_quality_column: MatchQualityColumn) -> None: - """Test MatchQualityColumn initialization.""" - assert match_quality_column.title == "Match Quality" - assert match_quality_column.key == "~match_quality" - assert match_quality_column.width == 120 - assert match_quality_column.size == QtCore.QSize(16, 16) - - def test_paint_does_nothing(self, match_quality_column: MatchQualityColumn) -> None: - """Test that paint method does nothing (handled by delegate).""" - painter = Mock() - rect = QtCore.QRect(0, 0, 100, 20) - # Should not raise any exception - match_quality_column.paint(painter, rect) + def test_init(self, provider: MatchQualityProvider) -> None: + """Test MatchQualityProvider initialization.""" + assert provider is not None + assert provider.get_delegate_class() == MatchQualityColumnDelegate @pytest.mark.parametrize( - ("matched", "total", "expected_icon_index"), + ("matched", "total", "expected_percentage"), [ - (0, 0, 5), # No tracks - use pending icon - (5, 10, 0), # 50% match - (6, 10, 1), # 60% match - (7, 10, 2), # 70% match - (8, 10, 3), # 80% match - (9, 10, 4), # 90% match - (10, 10, 5), # 100% match - (3, 10, 0), # 30% match - use worst icon + (0, 0, "0.0"), # No tracks + (5, 10, "0.5"), # 50% match + (6, 10, "0.6"), # 60% match + (7, 10, "0.7"), # 70% match + (8, 10, "0.8"), # 80% match + (9, 10, "0.9"), # 90% match + (10, 10, "1.0"), # 100% match + (3, 10, "0.3"), # 30% match ], ) - def test_get_match_icon_percentage_based( + def test_evaluate_percentage_calculation( self, - match_quality_column: MatchQualityColumn, + provider: MatchQualityProvider, mock_album: Mock, matched: int, total: int, - expected_icon_index: int, + expected_percentage: str, ) -> None: - """Test get_match_icon returns correct icon based on percentage.""" + """Test evaluate returns correct percentage as string.""" # Setup mock album mock_album.get_num_matched_tracks.return_value = matched mock_album.tracks = [Mock() for _ in range(total)] - # Mock FileItem.match_icons - patch the import inside the method - with patch("picard.ui.itemviews.FileItem") as mock_file_item: - mock_file_item.match_icons = [Mock() for _ in range(6)] - mock_file_item.match_pending_icons = [Mock() for _ in range(6)] - - icon = match_quality_column.get_match_icon(mock_album) - - if total == 0: - # Should use pending icon for zero tracks - assert icon == mock_file_item.match_pending_icons[5] - else: - # Should use match icon based on percentage - assert icon == mock_file_item.match_icons[expected_icon_index] + result = provider.evaluate(mock_album) + assert result == expected_percentage - def test_get_match_icon_no_album_attributes(self, match_quality_column: MatchQualityColumn) -> None: - """Test get_match_icon returns None for objects without album attributes.""" + def test_evaluate_no_album_attributes(self, provider: MatchQualityProvider) -> None: + """Test evaluate returns 0.0 for objects without album attributes.""" obj = Mock() # Remove album attributes del obj.get_num_matched_tracks del obj.tracks - icon = match_quality_column.get_match_icon(obj) - assert icon is None - - def test_get_match_icon_no_fileitem_icons(self, match_quality_column: MatchQualityColumn, mock_album: Mock) -> None: - """Test get_match_icon handles missing FileItem icons gracefully.""" - mock_album.get_num_matched_tracks.return_value = 5 - mock_album.tracks = [Mock() for _ in range(10)] - - with patch("picard.ui.itemviews.FileItem") as mock_file_item: - # Remove match_icons attribute - del mock_file_item.match_icons - - icon = match_quality_column.get_match_icon(mock_album) - assert icon is None + result = provider.evaluate(obj) + assert result == "0.0" @pytest.mark.parametrize( ("matched", "total", "unmatched", "duplicates", "extra", "missing"), @@ -156,7 +108,7 @@ def test_get_match_icon_no_fileitem_icons(self, match_quality_column: MatchQuali ) def test_get_match_stats( self, - match_quality_column: MatchQualityColumn, + provider: MatchQualityProvider, mock_album: Mock, matched: int, total: int, @@ -165,7 +117,7 @@ def test_get_match_stats( extra: int, missing: int, ) -> None: - """Test get_match_stats returns correct statistics.""" + """Test _get_match_stats returns correct statistics.""" # Setup mock album mock_album.get_num_matched_tracks.return_value = matched mock_album.get_num_unmatched_files.return_value = unmatched @@ -181,7 +133,7 @@ def test_get_match_stats( else: track.files = [Mock()] # Normal tracks - stats = match_quality_column.get_match_stats(mock_album) + stats = provider.get_match_stats(mock_album) assert stats is not None assert stats['matched'] == matched @@ -191,17 +143,175 @@ def test_get_match_stats( assert stats['extra'] == extra assert stats['missing'] == missing - def test_get_match_stats_no_album_attributes(self, match_quality_column: MatchQualityColumn) -> None: - """Test get_match_stats returns None for objects without album attributes.""" + def test_get_match_stats_no_album_attributes(self, provider: MatchQualityProvider) -> None: + """Test _get_match_stats returns None for objects without album attributes.""" obj = Mock() # Remove album attributes del obj.get_num_matched_tracks del obj.tracks - stats = match_quality_column.get_match_stats(obj) + stats = provider.get_match_stats(obj) assert stats is None +class TestDelegateColumn: + """Test the DelegateColumn class.""" + + @pytest.fixture + def provider(self) -> MatchQualityProvider: + """Create a MatchQualityProvider instance for testing.""" + return MatchQualityProvider() + + @pytest.fixture + def delegate_column(self, provider: MatchQualityProvider) -> DelegateColumn: + """Create a DelegateColumn instance for testing.""" + return DelegateColumn("Match Quality", "~match_quality", provider, width=57, size=QtCore.QSize(16, 16)) + + def test_init(self, delegate_column: DelegateColumn) -> None: + """Test DelegateColumn initialization.""" + assert delegate_column.title == "Match Quality" + assert delegate_column.key == "~match_quality" + assert delegate_column.width == 57 + assert delegate_column.size == QtCore.QSize(16, 16) + assert delegate_column.delegate_class == MatchQualityColumnDelegate + + def test_delegate_class_property(self, delegate_column: DelegateColumn) -> None: + """Test delegate_class property returns correct class.""" + assert delegate_column.delegate_class == MatchQualityColumnDelegate + + def test_size_attribute(self, delegate_column: DelegateColumn) -> None: + """Test size attribute is accessible for delegate compatibility.""" + assert hasattr(delegate_column, 'size') + assert delegate_column.size == QtCore.QSize(16, 16) + + def test_default_size(self, provider: MatchQualityProvider) -> None: + """Test DelegateColumn uses default size when none provided.""" + column = DelegateColumn("Test", "test", provider) + assert column.size == QtCore.QSize(16, 16) + + +class TestMatchQualityColumnFactory: + """Test the match quality column factory.""" + + def test_create_match_quality_column(self) -> None: + """Test create_match_quality_column returns properly configured column.""" + column = create_match_quality_column() + + # Check it's a DelegateColumn + assert isinstance(column, DelegateColumn) + + # Check basic properties + assert column.title == "Match" + assert column.key == "~match_quality" + assert column.width == 57 + assert column.size == QtCore.QSize(16, 16) + + # Check delegate class + assert column.delegate_class == MatchQualityColumnDelegate + + # Check it's marked as default + assert column.is_default is True + + def test_factory_uses_numeric_sorting(self) -> None: + """Test that the factory creates a column with numeric sorting.""" + column = create_match_quality_column() + + assert hasattr(column, 'sortkey') + + # Test that it can handle numeric evaluation + from unittest.mock import Mock + + mock_album = Mock() + mock_album.get_num_matched_tracks.return_value = 5 + mock_album.tracks = [Mock() for _ in range(10)] + + # The column's sortkey should return a tuple for sorting + sort_key = column.sortkey(mock_album) + assert isinstance(sort_key, tuple) + assert len(sort_key) == 2 + assert sort_key[0] == 0 # Numeric values get priority + assert sort_key[1] == 0.5 # The actual percentage + + +class TestMatchQualityColumn: + """Test the match quality column using the factory for regression testing.""" + + @pytest.fixture + def match_quality_column(self) -> DelegateColumn: + """Create a match quality column instance for testing using the factory.""" + return create_match_quality_column() + + @pytest.fixture + def mock_album(self) -> Mock: + """Create a mock album with tracks and files.""" + album = Mock(spec=Album) + album.tracks = [] + album.get_num_matched_tracks.return_value = 0 + album.get_num_unmatched_files.return_value = 0 + album.unmatched_files = Mock() + album.unmatched_files.files = [] + return album + + @pytest.fixture + def mock_track(self) -> Mock: + """Create a mock track.""" + track = Mock(spec=Track) + track.files = [] + return track + + def test_init(self, match_quality_column: DelegateColumn) -> None: + """Test match quality column initialization.""" + assert match_quality_column.title == "Match" + assert match_quality_column.key == "~match_quality" + assert match_quality_column.width == 57 + assert match_quality_column.size == QtCore.QSize(16, 16) + assert isinstance(match_quality_column, DelegateColumn) + + def test_delegate_class(self, match_quality_column: DelegateColumn) -> None: + """Test that the column has the correct delegate class.""" + assert match_quality_column.delegate_class == MatchQualityColumnDelegate + + def test_provider_evaluation(self, match_quality_column: DelegateColumn, mock_album: Mock) -> None: + """Test that the provider correctly evaluates match percentage.""" + # Setup mock album + mock_album.get_num_matched_tracks.return_value = 5 + mock_album.tracks = [Mock() for _ in range(10)] + + # Test that the provider evaluates correctly + result = match_quality_column.delegate_provider.evaluate(mock_album) + assert result == "0.5" # 5/10 = 0.5 + + def test_provider_stats(self, match_quality_column: DelegateColumn, mock_album: Mock) -> None: + """Test that the provider correctly calculates match statistics.""" + # Setup mock album + mock_album.get_num_matched_tracks.return_value = 3 + mock_album.get_num_unmatched_files.return_value = 1 + mock_album.tracks = [Mock() for _ in range(5)] + mock_album.unmatched_files.files = [Mock()] + + # Setup tracks with appropriate file counts + for i, track in enumerate(mock_album.tracks): + if i < 1: + track.files = [] # Missing track + elif i < 2: + track.files = [Mock(), Mock()] # Duplicate files + else: + track.files = [Mock()] # Normal tracks + + # Test the provider's get_match_stats method + provider = match_quality_column.delegate_provider + assert provider is not None + stats = provider.get_match_stats(mock_album) + + assert stats is not None + assert stats['matched'] == 3 + assert stats['total'] == 5 + assert stats['unmatched'] == 1 + assert stats['duplicates'] == 1 + assert stats['extra'] == 1 + assert stats['missing'] == 1 + + class TestMatchQualityColumnDelegate: """Test the MatchQualityColumnDelegate class.""" @@ -324,10 +434,10 @@ def test_paint_wrong_column_type( mock_option: Mock, mock_index: Mock, ) -> None: - """Test paint method when column is not MatchQualityColumn.""" + """Test paint method when column is not DelegateColumn.""" painter = Mock() mock_tree_widget.itemFromIndex.return_value = mock_item - mock_item.columns[2] = Mock() # Not MatchQualityColumn + mock_item.columns[2] = Mock() # Not DelegateColumn # Mock initStyleOption to avoid Qt type issues with patch.object(delegate, "parent", return_value=mock_tree_widget): @@ -349,11 +459,24 @@ def test_paint_without_stats( painter = Mock() mock_tree_widget.itemFromIndex.return_value = mock_item - # Mock the column to return None for stats - mock_column = Mock(spec=MatchQualityColumn) - mock_column.get_match_stats.return_value = None + # Mock the column with delegate_provider + mock_provider = Mock(spec=['get_match_stats']) # Only allow get_match_stats attribute + mock_provider.get_match_stats.return_value = None # No stats available + + mock_column = Mock(spec=DelegateColumn) + mock_column.delegate_provider = mock_provider + mock_column.size = QtCore.QSize(16, 16) # Add size attribute mock_item.columns[2] = mock_column + # Mock the index to return column 2 + mock_index.column.return_value = 2 + + # Mock the object to not have album attributes so get_match_stats returns None + mock_item.obj = Mock() + # Remove album attributes so get_match_stats returns None + del mock_item.obj.get_num_matched_tracks + del mock_item.obj.tracks + # Mock initStyleOption to avoid Qt type issues with patch.object(delegate, "parent", return_value=mock_tree_widget): with patch.object(delegate, "initStyleOption"): @@ -414,14 +537,14 @@ def test_helpEvent_without_columns( def test_helpEvent_wrong_column_type( self, delegate: MatchQualityColumnDelegate, mock_tree_widget: Mock, mock_item: Mock ) -> None: - """Test helpEvent method when column is not MatchQualityColumn.""" + """Test helpEvent method when column is not DelegateColumn.""" event = Mock() view = mock_tree_widget option = Mock() index = Mock() index.column.return_value = 2 mock_tree_widget.itemFromIndex.return_value = mock_item - mock_item.columns[2] = Mock() # Not MatchQualityColumn + mock_item.columns[2] = Mock() # Not DelegateColumn result = delegate.helpEvent(event, view, option, index) assert result is False @@ -438,8 +561,7 @@ def test_helpEvent_without_stats( mock_tree_widget.itemFromIndex.return_value = mock_item # Mock the column to return None for stats - mock_column = Mock(spec=MatchQualityColumn) - mock_column.get_match_stats.return_value = None + mock_column = Mock(spec=DelegateColumn) mock_item.columns[2] = mock_column result = delegate.helpEvent(event, view, option, index) @@ -457,12 +579,43 @@ def test_helpEvent_with_stats( index.column.return_value = 2 mock_tree_widget.itemFromIndex.return_value = mock_item + # Mock the album object with proper attributes + mock_album = Mock() + mock_album.get_num_matched_tracks.return_value = 5 + mock_album.get_num_unmatched_files.return_value = 2 + mock_album.tracks = [Mock() for _ in range(10)] # 10 tracks + mock_album.unmatched_files = Mock() + mock_album.unmatched_files.files = [Mock()] # 1 unmatched file + + # Setup tracks with appropriate file counts for stats calculation + for i, track in enumerate(mock_album.tracks): + if i < 2: # 2 missing tracks + track.files = [] + elif i < 3: # 1 track with duplicates + track.files = [Mock(), Mock()] + else: # Normal tracks + track.files = [Mock()] + + mock_item.obj = mock_album + # Set up delegate's parent relationship with patch.object(delegate, "parent", return_value=mock_tree_widget): - # Mock the column - mock_column = Mock(spec=MatchQualityColumn) - stats = {'matched': 5, 'total': 10, 'missing': 2, 'duplicates': 1, 'extra': 1, 'unmatched': 2} - mock_column.get_match_stats.return_value = stats + # Mock the column with delegate_provider + mock_provider = Mock() + mock_provider.get_match_stats.return_value = { + 'matched': 5, + 'total': 10, + 'missing': 2, + 'duplicates': 1, + 'extra': 1, + 'unmatched': 2, + } + # Ensure the provider is not wrapped by an adapter for this test + if hasattr(mock_provider, '_base'): + del mock_provider._base + + mock_column = Mock(spec=DelegateColumn) + mock_column.delegate_provider = mock_provider mock_item.columns[2] = mock_column with patch("picard.ui.itemviews.match_quality_column.QtWidgets.QToolTip") as mock_tooltip: @@ -495,7 +648,7 @@ def test_sizeHint(self, delegate: MatchQualityColumnDelegate) -> None: class TestItemViewsIntegration: - """Test integration of MatchQualityColumn with existing itemviews system.""" + """Test integration of new delegate column architecture with existing itemviews system.""" @pytest.fixture def mock_album(self) -> Mock: @@ -508,14 +661,14 @@ def mock_album(self) -> Mock: album.unmatched_files.files = [Mock()] return album - def test_match_quality_column_in_itemview_columns(self) -> None: - """Test that MatchQualityColumn is included in ITEMVIEW_COLUMNS (album view).""" + def test_delegate_column_in_album_view_columns(self) -> None: + """Test that DelegateColumn is included in ALBUMVIEW_COLUMNS (album view).""" from picard.ui.itemviews.columns import ALBUMVIEW_COLUMNS # Find the match quality column in album view columns match_quality_column = None for column in ALBUMVIEW_COLUMNS: - if isinstance(column, MatchQualityColumn): + if isinstance(column, DelegateColumn) and column.key == "~match_quality": match_quality_column = column break @@ -524,16 +677,16 @@ def test_match_quality_column_in_itemview_columns(self) -> None: assert match_quality_column.key == "~match_quality" assert match_quality_column.sortable is True assert match_quality_column.sort_type is not None - assert match_quality_column.sortkey is not None + assert match_quality_column.delegate_class == MatchQualityColumnDelegate - def test_match_quality_column_not_in_fileview_columns(self) -> None: - """Test that MatchQualityColumn is NOT included in FILEVIEW_COLUMNS.""" + def test_delegate_column_not_in_fileview_columns(self) -> None: + """Test that DelegateColumn is NOT included in FILEVIEW_COLUMNS.""" from picard.ui.itemviews.columns import FILEVIEW_COLUMNS - # Verify that no match quality column exists in file view columns + # Verify that no match quality delegate column exists in file view columns match_quality_column = None for column in FILEVIEW_COLUMNS: - if isinstance(column, MatchQualityColumn): + if isinstance(column, DelegateColumn) and column.key == "~match_quality": match_quality_column = column break @@ -549,41 +702,50 @@ def test_album_view_has_match_quality_column_after_album_artist(self) -> None: assert idx_match == idx_albumartist + 1 match_quality_column = ALBUMVIEW_COLUMNS[idx_match] - assert isinstance(match_quality_column, MatchQualityColumn) + assert isinstance(match_quality_column, DelegateColumn) assert match_quality_column.key == "~match_quality" album_artist_column = ALBUMVIEW_COLUMNS[idx_albumartist] assert album_artist_column.key == "albumartist" - def test_sortkey_progress_function(self) -> None: - """Test the _sortkey_progress function used by MatchQualityColumn.""" - from picard.ui.itemviews.columns import _sortkey_match_quality + def test_numeric_sorting_with_provider(self) -> None: + """Test that the provider uses numeric sorting correctly.""" + from picard.ui.itemviews.columns import ALBUMVIEW_COLUMNS + + # Find the match quality column + match_quality_column = None + for column in ALBUMVIEW_COLUMNS: + if isinstance(column, DelegateColumn) and column.key == "~match_quality": + match_quality_column = column + break + + assert match_quality_column is not None # Test with album object album = Mock() album.get_num_matched_tracks.return_value = 3 album.tracks = [Mock() for _ in range(5)] - result = _sortkey_match_quality(album) - expected = _apply_platform_multiplier(0.6) - assert result == expected + # Test that the provider correctly evaluates match percentage + result = match_quality_column.delegate_provider.evaluate(album) + assert result == "0.6" # 3/5 = 0.6 # Test with track object (should return 0.0) track = Mock() # Mock hasattr to return False for track objects with patch("builtins.hasattr", return_value=False): - result = _sortkey_match_quality(track) - assert result == 0.0 + result = match_quality_column.delegate_provider.evaluate(track) + assert result == "0.0" # Test with album with no tracks album.tracks = [] - result = _sortkey_match_quality(album) - assert result == 0.0 + result = match_quality_column.delegate_provider.evaluate(album) + assert result == "0.0" - def test_treeitem_handles_match_quality_column(self, mock_album: Mock) -> None: - """Test that TreeItem properly handles MatchQualityColumn.""" - # Create a mock column - column = Mock(spec=MatchQualityColumn) + def test_treeitem_handles_delegate_column(self, mock_album: Mock) -> None: + """Test that TreeItem properly handles DelegateColumn.""" + # Create a mock delegate column + column = Mock(spec=DelegateColumn) column.size = QtCore.QSize(16, 16) # Create a mock item @@ -593,7 +755,7 @@ def test_treeitem_handles_match_quality_column(self, mock_album: Mock) -> None: # Mock the update_colums_text method with patch.object(TreeItem, "update_colums_text"): # Simulate the condition in update_colums_text - if isinstance(column, MatchQualityColumn): + if isinstance(column, DelegateColumn): item.setSizeHint(1, column.size) # Verify setSizeHint was called @@ -670,106 +832,3 @@ def test_cluster_item_formatting(self) -> None: with patch.object(ClusterItem, "columns", mock_columns): item = ClusterItem(cluster, parent=None) assert item is not None - - -class TestMatchQualitySorting: - """Test match quality sorting behavior.""" - - @pytest.fixture - def mock_album_loading(self): - """Create a mock album that is still loading.""" - album = Mock() - album.status = AlbumStatus.LOADING - album.get_num_matched_tracks.return_value = 0 - album.tracks = [] - return album - - @pytest.fixture - def mock_album_loaded(self): - """Create a mock album that has finished loading.""" - album = Mock() - album.status = AlbumStatus.LOADED - album.get_num_matched_tracks.return_value = 3 - album.tracks = [Mock(), Mock(), Mock(), Mock(), Mock()] # 5 tracks total - return album - - @pytest.fixture - def mock_track(self): - """Create a mock track object.""" - track = Mock() - # Track objects don't have get_num_matched_tracks or tracks attributes - # We'll use patch to mock hasattr behavior - return track - - def test_sortkey_match_quality_loading_album(self, mock_album_loading): - """Test that loading albums return 0.0 to avoid premature sorting.""" - result = _sortkey_match_quality(mock_album_loading) - assert result == 0.0 - - def test_sortkey_match_quality_loaded_album(self, mock_album_loaded): - """Test that loaded albums return correct match percentage.""" - result = _sortkey_match_quality(mock_album_loaded) - # 3 matched out of 5 total = 0.6 - expected = _apply_platform_multiplier(0.6) - assert result == expected - - def test_sortkey_match_quality_track_object(self, mock_track): - """Test that track objects return 0.0.""" - # Mock hasattr to return False for track objects - with patch("builtins.hasattr", side_effect=lambda obj, attr: attr not in ("get_num_matched_tracks", "tracks")): - result = _sortkey_match_quality(mock_track) - assert result == 0.0 - - def test_sortkey_match_quality_no_tracks(self, mock_album_loaded): - """Test that albums with no tracks return 0.0.""" - mock_album_loaded.tracks = [] - result = _sortkey_match_quality(mock_album_loaded) - assert result == 0.0 - - def test_sortkey_match_quality_all_matched(self, mock_album_loaded): - """Test that albums with all tracks matched return correct value.""" - mock_album_loaded.get_num_matched_tracks.return_value = 5 - result = _sortkey_match_quality(mock_album_loaded) - expected = _apply_platform_multiplier(1.0) - assert result == expected - - def test_sortkey_match_quality_no_matches(self, mock_album_loaded): - """Test that albums with no matches return 0.0.""" - mock_album_loaded.get_num_matched_tracks.return_value = 0 - result = _sortkey_match_quality(mock_album_loaded) - assert result == 0.0 - - def test_sortkey_match_quality_partial_matches(self, mock_album_loaded): - """Test that albums with partial matches return correct percentage.""" - mock_album_loaded.get_num_matched_tracks.return_value = 2 - result = _sortkey_match_quality(mock_album_loaded) - # 2 matched out of 5 total = 0.4 - expected = _apply_platform_multiplier(0.4) - assert result == expected - - def test_sortkey_match_quality_no_status_attribute(self): - """Test that objects without status attribute are handled gracefully.""" - obj = Mock() - obj.get_num_matched_tracks.return_value = 2 - obj.tracks = [Mock(), Mock(), Mock()] # 3 tracks - # No status attribute - result = _sortkey_match_quality(obj) - # Should calculate normally: 2/3 = 0.666... - expected = _apply_platform_multiplier(0.6666666666666666) - assert result == pytest.approx(expected, rel=1e-10) - - def test_sortkey_match_quality_error_status(self, mock_album_loaded): - """Test that albums with error status are handled correctly.""" - mock_album_loaded.status = AlbumStatus.ERROR - result = _sortkey_match_quality(mock_album_loaded) - # Should still calculate the match percentage - expected = _apply_platform_multiplier(0.6) - assert result == expected - - def test_sortkey_match_quality_none_status(self, mock_album_loaded): - """Test that albums with None status are handled correctly.""" - mock_album_loaded.status = None - result = _sortkey_match_quality(mock_album_loaded) - # Should still calculate the match percentage - expected = _apply_platform_multiplier(0.6) - assert result == expected diff --git a/test/test_ui_columns.py b/test/test_ui_columns.py index 051901b584..74ca9dfbf5 100644 --- a/test/test_ui_columns.py +++ b/test/test_ui_columns.py @@ -29,7 +29,7 @@ DefaultColumn, ImageColumn, ) -from picard.ui.itemviews.columns import IconColumn +from picard.ui.itemviews.custom_columns import IconColumn class ColumnTest(PicardTestCase): @@ -68,9 +68,18 @@ def test_default_column(self): self.assertTrue(column.is_default) def test_icon_column(self): - column = IconColumn('title', 'key') + class _Provider: + def __init__(self): + self._icon = None + + def get_icon(self): + if self._icon is None: + self._icon = 'icon' + return self._icon + + provider = _Provider() + column = IconColumn('title', 'key', provider) self.assertIsInstance(column, ImageColumn) - column.header_icon_func = lambda: 'icon' self.assertEqual(column.header_icon, 'icon') column.set_header_icon_size(10, 20, 2) self.assertEqual(column.header_icon_size.width(), 10) From c6f7cbc330958c86f743bca257f2e9f54166bca7 Mon Sep 17 00:00:00 2001 From: Khoa Nguyen Date: Mon, 22 Sep 2025 11:51:11 -0400 Subject: [PATCH 2/5] fix(fileview): preserve 'Unclustered Files' title while hiding custom values on group rows\n\nHide custom column values for and special rows except for the Title and status icon columns. This prevents showing on group rows while keeping the label visible after the refactor. --- picard/ui/itemviews/__init__.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/picard/ui/itemviews/__init__.py b/picard/ui/itemviews/__init__.py index 12365f7cf1..3e2e07b59f 100644 --- a/picard/ui/itemviews/__init__.py +++ b/picard/ui/itemviews/__init__.py @@ -55,7 +55,7 @@ from picard import log from picard.album import NatAlbum -from picard.cluster import ClusterList, UnclusteredFiles +from picard.cluster import Cluster, ClusterList from picard.file import ( File, FileErrorType, @@ -421,11 +421,14 @@ def update_colums_text(self, color=None, bgcolor=None): 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 - # display per-entity values. - if isinstance(self.obj, ClusterList | UnclusteredFiles): + # Hide custom column values for container/group rows, but preserve Title and status icon. + # - ClusterList: Represents the "Clusters" root. Title is set elsewhere. + # - Special Cluster instances (e.g. "Unclustered Files"): Should show their Title + # but no other per-entity values in custom columns. + is_group_row = isinstance(self.obj, ClusterList) or ( + isinstance(self.obj, Cluster) and getattr(self.obj, 'special', False) + ) + if is_group_row and (column.key != 'title' and not column.status_icon): self.setText(i, "") continue From 2a1e93b0bf8c85940fd3c72945ecc77cd934fb70 Mon Sep 17 00:00:00 2001 From: knguyen Date: Sun, 28 Sep 2025 16:24:57 -0400 Subject: [PATCH 3/5] Run ruff/lint --- picard/ui/itemviews/custom_columns/column.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/picard/ui/itemviews/custom_columns/column.py b/picard/ui/itemviews/custom_columns/column.py index a4773546d4..e2df9d1221 100644 --- a/picard/ui/itemviews/custom_columns/column.py +++ b/picard/ui/itemviews/custom_columns/column.py @@ -89,6 +89,7 @@ def __init__( status_icon=status_icon, ) self.provider = provider + def invalidate_cache(self, obj=None): # pragma: no cover - UI-driven """Invalidate any caches on the provider if supported. @@ -101,6 +102,7 @@ def invalidate_cache(self, obj=None): # pragma: no cover - UI-driven # type: ignore[attr-defined] self.provider.invalidate(obj) # noqa: PGH003 + class DelegateColumn(Column): """A column that uses a delegate for custom rendering and optional sorting.""" From 21920ffe8e626d440bf2dbaec6fb7062a90cbf1d Mon Sep 17 00:00:00 2001 From: knguyen Date: Mon, 29 Sep 2025 06:28:58 -0400 Subject: [PATCH 4/5] refactor: enhance custom column creation with is_default parameter - Introduced `is_default` parameter in column creation functions to specify default visibility. - Updated `create_common_columns` to utilize the new parameter for standard columns. - Improved documentation for column creation functions to clarify parameter usage. --- .../custom_columns/common_columns.py | 13 ++--- picard/ui/itemviews/custom_columns/factory.py | 53 +++++++++++++++---- 2 files changed, 48 insertions(+), 18 deletions(-) diff --git a/picard/ui/itemviews/custom_columns/common_columns.py b/picard/ui/itemviews/custom_columns/common_columns.py index d75496e731..a696f132f8 100644 --- a/picard/ui/itemviews/custom_columns/common_columns.py +++ b/picard/ui/itemviews/custom_columns/common_columns.py @@ -20,14 +20,12 @@ """Factories for creating standard built-in columns (match, file, etc.).""" -from __future__ import annotations - from PyQt6 import QtCore from picard.i18n import N_ from picard.util import icontheme -from picard.ui.columns import ColumnAlign, ColumnSortType +from picard.ui.columns import Column, ColumnAlign, ColumnSortType from picard.ui.itemviews.custom_columns import make_delegate_column, make_field_column, make_numeric_field_column from picard.ui.itemviews.custom_columns.factory import make_icon_header_column from picard.ui.itemviews.custom_columns.providers import LazyHeaderIconProvider @@ -83,7 +81,7 @@ def create_fingerprint_status_column(): return column -def create_common_columns(): +def create_common_columns() -> tuple[Column, ...]: """Create the built-in common columns using factories. Returns @@ -99,8 +97,8 @@ def create_common_columns(): width=250, always_visible=True, status_icon=True, + is_default=True, ) - title_col.is_default = True # Length with numeric sort key from metadata.length length_col = make_numeric_field_column( @@ -109,12 +107,11 @@ def create_common_columns(): parse_time_format, width=50, align=ColumnAlign.RIGHT, + is_default=True, ) - length_col.is_default = True # Artist - artist_col = make_field_column(N_("Artist"), 'artist', width=200) - artist_col.is_default = True + artist_col = make_field_column(N_("Artist"), 'artist', width=200, is_default=True) # Others (mostly field columns) album_artist = make_field_column(N_("Album Artist"), 'albumartist') diff --git a/picard/ui/itemviews/custom_columns/factory.py b/picard/ui/itemviews/custom_columns/factory.py index 56a35ba5ef..768ee06a2a 100644 --- a/picard/ui/itemviews/custom_columns/factory.py +++ b/picard/ui/itemviews/custom_columns/factory.py @@ -81,6 +81,7 @@ def _create_custom_column( always_visible: bool = False, sort_type: ColumnSortType | None = None, status_icon: bool = False, + is_default: bool = False, ) -> CustomColumn: """Create `CustomColumn`. @@ -99,7 +100,7 @@ def _create_custom_column( # downgrade to TEXT to avoid invalid configuration if inferred_sort_type == ColumnSortType.SORTKEY and not isinstance(provider, SortKeyProvider): inferred_sort_type = ColumnSortType.TEXT - return CustomColumn( + column = CustomColumn( title, key, provider, @@ -109,6 +110,8 @@ def _create_custom_column( always_visible=always_visible, status_icon=status_icon, ) + column.is_default = is_default + return column def make_field_column( @@ -120,18 +123,33 @@ def make_field_column( always_visible: bool = False, sort_type: ColumnSortType = ColumnSortType.TEXT, status_icon: bool = False, + is_default: bool = False, ) -> CustomColumn: - """Create column that displays a field via `obj.column(key)`. + """Create a column that displays a field via ``obj.column(key)``. Parameters ---------- - title, key, width, align, always_visible - Column configuration. + title : str + Display title of the column. + key : str + Internal key used to identify the column and field source. + width : int | None, optional + Fixed width in pixels. If ``None``, uses default behavior. + align : ColumnAlign, default ``ColumnAlign.LEFT`` + Text alignment for the cell contents. + always_visible : bool, default ``False`` + If ``True``, hides the column visibility toggle in the UI. + sort_type : ColumnSortType, default ``ColumnSortType.TEXT`` + Sorting behavior for the column. + status_icon : bool, default ``False`` + If ``True``, marks this as the single status icon column. + is_default : bool, default ``False`` + If ``True``, marks this column as visible by default. Returns ------- CustomColumn - The field column. + The configured field column. """ provider = FieldReferenceProvider(key) return _create_custom_column( @@ -143,6 +161,7 @@ def make_field_column( always_visible=always_visible, sort_type=sort_type, status_icon=status_icon, + is_default=is_default, ) @@ -155,21 +174,34 @@ def make_numeric_field_column( align: ColumnAlign = ColumnAlign.LEFT, always_visible: bool = False, status_icon: bool = False, + is_default: bool = False, ) -> CustomColumn: - """Create column that displays a field with numeric sorting. + """Create a field column with numeric sorting support. Parameters ---------- - title, key, width, align, always_visible, status_icon - Column configuration. + title : str + Display title of the column. + key : str + Internal key used to identify the column and field source. parser : Callable[[str], float] | None, optional Function to parse the field value to a numeric value for sorting. - If None, uses the default float() parser. + If ``None``, uses the default ``float()`` parser. + width : int | None, optional + Fixed width in pixels. If ``None``, uses default behavior. + align : ColumnAlign, default ``ColumnAlign.LEFT`` + Text alignment for the cell contents. + always_visible : bool, default ``False`` + If ``True``, hides the column visibility toggle in the UI. + status_icon : bool, default ``False`` + If ``True``, marks this as the single status icon column. + is_default : bool, default ``False`` + If ``True``, marks this column as visible by default. Returns ------- CustomColumn - The numeric field column with proper sorting. + The configured numeric field column with proper sorting. """ base_provider = FieldReferenceProvider(key) numeric_provider = NumericSortAdapter(base_provider, parser=parser) @@ -182,6 +214,7 @@ def make_numeric_field_column( always_visible=always_visible, sort_type=ColumnSortType.SORTKEY, status_icon=status_icon, + is_default=is_default, ) From 1dde850a2f743673ccd87985bc7fbd5fe8d82635 Mon Sep 17 00:00:00 2001 From: Khoa Nguyen Date: Tue, 30 Sep 2025 01:41:30 -0400 Subject: [PATCH 5/5] refactor: consolidate column creation logic into columns.py - Moved column creation functions from common_columns.py to columns.py for better organization. - Introduced `create_match_quality_column` and `create_fingerprint_status_column` functions. - Updated `create_common_columns` to utilize new column creation methods. - Removed the now redundant common_columns.py file. - Adjusted tests to reflect changes in column imports. --- picard/ui/itemviews/columns.py | 161 +++++++++++++++- .../custom_columns/common_columns.py | 180 ------------------ test/test_itemviews_match_quality.py | 2 +- 3 files changed, 159 insertions(+), 184 deletions(-) delete mode 100644 picard/ui/itemviews/custom_columns/common_columns.py diff --git a/picard/ui/itemviews/columns.py b/picard/ui/itemviews/columns.py index 987ca26ab3..f165751670 100644 --- a/picard/ui/itemviews/columns.py +++ b/picard/ui/itemviews/columns.py @@ -43,14 +43,169 @@ # along with this program; if not, write to the Free Software # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +from PyQt6 import QtCore + +from picard.i18n import N_ +from picard.util import icontheme from picard.ui.columns import ( + Column, + ColumnAlign, Columns, + ColumnSortType, ) -from picard.ui.itemviews.custom_columns.common_columns import ( - create_common_columns, - create_match_quality_column, +from picard.ui.itemviews.custom_columns import make_delegate_column, make_field_column, make_numeric_field_column +from picard.ui.itemviews.custom_columns.factory import make_icon_header_column +from picard.ui.itemviews.custom_columns.providers import LazyHeaderIconProvider +from picard.ui.itemviews.custom_columns.sorting_adapters import NumericSortAdapter +from picard.ui.itemviews.custom_columns.utils import ( + parse_bitrate, + parse_file_size, + parse_time_format, ) +from picard.ui.itemviews.match_quality_column import MatchQualityProvider + + +def create_match_quality_column(): + """Create a match quality delegate column with proper sorting. + + Returns + ------- + DelegateColumn + The configured match quality column. + """ + base_provider = MatchQualityProvider() + sorter = NumericSortAdapter(base_provider) + column = make_delegate_column( + N_("Match"), + '~match_quality', + base_provider, + width=57, + sort_type=ColumnSortType.SORTKEY, + size=QtCore.QSize(16, 16), + sort_provider=sorter, + ) + column.is_default = True + return column + + +def create_fingerprint_status_column(): + """Create the fingerprint status icon header column. + + Returns + ------- + IconColumn + The configured fingerprint status column. + """ + provider = LazyHeaderIconProvider(lambda: icontheme.lookup('fingerprint-gray', icontheme.ICON_SIZE_MENU)) + column = make_icon_header_column( + N_("Fingerprint status"), + '~fingerprint', + provider, + icon_width=16, + icon_height=16, + border=1, + ) + return column + + +def create_common_columns() -> tuple[Column, ...]: + """Create the built-in common columns using factories. + + Returns + ------- + tuple + Tuple of configured column objects for both views. + """ + # Title (status icon column) + title_col = make_field_column( + N_("Title"), + 'title', + sort_type=ColumnSortType.NAT, + width=250, + always_visible=True, + status_icon=True, + is_default=True, + ) + + # Length with numeric sort key from metadata.length + length_col = make_numeric_field_column( + N_("Length"), + '~length', + parse_time_format, + width=50, + align=ColumnAlign.RIGHT, + is_default=True, + ) + + # Artist + artist_col = make_field_column(N_("Artist"), 'artist', width=200, is_default=True) + + # Others (mostly field columns) + album_artist = make_field_column(N_("Album Artist"), 'albumartist') + composer = make_field_column(N_("Composer"), 'composer') + album = make_field_column(N_("Album"), 'album', sort_type=ColumnSortType.NAT) + discsubtitle = make_field_column(N_("Disc Subtitle"), 'discsubtitle', sort_type=ColumnSortType.NAT) + trackno = make_field_column(N_("Track No."), 'tracknumber', align=ColumnAlign.RIGHT, sort_type=ColumnSortType.NAT) + discno = make_field_column(N_("Disc No."), 'discnumber', align=ColumnAlign.RIGHT, sort_type=ColumnSortType.NAT) + catalognumber = make_field_column(N_("Catalog No."), 'catalognumber', sort_type=ColumnSortType.NAT) + barcode = make_field_column(N_("Barcode"), 'barcode') + media = make_field_column(N_("Media"), 'media') + + # Size with numeric sort key + size_col = make_numeric_field_column( + N_("Size"), + '~filesize', + parse_file_size, + align=ColumnAlign.RIGHT, + ) + + # File Type + filetype = make_field_column(N_("File Type"), '~format', width=120) + + # Bitrate + bitrate = make_numeric_field_column( + N_("Bitrate"), + '~bitrate', + parse_bitrate, + width=80, + align=ColumnAlign.RIGHT, + ) + + genre = make_field_column(N_("Genre"), 'genre') + + fingerprint = create_fingerprint_status_column() + + date = make_field_column(N_("Date"), 'date') + originaldate = make_field_column(N_("Original Release Date"), 'originaldate') + releasedate = make_field_column(N_("Release Date"), 'releasedate') + cover = make_field_column(N_("Cover"), 'covercount') + coverdims = make_field_column(N_("Cover Dimensions"), 'coverdimensions') + + return ( + title_col, + length_col, + artist_col, + album_artist, + composer, + album, + discsubtitle, + trackno, + discno, + catalognumber, + barcode, + media, + size_col, + filetype, + bitrate, + genre, + fingerprint, + date, + originaldate, + releasedate, + cover, + coverdims, + ) # Common columns used by both views diff --git a/picard/ui/itemviews/custom_columns/common_columns.py b/picard/ui/itemviews/custom_columns/common_columns.py deleted file mode 100644 index a696f132f8..0000000000 --- a/picard/ui/itemviews/custom_columns/common_columns.py +++ /dev/null @@ -1,180 +0,0 @@ -# -*- 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 GNU General Public License 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. - -"""Factories for creating standard built-in columns (match, file, etc.).""" - -from PyQt6 import QtCore - -from picard.i18n import N_ -from picard.util import icontheme - -from picard.ui.columns import Column, ColumnAlign, ColumnSortType -from picard.ui.itemviews.custom_columns import make_delegate_column, make_field_column, make_numeric_field_column -from picard.ui.itemviews.custom_columns.factory import make_icon_header_column -from picard.ui.itemviews.custom_columns.providers import LazyHeaderIconProvider -from picard.ui.itemviews.custom_columns.sorting_adapters import NumericSortAdapter -from picard.ui.itemviews.custom_columns.utils import ( - parse_bitrate, - parse_file_size, - parse_time_format, -) -from picard.ui.itemviews.match_quality_column import MatchQualityProvider - - -def create_match_quality_column(): - """Create a match quality delegate column with proper sorting. - - Returns - ------- - DelegateColumn - The configured match quality column. - """ - base_provider = MatchQualityProvider() - sorter = NumericSortAdapter(base_provider) - column = make_delegate_column( - N_("Match"), - '~match_quality', - base_provider, - width=57, - sort_type=ColumnSortType.SORTKEY, - size=QtCore.QSize(16, 16), - sort_provider=sorter, - ) - column.is_default = True - return column - - -def create_fingerprint_status_column(): - """Create the fingerprint status icon header column. - - Returns - ------- - IconColumn - The configured fingerprint status column. - """ - provider = LazyHeaderIconProvider(lambda: icontheme.lookup('fingerprint-gray', icontheme.ICON_SIZE_MENU)) - column = make_icon_header_column( - N_("Fingerprint status"), - '~fingerprint', - provider, - icon_width=16, - icon_height=16, - border=1, - ) - return column - - -def create_common_columns() -> tuple[Column, ...]: - """Create the built-in common columns using factories. - - Returns - ------- - tuple - Tuple of configured column objects for both views. - """ - # Title (status icon column) - title_col = make_field_column( - N_("Title"), - 'title', - sort_type=ColumnSortType.NAT, - width=250, - always_visible=True, - status_icon=True, - is_default=True, - ) - - # Length with numeric sort key from metadata.length - length_col = make_numeric_field_column( - N_("Length"), - '~length', - parse_time_format, - width=50, - align=ColumnAlign.RIGHT, - is_default=True, - ) - - # Artist - artist_col = make_field_column(N_("Artist"), 'artist', width=200, is_default=True) - - # Others (mostly field columns) - album_artist = make_field_column(N_("Album Artist"), 'albumartist') - composer = make_field_column(N_("Composer"), 'composer') - album = make_field_column(N_("Album"), 'album', sort_type=ColumnSortType.NAT) - discsubtitle = make_field_column(N_("Disc Subtitle"), 'discsubtitle', sort_type=ColumnSortType.NAT) - trackno = make_field_column(N_("Track No."), 'tracknumber', align=ColumnAlign.RIGHT, sort_type=ColumnSortType.NAT) - discno = make_field_column(N_("Disc No."), 'discnumber', align=ColumnAlign.RIGHT, sort_type=ColumnSortType.NAT) - catalognumber = make_field_column(N_("Catalog No."), 'catalognumber', sort_type=ColumnSortType.NAT) - barcode = make_field_column(N_("Barcode"), 'barcode') - media = make_field_column(N_("Media"), 'media') - - # Size with numeric sort key - size_col = make_numeric_field_column( - N_("Size"), - '~filesize', - parse_file_size, - align=ColumnAlign.RIGHT, - ) - - # File Type - filetype = make_field_column(N_("File Type"), '~format', width=120) - - # Bitrate - bitrate = make_numeric_field_column( - N_("Bitrate"), - '~bitrate', - parse_bitrate, - width=80, - align=ColumnAlign.RIGHT, - ) - - genre = make_field_column(N_("Genre"), 'genre') - - fingerprint = create_fingerprint_status_column() - - date = make_field_column(N_("Date"), 'date') - originaldate = make_field_column(N_("Original Release Date"), 'originaldate') - releasedate = make_field_column(N_("Release Date"), 'releasedate') - cover = make_field_column(N_("Cover"), 'covercount') - coverdims = make_field_column(N_("Cover Dimensions"), 'coverdimensions') - - return ( - title_col, - length_col, - artist_col, - album_artist, - composer, - album, - discsubtitle, - trackno, - discno, - catalognumber, - barcode, - media, - size_col, - filetype, - bitrate, - genre, - fingerprint, - date, - originaldate, - releasedate, - cover, - coverdims, - ) diff --git a/test/test_itemviews_match_quality.py b/test/test_itemviews_match_quality.py index b5dab24ec2..0581e95da2 100644 --- a/test/test_itemviews_match_quality.py +++ b/test/test_itemviews_match_quality.py @@ -29,8 +29,8 @@ import pytest from picard.ui.itemviews import TreeItem +from picard.ui.itemviews.columns import create_match_quality_column from picard.ui.itemviews.custom_columns import DelegateColumn -from picard.ui.itemviews.custom_columns.common_columns import create_match_quality_column from picard.ui.itemviews.match_quality_column import MatchQualityColumnDelegate, MatchQualityProvider