Fix runtime dependencies of LibraryScanner

src/library/basetrackcache.h

@@ -17,7 +17,6 @@
 #include "util/string.h"
 
 class SearchQueryParser;
-class TrackDAO;
 class TrackCollection;
 
 class SortColumn {

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() {

src/library/scanner/libraryscanner.h

@@ -22,15 +22,13 @@
 
 class ScannerTask;
 class LibraryScannerDlg;
-class TrackCollection;
 
 class LibraryScanner : public QThread {
     FRIEND_TEST(LibraryScannerTest, ScannerRoundtrip);
     Q_OBJECT
   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;
 

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<BaseTrackCache> pTrackSo
         kLogger.warning() << "Track source has already been connected";
         return;
     }
+    kLogger.info() << "Connecting track source";
     m_pTrackSource = pTrackSource;
     connect(&m_trackDao,
             &TrackDAO::trackDirty,

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<TrackCollection> 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<TrackCollection>(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<TrackCollection>(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<LibraryScanner>(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) {

src/library/trackcollectionmanager.h

@@ -2,15 +2,17 @@
 
 #include <QDir>
 #include <QList>
-#include <QObject>
 #include <QSet>
 
-#include "library/scanner/libraryscanner.h"
+#include <memory>
+
+#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<ExternalTrackCollection*> m_externalCollections;
 
-    LibraryScanner m_scanner;
+    // TODO: Extract and decouple LibraryScanner from TrackCollectionManager
+    std::unique_ptr<LibraryScanner> m_pScanner;
 };

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;
 };