Better file export #3572
Better file export #3572
Conversation
* major rewrite of the export dialog * use a table to log all exported files and their errors * use template language to format the output destination * don't fail on errors, just log them * remove now obsolete export wizzard
|
Build fails: did you miss to add the FindGrantlee5.cmake file? You need also add libgrantlee5-dev to the tools/debian_buildenv.sh and to the packaging/debian/control.in file. |
|
According to this we need no own FindGrandlee file: http://www.grantlee.org/apidox/using_and_deploying.html |
src/library/export/trackexportdlg.h
Outdated
| TrackExportDlg(QWidget* parent, | ||
| UserSettingsPointer pConfig, | ||
| TrackPointerList& tracks, | ||
| Grantlee::Context* context = nullptr); |
There was a problem hiding this comment.
Can we remove the default nullptr?
There was a problem hiding this comment.
Why ? You can pass a custom context if you have additional information you want to pass. There are cases now that do not have any additional context to give, so they pass nothing.
|
& is exported as
It would be nice if we also export the playlist file to loose not the order of the tracks. By default we should pick a sub folder that is named like the playlist. This can be made optional. |
does not require you to create a playlist for exporting files
|
@daschuer you will be able to use the pattern like "{{playlist.name}}/{{index}} {{track.trackFilename}}" when exporting a playlist. |
|
Ah cool. Can that be made default? It requires some extra brain rounding to understand that the folder has to be placed in the file field. Probably to hard for non tech guys. Can this be added to the folder field instead? |
|
The Export folder is where the playlist mp3u will be placed and you can put your files anywhere. Also when djintorop library is landed and the export option active, the library will be searched there. |
|
I also try to get {{track.key}} to print something, but I don't know how to register a new type in grantlee that it has a transform handler. |
Use a Custom Formatter to escape all tags that are not universal compatible.
|
@daschuer can you check if the hanging still occurs ? I was not able to reproduce that |
|
Yes. Does it build? At least CI fails. |
|
@daschuer yes. I will fix the clazy complains today |
|
Thank you. It is getting better. Luckily I was able to catch the crasher in GDB. |
|
This is how the dialog pops up:
I think the buttons should go below the tabs, because they are valid for both. |
|
I found also a crash when '{{ }}' is entered and opened a ticket in grantlee: '{{}}' did not crash here, only as soon as a space is inserted |
|
I think I will have to fix this myself and we have to ship with a copy of grentlee. Alternatively we could look into cutelee which is a fork with non specified small differences. Don't know if cutelee will also crash there and if it is more active then grantlee. I choose it only because it is commenly shipped with distros. |
|
Interestingly, in the grantlee tests I found exactly this case as a test, seems I have to digg further what is going on. |
|
It would be much more convenient to have a workaround in our code. This way we don't need to ship the patched grantlee library in our lib folder. |
|
@daschuer I was able to fix the crash without fixing grantlee. |
| QString fullPattern; | ||
| if (m_pattern) { | ||
| QString trimmed = m_pattern->trimmed(); | ||
| fullPattern = m_destDir + QDir::separator().toLatin1() + trimmed; |
There was a problem hiding this comment.
I think we can omit .toLatin1() here and below.
| @@ -30,6 +30,7 @@ const auto kResultCantCreateFile = QStringLiteral("Could not create file"); | |||
| const auto kDefaultPattern = QStringLiteral( | |||
| "{{ track.basename }}{% if dup %}-{{dup}}{% endif %}" | |||
| ".{{track.extension}}"); | |||
| const auto kEmptyMsg = QLatin1String(""); | |||
There was a problem hiding this comment.
This can become just:
const QString kEmptyMsg;
| @@ -23,6 +24,32 @@ PlaylistDAO::PlaylistDAO() | |||
| void PlaylistDAO::initialize(const QSqlDatabase& database) { | |||
| DAO::initialize(database); | |||
| populatePlaylistMembershipCache(); | |||
|
|
|||
| // create temporary views | |||
| QString queryString = QLatin1String( | |||
There was a problem hiding this comment.
| QString queryString = QLatin1String( | |
| QString queryString = QStringLiteral( |
| if (QFileInfo::exists(playlistFilePath)) { | ||
| auto overwrite = QMessageBox::question( | ||
| nullptr, | ||
| tr("Overwrite File?"), |
There was a problem hiding this comment.
This sound techy to me. How about
How about:
"Playlist file ready exists"
"A playlist file with the name "%1" already exists.\n"
"The default "m3u" extension was added because none was specified.\n\n"
"Do you really want to replace it?"
There was a problem hiding this comment.
Why do we have only the message box in case the extension was added?
There was a problem hiding this comment.
Jup, let's have a common way to deal with file overwrite.
For replacing an exitisting controller mapping there is:
https://github.com/mixxxdj/mixxx/blob/main/src/controllers/dlgprefcontroller.cpp#L559-L563
QString overwriteTitle = tr("Mapping already exists.");
QString overwriteLabel = tr(
"<b>%1</b> already exists in user mapping folder.<br>"
"Overwrite or save with a new name?");
QString overwriteCheckLabel = tr("Always overwrite during this session");
| } else { | ||
| //default export to M3U if file extension is missing | ||
| if (!playlistFilePath.endsWith( | ||
| QStringLiteral(".m3u"), |
There was a problem hiding this comment.
The m3u playlist is Windows-1252 only.
Should we default to m3u8 to avoid data loss?
The attempt to export a m3u playlist with invalid charters fails with a message box.
There was a problem hiding this comment.
off-topic but related: I think we should have a helper class to check for valid filenames that alows to reduce redundancy (mapping export, playlist export, effect chain export and what not).
There was a problem hiding this comment.
I already added helper functions here to ensure a generated filename is readable on all platforms (afaik), as well as template generators and escape filters.
i would like to later consolidate the crates export (djinterop) to just be part of this export. later also transcoding into different fileformats while exporting (cdj compatible format (aiff/wav)), etc.
|
This PR is marked as stale because it has been open 90 days with no activity. |
github-actions
bot
added
the
stale
@poelzi I added a Grantlee port to VCPKG upstream: microsoft/vcpkg#28809 |
|
Cool, thank you for pushing the merge forward. |
Greatly enhance the track export feature.
Done
TODO
Future