Improve parsing of artist/title from file names (+ tests)

src/sources/soundsourceproxy.cpp

@@ -298,39 +298,6 @@ 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());
-            if (!artist.isEmpty()) {
-                pTrackMetadata->refTrackInfo().setArtist(artist);
-                parsed = true;
-            }
-        }
-        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;
-        }
-    }
-    return parsed;
-}
-} // anonymous namespace
-
 void SoundSourceProxy::updateTrackFromSource(
         ImportTrackMetadataMode importTrackMetadataMode) const {
     DEBUG_ASSERT(m_pTrack);
@@ -433,16 +400,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 (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.

src/test/trackmetadata_test.cpp

@@ -0,0 +1,63 @@
+#include <gtest/gtest.h>
+
+#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());
+    }
+}

src/track/trackinfo.cpp

@@ -23,6 +23,45 @@ void TrackInfo::resetUnsupportedValues() {
     setWork(QString());
 }
 
+namespace {
+
+const QString kArtistTitleSeparatorWithSpaces = " - ";
+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(kArtistTitleSeparatorWithSpaces, 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()) &&

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();