PICARD-2103: Add UI to manage custom columns

[knguyen1]

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 UI to manage custom columns from the header menu: create, edit, duplicate, delete, place, and apply to File/Album views.

Problem

• JIRA ticket (optional): PICARD-2103

Users had no built-in UI to add/manage custom columns; changes required backend code or were not discoverable from the views.

Solution

• Header context menu: new “Manage Custom Columns…” entry (unlock columns if needed).
• Manager dialog:
  • List of custom columns with Type, Expression, Align, Width.
  • Actions: Add…, Edit…, Duplicate, Delete, “Make It So!” (apply).
• Expression dialog (per column):
  • Field Name, Type (Field or Script), Expression, Width, Align.
  • Add to views: File view / Album view.
  • Insert after key: position by existing column key (e.g. title, artist).
• UX details:
  • Immediate registration into live views on apply; closing applies pending changes.
  • Idempotent updates when editing or duplicating; safe deletion with confirmation.
  • Columns can also be toggled from the header menu like built-ins.

Demo

Action

Additional actions required:

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

Note: API PR #2709 must be merged first; this PR only covers the UI/UX management layer.

[rdswift]

I haven't looked at this in detail yet, but have some initial thoughts:

1. It may be better to just implement the api hooks, and leave the rest to plugins. [I just noticed that the API is in a different PR (PICARD-2103: Add API for custom columns (field ref, scripts, transforms, etc.) #2709).] If we are to include the "Custom Columns" dialog, it might be better to have this as a new option settings page under the UI section, rather than as a context menu. Custom column specs could be managed similarly to tagging scripts for definition, ordering and enable/disable selection.

2. Any custom column definitions (and activations) that have been stored by the user should be managed through the option profiles.

3. I'm not sure about the README files in the code tree (which seems more appropriate to a plugin). They do however provide information that should be in the Picard documentation (with some editing).

[rdswift]

I see that many of the files in this PR are also in the API PR #2709 (which you indicated must be merged first). This makes the review of this PR much more difficult because we're either reviewing the same changes twice (in this PR and PR #2709) or it isn't clear what changes in this PR will be applied to modify the changes in PR #2709.

My suggestion is that we wait until PR #2709 is merged, and this PR rebased before we review this PR in detail.

[zas]

Overall that's a good move, but that's also a lot of changes at once.

I didn't test (just reviewed code), I'll do later.
Also I wonder about how it fits with plugin V3 (we tried to regroup "API" access in picard/extension_points)

[knguyen1]

Overall that's a good move, but that's also a lot of changes at once.

I recommend reviewing and merging #2709 first. It'll reduce the number of files changed in #2714.

Plus, changes requested there would likely result in changes here.

[knguyen1]

@rdswift

Any custom column definitions (and activations) that have been stored by the user should be managed through the option profiles.

storage.py handles persistence: https://github.com/metabrainz/picard/blob/a2cde3daeb3accd48e9f44ab682ee06ad739ae08/picard/ui/itemviews/custom_columns/storage.py#L317C1-L322C19

And recall: https://github.com/metabrainz/picard/blob/a2cde3daeb3accd48e9f44ab682ee06ad739ae08/picard/ui/itemviews/custom_columns/storage.py#L405C1-L422C24

Persistence is saved to the global config object.

[knguyen1]

@rdswift

If we are to include the "Custom Columns" dialog, it might be better to have this as a new option settings page under the UI section, rather than as a context menu.

I disagree, as adding these menu/dialogs there to an already crowded Options/Settings page. The Manage Custom Columns... context menu feels cleaner. But feel free to put this to a vote and I'll be happy to move it to Options/Settings -> UI section.

[rdswift]

Persistence is saved to the global config object.

However, as currently handled it is not being managed through option profiles.

[rdswift]

If we are to include the "Custom Columns" dialog, it might be better to have this as a new option settings page under the UI section, rather than as a context menu.

I disagree, as adding these menu/dialogs there to an already crowded Options/Settings page. The Manage Custom Columns... context menu feels cleaner. But feel free to put this to a vote and I'll be happy to move it to Options/Settings -> UI section.

@zas / @phw what are your thoughts on this? My thinking is that by including these under Option Settings we can have a standardized approach (based on something like the tagger scripts page), the UI settings will be accessed in one place, and it will allow the Custom Columns settings to be managed via option profiles.

[knguyen1]

Persistence is saved to the global config object.

However, as currently handled it is not being managed through option profiles.

As-is, it's pretty seamless to the user (it's opaque to them where these settings get saved). What's the advantage of moving it to profiles? To the user? To the dev?

[rdswift]

Last night, instead of sleeping, I spent a few hours thinking about this UI and how I believe it should be set up. I offer the following for consideration:

1. The custom column definitions should be in a separate page (e.g. "Custom Columns for Lists") under the "User Interface" section of the option settings. This would be more consistent with the way other items are managed, and could be in a format similar to the "Scripting" options page. This should support adding, removing and editing custom column definitions, and setting their default display order.

2. The custom column definitions, including default sort order, should be stored in a config.setting[] item that is not managed by option profiles. My thinking is that all custom columns should be available for use in all profiles, in the same way that all file naming scripts are available to all profiles.

3. The custom column definitions page should use the ScriptTextEdit widget for script entry. This will allow for autocompletion, function and variable tooltip information, and the ability to confirm that the script is valid before saving.

4. The new "Custom Columns for Lists" options page should include the ability to select which custom columns are included for each of the "files" and "albums" lists, and the column display order for each list. This could be via a separate dialog for each of "files" and "albums", which displays a sortable list of all available columns with a checkbox on each to enable or disable the display of that column. My initial thoughts are that the selection and order settings should be stored in a separate config.setting[] item for each list, perhaps containing a list of the enabled columns in the selected display order.

5. The "files" and "albums" selected columns and column display order settings should be managed by option profiles. My thinking behind this is that a user may have a workflow that depends on a different set of settings in addition to different columns displayed in a different order, and switching to or from this collection of settings is quickly and easily done by enabling or disabling the appropriate option profile. That's one of the reasons that option profiles were created.

6. Similar to Item 4, the ability to select which custom columns are included for each of the "files" and "albums" lists, and the column display order for each list should be available via the context menu from each of the lists. Right clicking on the header of each list already displays a list of the available columns with the ability to display or hide each item. All of the defined custom columns should be included in this list. The columns display order can currently be modified by dragging a column header to the appropriate location (left or right), and this functionality should be retained but should also include the enabled custom columns. I don't believe that the current column selection and display order is managed through option profiles, however I believe that it should be.

I believe that this would provide the most flexibility while remaining consistent and intuitive in accessing the configuration. The learning curve and impact to the users should be minimal.

[knguyen1]

1. IMO: I think we should focus on MVP (minimum viable product) and save these for future enhancements. Happy to discuss/follow along with group consensus on this though.

2. I think it's helpful if you draw up some wireframes for these.

Also, imo column definitions should only be defined once per column/per use-case, in the spirit of DRY. Then they can/should be mapped to different profiles/views.

[phw]

Nice. Just a few initial comments after looking at the UI. I'll do a code review once #2709 has been merged.

• It's uncommon to have a "apply" and a "close" button in Picard. We should be consistent and have a button that saves the changes and closes the dialog and a cancel button that closes the dialog without saving changes.
• As a user I couldn't figure out how the "field" column type works. From reading the code I should be able to set one of those field names we use for the standard columns (which gets resolved by the column function of the underlying object), but this didn't give me results.
  In any case this is a bit difficult option, even if it seems simpler then the script option on first sight. The "column" names currently are an implementation detail. They can be exactly metadata names, but we also have special ones. Maybe if we offer such option we should provide the user with a select box for choosing a value. The values available probably would be a mix of default columns and known metadata tags.
• The alignment values in the UI should be lowercase "left" and "right" and be marked as translatable.
• Saving and applying the changes seems a bit wonky. Actual making new columns visible seems to require a restart. And I also managed it somehow to get all custom columns hidden again after editing the custom columns.
• I don't think we need the insert after input. The UI is confusing, and it exposes some internal implementation that does not necessarily reflect actual visual columns position. See my comment at #2709 (comment)

@zas / @phw what are your thoughts on this? My thinking is that by including these under Option Settings we can have a standardized approach (based on something like the tagger scripts page), the UI settings will be accessed in one place, and it will allow the Custom Columns settings to be managed via option profiles.

I actually like the approach here of adding it to the columns context menu. This is already used to customize columns displayed, together with other actions like resizing and reordering which is also done on the header.

I think we'll need to refactor the context menu a bit as one of the next steps, though. It has become quite long. We could add submenus and group the columns a bit. But that's something for a PR afterwards.

[rdswift]

Regardless of the UI, I still think it would be good to have the selected columns and position settings managed through the option profiles.

[knguyen1]

• Saving and applying the changes seems a bit wonky. Actual making new columns visible seems to require a restart.

@phw I struggled with this too. Attempts to make newly created columns in a "live" manner required touching registry.py which I did not want to do in this PR. I'll make more attempts once we merge #2709

[zas]

@knguyen1 now that #2709 was merged, please rebase this one

[knguyen1]

Thanks. Will fix. (Grumble: I hate UI dev lol. So many bugs and logical pathways that can go wrong)

[rdswift]

Grumble: I hate UI dev lol. So many bugs and logical pathways that can go wrong

Which is one of the reasons I will usually start with the UI and then build (tweak) the back end to accommodate. I find that having a good vision and understanding of the desired end product (especially the user-facing ones) from the start helps.

[knguyen1]

I noticed an issue with View checkboxes:

• Create a new column and press OK
• Both File & Album Views shows the new column:
• Now edit the column, and uncheck File View, press OK -> the column is still visible (and can still be toggled)

This is the fix: Unchecking views should de-register col from view

[zas]

View toggling now works well.

I managed to crash Picard using a Numeric Sort on a %tracknumber% column.

TypeError: '<' not supported between instances of 'float' and 'QCollatorSortKey'
Traceback (most recent call last):
  File "/home/zas/src/picard/picard/ui/itemviews/__init__.py", line 383, in __lt__
    return self.sortkey(column) < other.sortkey(column)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: '<' not supported between instances of 'QCollatorSortKey' and 'float'
Traceback (most recent call last):
  File "/home/zas/src/picard/picard/ui/itemviews/__init__.py", line 383, in __lt__
    return self.sortkey(column) < other.sortkey(column)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: '<' not supported between instances of 'QCollatorSortKey' and 'float'
Traceback (most recent call last):
  File "/home/zas/src/picard/picard/ui/itemviews/__init__.py", line 383, in __lt__
    return self.sortkey(column) < other.sortkey(column)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: '<' not supported between instances of 'QCollatorSortKey' and 'float'
Traceback (most recent call last):
  File "/home/zas/src/picard/picard/ui/itemviews/__init__.py", line 383, in __lt__
    return self.sortkey(column) < other.sortkey(column)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[knguyen1]

I managed to crash Picard using a Numeric Sort on a %tracknumber% column.

I can't repro this. This is a screencast using numeric and descending numeric on Windows. https://1drv.ms/v/c/0e7c7882e6da112b/EerZviwrTCZPv0oZJDAie74B1H3LYJNEaqLUX5_NZeikpw?e=LmTElu

What OS did you see this on? With what files?

EDIT: Confirmed sorting numeric working on macos too.

[zas]

@knguyen1 I can't reproduce it either, tried a lot of things but nope, not sure how I managed to have it but for sure it was while I was testing columns and numeric sort.

My env is the following:

Linux Mint 22.2
Picard 3.0.0.dev7, Python 3.12.3, PyQt 6.9.1, Qt 6.9.1, Mutagen 1.47.0, Discid discid 1.3.0, libdiscid 0.6.4, astrcmp C, SSL OpenSSL 3.0.13 30 Jan 2024

There's only one place where we generate a QCollatorSortKey

picard/picard/i18n.py

Line 282 in 4f2d979

return collator.sortKey(string)

It cannot happen on Windows because of:

picard/picard/i18n.py

Lines 238 to 251 in 4f2d979

	def sort_key(string, numeric=False):
	"""Transforms a string to one that can be used in locale-aware comparisons.
	Args:
	string: The string to convert
	numeric: Boolean indicating whether to use number aware sorting (natural sorting)
	Returns: An object that can be compared locale-aware
	"""
	# QCollator.sortKey is broken, see https://bugreports.qt.io/browse/QTBUG-128170
	if IS_WIN:
	return _sort_key_strxfrm(string, numeric)
	else:
	return _sort_key_qt(string, numeric)

The crash was because of an infinite loop (the error shown repeated a lot, likely some recursion).
I suggest we ignore this issue for now, until we manage to reproduce it.

[zas]

I found another issue:

What I do:

• I add a column in File view that sets tracknumber and displays it: set(tracknumber,1)%tracknumber%
• Press OK -> the column appears, everything at 1
• I open the custom column dialog again
• Change the number to 2: set(tracknumber,2)%tracknumber%
• Press OK -> rows are only partially refreshed, some still display 1
• and so on...

See the screenshot above, col column should display 4 everywhere, it doesn't.

[knguyen1]

Can't repro partial refreshes issue. Screencast: https://1drv.ms/v/c/0e7c7882e6da112b/ETxhaWcGUeBIqXjsrvT3TZ0BdEEcuyawNAYJXSSZepgihQ?e=o2UfWl

This is after 9 iterations and refreshes are perfect every time. So I can't repro...

Edit: Interestingly the builtin Title column seems to be lagging behind by -1. It'll probably get fixed during the refactor: https://tickets.metabrainz.org/projects/PICARD/issues/PICARD-3119

[zas]

Thanks for the fixes 👍

Another thing:

Unclustered Files is showing something for custom column (col = !!), I think it doesn't make much sense (we don't display any artist or length for it).

