-
-
Notifications
You must be signed in to change notification settings - Fork 412
PICARD-442: Add match quality column and allow sorting #2696
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
PICARD-442: Add match quality column and allow sorting #2696
Conversation
FYI.. changes unrelated to the feature is due to |
@@ -91,6 +91,7 @@ | |||
from picard.ui.collectionmenu import CollectionMenu | |||
from picard.ui.enums import MainAction | |||
from picard.ui.filter import Filter | |||
from picard.ui.itemviews.match_quality_column import MatchQualityColumn, MatchQualityColumnDelegate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the new style for import
formatting because of ruff? Wasn't our standard to format multiple imports as a tuple with each import (tuple item) on a separate line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This is from the pre-commit
hook ruff format
, the rule is governed by:
pyproject.toml:157-157
line-length = 120
This particularly line is 100 chars long, so ruff/black
will prefer to keep it on one line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but there was an isort rule force_grid_wrap = 2
(along with other formatting rules) in the pyproject.toml
file that differ from the current ruff configuration. Thus the reason for my question, because the include
style is now different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ruff prefers black
style (can't change it). That's another thing noticed that was missing from the previous isort
configuration.
Anyway that's a pyproject.toml
/formatting change, unrelated to this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ruff prefers
black
style (can't change it).
I see that, and it appears that the ruff development team have no intention of supporting isort formatting. That's unfortunate because the way that we used to format the imports made a lot more sense (in my opinion) when reviewing git diffs.
Anyway that's a
pyproject.toml
/formatting change, unrelated to this PR.
Agreed. It's just that this PR happens to be where I noticed the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should review changes (in a separate PR) after running ruff format
. I just did a test on current master branch: 273 files changed, 5013 insertions(+), 4555 deletions(-)
We need to ensure format is correct and no issue is created by running ruff format
.
Also we need to update documentation about coding style if we go this way.
I would apply those changes first, and get rid of stylistic changes in PRs. I mean if we agree to use black
style, I'm not totally happy with it style-wise, but ruff format
is very convenient and fast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally agree with @zas. I'm also not really happy with the black style. The changing of import style based on line length is rather annoying and can lead to unnecessary and hard to read changed lines in diffs rather randomly. We had quite lengthy discussions about this when we introduced isort managing the imports, and the current system of having multiple imports on separate lines worked really well.
But if we really can't change it I would prefer the convenience of ruff format
over my personal taste.
We should do a complete formatting run in a separate PR and check the result. Maybe there is more changes that we need to discuss and probably tweak one way or another.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phw has summarized my thoughts exactly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should review changes (in a separate PR) after running ruff format. I just did a test on current master branch: 273 files changed, 5013 insertions(+), 4555 deletions(-)
We need to ensure format is correct and no issue is created by running ruff format.
Yes. I noted this in this PR comment: #2690 (comment)
I also suggest the same: We need a massive ruff format
refactor so that (a) we can add ruff format --check
to the CI pipelines and (b) so that we can avoid seeing so much non-PR related formatting noise in future PR's.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran ruff format
on current master branch, and created a PR for us to review changes: #2697
There was a problem hiding this 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.
I think that would be my preference as well, since all the details are available via the tooltip.
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. |
Mine too, as all infos are available elsewhere (tooltip and/or title). |
+1 for both of those from me, too |
This commit: 3f7260f
![]() |
Just tested on Linux and confirmed. The sorting order icon seems reversed on linux. Commit: 3039a01 |
Summary
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
MatchQualityColumn
class that displays match statistics at the album levelMatchQualityColumnDelegate
) for rendering icons and textAction
Additional actions required: