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

SearchQuery: Add ability to filter tracks in Mixxx library by iTunes playlist #11955

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

Conversation

fwcd
Copy link
Member

@fwcd fwcd commented Sep 10, 2023

This adds a new filter node to the search query machinery, namely ExternalPlaylistFilterNode that matches the name of an external playlist, along with a 'reference implementation' ITunesFilterNode that uses the iTunes tables (though the mechanism is explicitly intended to be generic, so support for Serato, Traktor etc. could easily be added in the future).

Thus, if the user has an iTunes playlist named "House", queries like itunes:house or itunes:house daft punk will now explicitly search for tracks in that playlist in a similar way to the crate operator.

External tracks are matched with library tracks via their location (since external libraries generally use a different track indexing scheme). Consequentially, this only affects the case where the external tracks are in the user's Mixxx library too (not necessarily the case, but probably common enough to make this a worthwhile use case).

To do

  • Perform fuzzy matching on the playlist name (instead of requiring the exact name)
  • Investigate whether this is performant enough to warrant the inclusion of the untagged ITunesFilterNode (if not, we might want to consider gating this behind the explicit itunes operator)
    • We'll require the itunes: operator for now to avoid any potential performance regressions in regular searches, but leave this open for revisiting it in the future (See 1a2fca1)
  • Add tests

Future Directions

  • Add Serato, Traktor etc. support as mentioned above (likely pretty easy, would only require subclassing ExternalPlaylistFilterNode and adding the corresponding operator to SearchQueryParser)
  • Filter by non-uniquified iTunes playlist name (see ITunesDAO::uniquifyPlaylistName, we might want to store the non-unique playlist name in a separate column, that would require a schema change though and also require us to abstract over the playlist name column in ExternalPlaylistFilterNode since other external playlist integrations may not necessarily have this machinery).

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.

small comments, full review to come some other time

src/library/itunes/itunesdao.cpp Show resolved Hide resolved
src/library/itunes/itunesschema.h Outdated Show resolved Hide resolved
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.

this review was pending from ages ago but I forgot to finish it. Better comment late and unfinished than never.

Comment on lines +51 to +53
QStringLiteral("INSERT INTO %1 (id, artist, title, album, "
"album_artist, genre, grouping, year, duration, "
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this compile time instead then?

Suggested change
QStringLiteral("INSERT INTO %1 (id, artist, title, album, "
"album_artist, genre, grouping, year, duration, "
QStringLiteral("INSERT INTO " ITUNES_LIBRARY_TABLE " (id, artist, title, album, "
"album_artist, genre, grouping, year, duration, "

Copy link
Member Author

Choose a reason for hiding this comment

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

That would've worked with the macro solution, but unfortunately not with constexpr statics.

Copy link
Member

Choose a reason for hiding this comment

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

Right, the comment was from before we agreed to change it. I mean, if we didn't use QString it would've worked but we don't so yeah...

src/library/itunes/itunesschema.h Outdated Show resolved Hide resolved
src/library/searchquery.cpp Outdated Show resolved Hide resolved
@fwcd fwcd added the itunes label Oct 14, 2023
Copy link

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Jan 19, 2024
@github-actions github-actions bot removed the stale Stale issues that haven't been updated for a long time. label Oct 19, 2024
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.

2 participants