Skip to content

Conversation

knguyen1
Copy link
Contributor

Summary

  • This is a…
    • Bug fix
    • Feature addition
    • Refactoring
    • Minor / simple change (like a typo)
    • Other
  • Describe this change in 1-2 sentences: Prevents the columns picker context menu from auto-closing when toggling column visibility checkboxes, allowing users to select multiple columns in a single session.

Problem

Currently, when users right-click on column headers and toggle column visibility, the context menu closes after each selection. This forces users to repeatedly open the menu to show/hide multiple columns, resulting in poor UX.

Solution

Applied the same QWidgetAction + QCheckBox pattern from PICARD-3114 to the columns picker menu in configurablecolumnsheader.py:

  • Replaced checkable QAction items with QWidgetAction containing embedded QCheckBox widgets
  • Added dynamic enable/disable behavior: when "Lock columns" is toggled, all dependent UI elements (column checkboxes, restore action, manage action) immediately update their state within the same menu session
  • Refactored contextMenuEvent following SOLID principles for improved maintainability

The menu now stays open while users toggle multiple columns, and the lock state dynamically controls the enabled state of all dependent actions.

Action

Additional actions required:

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

… column management

- Introduced helper methods to create checkbox actions and manage column visibility in the context menu, improving code organization and readability.
- Added dynamic state updates for column visibility, restore defaults, and lock actions, enhancing user experience.
- Implemented a callback mechanism to synchronize UI elements based on the lock state, ensuring consistent behavior in the context menu.
…ction creation

- Enhanced readability by formatting parameters in the checkbox action creation method, ensuring clearer code structure.
restore_action = self._add_restore_action(menu)
lock_checkbox = self._add_lock_action(menu)
menu.addSeparator()
manage_action = self._add_manage_action(menu)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should re-order those to:

        menu.addSeparator()
        restore_action = self._add_restore_action(menu)
        manage_action = self._add_manage_action(menu)
        menu.addSeparator()
        lock_checkbox = self._add_lock_action(menu)
Image

Also we should be consistent regarding sentence case vs title case in menus (see https://tickets.metabrainz.org/browse/PICARD-562)

This looks better to me:

Image

@knguyen1 @phw @rdswift : What's your take on this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also we should be consistent regarding sentence case vs title case in menus

Agree 100%. I prefer the second image as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No opinion, really. Agreed on the caps consistency.

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

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.

3 participants