From b26ddff2a2e6acea511afca2495529309b23c9bf Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 26 Sep 2019 15:36:39 +0200 Subject: [PATCH 1/4] More restrictive artist/title parsing from file names - at least the title must be empty - splitting into artist/title only if both fields are empty --- src/sources/soundsourceproxy.cpp | 86 ++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 32 deletions(-) diff --git a/src/sources/soundsourceproxy.cpp b/src/sources/soundsourceproxy.cpp index 6bfa20774142..2b4155a73978 100644 --- a/src/sources/soundsourceproxy.cpp +++ b/src/sources/soundsourceproxy.cpp @@ -299,36 +299,42 @@ void SoundSourceProxy::initSoundSource() { } namespace { -// Parses artist/title from the file name and returns the file type. -// Assumes that the file name is written like: "artist - title.xxx" -// or "artist_-_title.xxx". -// This function does not overwrite any existing (non-empty) artist -// and title fields! -bool parseMetadataFromFileName(mixxx::TrackMetadata* pTrackMetadata, QString fileName) { - fileName.replace("_", " "); - QString titleWithFileType; - bool parsed = false; - if (fileName.count('-') == 1) { - if (pTrackMetadata->getTrackInfo().getArtist().isEmpty()) { - const QString artist(fileName.section('-', 0, 0).trimmed()); + +const QString kArtistTitleSeparator("_-_"); + +// Parses only title or artist/title from the file name and returns true +// if the track has been modified. Assumes that the file name is written +// like "artist - title.xxx" or "artist_-_title.xxx". +bool parseArtistTitleFromFileName( + mixxx::TrackMetadata* pTrackMetadata, + QString fileName, + bool splitArtistTitle) { + bool modified = false; + QString titleWithFileType = fileName.trimmed(); + if (splitArtistTitle) { + fileName.replace(" - ", kArtistTitleSeparator); + if (fileName.count(kArtistTitleSeparator) == 1) { + auto artist = fileName.section(kArtistTitleSeparator, 0, 0).trimmed(); if (!artist.isEmpty()) { pTrackMetadata->refTrackInfo().setArtist(artist); - parsed = true; + modified = true; } + titleWithFileType = fileName.section(kArtistTitleSeparator, 1).trimmed(); } - titleWithFileType = fileName.section('-', 1, 1).trimmed(); - } else { - titleWithFileType = fileName.trimmed(); } - if (pTrackMetadata->getTrackInfo().getTitle().isEmpty()) { - const QString title(titleWithFileType.section('.', 0, -2).trimmed()); - if (!title.isEmpty()) { - pTrackMetadata->refTrackInfo().setTitle(title); - parsed = true; - } + auto title = titleWithFileType; + if (titleWithFileType.contains('.')) { + // Strip file extension starting at the right-most '.' + title = titleWithFileType.section('.', 0, -2); + } + title = title.trimmed(); + if (!title.isEmpty()) { + pTrackMetadata->refTrackInfo().setTitle(title); + modified = true; } - return parsed; + return modified; } + } // anonymous namespace void SoundSourceProxy::updateTrackFromSource( @@ -433,16 +439,32 @@ void SoundSourceProxy::updateTrackFromSource( } } - // Fallback: If artist or title fields are blank then try to populate - // them from the file name. This might happen if tags are unavailable, - // unreadable, or partially/completely missing. - if (trackMetadata.getTrackInfo().getArtist().isEmpty() || - trackMetadata.getTrackInfo().getTitle().isEmpty()) { - kLogger.info() - << "Adding missing artist/title from file name" - << getUrl().toString(); + // Fallback: If the title field is empty then try to populate title + // (and optionally artist) from the file name. This might happen if + // tags are unavailable, unreadable, or partially/completely missing. + if (trackMetadata.getTrackInfo().getTitle().trimmed().isEmpty()) { + // Only parse artist and title if both fields are empty to avoid + // inconsistencies. Otherwise the file name (without extension) + // is used as the title and the artist is unmodified. + // + // TODO(XXX): Disable splitting of artist/title in settings, i.e. + // optionally don't split even if both title and artist are empty? + // Some users might want to import the whole file name of untagged + // files as the title without splitting the artist: + // https://www.mixxx.org/forums/viewtopic.php?f=3&t=12838 + // NOTE(uklotzde, 2019-09-26): Whoever needs this should simply set + // splitArtistTitle = false here and compile their custom version! + // It is not worth extending the settings and injecting them into + // SoundSourceProxy for just a few people. + const bool splitArtistTitle = + trackMetadata.getTrackInfo().getArtist().trimmed().isEmpty(); const auto trackFile = m_pTrack->getFileInfo(); - if (parseMetadataFromFileName(&trackMetadata, trackFile.fileName()) && + kLogger.info() + << "Parsing missing" + << (splitArtistTitle ? "artist/title" : "title") + << "from file name:" + << trackFile; + if (parseArtistTitleFromFileName(&trackMetadata, trackFile.fileName(), splitArtistTitle) && metadataImported.second.isNull()) { // Since this is also some kind of metadata import, we mark the // track's metadata as synchronized with the time stamp of the file. From 98873d98addd8f8f3e51a71dcb5cb7805a964f3f Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 26 Sep 2019 22:58:09 +0200 Subject: [PATCH 2/4] Move file name parsing into TrackInfo --- src/sources/soundsourceproxy.cpp | 41 +------------------------------- src/track/trackinfo.cpp | 38 +++++++++++++++++++++++++++++ src/track/trackinfo.h | 5 ++++ 3 files changed, 44 insertions(+), 40 deletions(-) diff --git a/src/sources/soundsourceproxy.cpp b/src/sources/soundsourceproxy.cpp index 2b4155a73978..79d12b4e936e 100644 --- a/src/sources/soundsourceproxy.cpp +++ b/src/sources/soundsourceproxy.cpp @@ -298,45 +298,6 @@ void SoundSourceProxy::initSoundSource() { } } -namespace { - -const QString kArtistTitleSeparator("_-_"); - -// Parses only title or artist/title from the file name and returns true -// if the track has been modified. Assumes that the file name is written -// like "artist - title.xxx" or "artist_-_title.xxx". -bool parseArtistTitleFromFileName( - mixxx::TrackMetadata* pTrackMetadata, - QString fileName, - bool splitArtistTitle) { - bool modified = false; - QString titleWithFileType = fileName.trimmed(); - if (splitArtistTitle) { - fileName.replace(" - ", kArtistTitleSeparator); - if (fileName.count(kArtistTitleSeparator) == 1) { - auto artist = fileName.section(kArtistTitleSeparator, 0, 0).trimmed(); - if (!artist.isEmpty()) { - pTrackMetadata->refTrackInfo().setArtist(artist); - modified = true; - } - titleWithFileType = fileName.section(kArtistTitleSeparator, 1).trimmed(); - } - } - auto title = titleWithFileType; - if (titleWithFileType.contains('.')) { - // Strip file extension starting at the right-most '.' - title = titleWithFileType.section('.', 0, -2); - } - title = title.trimmed(); - if (!title.isEmpty()) { - pTrackMetadata->refTrackInfo().setTitle(title); - modified = true; - } - return modified; -} - -} // anonymous namespace - void SoundSourceProxy::updateTrackFromSource( ImportTrackMetadataMode importTrackMetadataMode) const { DEBUG_ASSERT(m_pTrack); @@ -464,7 +425,7 @@ void SoundSourceProxy::updateTrackFromSource( << (splitArtistTitle ? "artist/title" : "title") << "from file name:" << trackFile; - if (parseArtistTitleFromFileName(&trackMetadata, trackFile.fileName(), splitArtistTitle) && + if (trackMetadata.refTrackInfo().parseArtistTitleFromFileName(trackFile.fileName(), splitArtistTitle) && metadataImported.second.isNull()) { // Since this is also some kind of metadata import, we mark the // track's metadata as synchronized with the time stamp of the file. diff --git a/src/track/trackinfo.cpp b/src/track/trackinfo.cpp index 5f257fa39ec6..72eaf64e9c36 100644 --- a/src/track/trackinfo.cpp +++ b/src/track/trackinfo.cpp @@ -23,6 +23,44 @@ void TrackInfo::resetUnsupportedValues() { setWork(QString()); } +namespace { + +const QString kArtistTitleSeparator = "_-_"; + +const QChar kFileExtensionSeparator = '.'; + +} // anonymous namespace + +bool TrackInfo::parseArtistTitleFromFileName( + QString fileName, + bool splitArtistTitle) { + bool modified = false; + fileName = fileName.trimmed(); + auto titleWithFileType = fileName; + if (splitArtistTitle) { + fileName.replace(" - ", kArtistTitleSeparator); + if (fileName.count(kArtistTitleSeparator) == 1) { + auto artist = fileName.section(kArtistTitleSeparator, 0, 0).trimmed(); + if (!artist.isEmpty()) { + setArtist(artist); + modified = true; + } + titleWithFileType = fileName.section(kArtistTitleSeparator, 1).trimmed(); + } + } + auto title = titleWithFileType; + if (titleWithFileType.contains(kFileExtensionSeparator)) { + // Strip file extension starting at the right-most '.' + title = titleWithFileType.section(kFileExtensionSeparator, 0, -2); + } + title = title.trimmed(); + if (!title.isEmpty()) { + setTitle(title); + modified = true; + } + return modified; +} + bool operator==(const TrackInfo& lhs, const TrackInfo& rhs) { return (lhs.getArtist() == rhs.getArtist()) && (lhs.getBpm() == rhs.getBpm()) && diff --git a/src/track/trackinfo.h b/src/track/trackinfo.h index 27c63fb0ea8b..b20b3f4d5ab7 100644 --- a/src/track/trackinfo.h +++ b/src/track/trackinfo.h @@ -55,6 +55,11 @@ class TrackInfo final { TrackInfo& operator=(TrackInfo&&) = default; TrackInfo& operator=(const TrackInfo&) = default; + // Returns true if modified + bool parseArtistTitleFromFileName( + QString fileName, + bool splitArtistTitle); + // TODO(XXX): Remove after all new fields have been added to the library void resetUnsupportedValues(); From 2c0430ff242eb3e86ee650b84419b722218f0374 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 26 Sep 2019 23:35:44 +0200 Subject: [PATCH 3/4] Add tests for parsing artist/title from file names --- src/test/trackmetadata_test.cpp | 63 +++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 src/test/trackmetadata_test.cpp diff --git a/src/test/trackmetadata_test.cpp b/src/test/trackmetadata_test.cpp new file mode 100644 index 000000000000..0bdd95891bcd --- /dev/null +++ b/src/test/trackmetadata_test.cpp @@ -0,0 +1,63 @@ +#include + +#include "track/trackmetadata.h" + +class TrackMetadataTest : public testing::Test { +}; + +TEST_F(TrackMetadataTest, parseArtistTitleFromFileName) { + { + mixxx::TrackInfo trackInfo; + trackInfo.parseArtistTitleFromFileName(" only - title ", false); + EXPECT_EQ(QString(), trackInfo.getArtist()); + EXPECT_EQ("only - title", trackInfo.getTitle()); + } + { + mixxx::TrackInfo trackInfo; + trackInfo.parseArtistTitleFromFileName(" only-title ", true); + EXPECT_EQ(QString(), trackInfo.getArtist()); + EXPECT_EQ("only-title", trackInfo.getTitle()); + } + { + mixxx::TrackInfo trackInfo; + trackInfo.parseArtistTitleFromFileName(" only -_title ", true); + EXPECT_EQ(QString(), trackInfo.getArtist()); + EXPECT_EQ("only -_title", trackInfo.getTitle()); + } + { + mixxx::TrackInfo trackInfo; + trackInfo.parseArtistTitleFromFileName(" only - title ", false); + EXPECT_EQ(QString(), trackInfo.getArtist()); + EXPECT_EQ("only - title", trackInfo.getTitle()); + } + { + mixxx::TrackInfo trackInfo; + trackInfo.parseArtistTitleFromFileName(" artist - title ", true); + EXPECT_EQ("artist", trackInfo.getArtist()); + EXPECT_EQ("title", trackInfo.getTitle()); + } + { + mixxx::TrackInfo trackInfo; + trackInfo.parseArtistTitleFromFileName(" only -\ttitle\t", true); + EXPECT_EQ(QString(), trackInfo.getArtist()); + EXPECT_EQ("only -\ttitle", trackInfo.getTitle()); + } + { + mixxx::TrackInfo trackInfo; + trackInfo.parseArtistTitleFromFileName(" - artist__-__title - ", true); + EXPECT_EQ("- artist_", trackInfo.getArtist()); + EXPECT_EQ("_title -", trackInfo.getTitle()); + } + { + mixxx::TrackInfo trackInfo; + trackInfo.parseArtistTitleFromFileName(" - only__-__title_-_", true); + EXPECT_EQ(QString(), trackInfo.getArtist()); + EXPECT_EQ("- only__-__title_-_", trackInfo.getTitle()); + } + { + mixxx::TrackInfo trackInfo; + trackInfo.parseArtistTitleFromFileName(" again - only_-_title _ ", true); + EXPECT_EQ(QString(), trackInfo.getArtist()); + EXPECT_EQ("again - only_-_title _", trackInfo.getTitle()); + } +} From 8629a7edf714b37b6562ca0f9e8545aa1b8c99c4 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 29 Sep 2019 01:18:45 +0200 Subject: [PATCH 4/4] Define constants for both artist/title separators --- src/track/trackinfo.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/track/trackinfo.cpp b/src/track/trackinfo.cpp index 72eaf64e9c36..1891389a1cc0 100644 --- a/src/track/trackinfo.cpp +++ b/src/track/trackinfo.cpp @@ -25,6 +25,7 @@ void TrackInfo::resetUnsupportedValues() { namespace { +const QString kArtistTitleSeparatorWithSpaces = " - "; const QString kArtistTitleSeparator = "_-_"; const QChar kFileExtensionSeparator = '.'; @@ -38,7 +39,7 @@ bool TrackInfo::parseArtistTitleFromFileName( fileName = fileName.trimmed(); auto titleWithFileType = fileName; if (splitArtistTitle) { - fileName.replace(" - ", kArtistTitleSeparator); + fileName.replace(kArtistTitleSeparatorWithSpaces, kArtistTitleSeparator); if (fileName.count(kArtistTitleSeparator) == 1) { auto artist = fileName.section(kArtistTitleSeparator, 0, 0).trimmed(); if (!artist.isEmpty()) {