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

RekordBoxFeature: add played functionality to rekordbox external library #13834

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

andreivn2
Copy link

Hello,

Because I use mixxx a lot for my DVS setup with rekordbox exports from different sources I decided to create this PR.

This PR introduces minimal and simple changes to the RekordboxFeature in order to display and correctly interpret the "played" column in the external library view.

All SQL modified is in regards to tables that are dropped and created with each restart of mixxx.
Changes are integrated with the current hierarchy of QAbstractDisplayTable.

@JoergAtGithub
Copy link
Member

Welcome at Mixxx!
As a first-time contributor we need you to sign the Mixxx Contributor Agreement and comment here when you have done so. It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

@andreivn2
Copy link
Author

Welcome at Mixxx! As a first-time contributor we need you to sign the Mixxx Contributor Agreement and comment here when you have done so. It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

Done

@JoergAtGithub
Copy link
Member

The pre-commit check is failing. The best way to fix pre-commit issues is to install pre-commit locally on your system, as described here: https://github.com/mixxxdj/mixxx/wiki/Using-Git#set-up-automatic-code-checking
Than it will fix these issues automatically before every commit.
Alternatively, you can download the pre-commit.patch file from the artifacts of this PR
grafik
unzip it, and apply it using:
git apply pre-commit.patch

@andreivn2
Copy link
Author

Applied pre-commit patch now.

I tried to do it before creating the PR, but had some issues with it failing with some python exception.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

couple nitpicky comments. I'm probably not the right person to do a proper review here because I'm not familiar enough with the database code.

src/library/rekordbox/rekordboxfeature.cpp Outdated Show resolved Hide resolved
src/library/rekordbox/rekordboxfeature.cpp Outdated Show resolved Hide resolved
src/library/rekordbox/rekordboxfeature.cpp Outdated Show resolved Hide resolved
Comment on lines 1323 to 1324
query.prepare("select id from " + kRekordboxLibraryTable + " where location=:location");
query.bindValue(":location", pTrack->getLocation());
Copy link
Member

Choose a reason for hiding this comment

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

rather than an in-memory set, have you considered adding a temporary table that stores the played track ids and then query against that here? That would result in improved scalability.

@andreivn2
Copy link
Author

Ok, so innocently i tried to use GitHub's features for the comments I got and it messed up my commits.
History looks really bad now.
Should I just create a new branch with a new PR?

@Swiftb0y
Copy link
Member

Swiftb0y commented Nov 5, 2024

don't worry with that for now. we can clean up the history later. no need to open a new PR or create new branch.

@daschuer
Copy link
Member

daschuer commented Nov 6, 2024

The History feature uses certain logic to prevent insain played numbers during scratching:

if (track_played_recently) {

I think it is a good idea to move this functionality into Playerinfo and feed both tables with the same value to have it consistent in all views.

@andreivn2
Copy link
Author

If you think this fits within the scope of this PR i can try and make a commit later tonight

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