Fix writing metadata via symlink and add a test that verified the iss…

…ue and the fix.
daschuer · Sep 30, 2024 · 3f3bd0c · 3f3bd0c
1 parent 794c71f
commit 3f3bd0c
Show file tree

Hide file tree

Showing 2 changed files with 69 additions and 8 deletions.
diff --git a/src/test/taglibtest.cpp b/src/test/taglibtest.cpp
@@ -70,3 +70,61 @@ TEST_F(TagLibTest, WriteID3v2Tag) {
         EXPECT_FALSE(mixxx::taglib::hasAPETag(mpegFile));
     }
 }
+
+#ifndef __WINDOWS__ // Note: Following Windows links (*.lnk shortcuts) is not supported by Mixxx yet
+TEST_F(TagLibTest, WriteID3v2TagViaLink) {
+    QTemporaryDir tempDir;
+    ASSERT_TRUE(tempDir.isValid());
+
+    // Generate a file name for the temporary file
+    const QString tmpFileName = tempDir.filePath("no_id3v1_mp3");
+
+    // Create the temporary file by copying an existing file
+    mixxxtest::copyFile(
+            MixxxTest::getOrInitTestDir().filePath(QStringLiteral("id3-test-data/empty.mp3")),
+            tmpFileName);
+
+    const QString linkFileName = tempDir.filePath("no_id3v1_mp3_link");
+    EXPECT_TRUE(QFile::link(tmpFileName, linkFileName));
+
+    auto linkFileInfoBefore = QFileInfo(linkFileName);
+    EXPECT_TRUE(linkFileInfoBefore.exists());
+    EXPECT_TRUE(linkFileInfoBefore.isSymLink());
+    EXPECT_EQ(linkFileInfoBefore.canonicalFilePath().toStdString(), tmpFileName.toStdString());
+
+    // Verify that the file has no tags
+    {
+        TagLib::MPEG::File mpegFile(
+                TAGLIB_FILENAME_FROM_QSTRING(linkFileName));
+        EXPECT_FALSE(mixxx::taglib::hasID3v1Tag(mpegFile));
+        EXPECT_FALSE(mixxx::taglib::hasID3v2Tag(mpegFile));
+        EXPECT_FALSE(mixxx::taglib::hasAPETag(mpegFile));
+    }
+
+    qDebug() << "Setting track title";
+
+    // Write metadata -> only an ID3v2 tag should be added
+    mixxx::TrackMetadata trackMetadata;
+    trackMetadata.refTrackInfo().setTitle(QStringLiteral("title"));
+    const auto exported =
+            mixxx::MetadataSourceTagLib(
+                    linkFileName, "mp3")
+                    .exportTrackMetadata(trackMetadata);
+    ASSERT_EQ(mixxx::MetadataSource::ExportResult::Succeeded, exported.first);
+    ASSERT_FALSE(exported.second.isNull());
+
+    // Check that the file only has an ID3v2 tag after writing metadata
+    {
+        TagLib::MPEG::File mpegFile(
+                TAGLIB_FILENAME_FROM_QSTRING(linkFileName));
+        EXPECT_FALSE(mixxx::taglib::hasID3v1Tag(mpegFile));
+        EXPECT_TRUE(mixxx::taglib::hasID3v2Tag(mpegFile));
+        EXPECT_FALSE(mixxx::taglib::hasAPETag(mpegFile));
+    }
+
+    auto linkFileInfoAfter = QFileInfo(linkFileName);
+
+    EXPECT_TRUE(linkFileInfoAfter.exists());
+    EXPECT_EQ(linkFileInfoAfter.canonicalFilePath().toStdString(), tmpFileName.toStdString());
+}
+#endif
diff --git a/src/util/safelywritablefile.cpp b/src/util/safelywritablefile.cpp
@@ -58,8 +58,10 @@ SafelyWritableFile::SafelyWritableFile(QString origFileName,
     }
     switch (safetyMode) {
     case SafetyMode::Edit: {
-        QString tempFileName = origFileName + kSafelyWritableTempFileSuffix;
-        QFile origFile(origFileName);
+        QString origFilePath = QFileInfo(origFileName).canonicalFilePath();
+        QString tempFileName = origFilePath + kSafelyWritableTempFileSuffix;
+        QFile origFile(origFilePath);
+
         if (!origFile.copy(tempFileName)) {
             kLogger.warning()
                     << origFile.errorString()
@@ -135,15 +137,16 @@ bool SafelyWritableFile::commit() {
         return true; // nothing to do
     }
 
-    QString backupFileName = m_origFileName + kSafelyWritableOrigFileSuffix;
+    QString origFilePath = QFileInfo(m_origFileName).canonicalFilePath();
+    QString backupFileName = origFilePath + kSafelyWritableOrigFileSuffix;
 #ifdef __WINDOWS__
     // After Mixxx has closed the track file, the indexer or virus scanner
     // might kick in and fail ReplaceFileW() with a sharing violation when
     // replacing the original file with the one with the updated metadata.
     int i = 0;
     for (; i < kWindowsSharingViolationMaxRetries; ++i) {
         if (ReplaceFileW(
-                    reinterpret_cast<LPCWSTR>(m_origFileName.utf16()),
+                    reinterpret_cast<LPCWSTR>(origFilePath.utf16()),
                     reinterpret_cast<LPCWSTR>(m_tempFileName.utf16()),
                     reinterpret_cast<LPCWSTR>(backupFileName.utf16()),
                     REPLACEFILE_IGNORE_MERGE_ERRORS | REPLACEFILE_IGNORE_ACL_ERRORS,
@@ -238,7 +241,7 @@ bool SafelyWritableFile::commit() {
                 << newFile.fileName();
         return false;
     }
-    QFile oldFile(m_origFileName);
+    QFile oldFile(origFilePath);
     if (oldFile.exists()) {
         DEBUG_ASSERT(!QFile::exists(backupFileName)); // very unlikely, otherwise renaming fails
         if (!oldFile.rename(backupFileName)) {
@@ -251,8 +254,8 @@ bool SafelyWritableFile::commit() {
             return false;
         }
     }
-    DEBUG_ASSERT(!QFile::exists(m_origFileName));
-    if (!newFile.rename(m_origFileName)) {
+    DEBUG_ASSERT(!QFile::exists(origFilePath));
+    if (!newFile.rename(origFilePath)) {
         kLogger.critical()
                 << newFile.errorString()
                 << "- Failed to rename temporary file after writing:"
@@ -261,7 +264,7 @@ bool SafelyWritableFile::commit() {
                 << m_origFileName;
         if (oldFile.exists()) {
             // Try to restore the original file
-            if (!oldFile.rename(m_origFileName)) {
+            if (!oldFile.rename(origFilePath)) {
                 // Undo operation failed
                 kLogger.warning()
                         << oldFile.errorString()