Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Do not merge] Demonstrate that ColumnCache is seriously broken #3135

Closed
wants to merge 1 commit into from
Closed

[Do not merge] Demonstrate that ColumnCache is seriously broken #3135

wants to merge 1 commit into from

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented Sep 27, 2020

Start Mixxx and watch the debug assertions.

Hopefully, someone can explain how this is supposed to work correctly? I don't get it.

@Be-ing Be-ing changed the base branch from master to main October 23, 2020 23:08
@ywwg
Copy link
Member

ywwg commented Nov 14, 2020

pasting from chat: Owen W.: (@uwe Klotz) somewhere along the way, columncache got broken. In basetrackcache it is correctly inited with all of the columns (done when it's constructed inside mixxxlibraryfeature.cpp). However in basesqltablemodel, it only gets inited with a few columns -- id, preview, and coverart -- inside LibraryTableModel::setTableModel. This means header column lookup by name is broken, which means saving header layouts is broken, which explains why I'm having trouble getting the last_played_at column added

Owen W.: we need to always init the columncache with all of the column names so its indexes are correct. We could either make columncache static, since it's a readonly object, or use a factory function or something

@ywwg
Copy link
Member

ywwg commented Nov 14, 2020

I think what happened is during a refactor, someone didn't understand what SetColumns was doing, because it was poorly named. So when a second columncache was created, it was not initialized correctly

@ywwg
Copy link
Member

ywwg commented Nov 14, 2020

Probably the correct fix is to have columncache build itself correctly, and mixxxlibraryfeature should use it as a source of truth instead of the other way around.

@uklotzde
Copy link
Contributor Author

Hopefully obsolete with #3328 ;)

@uklotzde uklotzde closed this Nov 16, 2020
@uklotzde uklotzde deleted the columncache branch December 17, 2020 07:17
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