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

BaseExternalLibraryFeature: Add ability to import external playlists as crates #11852

Merged
merged 2 commits into from
Aug 27, 2023

Conversation

fwcd
Copy link
Member

@fwcd fwcd commented Aug 21, 2023

This is a small extension to the existing import-as-playlist action found in BaseExternalLibraryFeature that enables users to import external playlists as crates too:

image

Crates have the unique property that their names must be unique, so the import can fail if the name already exists. Currently this is dealt with by presenting an error dialog, but we may consider prompting the user e.g. to specify a custom name instead or to merge the tracks into the existing crate. Ideas welcome.

Something else to consider is that the import is currently synchronous and can be quite slow for large external playlists (thousands of songs). This applies to the existing playlist import too though, therefore I would consider any optimizations to be out-of-scope for this PR.

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.

LGTM otherwise tbh. but I'd prefer if somebody with library experience (@ronso0 maybe) could express their opinion as well.

} else {
QMessageBox::warning(nullptr,
tr("Crate Creation Failed"),
tr("Could not create crate, it most likely already exists: ") + playlist);
Copy link
Member

Choose a reason for hiding this comment

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

would be nice to have a better error message here but I guess thats not currently possible with the insertCrate API, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly, anything more specific would likely require refactoring those APIs (aside from manually checking the crates, though I haven't dug too far into the crate APIs yet).

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.

welp, LGTM. lets merge.

@Swiftb0y Swiftb0y merged commit f654b00 into mixxxdj:main Aug 27, 2023
11 checks passed
@fwcd fwcd deleted the import-as-crate branch July 23, 2024 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants