PICARD-2103: Add UI to manage custom columns #2714
Conversation
|
I haven't looked at this in detail yet, but have some initial thoughts:
|
|
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. |
Persistence is saved to the global |
I disagree, as adding these menu/dialogs there to an already crowded Options/Settings page. The |
However, as currently handled it is not being managed through option profiles. |
@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. |
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? |
knguyen1
force-pushed
the
feat/PICARD-2103/add-ui-to-manage-custom-columns
branch
2 times, most recently
from
c8e47a9 to
63ed30b
Compare
|
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:
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. |
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. |
There was a problem hiding this comment.
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
columnfunction 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.
|
Regardless of the UI, I still think it would be good to have the selected columns and position settings managed through the option profiles. |
@phw I struggled with this too. Attempts to make newly created columns in a "live" manner required touching |
|
Try this: Default col name and focus on add; fix bug invalid spec closing window Look for this function. It's part of the def clear_for_new(self, default_width: int) -> None:
"""Clear form for new column creation.
Parameters
----------
default_width : int
Default width value to set for the new column.
"""
self.set_enabled(True)
self._title_input.clear()
self._title_input.setText(DEFAULT_NEW_COLUMN_NAME)
self._title_input.setFocus()
self._expression_input.setPlainText("")
self._width_input.setValue(default_width)
idx = self._align_input.findData(normalize_align_name(ALIGN_LEFT_NAME))
if idx >= 0:
self._align_input.setCurrentIndex(idx)
# Reset sorting adapter to default (first item)
self._sorting_adapter_input.setCurrentIndex(0)
# Select all views by default
with suppress(AttributeError):
self._view_selector.select_all() |
|
I prefer it like this (without the Save button). I noticed few issues:
(see col4, shown in table, but not checked in the menu)
|
| # Always sync _current_row with actual selection before commit | ||
| actual_selected_row = self._selected_row() | ||
| if self._current_row != actual_selected_row: | ||
| self._current_row = actual_selected_row |
There was a problem hiding this comment.
Maybe simplify here as the test isn't really useful and actual_selected_row isn't used later :
self._current_row = self._selected_row()Could we avoid this totally, just by using self._selected_row() (with local variable caching if needed)?
There was a problem hiding this comment.
| if self._current_row != actual_selected_row: | ||
| self._current_row = actual_selected_row | ||
|
|
||
| if self._populating or self._deleting or self._current_row < 0 or self._current_row >= self._model.rowCount(): |
There was a problem hiding this comment.
I would split this in two:
if self._populating or self._deleting:
return
if self._current_row < 0 or self._current_row >= self._model.rowCount():
returnAlso shouldn't we set self._current_row only if not populating or deleting?
def _commit_form_to_model(self) -> None:
if self._populating or self._deleting:
return
self._current_row = self._selected_row()
if self._current_row < 0 or self._current_row >= self._model.rowCount():
returnThere was a problem hiding this comment.
| if name == TransformName.BRACKETS: | ||
| return lambda s: f"[{s}]" if s else "" | ||
| # Fallback no-op | ||
| return lambda s: s or "" |
There was a problem hiding this comment.
Perhaps just use a defaultdict for O(1) access:
from collections import defaultdict
_transforms = defaultdict(lambda: lambda s: s or "")
_transforms[TransformName.UPPER] = lambda s: (s or "").upper()
...
_transforms[TransformName.BRACKETS] = lambda s: f"[{s}]" if s else ""
def _make_transform_callable(name: TransformName) -> Callable[[str], str]:
return _transforms[name]There was a problem hiding this comment.
| expr_val = entry.get('expression') | ||
| if not isinstance(key_val, str) or not key_val.strip(): | ||
| continue | ||
| if not isinstance(expr_val, str) or not expr_val.strip(): |
There was a problem hiding this comment.
Move expr_val declaration just before this line:
key_val = entry.get('key')
if not isinstance(key_val, str) or not key_val.strip():
continue
expr_val = entry.get('expression')
if not isinstance(expr_val, str) or not expr_val.strip():There was a problem hiding this comment.
| # along with this program; if not, write to the Free Software | ||
| # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. | ||
|
|
||
| """Validation for CustomColumnSpec objects.""" |
|
|
||
| # Display an error message to the user | ||
| self._error_message_display = QtWidgets.QLabel(self._editor_panel) | ||
| self._error_message_display.setStyleSheet("QLabel { color : red; }") |
There was a problem hiding this comment.
But it should be stored in a variable somehow, like in
picard/picard/ui/options/scripting.py
Line 229 in cc0b900
Having
STYLESHEET_ERROR will help identifying places were we have those hard-coded colors.
Also we could use a color defined in
Lines 91 to 126 in cc0b900
Ideally we should get them from themes somehow.
There was a problem hiding this comment.
I prefer the manual Save button actually. Without it, the workflow becomes confusing: users add a column, modify it, but have no clear signal that it’s safe to proceed or that changes are staged. There’s no obvious way to cancel unwanted changes either. The manual staging button gives users control and clear feedback about their actions. This mirrors familiar patterns like repeatedly hitting Ctrl+S to save documents, or gamers using save points despite auto-save features. Sometimes the illusion of control improves user experience, even if it adds an extra step. Thoughts? |
|
Is there a reason that we can't show the newly created column definition in the list on the left and select it for editing when we click the "Add" button? |
|
That is the current behaviour. Click Add then "untitled column" appears and available for edit right away. Problem is it isn't clear to user if it's safe to click Add for another. "I'm done. Do I click Make it So? Can I click add again? Did my changes get saved (staged)? If I click Add We need some indicator that it's Staged. Maybe a debounce event that puts a star next to a column indicator it's not yet staged.
|
Couldn't this be done by simply trying to stage the current edits when "Add" (or "Duplicate") button is clicked before adding the new item? If there are errors preventing the current edits from being staged, then the new item would not be created and the editor would remain on the current item (with the error message displayed). The "Delete" should always be honoured, and remove the current item (after the confirmation). |
Well, things are already being silently staged. Nowhere on the UI right now is it clear that it's safe to click Add while in the middle of an edit though. |
|
Aren't the current edits staged (or applied or saved or whetever the right term is) automatically when you select a different item in the column definitions list? Couldn't the same be done when the user clicks "Add" or "Duplicate"? It should be clear that leaving the current edit is an indicator that the user is done making changes (at least for the time being) and edits should be saved. |
|
I think we're talking past each other. If you feel that it's clear to user that they feel it's safe to click Add, we'll move on. I'll address the change requests and close out this PR. Discussions getting long.
This is already happening. |
In that case let me make it clear (I hope)... What I would like to see is that when the user clicks "Add" or "Duplicate", the new item appears immediately in the list on the left and is selected for editing. This would then be consistent with the behavior of other sections of Picard, such as the Options... -> Scripting page. I believe that we should be consistent as much as possible in the way the UI works. |
@zas / @phw, am I out of line here? I don't want to drive @knguyen1 crazy coding something that I would like to see, only to find out that it's something that you don't agree with. If it's just me, I'll hold my nose and back off. I think we all agree that we're to the point where we should merge this so we can move on to the next step of revamping the columns selection context menu and managing the column selections through the option profiles system. |
Try this one Add should create a placeholder spec |
@rdswift I agree we should be consistent. |
I just ran the latest commit and it gets two thumbs up from me. 👍 👍 This is exactly what I had in mind. Thanks for all the extra effort you put in to make it happen. |
|
Just found one other issue, but I suspect it's easy to fix. I was testing the latest commit and had two custom columns already defined, and when I tried editing one of them I was presented with a "Key must be a valid UUID" error. This is a problem because there is absolutely nothing that the user can do to fix this. Perhaps we only need to check that the key is not empty, and not that it is a valid UUID. I understand that when this is placed in production it is unlikely that this error will occur, but I am really reluctant to block the user with an error that they have no means of correcting. 
|
@rdswift Can you delete all and recreate? I think it's because you had created these columns before they were key'ed to UUID's. At this point, I can't reproduce. I relaxed the rule: Relax key rule; mark and translate errors |
That is exactly the case. The IDs for the two existing items were "1" and "3". I just tried the latest commit and it works fine. Thanks. Now I did notice something in the debug log. A lot of lines like: It appears that there is an entry for each of the standard columns available. This doesn't seem to impact the way that the column selection and display works, but it kind of junks up the log. Not sure whether or not that's enough of a concern to worry about. |
|
It's from this change: e3c2c0c reported by #2714 (comment) Without the change, you have to manually That try-catch and log is needed for automatically recomputing data on custom column apply. Without that try-except, the programme crashes. We could remove the |
I'd rather not because it does provide good information in the case of custom columns. Perhaps we could suppress the logging if the key is in EDIT: Actually, I don't have a strong opinion either way. I can live with whatever you decide. |
Summary
Adds a UI to manage custom columns from the header menu: create, edit, duplicate, delete, place, and apply to File/Album views.
Problem
Users had no built-in UI to add/manage custom columns; changes required backend code or were not discoverable from the views.
Solution
Demo
Action
Additional actions required:
Note: API PR #2709 must be merged first; this PR only covers the UI/UX management layer.