lp1865957: Fetch multiple tracks from MusicBrainz

src/library/dlgtagfetcher.cpp

@@ -84,25 +84,26 @@ void DlgTagFetcher::init() {
 }
 
 void DlgTagFetcher::loadTrack(const TrackPointer& track) {
-    if (track == NULL) {
+    if (!track) {
         return;
     }
     results->clear();
-    disconnect(track.get(),
+    disconnect(m_track.get(),
             &Track::changed,
             this,
             &DlgTagFetcher::slotTrackChanged);
 
     m_track = track;
     m_data = Data();
     m_networkResult = NetworkResult::Ok;
-    m_tagFetcher.startFetch(m_track);
 
-    connect(track.get(),
+    connect(m_track.get(),
             &Track::changed,
             this,
             &DlgTagFetcher::slotTrackChanged);
 
+    m_tagFetcher.startFetch(m_track);
+
     updateStack();
 }
 
@@ -176,7 +177,12 @@ void DlgTagFetcher::apply() {
                 trackRelease.releaseGroupId);
     }
 #endif // __EXTRA_METADATA__
-    m_track->importMetadata(std::move(trackMetadata));
+    m_track->importMetadata(
+            std::move(trackMetadata),
+            // Prevent re-import of outdated metadata from file tags
+            // by explicitly setting the synchronization time stamp
+            // to the current time.
+            QDateTime::currentDateTimeUtc());
 }
 
 void DlgTagFetcher::quit() {

src/musicbrainz/tagfetcher.cpp

@@ -20,52 +20,49 @@ TagFetcher::TagFetcher(QObject* parent)
 
 void TagFetcher::startFetch(
         TrackPointer pTrack) {
+    DEBUG_ASSERT(thread() == QThread::currentThread());
     cancel();
 
     m_pTrack = pTrack;
 
     emit fetchProgress(tr("Fingerprinting track"));
-    const auto fingerprintTask = QtConcurrent::run([this, pTrack] {
-        return ChromaPrinter(this).getFingerprint(pTrack);
+    const auto fingerprintTask = QtConcurrent::run([pTrack] {
+        return ChromaPrinter().getFingerprint(pTrack);
     });
     m_fingerprintWatcher.setFuture(fingerprintTask);
+    DEBUG_ASSERT(!m_pAcoustIdTask);
     connect(
             &m_fingerprintWatcher,
             &QFutureWatcher<QString>::finished,
             this,
             &TagFetcher::slotFingerprintReady);
 }
 
-void TagFetcher::abortAcoustIdTask() {
+void TagFetcher::cancel() {
+    DEBUG_ASSERT(thread() == QThread::currentThread());
+    m_pTrack.reset();
+    m_fingerprintWatcher.disconnect(this);
+    m_fingerprintWatcher.cancel();
     if (m_pAcoustIdTask) {
-        disconnect(m_pAcoustIdTask.get());
-        m_pAcoustIdTask->deleteBeforeFinished();
+        m_pAcoustIdTask->disconnect(this);
+        m_pAcoustIdTask->deleteLater();
         m_pAcoustIdTask = nullptr;
     }
-}
-
-void TagFetcher::abortMusicBrainzTask() {
     if (m_pMusicBrainzTask) {
-        disconnect(m_pMusicBrainzTask.get());
-        m_pMusicBrainzTask->deleteBeforeFinished();
+        m_pMusicBrainzTask->disconnect(this);
+        m_pMusicBrainzTask->deleteLater();
         m_pMusicBrainzTask = nullptr;
     }
 }
 
-void TagFetcher::cancel() {
-    // qDebug()<< "Cancel tagfetching";
-    m_fingerprintWatcher.cancel();
-    abortAcoustIdTask();
-    abortMusicBrainzTask();
-    m_pTrack.reset();
-}
-
 void TagFetcher::slotFingerprintReady() {
+    DEBUG_ASSERT(thread() == QThread::currentThread());
     if (!m_pTrack ||
             !m_fingerprintWatcher.isFinished()) {
         return;
     }
 
+    DEBUG_ASSERT(m_fingerprintWatcher.isFinished());
     const QString fingerprint = m_fingerprintWatcher.result();
     if (fingerprint.isEmpty()) {
         emit resultAvailable(
@@ -74,24 +71,26 @@ void TagFetcher::slotFingerprintReady() {
         return;
     }
 
-    abortAcoustIdTask();
-
     emit fetchProgress(tr("Identifying track through Acoustid"));
     DEBUG_ASSERT(!m_pAcoustIdTask);
     m_pAcoustIdTask = make_parented<mixxx::AcoustIdLookupTask>(
             &m_network,
             fingerprint,
             m_pTrack->getDurationInt(),
             this);
-    connect(m_pAcoustIdTask.get(),
+    connect(m_pAcoustIdTask,
             &mixxx::AcoustIdLookupTask::succeeded,
             this,
             &TagFetcher::slotAcoustIdTaskSucceeded);
-    connect(m_pAcoustIdTask.get(),
+    connect(m_pAcoustIdTask,
             &mixxx::AcoustIdLookupTask::failed,
             this,
             &TagFetcher::slotAcoustIdTaskFailed);
-    connect(m_pAcoustIdTask.get(),
+    connect(m_pAcoustIdTask,
+            &mixxx::AcoustIdLookupTask::aborted,
+            this,
+            &TagFetcher::slotAcoustIdTaskAborted);
+    connect(m_pAcoustIdTask,
             &mixxx::AcoustIdLookupTask::networkError,
             this,
             &TagFetcher::slotAcoustIdTaskNetworkError);
@@ -101,14 +100,15 @@ void TagFetcher::slotFingerprintReady() {
 
 void TagFetcher::slotAcoustIdTaskSucceeded(
         QList<QUuid> recordingIds) {
-    abortAcoustIdTask();
-    if (!m_pTrack) {
-        return;
-    }
+    DEBUG_ASSERT(thread() == QThread::currentThread());
+    DEBUG_ASSERT(m_pAcoustIdTask.get() ==
+            qobject_cast<mixxx::AcoustIdLookupTask*>(sender()));
 
     if (recordingIds.isEmpty()) {
+        auto pTrack = std::move(m_pTrack);
+        cancel();
         emit resultAvailable(
-                m_pTrack,
+                std::move(pTrack),
                 QList<mixxx::musicbrainz::TrackRelease>());
         return;
     }
@@ -119,15 +119,19 @@ void TagFetcher::slotAcoustIdTaskSucceeded(
             &m_network,
             std::move(recordingIds),
             this);
-    connect(m_pMusicBrainzTask.get(),
+    connect(m_pMusicBrainzTask,
             &mixxx::MusicBrainzRecordingsTask::succeeded,
             this,
             &TagFetcher::slotMusicBrainzTaskSucceeded);
-    connect(m_pMusicBrainzTask.get(),
+    connect(m_pMusicBrainzTask,
             &mixxx::MusicBrainzRecordingsTask::failed,
             this,
             &TagFetcher::slotMusicBrainzTaskFailed);
-    connect(m_pMusicBrainzTask.get(),
+    connect(m_pMusicBrainzTask,
+            &mixxx::MusicBrainzRecordingsTask::aborted,
+            this,
+            &TagFetcher::slotMusicBrainzTaskAborted);
+    connect(m_pMusicBrainzTask,
             &mixxx::MusicBrainzRecordingsTask::networkError,
             this,
             &TagFetcher::slotMusicBrainzTaskNetworkError);
@@ -137,37 +141,78 @@ void TagFetcher::slotAcoustIdTaskSucceeded(
 
 void TagFetcher::slotAcoustIdTaskFailed(
         mixxx::network::JsonWebResponse response) {
+    DEBUG_ASSERT(thread() == QThread::currentThread());
+    DEBUG_ASSERT(m_pAcoustIdTask.get() ==
+            qobject_cast<mixxx::AcoustIdLookupTask*>(sender()));
+
     cancel();
+
     emit networkError(
             response.statusCode,
             "AcoustID",
             response.content.toJson(),
             -1);
 }
 
+void TagFetcher::slotAcoustIdTaskAborted() {
+    DEBUG_ASSERT(thread() == QThread::currentThread());
+    DEBUG_ASSERT(m_pAcoustIdTask.get() ==
+            qobject_cast<mixxx::AcoustIdLookupTask*>(sender()));
+
+    auto pTrack = std::move(m_pTrack);
+    cancel();
+
+    emit resultAvailable(
+            std::move(pTrack),
+            QList<mixxx::musicbrainz::TrackRelease>{});
+}
+
 void TagFetcher::slotAcoustIdTaskNetworkError(
         QUrl requestUrl,
         QNetworkReply::NetworkError errorCode,
         QString errorString,
         QByteArray errorContent) {
     Q_UNUSED(requestUrl);
     Q_UNUSED(errorContent);
+    DEBUG_ASSERT(thread() == QThread::currentThread());
+    DEBUG_ASSERT(m_pAcoustIdTask.get() ==
+            qobject_cast<mixxx::AcoustIdLookupTask*>(sender()));
+
     cancel();
+
     emit networkError(
             mixxx::network::kHttpStatusCodeInvalid,
             "AcoustID",
             errorString,
             errorCode);
 }
 
+void TagFetcher::slotMusicBrainzTaskAborted() {
+    DEBUG_ASSERT(thread() == QThread::currentThread());
+    DEBUG_ASSERT(m_pMusicBrainzTask.get() ==
+            qobject_cast<mixxx::MusicBrainzRecordingsTask*>(sender()));
+
+    auto pTrack = std::move(m_pTrack);
+    cancel();
+
+    emit resultAvailable(
+            std::move(pTrack),
+            QList<mixxx::musicbrainz::TrackRelease>{});
+}
+
 void TagFetcher::slotMusicBrainzTaskNetworkError(
         QUrl requestUrl,
         QNetworkReply::NetworkError errorCode,
         QString errorString,
         QByteArray errorContent) {
     Q_UNUSED(requestUrl);
     Q_UNUSED(errorContent);
+    DEBUG_ASSERT(thread() == QThread::currentThread());
+    DEBUG_ASSERT(m_pMusicBrainzTask.get() ==
+            qobject_cast<mixxx::MusicBrainzRecordingsTask*>(sender()));
+
     cancel();
+
     emit networkError(
             mixxx::network::kHttpStatusCodeInvalid,
             "MusicBrainz",
@@ -179,7 +224,12 @@ void TagFetcher::slotMusicBrainzTaskFailed(
         mixxx::network::WebResponse response,
         int errorCode,
         QString errorMessage) {
+    DEBUG_ASSERT(thread() == QThread::currentThread());
+    DEBUG_ASSERT(m_pMusicBrainzTask.get() ==
+            qobject_cast<mixxx::MusicBrainzRecordingsTask*>(sender()));
+
     cancel();
+
     emit networkError(
             response.statusCode,
             "MusicBrainz",
@@ -189,13 +239,14 @@ void TagFetcher::slotMusicBrainzTaskFailed(
 
 void TagFetcher::slotMusicBrainzTaskSucceeded(
         QList<mixxx::musicbrainz::TrackRelease> guessedTrackReleases) {
-    auto pOriginalTrack = std::move(m_pTrack);
-    abortMusicBrainzTask();
-    if (!pOriginalTrack) {
-        // aborted
-        return;
-    }
+    DEBUG_ASSERT(thread() == QThread::currentThread());
+    DEBUG_ASSERT(m_pMusicBrainzTask.get() ==
+            qobject_cast<mixxx::MusicBrainzRecordingsTask*>(sender()));
+
+    auto pTrack = std::move(m_pTrack);
+    cancel();
+
     emit resultAvailable(
-            pOriginalTrack,
+            std::move(pTrack),
             std::move(guessedTrackReleases));
 }

src/musicbrainz/tagfetcher.h

@@ -9,16 +9,17 @@
 #include "util/parented_ptr.h"
 
 class TagFetcher : public QObject {
-  Q_OBJECT
+    Q_OBJECT
 
-  // Implements retrieval of metadata in 3 subsequent stages:
-  //   1. Chromaprint -> AcoustID fingerprint
-  //   2. AcoustID -> MusicBrainz recording UUIDs
-  //   3. MusicBrainz -> MusicBrainz track releases
+    // Implements retrieval of metadata in 3 subsequent stages:
+    //   1. Chromaprint -> AcoustID fingerprint
+    //   2. AcoustID -> MusicBrainz recording UUIDs
+    //   3. MusicBrainz -> MusicBrainz track releases
 
   public:
     explicit TagFetcher(
             QObject* parent = nullptr);
+    ~TagFetcher() override = default;
 
     void startFetch(
             TrackPointer pTrack);
@@ -45,6 +46,7 @@ class TagFetcher : public QObject {
             QList<QUuid> recordingIds);
     void slotAcoustIdTaskFailed(
             mixxx::network::JsonWebResponse response);
+    void slotAcoustIdTaskAborted();
     void slotAcoustIdTaskNetworkError(
             QUrl requestUrl,
             QNetworkReply::NetworkError errorCode,
@@ -57,16 +59,14 @@ class TagFetcher : public QObject {
             mixxx::network::WebResponse response,
             int errorCode,
             QString errorMessage);
+    void slotMusicBrainzTaskAborted();
     void slotMusicBrainzTaskNetworkError(
             QUrl requestUrl,
             QNetworkReply::NetworkError errorCode,
             QString errorString,
             QByteArray errorContent);
 
   private:
-    void abortAcoustIdTask();
-    void abortMusicBrainzTask();
-
     QNetworkAccessManager m_network;
 
     QFutureWatcher<QString> m_fingerprintWatcher;

src/musicbrainz/web/acoustidlookuptask.cpp

@@ -112,7 +112,7 @@ QNetworkReply* AcoustIdLookupTask::sendNetworkRequest(
 }
 
 void AcoustIdLookupTask::onFinished(
-        network::JsonWebResponse response) {
+        network::JsonWebResponse&& response) {
     if (!response.isStatusCodeSuccess()) {
         kLogger.warning()
                 << "Request failed with HTTP status code"
@@ -200,7 +200,7 @@ void AcoustIdLookupTask::onFinished(
                 const auto recordingId =
                         QUuid(recordingObject.value(QLatin1String("id")).toString());
                 VERIFY_OR_DEBUG_ASSERT(!recordingId.isNull()) {
-                     continue;
+                    continue;
                 }
                 recordingIds.append(recordingId);
             }
@@ -211,15 +211,15 @@ void AcoustIdLookupTask::onFinished(
 
 void AcoustIdLookupTask::emitSucceeded(
         QList<QUuid>&& recordingIds) {
-    const auto signal = QMetaMethod::fromSignal(
-            &AcoustIdLookupTask::succeeded);
-    DEBUG_ASSERT(receivers(signal.name()) <= 1); // unique connection
-    if (isSignalConnected(signal)) {
-        emit succeeded(
-                std::move(recordingIds));
-    } else {
+    VERIFY_OR_DEBUG_ASSERT(
+            isSignalFuncConnected(&AcoustIdLookupTask::succeeded)) {
+        kLogger.warning()
+                << "Unhandled succeeded signal";
         deleteLater();
+        return;
     }
+    emit succeeded(
+            std::move(recordingIds));
 }
 
 } // namespace mixxx