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

playlist export: update with newly selected extension, don't just append #11332

Merged
merged 1 commit into from
Mar 21, 2023

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Mar 6, 2023

Fixes #11327

Quick fix, adds include QRegExp so we can use QRegExp::indexIn.
I bet there are a million other ways do achieve the same, so feel free to optimize this.

@ronso0 ronso0 added this to the 2.3.5 milestone Mar 6, 2023
@ronso0 ronso0 linked an issue Mar 6, 2023 that may be closed by this pull request
@ronso0
Copy link
Member Author

ronso0 commented Mar 6, 2023

Actually, it feels like we're now pretty close to reimplementing QFileDialog::getSaveFileName and would "just" need to hook up to the filterSelected signal to get good UX inside the dialog (= immediately replace/append the selected extension) ...

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Even though I cannot confirm the issue, I don't see a regression form this PR.

As a test I have called a "playlist.pls" and the proposed export name was "playlist.pls.m3u"
I think this is the expected behavior.

There is another pending issue, that there is no proposed extension in the crate case and the crate export is actually not working.

Comment on lines 9 to 12
const QRegularExpression kExtractExtensionRegex(R"(\(\*\.(.*)\)$)");
// lazy match doesn't work here "(\(\*\.(.*?)\))"
const QRegExp kFileFilterRegex(R"(\(\*\.(.[^\)]*)\))");
Copy link
Member

Choose a reason for hiding this comment

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

The difference between the kExtractExtensionRegex and kFileFilterRegex is not easy to discover.
Can you add an example string, that should be matched?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will do.

@daschuer
Copy link
Member

daschuer commented Mar 7, 2023

The crate export as Playlist works on the 2.3 branch. The extension is added silently. The export is broken in the 2.4 branch.

@ronso0
Copy link
Member Author

ronso0 commented Mar 7, 2023

As a test I have called a "playlist.pls" and the proposed export name was "playlist.pls.m3u"
I think this is the expected behavior.

If you name a file file.pls manually and don't change m3u than this is already added by the dialog, and the 'extension fix' branch in filePathWithSelectedExtension() is skipped.

@ronso0
Copy link
Member Author

ronso0 commented Mar 19, 2023

This is still ready to go.

@daschuer
Copy link
Member

It still works without a regression on Ubuntu Focal 20.4

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Thank you. LGTM

@daschuer daschuer merged commit e9d1ed8 into mixxxdj:2.3 Mar 21, 2023
@ronso0 ronso0 deleted the playlist-export branch March 21, 2023 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Export Playlist file naming issue
2 participants