fix & improve Sampler export by ronso0 · Pull Request #4539 · mixxxdj/mixxx

ronso0 · 2021-11-23T01:48:17Z

before .xml was added to any specified export file even if it ended with .xml
remember the import/export directory

Holzhaus

If the file "foo.xml" already exists and I type "foo.xml" into the filename field of save file dialog (or if I just select the existing file), I'm asked if I really want to overwrite the existing file. If I just type "foo", the ".xml" suffix is appended and the file is overwritten without warning.

ronso0 · 2021-12-13T18:44:06Z

If the file "foo.xml" already exists and I type "foo.xml" into the filename field of save file dialog (or if I just select the existing file), I'm asked if I really want to overwrite the existing file. If I just type "foo", the ".xml" suffix is appended and the file is overwritten without warning.

I'm really annoyed by havin to work around this Qt bug (missing file extension, which was fixed for Windows btw but not for Linux).
So we could either
A put that dialog into a loop (while file exists) and feed it back as starting point so Qt's own "Overwrite?" mechanism can kick in or
B we show out own oerwrite dialog, like we do for controller mappings?

ronso0 · 2021-12-13T19:03:16Z

Actually I'm in favor of A (IINM we'd just need to feed back the fileFilter and the file location) even though that would create small UX hickup: when clicking Save that would open the dialog again with the file and the extension being preselected. Then Save and you'd get asked if to overwrite or not.

daschuer · 2021-12-13T21:20:41Z

We already have the override Dialog in main? I think this is a good solution, because the is use case for overriding when the user has lets say 4 favorit sets and wants to adjust one of it.

daschuer · 2021-12-13T21:26:07Z

Oh, I now understand. We bypass the current solution by the added extension.
A works for me as well.

ronso0 · 2021-12-13T23:55:18Z

B we show out own oerwrite dialog, like we do for controller mappings?

Downside of this is that it's needed only for Linux environments which don't supply a native file dialog -- all other OS and distros would request overwrite confirmation twice :\

All in all this feels like re-inventing the wheel. We could as well use an own QFileDialog with the root issue fixed.

ronso0 · 2021-12-14T17:29:19Z

I implemented A in #4531. It's the last two commits and for now only implemented for exporting playlists from the Playlist feature.
Please check if this works for you.

ronso0 · 2021-12-17T01:28:02Z

This is now based on #4531 so only the changes in /src/mixer are relevant.
I'll rebase as soon as that is merged.

ronso0 · 2021-12-17T01:30:47Z


+const QString SamplerBank::samplerFiletype() {
+    return tr("Mixxx Sampler Banks (*.xml)");
+}


Doesn't feel this is the smartest way to define a tr string which is used multple times.
Are there any other ways to do that?

QT_TR_NOOP seems to be the weapon of choice, put in the anonymous namespace.
const QString kSamplerFileType = QT_TR_NOOP("Mixxx Sampler Banks (*.xml)");

Let me know what you think.

Shouldn't const QString kSamplerFileType = QObject::tr("Mixxx Sampler Banks (*.xml)"); work just as well? According to Qt docs, QT_TR_NOOP marks a string for "delayed translation", but I don't know what that means, maybe lazy loading of the translated string on first use?

I'll try to test the translation for both to be sure. Never did that before though..

Does nothing:
#define QT_TR_NOOP(x) x
https://github.com/qt/qtbase/blob/eba9196304dcc9129a7bb68db87c6fb42c953090/src/corelib/global/qglobal.h#L1356

But ... lupdate finds it so that later use of tr() with this variable can be translated.

This is demontarted here:
https://github.com/qt/qtbase/blob/fb33e2a8e82cfc28cf3c3c4bad657e0400640cb2/src/corelib/doc/snippets/code/src_corelib_global_qglobal.cpp#L389

This means the current code translates the string on every call. The proposed solution of @Swiftb0y only translates one time. However I am not sure if the translation is initialized before the string is initialized.

Unfortunately I'm not familiar with the translation workflow, so I appreciate any advice how to proceed here.
Edit just noticed you reply below, I'll take a look.

daschuer · 2021-12-17T17:04:45Z

#4531 is now merged. Can you rebase this again?

daschuer · 2021-12-17T17:29:46Z

+
+const ConfigKey kConfigkeyLastImportExportDirectory(
+        "[Samplers]", "last_import_export_directory");
+const QString kSamplerFileType = QT_TR_NOOP("Mixxx Sampler Banks (*.xml)");


Suggested change

const QString kSamplerFileType = QT_TR_NOOP("Mixxx Sampler Banks (*.xml)");

const char kSamplerFileType[] = QT_TR_NOOP("Mixxx Sampler Banks (*.xml)");

can explain why a char[] is preferred here? QT_TR_NOOP is used nowhere else in mixxx so explaining why its used here definitively deserves explanation in the code IMO.

ping
Shall we add a comment, then merge?

hehe, this was meant for @daschuer

QT_TR_NOOP should always used with char[] or char*, because the result will be used as key for fetching the translated string in tr()

https://doc.qt.io/qt-5/qobject.html#tr

Using QString will convert from char[] and back to char*

ronso0 · 2021-12-17T17:57:24Z

Thank you!

I rebased onto 2.3, added recent tr suggestions and made the dialog preselect samplers.xml.
Please check, and if it's okay I'll squash the commits.

daschuer

LGTM.
There is still room for improving redundant translations lookups, but let's move on to more important tasks

Co-authored-by: Daniel Schürmann <daschuer@mixxx.org>

ronso0 · 2021-12-17T18:57:26Z

Alright, done here.

Holzhaus

LGTM, thank you!

ronso0 · 2021-12-30T19:08:42Z

Okay, great. Ready!

daschuer · 2021-12-30T20:48:55Z

Oh sorry, I guess that is my dog food. I will prepare a fixed branch you can reset too, or we can merge this and fix it upstream.
https://github.com/daschuer/mixxx/tree/sampler-export

ronso0 · 2021-12-30T21:44:48Z

~~I don't spot the difference? I commited your comment suggestion, what else is missing?~~

ah, okay: pre-commit, trailing whitespaces
I'll fix and foce-push.

Co-authored-by: Daniel Schürmann <daschuer@mixxx.org>

ronso0 · 2021-12-31T00:12:45Z

Done and CI all green.

daschuer · 2021-12-31T00:23:49Z

Thank you!

ronso0 changed the base branch from main to 2.3 November 23, 2021 01:48

ronso0 added this to the 2.3.2 milestone Nov 23, 2021

ronso0 force-pushed the sampler-export branch from 377b85b to 66854d2 Compare November 24, 2021 01:03

Holzhaus reviewed Nov 24, 2021

View reviewed changes

Comment thread src/mixer/samplerbank.cpp Outdated

ronso0 force-pushed the sampler-export branch from 66854d2 to 9c64640 Compare November 24, 2021 11:26

ronso0 marked this pull request as ready for review November 24, 2021 12:26

Holzhaus reviewed Dec 6, 2021

View reviewed changes

Comment thread src/mixer/samplerbank.cpp Outdated

ronso0 force-pushed the sampler-export branch from 9c64640 to 9c66ff4 Compare December 8, 2021 00:24

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

Holzhaus reviewed Dec 13, 2021

View reviewed changes

Comment thread src/mixer/samplerbank.cpp Outdated

Comment thread src/mixer/samplerbank.cpp

ronso0 mentioned this pull request Dec 13, 2021

playlist export: add missing file extension #4531

Merged

ronso0 marked this pull request as draft December 16, 2021 00:07

ronso0 force-pushed the sampler-export branch from 9c66ff4 to 364635e Compare December 17, 2021 01:27

github-actions Bot added the library label Dec 17, 2021

ronso0 commented Dec 17, 2021

View reviewed changes

daschuer reviewed Dec 17, 2021

View reviewed changes

Comment thread src/mixer/samplerbank.cpp Outdated

ronso0 force-pushed the sampler-export branch from 7cc97e1 to fc9a5a2 Compare December 17, 2021 17:56

daschuer approved these changes Dec 17, 2021

View reviewed changes

sampler import/export: verify file extension, remember directory

9d7ecb5

Co-authored-by: Daniel Schürmann <daschuer@mixxx.org>

ronso0 force-pushed the sampler-export branch from fc9a5a2 to 9d7ecb5 Compare December 17, 2021 18:57

ronso0 marked this pull request as ready for review December 17, 2021 18:58

Holzhaus approved these changes Dec 22, 2021

View reviewed changes

daschuer reviewed Dec 30, 2021

View reviewed changes

Comment thread src/mixer/samplerbank.cpp

samplerbank: explain use of QT_TR_NOOP

555455e

Co-authored-by: Daniel Schürmann <daschuer@mixxx.org>

ronso0 force-pushed the sampler-export branch from 832014e to 555455e Compare December 30, 2021 22:15

daschuer merged commit 32106f9 into mixxxdj:2.3 Dec 31, 2021

ronso0 deleted the sampler-export branch December 31, 2021 00:53

ronso0 mentioned this pull request Mar 17, 2022

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

Merged

	const QString kSamplerFileType = QT_TR_NOOP("Mixxx Sampler Banks (*.xml)");
	const char kSamplerFileType[] = QT_TR_NOOP("Mixxx Sampler Banks (*.xml)");

Uh oh!

Conversation

ronso0 commented Nov 23, 2021

Uh oh!

Uh oh!

Uh oh!

Holzhaus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

ronso0 commented Dec 13, 2021

Uh oh!

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

Uh oh!

Uh oh!

daschuer commented Dec 13, 2021

Uh oh!

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

Uh oh!

Uh oh!

ronso0 commented Dec 14, 2021

Uh oh!

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

Uh oh!

Uh oh!

ronso0 Dec 17, 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!

Swiftb0y Dec 17, 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!

ronso0 Dec 17, 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!

daschuer commented Dec 17, 2021

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!

ronso0 commented Dec 17, 2021

Uh oh!

daschuer left a comment

Choose a reason for hiding this comment

Uh oh!

ronso0 commented Dec 17, 2021

Uh oh!

Holzhaus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ronso0 commented Dec 30, 2021

Uh oh!

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

ronso0 commented Dec 13, 2021 •

edited

Loading

daschuer commented Dec 13, 2021 •

edited

Loading

ronso0 commented Dec 13, 2021 •

edited

Loading

ronso0 commented Dec 17, 2021 •

edited

Loading

ronso0 Dec 17, 2021 •

edited

Loading

Swiftb0y Dec 17, 2021 •

edited

Loading

ronso0 Dec 17, 2021 •

edited

Loading

daschuer commented Dec 30, 2021 •

edited

Loading

ronso0 commented Dec 30, 2021 •

edited

Loading