Merge pull request #4603 from uklotzde/trackdao

TrackDAO::saveTrack(): Return success/failure result

src/library/dao/trackdao.cpp

@@ -299,9 +299,9 @@ QString TrackDAO::getTrackLocation(TrackId trackId) const {
     return trackLocation;
 }
 
-void TrackDAO::saveTrack(Track* pTrack) const {
+bool TrackDAO::saveTrack(Track* pTrack) const {
     VERIFY_OR_DEBUG_ASSERT(pTrack) {
-        return;
+        return false;
     }
     DEBUG_ASSERT(pTrack->isDirty());
 
@@ -310,14 +310,18 @@ void TrackDAO::saveTrack(Track* pTrack) const {
     qDebug() << "TrackDAO: Saving track"
             << trackId
             << pTrack->getFileInfo();
-    if (updateTrack(*pTrack)) {
-        // BaseTrackCache must be informed separately, because the
-        // track has already been disconnected and TrackDAO does
-        // not receive any signals that are usually forwarded to
-        // BaseTrackCache.
-        pTrack->markClean();
-        emit mixxx::thisAsNonConst(this)->trackClean(trackId);
+    if (!updateTrack(*pTrack)) {
+        return false;
     }
+
+    // BaseTrackCache must be informed separately, because the
+    // track has already been disconnected and TrackDAO does
+    // not receive any signals that are usually forwarded to
+    // BaseTrackCache.
+    pTrack->markClean();
+    emit mixxx::thisAsNonConst(this)->trackClean(trackId);
+
+    return true;
 }
 
 void TrackDAO::slotDatabaseTracksChanged(const QSet<TrackId>& changedTrackIds) {

src/library/dao/trackdao.h

@@ -74,7 +74,7 @@ class TrackDAO : public QObject, public virtual DAO, public virtual GlobalTrackC
             volatile const bool* pCancel) const;
 
     // Only used by friend class TrackCollection, but public for testing!
-    void saveTrack(Track* pTrack) const;
+    bool saveTrack(Track* pTrack) const;
 
     /// Update the play counter properties according to the corresponding
     /// aggregated properties obtained from the played history.

src/library/trackcollection.cpp

@@ -532,10 +532,10 @@ bool TrackCollection::updateAutoDjCrate(
     return updateCrate(crate);
 }
 
-void TrackCollection::saveTrack(Track* pTrack) const {
+bool TrackCollection::saveTrack(Track* pTrack) const {
     DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
 
-    m_trackDao.saveTrack(pTrack);
+    return m_trackDao.saveTrack(pTrack);
 }
 
 TrackPointer TrackCollection::getTrackById(

src/library/trackcollection.h

@@ -163,7 +163,7 @@ class TrackCollection : public QObject,
 
     void relocateDirectory(const QString& oldDir, const QString& newDir);
 
-    void saveTrack(Track* pTrack) const;
+    bool saveTrack(Track* pTrack) const;
 
     QSqlDatabase m_database;
 

src/library/trackcollectionmanager.cpp

@@ -1,5 +1,7 @@
 #include "library/trackcollectionmanager.h"
 
+#include <utility>
+
 #include "library/externaltrackcollection.h"
 #include "library/library_prefs.h"
 #include "library/scanner/libraryscanner.h"
@@ -57,7 +59,7 @@ TrackCollectionManager::TrackCollectionManager(
     } else {
         // TODO: Add external collections
     }
-    for (const auto& externalCollection : qAsConst(m_externalCollections)) {
+    for (const auto& externalCollection : std::as_const(m_externalCollections)) {
         kLogger.info()
                 << "Connecting to"
                 << externalCollection->name();
@@ -149,7 +151,7 @@ TrackCollectionManager::~TrackCollectionManager() {
     // components are accessing those files at this point.
     GlobalTrackCacheLocker().deactivateCache();
 
-    for (const auto& externalCollection : qAsConst(m_externalCollections)) {
+    for (const auto& externalCollection : std::as_const(m_externalCollections)) {
         kLogger.info()
                 << "Disconnecting from"
                 << externalCollection->name();
@@ -221,55 +223,66 @@ TrackCollectionManager::SaveTrackResult TrackCollectionManager::saveTrack(
         return SaveTrackResult::Skipped;
     }
 
-    // The dirty flag is reset while saving the track in the internal
-    // collection!
+    if (!pTrack->getId().isValid()) {
+        // Track has been purged from the internal collection/database
+        // while it was cached in-memory.
+        // TODO: Is this race condition even possible?? The debug assertion
+        // in TrackDAO::saveTrack() never triggered so it must at least be
+        // very unlikely.
+        if (!m_externalCollections.isEmpty()) {
+            kLogger.debug()
+                    << "Purging deleted track"
+                    << pTrack->getLocation()
+                    << "from"
+                    << m_externalCollections.size()
+                    << "external collection(s)";
+            for (const auto& externalTrackCollection : std::as_const(m_externalCollections)) {
+                externalTrackCollection->purgeTracks(
+                        QStringList{pTrack->getLocation()});
+            }
+        }
+        // Only the metadata needs to be exported for saving this track.
+        // Reset the dirty flag as the TrackDAO would have done.
+        pTrack->markClean();
+        return SaveTrackResult::Saved;
+    }
+
     if (!pTrack->isDirty()) {
+        // Neither purged nor modified
         return SaveTrackResult::Skipped;
     }
 
     // This operation must be executed synchronously while the cache is
     // locked to prevent that a new track is created from outdated
-    // metadata in the database before saving finished.
+    // metadata in the database before saving has finished.
     kLogger.debug()
             << "Saving track"
             << pTrack->getLocation()
             << "in internal collection";
-    m_pInternalCollection->saveTrack(pTrack);
-    const auto res = pTrack->isDirty() ? SaveTrackResult::Failed : SaveTrackResult::Saved;
-
-    if (m_externalCollections.isEmpty()) {
-        return res;
+    if (!m_pInternalCollection->saveTrack(pTrack)) {
+        // The dirty flag is not reset when saving fails
+        DEBUG_ASSERT(pTrack->isDirty());
+        return SaveTrackResult::Failed;
     }
-    if (pTrack->getId().isValid()) {
+    // The dirty flag is reset after the track has been saved successfully
+    DEBUG_ASSERT(!pTrack->isDirty());
+
+    if (!m_externalCollections.isEmpty()) {
         // Track still exists in the internal collection/database
         kLogger.debug()
                 << "Saving modified track"
                 << pTrack->getLocation()
                 << "in"
                 << m_externalCollections.size()
                 << "external collection(s)";
-        for (const auto& externalTrackCollection : qAsConst(m_externalCollections)) {
+        for (const auto& externalTrackCollection : std::as_const(m_externalCollections)) {
             externalTrackCollection->saveTrack(
                     *pTrack,
                     ExternalTrackCollection::ChangeHint::Modified);
         }
-    } else {
-        // Track has been deleted from the internal collection/database
-        // while it was cached in-memory
-        kLogger.debug()
-                << "Purging deleted track"
-                << pTrack->getLocation()
-                << "from"
-                << m_externalCollections.size()
-                << "external collection(s)";
-        for (const auto& externalTrackCollection : qAsConst(m_externalCollections)) {
-            externalTrackCollection->purgeTracks(
-                    QStringList{pTrack->getLocation()});
-        }
     }
-    // After saving a track successfully the dirty flag must have been reset
-    DEBUG_ASSERT(!(res == SaveTrackResult::Saved && pTrack->isDirty()));
-    return res;
+
+    return SaveTrackResult::Saved;
 }
 
 void TrackCollectionManager::exportTrackMetadata(