@phw @rdswift @knguyen1 What do you think? Is there a reason to display a value for custom columns there?

[rdswift]

What do you think? Is there a reason to display a value for custom columns there?

I don't see any value in it.

[rdswift]

Since the last time I tested this, I see that there is a new field added to the column definition -- "Sorting" -- with a number of options from which to choose. Is there value to having a brief explanation of each of the choices available, either through a tooltip on the items in the selection list or perhaps in a new tab added to the "Functions" and "Tags" tabs?

[knguyen1]

Thanks for the fixes 👍

Another thing:

Unclustered Files is showing something for custom column (col = !!), I think it doesn't make much sense (we don't display any artist or length for it).

@phw @rdswift @knguyen1 What do you think? Is there a reason to display a value for custom columns there?

But it's working as intended. For that column, I guess you hardcoded !, e.g. !%artist%!, so the two !! will show up since clusters don't have %artist%. It's the same if I hardcoded foo. Here's the change though: Do not show hardcoded values for cluster/noncluster

I could write more code to hide it... But I hesitate to because it's "working as intended".

[zas]

It works for me, most issues were fixed, I did a lot of tests and it seems to work as expected (at least on Linux).
Good job, and very nice new feature. I look forward next step (PICARD-3119).

[rdswift]

I look forward next step (PICARD-3119).

I also look forward to this being managed through option profiles.

[phw]

Working well for me. I'd be in favor merging this for now. The ascending/descending sorting issue is something I'd like to see handled better, but we can do so separately.