diff --git a/src/library/basetrackcache.h b/src/library/basetrackcache.h index 92dcf4e74559..b66b3519191d 100644 --- a/src/library/basetrackcache.h +++ b/src/library/basetrackcache.h @@ -17,7 +17,6 @@ #include "util/string.h" class SearchQueryParser; -class TrackDAO; class TrackCollection; class SortColumn { diff --git a/src/library/scanner/libraryscanner.cpp b/src/library/scanner/libraryscanner.cpp index cd48824ce35d..fb0742c00d5e 100644 --- a/src/library/scanner/libraryscanner.cpp +++ b/src/library/scanner/libraryscanner.cpp @@ -6,7 +6,6 @@ #include "library/scanner/scannertask.h" #include "library/queryutil.h" #include "library/coverartutils.h" -#include "library/trackcollection.h" #include "util/logger.h" #include "util/trace.h" #include "util/file.h" @@ -41,10 +40,8 @@ int execCleanupQuery(FwdSqlQuery& query) { LibraryScanner::LibraryScanner( mixxx::DbConnectionPoolPtr pDbConnectionPool, - TrackCollection* pTrackCollection, const UserSettingsPointer& pConfig) : m_pDbConnectionPool(std::move(pDbConnectionPool)), - m_pTrackCollection(pTrackCollection), m_analysisDao(pConfig), m_trackDao(m_cueDao, m_playlistDao, m_analysisDao, m_libraryHashDao, @@ -53,7 +50,6 @@ LibraryScanner::LibraryScanner( m_state(IDLE) { // Move LibraryScanner to its own thread so that our signals/slots will // queue to our event loop. - kLogger.debug() << "Starting thread"; moveToThread(this); m_pool.moveToThread(this); @@ -66,24 +62,6 @@ LibraryScanner::LibraryScanner( // connect them to our slots to run the command on the scanner thread. connect(this, &LibraryScanner::startScan, this, &LibraryScanner::slotStartScan); - // Force the GUI thread's Track cache to be cleared when a library - // scan is finished, because we might have modified the database directly - // when we detected moved files, and the TIOs corresponding to the moved - // files would then have the wrong track location. - TrackDAO* trackDao = &(m_pTrackCollection->getTrackDAO()); - connect(this, - &LibraryScanner::trackAdded, - trackDao, - &TrackDAO::databaseTrackAdded); - connect(this, - &LibraryScanner::tracksChanged, - trackDao, - &TrackDAO::databaseTracksChanged); - connect(this, - &LibraryScanner::tracksRelocated, - trackDao, - &TrackDAO::databaseTracksRelocated); - m_pProgressDlg.reset(new LibraryScannerDlg()); connect(this, &LibraryScanner::progressLoading, @@ -113,8 +91,6 @@ LibraryScanner::LibraryScanner( &TrackDAO::progressCoverArt, m_pProgressDlg.data(), &LibraryScannerDlg::slotUpdateCover); - - start(); } LibraryScanner::~LibraryScanner() { diff --git a/src/library/scanner/libraryscanner.h b/src/library/scanner/libraryscanner.h index 4761e7f57774..683862f13a66 100644 --- a/src/library/scanner/libraryscanner.h +++ b/src/library/scanner/libraryscanner.h @@ -22,7 +22,6 @@ class ScannerTask; class LibraryScannerDlg; -class TrackCollection; class LibraryScanner : public QThread { FRIEND_TEST(LibraryScannerTest, ScannerRoundtrip); @@ -30,7 +29,6 @@ class LibraryScanner : public QThread { public: LibraryScanner( mixxx::DbConnectionPoolPtr pDbConnectionPool, - TrackCollection* pTrackCollection, const UserSettingsPointer& pConfig); ~LibraryScanner() override; @@ -100,10 +98,6 @@ class LibraryScanner : public QThread { mixxx::DbConnectionPoolPtr m_pDbConnectionPool; - // The library trackcollection. Do not touch this from the library scanner - // thread. - TrackCollection* m_pTrackCollection; - // The pool of threads used for worker tasks. QThreadPool m_pool; diff --git a/src/library/trackcollection.cpp b/src/library/trackcollection.cpp index 067c6716478a..4b3bc35bb0b6 100644 --- a/src/library/trackcollection.cpp +++ b/src/library/trackcollection.cpp @@ -36,12 +36,14 @@ TrackCollection::~TrackCollection() { void TrackCollection::repairDatabase(QSqlDatabase database) { DEBUG_ASSERT(QApplication::instance()->thread() == QThread::currentThread()); + kLogger.info() << "Repairing database"; m_crates.repairDatabase(database); } void TrackCollection::connectDatabase(QSqlDatabase database) { DEBUG_ASSERT(QApplication::instance()->thread() == QThread::currentThread()); + kLogger.info() << "Connecting database"; m_database = database; m_trackDao.initialize(database); m_playlistDao.initialize(database); @@ -55,6 +57,7 @@ void TrackCollection::connectDatabase(QSqlDatabase database) { void TrackCollection::disconnectDatabase() { DEBUG_ASSERT(QApplication::instance()->thread() == QThread::currentThread()); + kLogger.info() << "Disconnecting database"; m_database = QSqlDatabase(); m_trackDao.finish(); m_crates.disconnectDatabase(); @@ -67,6 +70,7 @@ void TrackCollection::connectTrackSource(QSharedPointer pTrackSo kLogger.warning() << "Track source has already been connected"; return; } + kLogger.info() << "Connecting track source"; m_pTrackSource = pTrackSource; connect(&m_trackDao, &TrackDAO::trackDirty, diff --git a/src/library/trackcollectionmanager.cpp b/src/library/trackcollectionmanager.cpp index 2b745e7cf6b8..f8d641a96ec2 100644 --- a/src/library/trackcollectionmanager.cpp +++ b/src/library/trackcollectionmanager.cpp @@ -1,9 +1,8 @@ -#include "database/mixxxdb.h" - #include "library/trackcollectionmanager.h" -#include "library/trackcollection.h" #include "library/externaltrackcollection.h" +#include "library/scanner/libraryscanner.h" +#include "library/trackcollection.h" #include "sources/soundsourceproxy.h" #include "util/db/dbconnectionpooled.h" @@ -14,76 +13,124 @@ namespace { const mixxx::Logger kLogger("TrackCollectionManager"); -const QString kConfigGroup("[TrackCollection]"); +const QString kConfigGroup = QStringLiteral("[TrackCollection]"); const ConfigKey kConfigKeyRepairDatabaseOnNextRestart(kConfigGroup, "RepairDatabaseOnNextRestart"); +inline +parented_ptr createInternalTrackCollection( + TrackCollectionManager* parent, + const UserSettingsPointer& pConfig, + deleteTrackFn_t deleteTrackFn) { + // Ensure that GlobalTrackCache is ready before creating + // the internal TrackCollection. + GlobalTrackCache::createInstance(parent, deleteTrackFn); + return make_parented(parent, pConfig); +} + } // anonymous namespace TrackCollectionManager::TrackCollectionManager( QObject* parent, UserSettingsPointer pConfig, mixxx::DbConnectionPoolPtr pDbConnectionPool, - deleteTrackFn_t /*only-needed-for-testing*/ deleteTrackFn) + deleteTrackFn_t /*only-needed-for-testing*/ deleteTrackForTestingFn) : QObject(parent), m_pConfig(pConfig), - m_pInternalCollection(make_parented(this, pConfig)), - m_scanner(pDbConnectionPool, m_pInternalCollection, pConfig) { - - const QSqlDatabase dbConnection = mixxx::DbConnectionPooled(std::move(pDbConnectionPool)); + m_pInternalCollection(createInternalTrackCollection(this, pConfig, deleteTrackForTestingFn)) { + const QSqlDatabase dbConnection = mixxx::DbConnectionPooled(pDbConnectionPool); // TODO(XXX): Add a checkbox in the library preferences for checking // and repairing the database on the next restart of the application. if (pConfig->getValue(kConfigKeyRepairDatabaseOnNextRestart, false)) { - kLogger.info() << "Checking and repairing database (if necessary)"; m_pInternalCollection->repairDatabase(dbConnection); // Reset config value pConfig->setValue(kConfigKeyRepairDatabaseOnNextRestart, false); } - kLogger.info() << "Connecting database"; m_pInternalCollection->connectDatabase(dbConnection); - if (deleteTrackFn) { - kLogger.info() << "External collections are not available in test mode"; + if (deleteTrackForTestingFn) { + kLogger.info() << "External collections are disabled in test mode"; } else { // TODO: Add external collections } - - kLogger.info() << "Connecting external collections"; for (const auto& externalCollection : m_externalCollections) { - kLogger.info() << "Connecting" << externalCollection->name(); + kLogger.info() + << "Connecting to" + << externalCollection->name(); externalCollection->establishConnection(); } - // Forward signals - connect(&m_scanner, - &LibraryScanner::scanStarted, - this, - &TrackCollectionManager::libraryScanStarted); - connect(&m_scanner, - &LibraryScanner::scanFinished, - this, - &TrackCollectionManager::libraryScanFinished); - - // Handle signals - connect(&m_scanner, - &LibraryScanner::trackAdded, - this, - &TrackCollectionManager::slotScanTrackAdded); - connect(&m_scanner, - &LibraryScanner::tracksChanged, - this, - &TrackCollectionManager::slotScanTracksUpdated); - connect(&m_scanner, - &LibraryScanner::tracksRelocated, - this, - &TrackCollectionManager::slotScanTracksRelocated); - - GlobalTrackCache::createInstance(this, deleteTrackFn); + // TODO: Extract and decouple LibraryScanner from TrackCollectionManager + if (deleteTrackForTestingFn) { + // Exclude the library scanner from tests + kLogger.info() << "Libary scanner is disabled in test mode"; + } else { + m_pScanner = std::make_unique(pDbConnectionPool, pConfig); + + // Forward signals + connect(m_pScanner.get(), + &LibraryScanner::scanStarted, + this, + &TrackCollectionManager::libraryScanStarted, + /*signal-to-signal*/ Qt::DirectConnection); + connect(m_pScanner.get(), + &LibraryScanner::scanFinished, + this, + &TrackCollectionManager::libraryScanFinished, + /*signal-to-signal*/ Qt::DirectConnection); + + // Handle signals + connect(m_pScanner.get(), + &LibraryScanner::trackAdded, + this, + &TrackCollectionManager::slotScanTrackAdded); + connect(m_pScanner.get(), + &LibraryScanner::tracksChanged, + this, + &TrackCollectionManager::slotScanTracksUpdated); + connect(m_pScanner.get(), + &LibraryScanner::tracksRelocated, + this, + &TrackCollectionManager::slotScanTracksRelocated); + + // Force the GUI thread's Track cache to be cleared when a library + // scan is finished, because we might have modified the database directly + // when we detected moved files, and the TIOs corresponding to the moved + // files would then have the wrong track location. + TrackDAO* pTrackDAO = &(m_pInternalCollection->getTrackDAO()); + connect(m_pScanner.get(), + &LibraryScanner::trackAdded, + pTrackDAO, + &TrackDAO::databaseTrackAdded); + connect(m_pScanner.get(), + &LibraryScanner::tracksChanged, + pTrackDAO, + &TrackDAO::databaseTracksChanged); + connect(m_pScanner.get(), + &LibraryScanner::tracksRelocated, + pTrackDAO, + &TrackDAO::databaseTracksRelocated); + + kLogger.info() << "Starting library scanner thread"; + m_pScanner->start(); + } } TrackCollectionManager::~TrackCollectionManager() { + if (m_pScanner) { + while (m_pScanner->isRunning()) { + kLogger.info() << "Stopping library scanner thread"; + m_pScanner->quit(); + if (m_pScanner->wait()) { + kLogger.info() << "Stopped library scanner thread"; + } + } + DEBUG_ASSERT(m_pScanner->isFinished()); + m_pScanner.reset(); + } + const auto pWeakTrackSource = m_pInternalCollection->disconnectTrackSource(); VERIFY_OR_DEBUG_ASSERT(pWeakTrackSource.isNull()) { kLogger.warning() << "BaseTrackCache is still in use"; @@ -92,29 +139,31 @@ TrackCollectionManager::~TrackCollectionManager() { // Evict all remaining tracks from the cache to trigger // updating of modified tracks. We assume that no other // components are accessing those files at this point. - kLogger.info() << "Deactivating GlobalTrackCache"; GlobalTrackCacheLocker().deactivateCache(); - if (!m_externalCollections.isEmpty()) { - kLogger.info() << "Disconnecting from external track collections"; - for (const auto& externalCollection : m_externalCollections) { - kLogger.info() << "Disconnecting from" << externalCollection->name(); - externalCollection->finishPendingTasksAndDisconnect(); - } + for (const auto& externalCollection : m_externalCollections) { + kLogger.info() + << "Disconnecting from" + << externalCollection->name(); + // TODO: Disconnecting from external track collections + // should be done asynchrously. The manager should poll + // the track collections until all have been disconnected. + externalCollection->finishPendingTasksAndDisconnect(); // synchronous } - kLogger.info() << "Disconnecting internal track collection from database"; m_pInternalCollection->disconnectDatabase(); GlobalTrackCache::destroyInstance(); } void TrackCollectionManager::startLibraryScan() { - m_scanner.scan(); + DEBUG_ASSERT(m_pScanner); + m_pScanner->scan(); } void TrackCollectionManager::stopLibraryScan() { - m_scanner.slotCancel(); + DEBUG_ASSERT(m_pScanner); + m_pScanner->slotCancel(); } bool TrackCollectionManager::saveTrack(const TrackPointer& pTrack) { diff --git a/src/library/trackcollectionmanager.h b/src/library/trackcollectionmanager.h index fe07b98a0a38..ab06f68d492f 100644 --- a/src/library/trackcollectionmanager.h +++ b/src/library/trackcollectionmanager.h @@ -2,15 +2,17 @@ #include #include -#include #include -#include "library/scanner/libraryscanner.h" +#include + +#include "library/relocatedtrack.h" #include "preferences/usersettings.h" #include "track/globaltrackcache.h" #include "util/db/dbconnectionpool.h" #include "util/parented_ptr.h" +class LibraryScanner; class TrackCollection; class ExternalTrackCollection; @@ -31,7 +33,7 @@ class TrackCollectionManager: public QObject, QObject* parent, UserSettingsPointer pConfig, mixxx::DbConnectionPoolPtr pDbConnectionPool, - deleteTrackFn_t deleteTrackFn = nullptr); + deleteTrackFn_t deleteTrackForTestingFn = nullptr); ~TrackCollectionManager() override; TrackCollection* internalCollection() { @@ -98,5 +100,6 @@ class TrackCollectionManager: public QObject, QList m_externalCollections; - LibraryScanner m_scanner; + // TODO: Extract and decouple LibraryScanner from TrackCollectionManager + std::unique_ptr m_pScanner; }; diff --git a/src/test/libraryscannertest.cpp b/src/test/libraryscannertest.cpp index 9994c7524edd..9f37f978cb2e 100644 --- a/src/test/libraryscannertest.cpp +++ b/src/test/libraryscannertest.cpp @@ -8,7 +8,7 @@ class LibraryScannerTest : public LibraryTest { protected: LibraryScannerTest() - : m_libraryScanner(dbConnectionPool(), internalCollection(), config()) { + : m_libraryScanner(dbConnectionPool(), config()) { } LibraryScanner m_libraryScanner; }; diff --git a/src/track/globaltrackcache.cpp b/src/track/globaltrackcache.cpp index 9e054a222a22..054ae6737585 100644 --- a/src/track/globaltrackcache.cpp +++ b/src/track/globaltrackcache.cpp @@ -198,12 +198,14 @@ void GlobalTrackCache::createInstance( GlobalTrackCacheSaver* pSaver, deleteTrackFn_t deleteTrackFn) { DEBUG_ASSERT(!s_pInstance); + kLogger.info() << "Creating instance"; s_pInstance = new GlobalTrackCache(pSaver, deleteTrackFn); } //static void GlobalTrackCache::destroyInstance() { DEBUG_ASSERT(s_pInstance); + kLogger.info() << "Destroying instance"; // Processing all pending events is required to evict all // remaining references from the cache. QCoreApplication::processEvents(); @@ -366,12 +368,8 @@ void GlobalTrackCache::saveEvictedTrack(Track* pEvictedTrack) const { void GlobalTrackCache::deactivate() { DEBUG_ASSERT(QThread::currentThread() == QCoreApplication::instance()->thread()); - if (!isEmpty()) { - kLogger.warning() - << "Not empty when deactivating:" - << m_tracksById.size() - << '/' - << m_tracksByCanonicalLocation.size(); + if (isEmpty()) { + return; } // Ideally the cache should be empty when destroyed. @@ -380,6 +378,13 @@ void GlobalTrackCache::deactivate() { // referenced or not. This ensures that the eviction // callback is triggered for all modified tracks before // exiting the application. + kLogger.warning() + << "Evicting all remaining" + << m_tracksById.size() + << '/' + << m_tracksByCanonicalLocation.size() + << "tracks from cache"; + while (!m_tracksById.empty()) { auto i = m_tracksById.begin(); Track* plainPtr= i->second->getPlainPtr();