playlist export: add missing file extension by ronso0 · Pull Request #4531 · mixxxdj/mixxx

ronso0 · 2021-11-19T21:59:07Z

original bug https://bugs.launchpad.net/mixxx/+bug/1197302
follow-up https://bugs.launchpad.net/mixxx/+bug/1889352
the fix from #430 got lost somehow.

related pending bug:
"extension is not autocompleted in dialog"
https://bugs.launchpad.net/mixxx/+bug/1455749

ronso0 · 2021-11-23T00:37:04Z

@uklotzde Refactored to use regexp in util/string.h

ronso0 · 2021-11-23T17:32:13Z

for anyone testing this without reading the code:
the file dialog behaves unexpectedly, but predictably. this behaviour is caused by QFileDialog itself, not the changes of this PR.

if the current file name has no suffix the selected suffix is not appended
if there IS a 'suffix' it is replaced. Though ANY .-separated segement at the end is replaced

so for example
Birtday-party-15.9.2021
becomes
Birtday-party-15.9.m3u

ronso0 · 2021-11-29T22:51:43Z

ping, this is ready.

ronso0 · 2021-12-08T22:08:15Z

This has grown way beyond re-applying an old 10-line commit. @Swiftb0y @uklotzde I'll try to wrap my head around your sugestions when I find time. Feel free to finish it.

Swiftb0y · 2021-12-08T23:58:41Z

@ronso0, I took the liberty and pushed my changes directly to your branch so my changes don't get detached from the PR.

ronso0 · 2021-12-09T00:12:10Z

Perfecto, thanks!

Swiftb0y · 2021-12-12T13:37:51Z

+const ConfigKey kConfigKeyLastImportExportCrateDirectoryKey(
+        "[Library]", "last_import_export_crate_directory");
+


@uklotzde I'd like to hear your opinion on this. Instead of defining kConfigKeyLastImportExportCrateDirectoryKey in a bunch of cpp files all at once, put it in a header file under the mixxx namespace and then #include it where its needed. Does that make sense, what do you need to specify for it to work? something like inline const ConfigKey ...?

nevermind, its not defined in more than one translation unit. Don't know how I got that idea.

ronso0 · 2021-12-12T13:40:28Z

Now that the cleanup is done I'd prefer to squash the later commits into 0171cc3 and 3d78e2f, so we have 3 instead of 7 commits.
Okay?

Swiftb0y

LGTM, though I'm not sure if I'm allowed to merge since this PR does contain commits I made.

ronso0 · 2021-12-12T18:28:48Z

sqashed, rebased, tested if all commits build.
Ready for final review.

ronso0 · 2021-12-12T18:32:46Z

LGTM, though I'm not sure if I'm allowed to merge since this PR does contain commits I made.

I think it doesn't make a difference if your review proposals are implemented or if you implment them yourself.
@uklotzde could take a final look though.

ronso0 · 2021-12-12T19:22:53Z

pushed again because I squashing wiped your co-authorship @Swiftb0y

Swiftb0y

figured I'd take another look.
I hope my perfectionism isn't driving you crazy.

Swiftb0y · 2021-12-12T19:52:44Z

+    QFileInfo fileInfo(playlist_files.first());
+    m_pConfig->set(kConfigKeyLastImportExportPlaylistDirectory,
+            ConfigValue(fileInfo.dir().absolutePath()));


Suggested change

QFileInfo fileInfo(playlist_files.first());

m_pConfig->set(kConfigKeyLastImportExportPlaylistDirectory,

ConfigValue(fileInfo.dir().absolutePath()));

m_pConfig->set(kConfigKeyLastImportExportPlaylistDirectory,

ConfigValue(QFileInfo(playlist_files.first()).dir().absolutePath()));

The explanation for this change got lost somehow:
The point of this change is that I don't really see a reason to reuse the fileInfo variable inside the loop. So inlining it here and instead declaring a const version in the loop seems like better style to me.

playlist_files.first() is a QString, so this won't work.

Sorry yes, noticed after the fact too. We can just construct a temporary QFileInfo (I updated the code suggestion accordingly). This pattern of getting the absolutePath of the folder containing some file has also been used multiple times. We might want to consider factoring that out into an utility function. Especially since I don't think this entire chain of QString -> QFileInfo -> QDir -> QString is particularly efficient.

I'm aware this chain is the opposite of 'simple', though I think refactoring should happen in main where there are more places where this is needed (effect chain import/export comes to mind).
I'll add a ToDo.

I suppose to extract the dir string we just need to chop everything after the last QDir::separator.

str.truncate(str.lastIndexOf(QDir::separator));

if we are sure that the path's we get are absolute, yes that should be sufficient.

Swiftb0y · 2021-12-12T19:58:13Z

+    QString fileFilter = tr("M3U Playlist (*.m3u)");
+    QString fileLocationInput = QFileDialog::getSaveFileName(
            nullptr,
            tr("Export Playlist"),
            lastPlaylistDirectory.append("/").append(playlistName),
            tr("M3U Playlist (*.m3u);;M3U8 Playlist (*.m3u8);;"
               "PLS Playlist (*.pls);;Text CSV (*.csv);;Readable Text (*.txt)"),
-            &filefilter);
+            &fileFilter);
+    QString fileLocation = filePathWithSelectedExtension(fileLocationInput, fileFilter);


add const to each, if ur already touching them

Swiftb0y · 2021-12-12T20:02:18Z

+    QFileInfo fileInfo(playlist_files.first());
+    m_pConfig->set(kConfigKeyLastImportExportCrateDirectoryKey,
+            ConfigValue(fileInfo.dir().absolutePath()));



apply suggestions from baseplaylistfeature here too

daschuer · 2021-12-13T07:47:13Z

+namespace {
+
+const ConfigKey kConfigKeyLastImportExportPlaylistDirectory(
+        "[Library]", "last_import_export_playlist_directory");


This changes the key. Was this intentionally?
This breaks the feature for users after upgrade.

The same applies to the Crate version

I thought if I touch it I might as well apply snake_case.
"This breaks the feature" is a bit of an exxageration, users just need to select it again. But if that's not okay for a bugfix release I'll revert it, of course.

Revert or keep it?

this is a good question. In general, our control object and configkey names are a gigantic mess and totally inconsistent. In general, we have preferred to stick to existing names, even if the names were poorly chosen, to preserve compatibility. Unless there's a compelling reason to change the name, I think it's better to stick with the old one.

ronso0 · 2021-12-13T20:43:53Z

Btw the overwrite issue @Holzhaus discovered in #4539 occurs here as well.

ronso0 · 2021-12-14T17:31:37Z

The last two commits contain the experimental workaround for the QFileDialog suffix bug (with the Qt dialog).
We could merge filePathWithSelectedExtension() into the new function if there's no spot where we need it without the file dialog.

daschuer · 2021-12-15T12:28:39Z

+    return fileLocation;
+}
+
+QString checkedFilePathWithSelectedExtension(const QString& caption,


The function name sounds as if it will just do the check. It also pops up a dialog.
This should be expressed by the name.
Something like:
SaveFileDialogWithVerifiedExtension() or such

yes, thank you. consider this WIP and the name will be adjusted.
What I care about here is whether this approach works for everyone, or not.

if this get's sufficient 👍 I'll implement it in other places, too, and change the name.

considering the return value I'd name it filePathWithVerifiedExtensionFromFileDialog

Try to name functions starting with an imperative verb if possible. Like good commit messages ;)

getFilePathWithVerifiedExtensionFromFileDialog ;)

daschuer · 2021-12-15T21:46:55Z

I have tested this and it works quite good. Since the roundtrip happens only in an exceptional case, it is a good solution.

I have noticed if you, misspell the extension, the correctly spelled extension is appended.
I guess that is the expected behavior ...

I think we can merge it after the function name has been adjusted.

daschuer · 2021-12-15T21:48:41Z

@uklotzde Are all your review findings solved.

ronso0 · 2021-12-15T22:17:21Z

I'll address @Swiftb0y's recent comments after implementing the new dialog.

uklotzde

The naming could be improved, but I don't have concerns regarding the code. Merge when ready.

uklotzde · 2021-12-15T22:30:27Z

+    return fileLocation;
+}
+
+QString checkedFilePathWithSelectedExtension(const QString& caption,


Try to name functions starting with an imperative verb if possible. Like good commit messages ;)

ronso0 · 2021-12-16T22:40:24Z

Okay then, thanks for reviewing!
As usual I'll squash all this back and forth changes into a few easily digestible commits.

ronso0 · 2021-12-17T00:02:01Z

Done. Every commit builds and works as intended.

daschuer

LGTM, Thank you for all these bug fixes of this type. That makes really a difference.

ronso0 · 2022-03-17T19:51:55Z

-    m_pConfig->set(ConfigKey("[Library]", "LastImportExportPlaylistDirectory"),
-            ConfigValue(fileName.dir().absolutePath()));
+    QString fileDirectory(playlistFile);
+    fileDirectory.truncate(playlistFile.lastIndexOf(QDir::separator()));


I suspect this always truncates at position 0, see https://bugs.launchpad.net/bugs/1964508

