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

Follow-up: Move definition of library default columns out of header file #3465

Merged
merged 1 commit into from
Dec 18, 2020
Merged

Follow-up: Move definition of library default columns out of header file #3465

merged 1 commit into from
Dec 18, 2020

Conversation

uklotzde
Copy link
Contributor

Follow-up of #3328. The definition of this constant does not belong in a header file.

Be careful when merging to main! The COVERART_DIGEST column probably needs to be added manually.

@uklotzde uklotzde added this to the 2.3.0 milestone Dec 18, 2020
@Holzhaus
Copy link
Member

I would be extremely convenient if the order of elements in the stringlist would also affect the default column order. It's quite complicated to grasp where the order comes from.

@uklotzde
Copy link
Contributor Author

I would be extremely convenient if the order of elements in the stringlist would also affect the default column order. It's quite complicated to grasp where the order comes from.

I agree. But this PR is just a technical clean up.

Figuring out how to entirely replace the table views in QML instead of rewriting this legacy code might be a more sustainable investment of work and effort.

@uklotzde
Copy link
Contributor Author

Ping.

@ywwg
Copy link
Member

ywwg commented Dec 18, 2020

Can you add a comment defining what is meant by "default columns"? Or maybe, it should be renamed to like "LIBRARY_VIEWABLE_COLUMNS". otherwise LGTM

@uklotzde
Copy link
Contributor Author

@ywwg I have only moved your code from a .h file into a .cpp file for technical reasons. I will not improve the code.

@Be-ing Be-ing merged commit b5097f8 into mixxxdj:2.3 Dec 18, 2020
@uklotzde uklotzde deleted the library-default-columns branch December 18, 2020 20:22
@Be-ing
Copy link
Contributor

Be-ing commented Dec 18, 2020

@ywwg if you want to make further improvements please open another PR. But keep in mind that asking others to expand the scope of PRs can be frustrating and slows down development.

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.

4 participants