PICARD-3119: refactor: standard columns to use new API #2739

knguyen1 · 2025-09-22T14:03:59Z

Summary

This is a…
- Bug fix
- Feature addition
- Refactoring
- Minor / simple change (like a typo)
- Other
Describe this change in 1-2 sentences: Refactor standard columns (common cols, size, bitrate, length, match quality) to use the new provider architecture, removing delegate adapter wrappers for cleaner implementation.

Problem

JIRA ticket (optional): PICARD-3119

Standard columns were using outdated delegate adapter patterns that added unnecessary complexity and didn't align with the new provider architecture.

Solution

Refactored standard columns to use the new provider architecture
Removed delegate adapter wrapper for cleaner implementation
Fixed size, bitrate, length columns to be field columns
Updated match quality column to use provider architecture
General cleanup and code improvements
Updated tests to reflect new column architecture

Action

Additional actions required:

Update Picard documentation (please include a reference to this PR)
Other (please specify below)

zas · 2025-09-22T15:11:29Z

This PR makes "Unclusted Files" title to disappear (compared to current master branch):

This branch:

Master:

knguyen1 · 2025-09-22T15:53:33Z

This PR makes "Unclusted Files" title to disappear (compared to current master branch):

That's actually from this #2714 (comment) and this change 4d05b75

And this is the fix: e95b182

zas

LGTM

rdswift · 2025-09-22T17:24:05Z

Are we actually going to manage these settings through the option profile system, or should I quit asking about it?

zas · 2025-09-23T07:22:33Z

Are we actually going to manage these settings through the option profile system, or should I quit asking about it?

What do you mean?
This PR is just about extending the use of CustomColumns, how does it relate to option profile system?

I mean I'm not against "manage these settings through the option profile system" but I fail to see how it is related to this PR.

rdswift · 2025-09-23T15:39:54Z

I've been asking for the custom columns to be managed through the option profile system right from the initial PR that introduced the functionality, but it keeps getting ignored. It's clear to me that @knguyen1 has no intention of ever doing this, so I guess I'll give up and quit asking.

zas · 2025-09-24T06:06:21Z

I've been asking for the custom columns to be managed through the option profile system right from the initial PR that introduced the functionality, but it keeps getting ignored. It's clear to me that @knguyen1 has no intention of ever doing this, so I guess I'll give up and quit asking.

@rdswift Can you clarify your expectations? Did you create ticket for that? Everything is still work in progress.

phw · 2025-09-24T14:50:12Z

@rdswift I don't think we have decided on making the custom columns part of the option profiles yet. If we decide to do this, we can do this in a separate PR. But I have stated my opinion elsewhere. To me the column configuration is about UI state, and as such does not really fit into the option profiles. It's hence also not fully clear which parts of the column configuration you would see in the profiles.

Only the configuration which custom columns are configured? But then this doesn't make sense if I switch profiles and have changing columns that are also part of the stored column header state (which includes the columns shown and the Qt state of their position, size and sort order). If we make the whole column state part of option profiles, will we also need to make window and pane size states part of the option profiles?

If there is a strong case for having the column state part of the option profiles and people really want this, then I'd be ok with having it implemented. It should just be clear that this is more involved then just making the custom_columns option part of the profiles.

phw · 2025-09-24T15:19:55Z

Just to elaborate on the problem I see in more detail:

The state of the header is currently stored in two options per pane, that are both considered state and are inside the "persist" config section:

(album_view|file_view)_header_state: This is an opaque value directly from QHeaderView. It encodes the state of which columns are visible, the order they are shown in, their size and their sort order.
(album_view|file_view)_header_locked: This is just a boolean, indicating whether the header is locked from changes

Previously the list of available columns was hard coded. Now we have another configuration, where the user can extend this list with custom columns. Those are stored in the "setting" config section as custom_columns. Now we consider this config, and we could consider this part of the option profiles then. This raises the following issues:

