-
-
Notifications
You must be signed in to change notification settings - Fork 420
PICARD-2103: Add API for custom columns (field ref, scripts, transforms, etc.) #2709
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
PICARD-2103: Add API for custom columns (field ref, scripts, transforms, etc.) #2709
Conversation
4a1744d
to
b90d846
Compare
Failing lint is from: 03d5c9d Then I'll rebase |
|
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.
Can you rebase this PR?
0df8df5
to
1446569
Compare
|
Co-authored-by: Laurent Monin <[email protected]>
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.
Making columns configurable is a great addition, just few more stylistic comments, but overall it looks good to me.
I reverted Ukrainian translation change in master, since it breaks everything, but there's also a problem with tests unrelated to the reverted change. Tests run ok locally though. |
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.
Patch looks good to me, let's wait for @phw review
I don't think it was the translation file per se. Tests passed for that PR: https://github.com/metabrainz/picard/actions/runs/17271183163 . So not sure what changed. For other failures I suspect there is some issue with some tests not being fully independent. The tests in CI always run randomized. We already had this last week that tests sometimes failed and on retry worked again. We need to investigate and fix the tests. |
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.
Multiple imports on the same line. I thought our standard was to have each on its own line (using trailing commas).
What happens if multiple custom columns are registered with the same key? What happens if custom columns are registered with the same key as a standard column (e.g. 'title') that is referenced in another custom column'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.
Thanks for this. As I said before this is very welcomed addition. And it looks good in general, I have a couple of comments though. We should get this right so we can merge it and use it as the basis to review the other PR with the UI changes.
The code is a bit high on abstraction, but on itself each one makes sense.
@knguyen1 Tests on master should now be more stable. |
That's a good idea; can address it via a new PR after merge. The API will have to be expanded for image columns ( Converting all text columns to the custom API is straightforward. Sorting parity is achievable with existing adapters. Example conversions:
make_provider_column(N_("Album"),
"album",
FieldReferenceProvider("album"),
sort_type=ColumnSortType.NAT)
make_provider_column(
N_("Size"), "~filesize",
NumericSortAdapter(FieldReferenceProvider("~filesize")),
align=ColumnAlign.RIGHT
) It would help unify/simplify the code quite a bit. e.g. today there is the bespoke sorting key for filesize/bitrate: 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):
# percent logic ... Looks like this: |
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.
LGTM
@knguyen1It would be convenient if we could chat about Picard on matrix (see https://musicbrainz.org/doc/Communication/ChatBrainz). |
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.
Thanks for the updates. LGTM
Summary
Problem
Solution
picard.ui.itemviews.custom_columns
package:ColumnValueProvider
,SortKeyProvider
),CustomColumn
type.make_field_column
,make_script_column
,make_callable_column
,make_transformed_column
.registry
to register/unregister columns in File/Album views.ChainedValueProvider
) with caching/perf thresholds and album-loading cache avoidance.Test With
Append to
./picard/ui/itemviews/columns.py
:Then
Demo: See
Artist - Title
custom columnAction
Additional actions required:
Note: A follow-up PR is required for custom column management UX (create/edit/delete, ordering, persistence). See: #2714