Merge pull request #3811 from uklotzde/save-track-result

Do not save missing tracks to prevent inconsistent metadata
mixxxdj · May 1, 2021 · 73a5e66 · 73a5e66
2 parents 1cc3903 + 0fa8c9c
commit 73a5e66
Show file tree

Hide file tree

Showing 5 changed files with 42 additions and 41 deletions.
diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp
@@ -288,15 +288,13 @@ QString TrackDAO::getTrackLocation(TrackId trackId) const {
 }
 
 void TrackDAO::saveTrack(Track* pTrack) const {
-    DEBUG_ASSERT(pTrack);
-    if (!pTrack->isDirty()) {
+    VERIFY_OR_DEBUG_ASSERT(pTrack) {
         return;
     }
+    DEBUG_ASSERT(pTrack->isDirty());
+
     const TrackId trackId = pTrack->getId();
-    // Only update the database if the track has already been added!
-    if (!trackId.isValid()) {
-        return;
-    }
+    DEBUG_ASSERT(trackId.isValid());
     qDebug() << "TrackDAO: Saving track"
             << trackId
             << pTrack->getFileInfo();

diff --git a/src/library/trackcollection.cpp b/src/library/trackcollection.cpp
@@ -532,7 +532,7 @@ bool TrackCollection::updateAutoDjCrate(
     return updateCrate(crate);
 }
 
-void TrackCollection::saveTrack(Track* pTrack) {
+void TrackCollection::saveTrack(Track* pTrack) const {
     DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
 
     m_trackDao.saveTrack(pTrack);

diff --git a/src/library/trackcollection.h b/src/library/trackcollection.h
@@ -156,7 +156,7 @@ class TrackCollection : public QObject,
 
     void relocateDirectory(const QString& oldDir, const QString& newDir);
 
-    void saveTrack(Track* pTrack);
+    void saveTrack(Track* pTrack) const;
 
     QSqlDatabase m_database;
 

diff --git a/src/library/trackcollectionmanager.cpp b/src/library/trackcollectionmanager.cpp
@@ -173,16 +173,13 @@ void TrackCollectionManager::stopLibraryScan() {
     m_pScanner->slotCancel();
 }
 
-bool TrackCollectionManager::saveTrack(const TrackPointer& pTrack) {
+TrackCollectionManager::SaveTrackResult TrackCollectionManager::saveTrack(
+        const TrackPointer& pTrack) const {
     VERIFY_OR_DEBUG_ASSERT(pTrack) {
-        return false;
+        return SaveTrackResult::Skipped;
     }
-    if (!pTrack->isDirty()) {
-        return false;
-    }
-    saveTrack(pTrack.get(), TrackMetadataExportMode::Deferred);
-    DEBUG_ASSERT(!pTrack->isDirty());
-    return true;
+    const auto res = saveTrack(pTrack.get(), TrackMetadataExportMode::Deferred);
+    return res;
 }
 
 // Export metadata and save the track in both the internal database
@@ -191,18 +188,19 @@ void TrackCollectionManager::saveEvictedTrack(Track* pTrack) noexcept {
     saveTrack(pTrack, TrackMetadataExportMode::Immediate);
 }
 
-void TrackCollectionManager::saveTrack(
+TrackCollectionManager::SaveTrackResult TrackCollectionManager::saveTrack(
         Track* pTrack,
-        TrackMetadataExportMode mode) {
+        TrackMetadataExportMode mode) const {
     DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
-    DEBUG_ASSERT(pTrack);
+    VERIFY_OR_DEBUG_ASSERT(pTrack) {
+        return SaveTrackResult::Skipped;
+    }
     DEBUG_ASSERT(pTrack->getDateAdded().isValid());
 
     // The dirty flag is reset while saving the track in the internal
     // collection!
-    const bool trackDirty = pTrack->isDirty();
-    if (!trackDirty) {
-        return;
+    if (!pTrack->isDirty()) {
+        return SaveTrackResult::Skipped;
     }
 
     const auto fileInfo = pTrack->getFileInfo();
@@ -218,7 +216,7 @@ void TrackCollectionManager::saveTrack(
         kLogger.debug()
                 << "Skip saving of missing track"
                 << fileInfo.location();
-        return;
+        return SaveTrackResult::Skipped;
     }
 
     // This operation must be executed synchronously while the cache is
@@ -229,24 +227,23 @@ void TrackCollectionManager::saveTrack(
             << pTrack->getLocation()
             << "in internal collection";
     m_pInternalCollection->saveTrack(pTrack);
+    const auto res = pTrack->isDirty() ? SaveTrackResult::Failed : SaveTrackResult::Saved;
 
     if (m_externalCollections.isEmpty()) {
-        return;
+        return res;
     }
     if (pTrack->getId().isValid()) {
         // Track still exists in the internal collection/database
-        if (trackDirty) {
-            kLogger.debug()
-                    << "Saving modified track"
-                    << pTrack->getLocation()
-                    << "in"
-                    << m_externalCollections.size()
-                    << "external collection(s)";
-            for (const auto& externalTrackCollection : qAsConst(m_externalCollections)) {
-                externalTrackCollection->saveTrack(
-                        *pTrack,
-                        ExternalTrackCollection::ChangeHint::Modified);
-            }
+        kLogger.debug()
+                << "Saving modified track"
+                << pTrack->getLocation()
+                << "in"
+                << m_externalCollections.size()
+                << "external collection(s)";
+        for (const auto& externalTrackCollection : qAsConst(m_externalCollections)) {
+            externalTrackCollection->saveTrack(
+                    *pTrack,
+                    ExternalTrackCollection::ChangeHint::Modified);
         }
     } else {
         // Track has been deleted from the internal collection/database
@@ -262,6 +259,9 @@ void TrackCollectionManager::saveTrack(
                     QStringList{pTrack->getLocation()});
         }
     }
+    // After saving a track successfully the dirty flag must have been reset
+    DEBUG_ASSERT(!(res == SaveTrackResult::Saved && pTrack->isDirty()));
+    return res;
 }
 
 void TrackCollectionManager::exportTrackMetadata(

diff --git a/src/library/trackcollectionmanager.h b/src/library/trackcollectionmanager.h
@@ -65,9 +65,12 @@ class TrackCollectionManager: public QObject,
     // Save the track in both the internal database and external collections.
     // Export of metadata is deferred until the track is evicted from the
     // cache to prevent file corruption due to concurrent access.
-    // Returns true if the track was dirty and has been saved, otherwise
-    // false.
-    bool saveTrack(const TrackPointer& pTrack);
+    enum class SaveTrackResult {
+        Saved,
+        Skipped, // e.g. unmodified or missing/deleted tracks
+        Failed,
+    };
+    SaveTrackResult saveTrack(const TrackPointer& pTrack) const;
 
   signals:
     void libraryScanStarted();
@@ -90,9 +93,9 @@ class TrackCollectionManager: public QObject,
         Immediate,
         Deferred,
     };
-    void saveTrack(
+    SaveTrackResult saveTrack(
             Track* pTrack,
-            TrackMetadataExportMode mode);
+            TrackMetadataExportMode mode) const;
     void exportTrackMetadata(
             Track* pTrack,
             TrackMetadataExportMode mode) const;