Native separator is \ on Windows and maybe Qt has already converted the path to use /? In that case lastIndexOf("\") would return -1 which is equivalent to passing 0 to truncate
https://doc.qt.io/qt-5/qstring.html#truncate
It is not recommended to use QDir::separator() for constructing paths, but appearantly it also creates issues when computing paths
https://doc.qt.io/qt-5/qdir.html#separator

ronso0 added the regression label Nov 19, 2021

ronso0 added this to the 2.3.2 milestone Nov 19, 2021

github-actions Bot added the library label Nov 19, 2021

uklotzde suggested changes Nov 22, 2021

View reviewed changes

Comment thread src/library/baseplaylistfeature.cpp Outdated

Comment thread src/library/baseplaylistfeature.cpp Outdated

ronso0 force-pushed the playlist-export-extension branch from 66a1695 to a18d321 Compare November 23, 2021 00:35

ronso0 force-pushed the playlist-export-extension branch from eb06a87 to 62d59c5 Compare November 23, 2021 02:16

uklotzde suggested changes Nov 23, 2021

View reviewed changes

ronso0 force-pushed the playlist-export-extension branch from 62d59c5 to aa72e10 Compare November 24, 2021 00:15

uklotzde reviewed Dec 3, 2021

View reviewed changes

Comment thread src/library/baseplaylistfeature.cpp Outdated

Comment thread src/library/baseplaylistfeature.cpp Outdated

Comment thread src/library/crate/cratefeature.cpp Outdated

Comment thread src/library/crate/cratefeature.cpp Outdated

Comment thread src/util/string.h Outdated

Swiftb0y reviewed Dec 3, 2021

View reviewed changes

Comment thread src/util/string.h Outdated

Comment thread src/util/string.h Outdated

Comment thread src/library/crate/cratefeature.cpp

ronso0 force-pushed the playlist-export-extension branch from aa72e10 to 22d2a47 Compare December 3, 2021 22:22

Swiftb0y reviewed Dec 3, 2021

View reviewed changes

Comment thread src/util/string.h Outdated

Comment thread src/util/string.h Outdated

Swiftb0y force-pushed the playlist-export-extension branch from 7a6241b to f07b964 Compare December 9, 2021 00:02

Swiftb0y requested a review from uklotzde December 9, 2021 01:29

Swiftb0y reviewed Dec 12, 2021

View reviewed changes

Swiftb0y approved these changes Dec 12, 2021

View reviewed changes

ronso0 force-pushed the playlist-export-extension branch from cc15cce to 371c925 Compare December 12, 2021 18:28

ronso0 added the changelog This PR should be included in the changelog label Dec 12, 2021

ronso0 mentioned this pull request Dec 12, 2021

Update CHANGELOG.md for 2.3.2 #4563

Merged

ronso0 force-pushed the playlist-export-extension branch from 371c925 to 4ead4ac Compare December 12, 2021 19:21

Swiftb0y reviewed Dec 12, 2021

View reviewed changes

daschuer reviewed Dec 13, 2021

View reviewed changes

Comment thread src/util/file.cpp

ronso0 mentioned this pull request Dec 14, 2021

fix & improve Sampler export #4539

Merged

daschuer reviewed Dec 15, 2021

View reviewed changes

uklotzde reviewed Dec 15, 2021

View reviewed changes

uklotzde approved these changes Dec 15, 2021

View reviewed changes

ronso0 added 4 commits December 17, 2021 00:20

add file export dialog helper to ensure selected extension

1b3b661

use file dialog helper for plalyist export

6c21929

use const ConfigKey for playlist/crate import/export

6b78d70

playlist import/export: consisten var names, more const

02705f6

ronso0 force-pushed the playlist-export-extension branch from c14ccfd to 02705f6 Compare December 17, 2021 00:00

daschuer approved these changes Dec 17, 2021

View reviewed changes

daschuer merged commit 4e0ae6e into mixxxdj:2.3 Dec 17, 2021

ronso0 deleted the playlist-export-extension branch December 17, 2021 11:57

ronso0 mentioned this pull request Jan 15, 2022

2.3.2 release #4625

Closed

ronso0 commented Mar 17, 2022

View reviewed changes

ronso0 mentioned this pull request Mar 17, 2022

Samplers, crates, playlists: fix storing import/export paths #4699

Merged

This was referenced Aug 22, 2022

When exporting a playlist, the extension is not autocompleted #8030

Closed

Playlist Save Dialog Linux only saves m3u #10066

Closed

Path to my playlist not remembered (Windows) #10679

Closed

ronso0 mentioned this pull request Mar 5, 2023

Export Playlist file naming issue #11327

Closed

		const ConfigKey kConfigKeyLastImportExportCrateDirectoryKey(
		"[Library]", "last_import_export_crate_directory");

Uh oh!

Conversation

ronso0 commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ronso0 commented Nov 23, 2021

Uh oh!

ronso0 commented Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ronso0 commented Nov 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ronso0 commented Dec 8, 2021

Uh oh!

Swiftb0y commented Dec 8, 2021

Uh oh!

ronso0 commented Dec 9, 2021

Uh oh!

Swiftb0y Dec 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronso0 commented Dec 12, 2021

Uh oh!

Swiftb0y left a comment

Choose a reason for hiding this comment

Uh oh!

ronso0 commented Dec 12, 2021

Uh oh!

ronso0 commented Dec 12, 2021

Uh oh!

ronso0 commented Dec 12, 2021

Uh oh!

Swiftb0y left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Swiftb0y Dec 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronso0 commented Nov 19, 2021 •

edited

Loading

ronso0 commented Nov 23, 2021 •

edited

Loading

Swiftb0y Dec 12, 2021 •

edited

Loading

Swiftb0y left a comment •

edited

Loading

Swiftb0y Dec 12, 2021 •

edited

Loading