diff options
author | Jan Holthuis <jan.holthuis@ruhr-uni-bochum.de> | 2022-01-07 17:05:54 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2022-01-07 17:05:54 +0100 |
commit | 2db5d90ad05df02c08a5789c69abd6ea260e7a04 (patch) | |
tree | f5faccc79f8f98e781807d3a374bc252990e0231 | |
parent | 52f7315f7007dbd5f6d430d841f7e466c38ea050 (diff) | |
parent | 7b288d0e089ab87b70686adcf8e9562b5367f5c9 (diff) |
Merge pull request #4603 from uklotzde/trackdao
TrackDAO::saveTrack(): Return success/failure result
-rw-r--r-- | src/library/dao/trackdao.cpp | 22 | ||||
-rw-r--r-- | src/library/dao/trackdao.h | 2 | ||||
-rw-r--r-- | src/library/trackcollection.cpp | 4 | ||||
-rw-r--r-- | src/library/trackcollection.h | 2 | ||||
-rw-r--r-- | src/library/trackcollectionmanager.cpp | 69 |
5 files changed, 58 insertions, 41 deletions
diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 0f66bf272b..98c532f507 100644 --- a/src/library/dao/trackdao.cpp +++ b/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) { diff --git a/src/library/dao/trackdao.h b/src/library/dao/trackdao.h index 6eb0102b8f..de18b7c6a0 100644 --- a/src/library/dao/trackdao.h +++ b/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. diff --git a/src/library/trackcollection.cpp b/src/library/trackcollection.cpp index 85c6a22746..179705bacd 100644 --- a/src/library/trackcollection.cpp +++ b/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( diff --git a/src/library/trackcollection.h b/src/library/trackcollection.h index ca1866b8e7..71fe79d9cc 100644 --- a/src/library/trackcollection.h +++ b/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; diff --git a/src/library/trackcollectionmanager.cpp b/src/library/trackcollectionmanager.cpp index 363fb7680d..645323918d 100644 --- a/src/library/trackcollectionmanager.cpp +++ b/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,26 +223,51 @@ 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" @@ -248,28 +275,14 @@ TrackCollectionManager::SaveTrackResult TrackCollectionManager::saveTrack( << "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( |