2.3 Musicbrainz fixes

CMakeLists.txt

@@ -1536,7 +1536,9 @@ add_executable(mixxx-test
   src/test/metaknob_link_test.cpp
   src/test/midicontrollertest.cpp
   src/test/mixxxtest.cpp
+  src/test/mock_networkaccessmanager.cpp
   src/test/movinginterquartilemean_test.cpp
+  src/test/musicbrainzrecordingstasktest.cpp
   src/test/nativeeffects_test.cpp
   src/test/performancetimer_test.cpp
   src/test/playcountertest.cpp

src/library/dlgtagfetcher.cpp

@@ -89,36 +89,44 @@ void DlgTagFetcher::init() {
 }
 
 void DlgTagFetcher::slotNext() {
-    QModelIndex nextRow = m_currentTrackIndex.sibling(
-            m_currentTrackIndex.row() + 1, m_currentTrackIndex.column());
-    if (nextRow.isValid()) {
-        loadTrack(nextRow);
+    if (isSignalConnected(QMetaMethod::fromSignal(&DlgTagFetcher::next))) {
         emit next();
+    } else {
+        QModelIndex nextRow = m_currentTrackIndex.sibling(
+                m_currentTrackIndex.row() + 1, m_currentTrackIndex.column());
+        if (nextRow.isValid()) {
+            loadTrack(nextRow);
+        }
     }
 }
 
 void DlgTagFetcher::slotPrev() {
-    QModelIndex prevRow = m_currentTrackIndex.sibling(
-            m_currentTrackIndex.row() - 1, m_currentTrackIndex.column());
-    if (prevRow.isValid()) {
-        loadTrack(prevRow);
+    if (isSignalConnected(QMetaMethod::fromSignal(&DlgTagFetcher::previous))) {
         emit previous();
+    } else {
+        QModelIndex prevRow = m_currentTrackIndex.sibling(
+                m_currentTrackIndex.row() - 1, m_currentTrackIndex.column());
+        if (prevRow.isValid()) {
+            loadTrack(prevRow);
+        }
     }
 }
 
-void DlgTagFetcher::loadTrackInternal(const TrackPointer& track) {
-    if (!track) {
-        return;
+void DlgTagFetcher::loadTrack(const TrackPointer& pTrack) {
+    if (m_track) {
+        results->clear();
+        disconnect(m_track.get(),
+                &Track::changed,
+                this,
+                &DlgTagFetcher::slotTrackChanged);
+        m_data = Data();
+        m_networkResult = NetworkResult::Ok;
     }
-    results->clear();
-    disconnect(m_track.get(),
-            &Track::changed,
-            this,
-            &DlgTagFetcher::slotTrackChanged);
 
-    m_track = track;
-    m_data = Data();
-    m_networkResult = NetworkResult::Ok;
+    m_track = pTrack;
+    if (!m_track) {
+        return;
+    }
 
     connect(m_track.get(),
             &Track::changed,
@@ -130,20 +138,10 @@ void DlgTagFetcher::loadTrackInternal(const TrackPointer& track) {
     updateStack();
 }
 
-void DlgTagFetcher::loadTrack(const TrackPointer& track) {
-    VERIFY_OR_DEBUG_ASSERT(!m_pTrackModel) {
-        return;
-    }
-    loadTrackInternal(track);
-}
-
 void DlgTagFetcher::loadTrack(const QModelIndex& index) {
-    VERIFY_OR_DEBUG_ASSERT(m_pTrackModel) {
-        return;
-    }
-    TrackPointer pTrack = m_pTrackModel->getTrack(index);
     m_currentTrackIndex = index;
-    loadTrackInternal(pTrack);
+    TrackPointer pTrack = m_pTrackModel->getTrack(index);
+    loadTrack(pTrack);
 }
 
 void DlgTagFetcher::slotTrackChanged(TrackId trackId) {

src/library/dlgtagfetcher.h

@@ -46,7 +46,6 @@ class DlgTagFetcher : public QDialog, public Ui::DlgTagFetcher {
     void slotPrev();
 
   private:
-    void loadTrackInternal(const TrackPointer& track);
     void updateStack();
     void addDivider(const QString& text, QTreeWidget* parent) const;
 

src/library/dlgtrackinfo.cpp

@@ -315,7 +315,7 @@ void DlgTrackInfo::loadTrack(TrackPointer pTrack) {
         return;
     }
     loadTrackInternal(pTrack);
-    if (m_pDlgTagFetcher && m_pLoadedTrack) {
+    if (m_pDlgTagFetcher && m_pDlgTagFetcher->isVisible()) {
         m_pDlgTagFetcher->loadTrack(m_pLoadedTrack);
     }
 }
@@ -327,7 +327,7 @@ void DlgTrackInfo::loadTrack(const QModelIndex& index) {
     TrackPointer pTrack = m_pTrackModel->getTrack(index);
     m_currentTrackIndex = index;
     loadTrackInternal(pTrack);
-    if (m_pDlgTagFetcher && m_currentTrackIndex.isValid()) {
+    if (m_pDlgTagFetcher && m_pDlgTagFetcher->isVisible()) {
         m_pDlgTagFetcher->loadTrack(m_currentTrackIndex);
     }
 }

src/musicbrainz/tagfetcher.cpp

@@ -26,7 +26,7 @@ TagFetcher::TagFetcher(QObject* parent)
 void TagFetcher::startFetch(
         TrackPointer pTrack) {
     DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
-    cancel();
+    terminate();
 
     m_pTrack = pTrack;
 
@@ -58,6 +58,26 @@ void TagFetcher::cancel() {
     }
 }
 
+void TagFetcher::terminate() {
+    DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
+    m_pTrack.reset();
+
+    m_fingerprintWatcher.disconnect(this);
+    m_fingerprintWatcher.cancel();
+
+    if (m_pAcoustIdTask) {
+        m_pAcoustIdTask->disconnect(this);
+        m_pAcoustIdTask->deleteLater();
+        m_pAcoustIdTask = nullptr;
+    }
+
+    if (m_pMusicBrainzTask) {
+        m_pMusicBrainzTask->disconnect(this);
+        m_pMusicBrainzTask->deleteLater();
+        m_pMusicBrainzTask = nullptr;
+    }
+}
+
 void TagFetcher::slotFingerprintReady() {
     DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
     if (!m_pTrack ||
@@ -115,7 +135,7 @@ void TagFetcher::slotAcoustIdTaskSucceeded(
 
     if (recordingIds.isEmpty()) {
         auto pTrack = std::move(m_pTrack);
-        cancel();
+        terminate();
 
         emit resultAvailable(
                 std::move(pTrack),
@@ -149,28 +169,14 @@ void TagFetcher::slotAcoustIdTaskSucceeded(
             kMusicBrainzTimeoutMillis);
 }
 
-bool TagFetcher::onAcoustIdTaskTerminated() {
-    DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
-    auto* const pAcoustIdTask = m_pAcoustIdTask.get();
-    DEBUG_ASSERT(sender());
-    VERIFY_OR_DEBUG_ASSERT(pAcoustIdTask ==
-            qobject_cast<mixxx::AcoustIdLookupTask*>(sender())) {
-        return false;
-    }
-    m_pAcoustIdTask = nullptr;
-    const auto taskDeleter = mixxx::ScopedDeleteLater(pAcoustIdTask);
-    pAcoustIdTask->disconnect(this);
-    return true;
-}
-
 void TagFetcher::slotAcoustIdTaskFailed(
         const mixxx::network::JsonWebResponse& response) {
     DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
-    if (!onAcoustIdTaskTerminated()) {
+    if (m_pAcoustIdTask.get() != sender()) {
+        // stray call from an already aborted try
         return;
     }
-
-    cancel();
+    terminate();
 
     emit networkError(
             response.statusCode(),
@@ -181,69 +187,53 @@ void TagFetcher::slotAcoustIdTaskFailed(
 
 void TagFetcher::slotAcoustIdTaskAborted() {
     DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
-    if (!onAcoustIdTaskTerminated()) {
+    if (m_pAcoustIdTask.get() != sender()) {
+        // stray call from an already aborted try
         return;
     }
-
-    cancel();
+    terminate();
 }
 
 void TagFetcher::slotAcoustIdTaskNetworkError(
         QNetworkReply::NetworkError errorCode,
         const QString& errorString,
         const mixxx::network::WebResponseWithContent& responseWithContent) {
-    Q_UNUSED(responseWithContent);
     DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
-    if (!onAcoustIdTaskTerminated()) {
+    if (m_pAcoustIdTask.get() != sender()) {
+        // stray call from an already aborted try
         return;
     }
-
-    cancel();
+    terminate();
 
     emit networkError(
-            mixxx::network::kHttpStatusCodeInvalid,
+            responseWithContent.statusCode(),
             QStringLiteral("AcoustID"),
             errorString,
             errorCode);
 }
 
-bool TagFetcher::onMusicBrainzTaskTerminated() {
-    DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
-    auto* const pMusicBrainzTask = m_pMusicBrainzTask.get();
-    DEBUG_ASSERT(sender());
-    VERIFY_OR_DEBUG_ASSERT(pMusicBrainzTask ==
-            qobject_cast<mixxx::MusicBrainzRecordingsTask*>(sender())) {
-        return false;
-    }
-    m_pMusicBrainzTask = nullptr;
-    const auto taskDeleter = mixxx::ScopedDeleteLater(pMusicBrainzTask);
-    pMusicBrainzTask->disconnect(this);
-    return true;
-}
-
 void TagFetcher::slotMusicBrainzTaskAborted() {
     DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
-    if (!onMusicBrainzTaskTerminated()) {
+    if (m_pMusicBrainzTask.get() != sender()) {
+        // stray call from an already aborted try
         return;
     }
-
-    cancel();
+    terminate();
 }
 
 void TagFetcher::slotMusicBrainzTaskNetworkError(
         QNetworkReply::NetworkError errorCode,
         const QString& errorString,
         const mixxx::network::WebResponseWithContent& responseWithContent) {
-    Q_UNUSED(responseWithContent);
     DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
-    if (!onMusicBrainzTaskTerminated()) {
+    if (m_pMusicBrainzTask.get() != sender()) {
+        // stray call from an already aborted try
         return;
     }
-
-    cancel();
+    terminate();
 
     emit networkError(
-            mixxx::network::kHttpStatusCodeInvalid,
+            responseWithContent.statusCode(),
             QStringLiteral("MusicBrainz"),
             errorString,
             errorCode);
@@ -254,11 +244,11 @@ void TagFetcher::slotMusicBrainzTaskFailed(
         int errorCode,
         const QString& errorMessage) {
     DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
-    if (!onMusicBrainzTaskTerminated()) {
+    if (m_pMusicBrainzTask.get() != sender()) {
+        // stray call from an already aborted try
         return;
     }
-
-    cancel();
+    terminate();
 
     emit networkError(
             response.statusCode(),
@@ -270,12 +260,12 @@ void TagFetcher::slotMusicBrainzTaskFailed(
 void TagFetcher::slotMusicBrainzTaskSucceeded(
         const QList<mixxx::musicbrainz::TrackRelease>& guessedTrackReleases) {
     DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
-    if (!onMusicBrainzTaskTerminated()) {
+    if (m_pMusicBrainzTask.get() != sender()) {
+        // stray call from an already aborted try
         return;
     }
-
-    auto pTrack = std::move(m_pTrack);
-    cancel();
+    auto pTrack = m_pTrack;
+    terminate();
 
     emit resultAvailable(
             std::move(pTrack),

src/musicbrainz/tagfetcher.h

@@ -65,8 +65,7 @@ class TagFetcher : public QObject {
             const mixxx::network::WebResponseWithContent& responseWithContent);
 
   private:
-    bool onAcoustIdTaskTerminated();
-    bool onMusicBrainzTaskTerminated();
+    void terminate();
 
     QNetworkAccessManager m_network;