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

Column cache fix #3328

Merged
merged 19 commits into from
Dec 17, 2020
Merged

Column cache fix #3328

merged 19 commits into from
Dec 17, 2020

Conversation

ywwg
Copy link
Member

@ywwg ywwg commented Nov 16, 2020

Possible fix for the columncache issues we've been having. This has a lot of debug junk in it in purpose -- please test this branch and let me know if it works for you.

@ywwg ywwg requested a review from uklotzde November 16, 2020 03:17
@ywwg ywwg marked this pull request as draft November 16, 2020 03:17
@ywwg ywwg requested a review from Be-ing November 16, 2020 03:17
@ywwg ywwg changed the title Column cache fix WIP: Column cache fix Nov 16, 2020
@ronso0 ronso0 added this to the 2.3.0 milestone Nov 16, 2020
@Holzhaus
Copy link
Member

Just wondering: Does this make it possible to modify the default column order? I'ld love to move the color column to the left by default for 2.3 if possible.

@ywwg
Copy link
Member Author

ywwg commented Nov 18, 2020

Just wondering: Does this make it possible to modify the default column order? I'ld love to move the color column to the left by default for 2.3 if possible.

yes, because headers are saved and restored by name, the default ordering is no longer important. (That's why I wrote this code, so headers wouldn't break on upgrade). I think this code is working in 2.2 so this shouldn't break people not on the beta build. Unfortunately 2.3 has been saving header states with blank names, so column loading will probably break for beta testers.

@ywwg
Copy link
Member Author

ywwg commented Nov 18, 2020

notes addressed

@ywwg
Copy link
Member Author

ywwg commented Nov 29, 2020

friendly ping

@Holzhaus
Copy link
Member

Holzhaus commented Nov 29, 2020

Do you want reviews? Then you should probably mark this as ready to review and remove WIP from the title :D

@ywwg ywwg changed the title WIP: Column cache fix Column cache fix Nov 30, 2020
@ywwg ywwg marked this pull request as ready for review November 30, 2020 19:42
@Holzhaus Holzhaus requested a review from uklotzde November 30, 2020 22:36
Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

Please fix the remaining pre-commit and clazy failures. We don't care about Travis ;)

@Holzhaus
Copy link
Member

Holzhaus commented Dec 3, 2020

Also 2 clazy issues:

src/library/columncache.cpp#L84
allocating an unneeded temporary container [-Wclazy-container-anti-pattern]

src/library/columncache.cpp#L84
c++11 range-loop might detach Qt container (QList) [-Wclazy-range-loop]

@ywwg
Copy link
Member Author

ywwg commented Dec 5, 2020

Ah, the API I used was too new. how about this one??

@ywwg
Copy link
Member Author

ywwg commented Dec 5, 2020

all checks passed yay

@ywwg
Copy link
Member Author

ywwg commented Dec 12, 2020

ping

@uklotzde
Copy link
Contributor

uklotzde commented Dec 13, 2020

Used for verification and to prevent new conflicts in the future: ywwg#9

@uklotzde
Copy link
Contributor

@Holzhaus After merging we may try again to reorder the columns.

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

I noticed no issue during a small manual test. Except for the open review comment this LGTM.

@uklotzde
Copy link
Contributor

@ywwg Please check my open PR with more validations and a minor improvement of the sort order code.

@ywwg
Copy link
Member Author

ywwg commented Dec 16, 2020

@ywwg Please check my open PR with more validations and a minor improvement of the sort order code.

which pr?

@Be-ing
Copy link
Contributor

Be-ing commented Dec 16, 2020

ywwg#9

@Be-ing
Copy link
Contributor

Be-ing commented Dec 16, 2020

@uklotzde please leave a comment with a link in the upstream PR when you make a PR for someone's fork.

@uklotzde
Copy link
Contributor

@uklotzde please leave a comment with a link in the upstream PR when you make a PR for someone's fork.

I already did, see above, cross-referenced from both sides.

@uklotzde
Copy link
Contributor

3 days ago

Used for verification and to prevent new conflicts in the future: ywwg#9

@ywwg
Copy link
Member Author

ywwg commented Dec 17, 2020

addressed note and merged sanity check code

@uklotzde
Copy link
Contributor

Let's now give it a try after two of us have checked it. Thank you, Owen! LGTM

@uklotzde uklotzde merged commit 70088b3 into mixxxdj:2.3 Dec 17, 2020
@uklotzde
Copy link
Contributor

I will push to main, but only after testing the merged result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants