BaseTrackTableModel: Fix column header mapping #13682
Conversation
uklotzde
changed the title
uklotzde
force-pushed
the
basetracktablemodel
branch
from
a3ddee7
to
a99001b
Compare
|
I'm not sure how to proceed here. Who is qualified enough to review this? |
uklotzde
force-pushed
the
basetracktablemodel
branch
from
bf4af50
to
dc287eb
Compare
uklotzde
changed the title
|
I recommend to squash this PR into a single commit when merging, using the title as the commit message. |
Co-authored-by: Daniel Schürmann <[email protected]>
|
I appreciate that this base class is still maintained: 8983ca8 Any chances to get these fixes merged? |
|
If you are not interested in these fixes I will close the PR, stash the commits, and maintain a local patch. Only affects my use case. |
|
friendly ping @daschuer |
| } | ||
| DEBUG_ASSERT(headerIndex < m_columnHeaders.size()); | ||
| m_columnHeaders[headerIndex].column = column; | ||
| DEBUG_ASSERT(mapColumn(headerIndex) == column); | ||
| } |
There was a problem hiding this comment.
It turns out that the whole loop Is redundant, because the header properties are reinitialized below. There is only a dummy entry missing rot the ID header. Something like:
setHeaderProperties(
ColumnCache::COLUMN_LIBRARYTABLE_ID,
QString(), // hidden
0);
You may assert below initHeaderProperties() that all m_columnHeaders have been initialized.
It would be better to not initalizer the header properties at runtime, because all columns are known at compile time. But it is OK for me to not extend the scope of this PR as suggested in the other comment.
There was a problem hiding this comment.
That's how an alternative initialization ordering looks like: e0d4374. IMHO more complicated and cluttered.
I suppose this code either vanishes or needs to be rewritten when migrating to QML. I wouldn't put too much effort in it.
Squashing can be done on merge. The PR title already contains the final commit message.
There was a problem hiding this comment.
I was probably not clear enough: Please remove this complicated and cluttered loop. It is not necessary.
Instead just this is requied:
setHeaderProperties(
ColumnCache::COLUMN_LIBRARYTABLE_ID,
QString(), // hidden
0);
| } | ||
| DEBUG_ASSERT(headerIndex < m_columnHeaders.size()); | ||
| m_columnHeaders[headerIndex].column = column; | ||
| DEBUG_ASSERT(mapColumn(headerIndex) == column); | ||
| } |
There was a problem hiding this comment.
I was probably not clear enough: Please remove this complicated and cluttered loop. It is not necessary.
Instead just this is requied:
setHeaderProperties(
ColumnCache::COLUMN_LIBRARYTABLE_ID,
QString(), // hidden
0);
|
@daschuer Sorry, but what you are proposing doesn't work. There are more hidden columns which remain uninitialized. Initializing them one by one is brittle and will break as soon as more (hidden) columns are added in the future. Initializing all columns in a loop is the only safe and sustainable option. I will maintain a patch for myself and leave the rest to you. |
|
The proposed changes could be found as a single commit in this (permanent) branch: https:/uklotzde/mixxx/tree/aoide. I decided to keep the branch accessible for everyone. It will be available as long as rebasing and maintaining is manageable. Thanks for the review. It has considerably helped improving and simplifying the code. |
Without these fixes
BaseTrackTableModeldoesn't work as expected when used as a base class, i.e. internal column values are undefined and visible columns are missing.However
BaseSqlTableModeldoesn't seem to suffer from these deficiencies and inconsistencies in its base class.