Skip to content

Conversation

knguyen1
Copy link
Contributor

@knguyen1 knguyen1 commented Aug 2, 2025

Summary

  • This is a…
    • Bug fix
    • Feature addition
    • Refactoring
    • Minor / simple change (like a typo)
    • Other
  • Describe this change in 1-2 sentences: Adds a new "Match Quality" column to the album view that displays match statistics using visual icons and text, showing matched/total tracks, missing tracks, duplicates, and extra files.

Problem

Users need a quick visual way to assess the quality of their file-to-track matches in albums. Currently, users must manually inspect each track to understand match status, which is time-consuming and error-prone.

Solution

  • Added MatchQualityColumn class that displays match statistics at the album level
  • Implemented custom delegate (MatchQualityColumnDelegate) for rendering icons and text
  • Column shows format: "matched/total; missing; duplicates; extra; unmatched"
  • Uses existing match icons from FileItem for visual consistency
  • Added re-sort trigger in Album._finalize_loading() to ensure accurate sorting after album loads
  • Column integrates with existing item views system and respects sorting/filtering
image

Action

Additional actions required:

@knguyen1
Copy link
Contributor Author

knguyen1 commented Aug 2, 2025

FYI.. changes unrelated to the feature is due to ruff format.

@zas zas requested review from phw, zas and rdswift August 2, 2025 07:09
Copy link
Collaborator

@rdswift rdswift left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks fine to me.

For clarification, I assume this new column is intended to augment (not replace) the current Album / Release status icon. In that case, it might be better positioned in a column adjacent to the current Album / Release status icon.

My other concern is with the width of this column -- always a minimum of 200 pixels (if I read the code correctly). Since the details are available for display as a tooltip on this new Match Quality icon, perhaps we can just display the icon and not the summary numbers (which can be viewed from the tooltip). This would save some of the limited available display space.

Finally, since this changes the standard display for the user, I believe that the documentation should be updated (at least the Album / Release Icons section, and possibly the images on the Main Screen page). I'll add a sub-task ticket for this so it doesn't get forgotten.

@knguyen1
Copy link
Contributor Author

knguyen1 commented Aug 3, 2025

For clarification, I assume this new column is intended to augment (not replace) the current Album / Release status icon. In that case, it might be better positioned in a column adjacent to the current Album / Release status icon.

Need to make some decisions here re: match/total; missing; duplicates; extra; unmatched details in the col. My preference is only the icon.

  • Display the full details (match/total; missing; duplicates; extra; unmatched) in the column?
image
  • Only matched/total?
image
  • Only the icon?
image

Also matched/total is available in the Title column already anyway. So have to make a decision there too...

And if we decide on only the icon, I suggest we rename the column -> Match.

@rdswift
Copy link
Collaborator

rdswift commented Aug 3, 2025

My preference is only the icon.

I think that would be my preference as well, since all the details are available via the tooltip.

And if we decide on only the icon, I suggest we rename the column -> Match.

That would help reduce the space required. Would it be possible to leave out the column name completely (and only have the sort direction indicator if required)? I'm thinking that it being next to the status icon might make it clear that it is the quality of the match. Then again, it might not be clear to other users.

@zas
Copy link
Collaborator

zas commented Aug 4, 2025

My preference is only the icon.

Mine too, as all infos are available elsewhere (tooltip and/or title).

@phw
Copy link
Member

phw commented Aug 4, 2025

My preference is only the icon.

And if we decide on only the icon, I suggest we rename the column -> Match.

+1 for both of those from me, too

@knguyen1
Copy link
Contributor Author

knguyen1 commented Aug 5, 2025

My preference is only the icon.

And if we decide on only the icon, I suggest we rename the column -> Match.

+1 for both of those from me, too

This commit: 3f7260f

  • Pull column to left, next to Title/Length/Artist
  • Rename column -> Match
  • Column always shows
  • Remove text from column
  • Column sort order seems to be different on linux vs. Windows (will confirm)
image

@knguyen1
Copy link
Contributor Author

knguyen1 commented Aug 5, 2025

Column sort order seems to be different on linux vs. Windows (will confirm)

Just tested on Linux and confirmed. The sorting order icon seems reversed on linux. Commit: 3039a01

@zas
Copy link
Collaborator

zas commented Aug 7, 2025

Having Match column for middle pane is a bit weird.

image

@zas
Copy link
Collaborator

zas commented Aug 10, 2025

@knguyen1 Overall the PR looks good, but UI wise there's likely few improvements we can make. Anyway, please resolve conflicts.

@phw
Copy link
Member

phw commented Aug 10, 2025

I think the match column on the right works fine. But as zas pointed out it would be great if we could have no Match column, just the title, on the middle pane.

@knguyen1 knguyen1 force-pushed the feat/add-match-quality-column branch from 3039a01 to c6b5abb Compare August 12, 2025 00:26
@knguyen1
Copy link
Contributor Author

Rebased and...

  • Split columns: introduced _common_columns, FILEVIEW_COLUMNS (no match column) and ALBUMVIEW_COLUMNS (includes match column).
  • Match column placement: _match_quality_column is now in ALBUMVIEW_COLUMNS after albumartist (index 4).
  • Removed legacy: deleted ITEMVIEW_COLUMNS and its usage.
  • View wiring: MainPanel now creates FileTreeView(FILEVIEW_COLUMNS) and AlbumTreeView(ALBUMVIEW_COLUMNS) instead of a single shared set.
  • Dynamic item columns: TreeItem.columns changed from a static class attribute to a @Property that returns treeWidget().columns; with a safe fallback to FILEVIEW_COLUMNS when not yet attached (prevents construction-time crashes).

If you've previously tested the code with Match all the way to the left, you'll need to temporarily disable restore_state.

# line 421 of picard\ui\itemviews\basetreeview.py
self.restore_state()  # <-- temporarily comment this out here
image

@phw
Copy link
Member

phw commented Aug 12, 2025

If you've previously tested the code with Match all the way to the left, you'll need to temporarily disable restore_state.

Do we maybe need a migration that resets the column state?

@knguyen1
Copy link
Contributor Author

Do we maybe need a migration that resets the column state?

We don't in my opinion. It affects people (us devs) who had a col of 85px width there. It causes the Title col to shrink to 85px.

@zas
Copy link
Collaborator

zas commented Aug 13, 2025

Overall looks good, I noticed we can't hide the Match column in context menu though.

image

@knguyen1
Copy link
Contributor Author

knguyen1 commented Aug 13, 2025

Overall looks good, I noticed we can't hide the Match column in context menu though.

Okay can make it hideable. this commit: 7fae0ed

Copy link
Collaborator

@zas zas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor change missed, but the whole patch looks good to me.

Copy link
Collaborator

@zas zas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, good work!

Copy link
Member

@phw phw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, code looks good and the match column is working great for me.

@phw phw merged commit c8cbe31 into metabrainz:master Aug 15, 2025
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants