Fix reading metadata for some file extensions #13218
Conversation
…ough the call stack to avoid guessing it from the file extension.
No longer guess the fileType from the file extension.
Co-authored-by: Antoine Colombier <7086688+acolombier@users.noreply.github.com>
src/test/soundproxy_test.cpp
Outdated
| @@ -1086,3 +1087,14 @@ TEST_F(SoundSourceProxyTest, freeModeGarbage) { | |||
| break; | |||
| } | |||
| } | |||
|
|
|||
| TEST_F(SoundSourceProxyTest, taglibStringToEnumFileType) { | |||
| for (const auto& fileType : SoundSourceProxy::getSupportedFileTypes()) { | |||
|
All green now. |
| @@ -82,7 +83,7 @@ double SeratoTags::guessTimingOffsetMillis( | |||
| // PR for more detailed information: | |||
| // https://github.com/mixxxdj/mixxx/pull/2119 | |||
| double timingOffset = 0; | |||
| if (taglib::getFileTypeFromFileName(filePath) == taglib::FileType::MP3) { | |||
| if (fileType == "mp3") { | |||
There was a problem hiding this comment.
QStringLiteral, otherwise implicitly converts
There was a problem hiding this comment.
Qt has explicit an operator for it and it is faster than using a QStringLiteral:
https://github.com/qt/qtbase/blob/5686af295c98a5ed4b0793ad0098cef76f39a2d7/src/corelib/text/qstring.h#L1376
| taglib::FileType fileType) | ||
| const QString& fileType) | ||
| : m_fileName(fileName), | ||
| m_fileType(fileType) { | ||
| m_fileType(taglib::stringToEnumFileType(fileType)) { |
There was a problem hiding this comment.
whats the advantage of taking a string here instead of shifting the conversion duty to the caller? This smells stringly-typed
There was a problem hiding this comment.
fyi, two of the other three comments were redundant if this would avoid strings better.
There was a problem hiding this comment.
We have the duality of a string typed fileType used in Soundsource classes outside of Taglib and the enum inside Taglib. We still need the "fileType" as string, because it holds the file extension in case the type has not been normalized. This is a left over from the legacy when Mixxx uses just the file extension.
I have moved the conversion here, to hide all implementation details of MetadataSourceTagLib and keep the taglib includes in its cpp file.
Else it would be here:
mixxx/src/sources/soundsource.cpp
Lines 84 to 88 in d1c09b5
Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>
|
Done |
| TypePair{"dff"_L1, FileType::DSDIFF}, | ||
| TypePair{"dsdiff"_L1, FileType::DSDIFF}}; | ||
|
|
||
| const auto it = std::find_if(lookupTable.cbegin(), |
There was a problem hiding this comment.
clang-tidy
| const auto it = std::find_if(lookupTable.cbegin(), | |
| const auto *const it = std::find_if(lookupTable.cbegin(), |
This replaces all file type guessing code form the file extension, by using the normalized file type stored in the Track object.
This makes sure all parts are using the same file type, even if the extension is wrong.
This fixes #13205