Skip to content

Comments

PICARD-3196: Implement custom persistence for column state in ConfigurableColumnsHeader#3043

Open
phw wants to merge 2 commits intometabrainz:masterfrom
phw:PICARD-3196-fix-column-persistence
Open

PICARD-3196: Implement custom persistence for column state in ConfigurableColumnsHeader#3043
phw wants to merge 2 commits intometabrainz:masterfrom
phw:PICARD-3196-fix-column-persistence

Conversation

@phw
Copy link
Member

@phw phw commented Feb 15, 2026

Summary

  • This is a…
    • Bug fix
    • Feature addition
    • Refactoring
    • Minor / simple change (like a typo)
    • Other
  • Describe this change in 1-2 sentences:

Problem

Changing the default columns for the main views can break the column display as the persisted state does not match the column model anymore. See e.g. #3040 and the example screenshot there.

Solution

Instead of relying on an opaque binary structure based on logical column indices use a custom structure which stores the state of each column based on its name. This fixes issues with column state breaking if the indices of the default columns change, e.g. when new default columns are addded or removed.

As there is not clean upgrade path there is a config upgrade to remove the old states. The columns will reset to their default state on upgrade.

AI Usage

In accordance with the AI use policy portion of the MetaBrainz Contribution Guidelines, the level of AI/LLM use in the development of this Pull Request is:

  • No AI/LLM use
  • Minimal use (e.g. autocompletion)
  • Moderate use (e.g. suggestions regarding code fragments)
  • Significant use (e.g. code structure, tests development, etc.)
  • Primarily AI developed
  • Other (please specify below)

Action

Additional actions required:

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

…rableColumnsHeader

Instead of relying on an opaque binary structure based on logical column
indices use a custom structure which stores the state of each column based
on its name. This fixes issues with column state breaking if the indices
of the default columns change, e.g. when new default columns are addded
or removed.

As there is not clean upgrade path there is a config upgrade to remove the
old states. The columns will reset to their default state on upgrade.
config = get_config()
header = self.header()
if header.prelock_state is not None:
state = header.prelock_state
Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed a small issue likely related to this removal, the sort indicator on a column disappears when locking/unlocking.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorted = column_state.get('sorted', None)
if sorted is not None:
self.setSortIndicator(i, QtCore.Qt.SortOrder(sorted))

Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding this method fixes the issue with sort indicator and column locking for me:

    def lock(self, is_locked):
        """Override parent's lock() to use custom dictionary-based state.

        This ensures the sort indicator and other column state is preserved
        across lock/unlock cycles using the same format as save/restore.
        """
        if is_locked:
            self.prelock_state = self.get_columns_state()
            super().lock(True)
        else:
            super().lock(False)
            if self.prelock_state is not None:
                self.restore_columns_state(self.prelock_state)
                self.prelock_state = None

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a bit more complicated. The primary reason the prelock state exists in the LockableHeaderView is that it was needed to cleanly restore the column resize mode. Locking sets all columns to fixed, so when we store this state and restore it all columns remain fixed. Hence the state was saved before locking, and resetting the resize mode was restoring the state. We had no other way, since the persisted Qt data is opaque.

But we can do better now, as ConfigurableColumnsHeader has proper knowlegde about all columns and we can store the resize mode of each column separately. I think the proper fix is to:

  1. Override lock completely to disable the default state restore
  2. Persists the resize mode

@phw
Copy link
Member Author

phw commented Feb 20, 2026

I found some other issue: After unlocking and then adding / removing columns the header does not directly update. Only after clicking on the header again. It's rather strange, but I cannot reproduce on master.

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.

2 participants