If I have different custom columns in different profiles and switch between them I'll break the header state. We would need to reset the header state. Which would be annoying to loose header state on each profile switch.
Or we make the header state also part of the profile. But it would be no good to allow the user to configure just the custom columns as part of the profile without the state. So we need a mechanism for users to select the columns being part of the profile, but then it actually affects multiple config keys, some of them in "persist".
Actually, if we allow storing the state in profile (which also holds the visible columns) why do we then need to have the custom_columns settings in the profile at all? Wouldn't it be nice to have columns globally, but only the state change between profiles?
If we store the header state, what about the rest of the view state (window size, pane sizes). After all my column sizes might depend on my view sizes as well.
If we do store all the view state, we need to update it all on profile switch.
What about all those other windows, that use similar logic (search dialogs etc.). Is even the pane size of the options dialog now subject to option profiles?

For me, option profiles are clearly about settings and should not also handle window state.

rdswift · 2025-09-24T16:07:41Z

As I mentioned elsewhere, which columns are displayed could be part of a user's workflow that is faciltated through a selected options profile. In any event, it seems that I'm alone in my request, so I'll drop it.

phw

This branch is now conflicting with master after the caching fixes have been merged. Otherwise looks good to me.

- 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

… 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.

knguyen1 · 2025-09-28T23:35:24Z

Rebased and resolved conflicts. MacOS tests passing locally.

picard/ui/itemviews/custom_columns/common_columns.py

- 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.

picard/ui/itemviews/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.

zas

LGTM

phw

Changes look good and work well for me. LGTM

zas requested review from phw, rdswift and zas September 22, 2025 15:02

zas previously approved these changes Sep 22, 2025

View reviewed changes

knguyen1 changed the title ~~refactor: standard columns to use new API~~ PICARD-3119: refactor: standard columns to use new API Sep 26, 2025

phw reviewed Sep 28, 2025

View reviewed changes

knguyen1 added 3 commits September 28, 2025 16:22

fix(fileview): preserve 'Unclustered Files' title while hiding custom…

c6f7cbc

… 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.

Run ruff/lint

2a1e93b

knguyen1 dismissed zas’s stale review via 2a1e93b September 28, 2025 20:25

knguyen1 force-pushed the refactor/PICARD-3119/refactor-standard-columns-to-use-new-api branch from e95b182 to 2a1e93b Compare September 28, 2025 20:25

zas requested changes Sep 29, 2025

View reviewed changes

picard/ui/itemviews/custom_columns/common_columns.py Outdated Show resolved Hide resolved

zas requested changes Sep 30, 2025

View reviewed changes

picard/ui/itemviews/columns.py Outdated Show resolved Hide resolved

zas approved these changes Sep 30, 2025

View reviewed changes

zas requested a review from phw September 30, 2025 22:10

phw approved these changes Oct 1, 2025

View reviewed changes

zas merged commit fec584a into metabrainz:master Oct 1, 2025
45 checks passed

Uh oh!

PICARD-3119: refactor: standard columns to use new API #2739

PICARD-3119: refactor: standard columns to use new API #2739

Uh oh!

Conversation

knguyen1 commented Sep 22, 2025

Summary

Problem

Solution

Action

Uh oh!

zas commented Sep 22, 2025

Uh oh!

knguyen1 commented Sep 22, 2025

Uh oh!

zas left a comment

Choose a reason for hiding this comment

Uh oh!

rdswift commented Sep 22, 2025

Uh oh!

zas commented Sep 23, 2025

Uh oh!

rdswift commented Sep 23, 2025

Uh oh!

zas commented Sep 24, 2025

Uh oh!

phw commented Sep 24, 2025

Uh oh!

phw commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rdswift commented Sep 24, 2025

Uh oh!

phw left a comment

Choose a reason for hiding this comment

Uh oh!

knguyen1 commented Sep 28, 2025

Uh oh!

Uh oh!

Uh oh!

zas left a comment

Choose a reason for hiding this comment

Uh oh!

phw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

phw commented Sep 24, 2025 •

edited

Loading