Refactor CDDA loading signal/slots

Fixes #1803

src/core/songloader.cpp

@@ -179,8 +179,10 @@ SongLoader::Result SongLoader::LoadAudioCD() {
 
 #ifdef HAVE_AUDIOCD
   CDDASongLoader *cdda_song_loader = new CDDASongLoader(QUrl(), this);
-  QObject::connect(cdda_song_loader, &CDDASongLoader::SongsDurationLoaded, this, &SongLoader::AudioCDTracksLoadFinishedSlot);
+  QObject::connect(cdda_song_loader, &CDDASongLoader::LoadError, this, &SongLoader::AudioCDTracksLoadErrorSlot);
-  QObject::connect(cdda_song_loader, &CDDASongLoader::SongsMetadataLoaded, this, &SongLoader::AudioCDTracksTagsLoaded);
+  QObject::connect(cdda_song_loader, &CDDASongLoader::SongsLoaded, this, &SongLoader::AudioCDTracksLoadedSlot);
+  QObject::connect(cdda_song_loader, &CDDASongLoader::SongsUpdated, this, &SongLoader::AudioCDTracksUpdatedSlot);
+  QObject::connect(cdda_song_loader, &CDDASongLoader::LoadingFinished, this, &SongLoader::AudioCDLoadingFinishedSlot);
   cdda_song_loader->LoadSongs();
   return Result::Success;
 #else
@@ -192,23 +194,38 @@ SongLoader::Result SongLoader::LoadAudioCD() {
 
 #ifdef HAVE_AUDIOCD
 
-void SongLoader::AudioCDTracksLoadFinishedSlot(const SongList &songs, const QString &error) {
+void SongLoader::AudioCDTracksLoadErrorSlot(const QString &error) {
 
-  songs_ = songs;
   errors_ << error;
-  Q_EMIT AudioCDTracksLoadFinished();
 
 }
 
-void SongLoader::AudioCDTracksTagsLoaded(const SongList &songs) {
+void SongLoader::AudioCDTracksLoadedSlot(const SongList &songs) {
+
+  songs_ = songs;
+
+  Q_EMIT AudioCDTracksLoaded();
+
+}
+
+void SongLoader::AudioCDTracksUpdatedSlot(const SongList &songs) {
+
+  songs_ = songs;
+
+  Q_EMIT AudioCDTracksUpdated();
+
+}
+
+void SongLoader::AudioCDLoadingFinishedSlot() {
 
   CDDASongLoader *cdda_song_loader = qobject_cast<CDDASongLoader*>(sender());
   cdda_song_loader->deleteLater();
-  songs_ = songs;
+
-  Q_EMIT LoadAudioCDFinished(true);
+  Q_EMIT AudioCDLoadingFinished(true);
 
 }
-#endif
+
+#endif  // HAVE_AUDIOCD
 
 SongLoader::Result SongLoader::LoadLocal(const QString &filename) {
 

src/core/songloader.h

@@ -90,17 +90,21 @@ class SongLoader : public QObject {
   QStringList errors() { return errors_; }
 
  Q_SIGNALS:
-  void AudioCDTracksLoadFinished();
+  void AudioCDTracksLoaded();
-  void LoadAudioCDFinished(const bool success);
+  void AudioCDTracksUpdated();
+  void AudioCDLoadingFinished(const bool success);
   void LoadRemoteFinished();
 
  private Q_SLOTS:
   void ScheduleTimeout();
   void Timeout();
   void StopTypefind();
+
 #ifdef HAVE_AUDIOCD
-  void AudioCDTracksLoadFinishedSlot(const SongList &songs, const QString &error);
+  void AudioCDTracksLoadErrorSlot(const QString &error);
-  void AudioCDTracksTagsLoaded(const SongList &songs);
+  void AudioCDTracksLoadedSlot(const SongList &songs);
+  void AudioCDTracksUpdatedSlot(const SongList &songs);
+  void AudioCDLoadingFinishedSlot();
 #endif  // HAVE_AUDIOCD
 
  private:

src/device/cddadevice.cpp

@@ -63,9 +63,8 @@ CDDADevice::CDDADevice(const QUrl &url,
   timer_disc_changed_->setInterval(1s);
 
   QObject::connect(&cdda_song_loader_, &CDDASongLoader::SongsLoaded, this, &CDDADevice::SongsLoaded);
-  QObject::connect(&cdda_song_loader_, &CDDASongLoader::SongsDurationLoaded, this, &CDDADevice::SongsLoaded);
+  QObject::connect(&cdda_song_loader_, &CDDASongLoader::SongsUpdated, this, &CDDADevice::SongsLoaded);
-  QObject::connect(&cdda_song_loader_, &CDDASongLoader::SongsMetadataLoaded, this, &CDDADevice::SongsLoaded);
+  QObject::connect(&cdda_song_loader_, &CDDASongLoader::LoadingFinished, this, &CDDADevice::SongLoadingFinished);
-  QObject::connect(&cdda_song_loader_, &CDDASongLoader::SongLoadingFinished, this, &CDDADevice::SongLoadingFinished);
   QObject::connect(this, &CDDADevice::SongsDiscovered, collection_model_, &CollectionModel::AddReAddOrUpdate);
   QObject::connect(timer_disc_changed_, &QTimer::timeout, this, &CDDADevice::CheckDiscChanged);
 

src/device/cddasongloader.cpp

@@ -56,7 +56,7 @@ CDDASongLoader::CDDASongLoader(const QUrl &url, QObject *parent)
       network_(make_shared<NetworkAccessManager>()) {
 
 #ifdef HAVE_MUSICBRAINZ
-  QObject::connect(this, &CDDASongLoader::MusicBrainzDiscIdLoaded, this, &CDDASongLoader::LoadMusicBrainzCDTags);
+  QObject::connect(this, &CDDASongLoader::LoadTagsFromMusicBrainz, this, &CDDASongLoader::LoadTagsFromMusicBrainzSlot);
 #endif  // HAVE_MUSICBRAINZ
 }
 
@@ -94,7 +94,7 @@ void CDDASongLoader::LoadSongsFromCDDA() {
     Error(QStringLiteral("%1: %2").arg(error->code).arg(QString::fromUtf8(error->message)));
   }
   if (!cdda) {
-    Q_EMIT SongLoadingFinished();
+    Error(tr("Could not create cdiocddasrc"));
     return;
   }
 
@@ -111,7 +111,6 @@ void CDDASongLoader::LoadSongsFromCDDA() {
     gst_object_unref(GST_OBJECT(cdda));
     cdda = nullptr;
     Error(tr("Error while setting CDDA device to ready state."));
-    Q_EMIT SongLoadingFinished();
     return;
   }
 
@@ -120,7 +119,6 @@ void CDDASongLoader::LoadSongsFromCDDA() {
     gst_object_unref(GST_OBJECT(cdda));
     cdda = nullptr;
     Error(tr("Error while setting CDDA device to pause state."));
-    Q_EMIT SongLoadingFinished();
     return;
   }
 
@@ -133,7 +131,6 @@ void CDDASongLoader::LoadSongsFromCDDA() {
     gst_object_unref(GST_OBJECT(cdda));
     cdda = nullptr;
     Error(tr("Error while querying CDDA tracks."));
-    Q_EMIT SongLoadingFinished();
     return;
   }
 
@@ -143,7 +140,6 @@ void CDDASongLoader::LoadSongsFromCDDA() {
     gst_object_unref(GST_OBJECT(cdda));
     cdda = nullptr;
     Error(tr("Error while querying CDDA tracks."));
-    Q_EMIT SongLoadingFinished();
     return;
   }
 
@@ -158,6 +154,7 @@ void CDDASongLoader::LoadSongsFromCDDA() {
     song.set_track(track_number);
     songs.insert(track_number, song);
   }
+
   Q_EMIT SongsLoaded(songs.values());
 
 #ifdef HAVE_MUSICBRAINZ
@@ -339,42 +336,50 @@ void CDDASongLoader::LoadSongsFromCDDA() {
   // This will also cause cdda to be unref'd.
   gst_object_unref(pipeline);
 
-  Q_EMIT SongsMetadataLoaded(songs.values());
-
   if ((track_artist_tags >= total_tracks && track_album_tags >= total_tracks && track_title_tags >= total_tracks)) {
     qLog(Info) << "Songs loaded from CD-Text";
-    Q_EMIT SongLoadingFinished();
+    Q_EMIT SongsUpdated(songs.values());
+    Q_EMIT LoadingFinished();
   }
-#ifdef HAVE_MUSICBRAINZ
   else {
+#ifdef HAVE_MUSICBRAINZ
     if (musicbrainz_discid.isEmpty()) {
       qLog(Info) << "CD is missing tags";
+      Q_EMIT LoadingFinished();
     }
     else {
       qLog(Info) << "MusicBrainz Disc ID:" << musicbrainz_discid;
-      Q_EMIT MusicBrainzDiscIdLoaded(musicbrainz_discid);
+      Q_EMIT LoadTagsFromMusicBrainz(musicbrainz_discid);
     }
-  }
+#else
+    Q_EMIT LoadingFinished();
 #endif  // HAVE_MUSICBRAINZ
+  }
+
 }
 
 #ifdef HAVE_MUSICBRAINZ
 
-void CDDASongLoader::LoadMusicBrainzCDTags(const QString &musicbrainz_discid) const {
+void CDDASongLoader::LoadTagsFromMusicBrainzSlot(const QString &musicbrainz_discid) const {
 
   MusicBrainzClient *musicbrainz_client = new MusicBrainzClient(network_);
-  QObject::connect(musicbrainz_client, &MusicBrainzClient::DiscIdFinished, this, &CDDASongLoader::MusicBrainzCDTagsLoaded);
+  QObject::connect(musicbrainz_client, &MusicBrainzClient::DiscIdFinished, this, &CDDASongLoader::LoadTagsFromMusicBrainzFinished);
   musicbrainz_client->StartDiscIdRequest(musicbrainz_discid);
 
 }
 
-void CDDASongLoader::MusicBrainzCDTagsLoaded(const QString &artist, const QString &album, const MusicBrainzClient::ResultList &results) {
+void CDDASongLoader::LoadTagsFromMusicBrainzFinished(const QString &artist, const QString &album, const MusicBrainzClient::ResultList &results, const QString &error) {
 
   MusicBrainzClient *musicbrainz_client = qobject_cast<MusicBrainzClient*>(sender());
   musicbrainz_client->deleteLater();
 
+  if (!error.isEmpty()) {
+    Error(error);
+    return;
+  }
+
   if (results.empty()) {
-    Q_EMIT SongLoadingFinished();
+    Q_EMIT LoadingFinished();
     return;
   }
 
@@ -398,8 +403,8 @@ void CDDASongLoader::MusicBrainzCDTagsLoaded(const QString &artist, const QStrin
     songs << song;
   }
 
-  Q_EMIT SongsMetadataLoaded(songs);
+  Q_EMIT SongsUpdated(songs);
-  Q_EMIT SongLoadingFinished();
+  Q_EMIT LoadingFinished();
 
 }
 
@@ -408,6 +413,8 @@ void CDDASongLoader::MusicBrainzCDTagsLoaded(const QString &artist, const QStrin
 void CDDASongLoader::Error(const QString &error) {
 
   qLog(Error) << error;
-  Q_EMIT SongsDurationLoaded(SongList(), error);
+
+  Q_EMIT LoadError(error);
+  Q_EMIT LoadingFinished();
 
 }

src/device/cddasongloader.h

@@ -58,17 +58,16 @@ class CDDASongLoader : public QObject {
   QUrl GetUrlFromTrack(const int track_number) const;
 
  Q_SIGNALS:
-  void SongsLoadError(const QString &error);
   void SongsLoaded(const SongList &songs);
-  void SongLoadingFinished();
+  void SongsUpdated(const SongList &songs);
-  void SongsDurationLoaded(const SongList &songs, const QString &error = QString());
+  void LoadError(const QString &error);
-  void SongsMetadataLoaded(const SongList &songs);
+  void LoadingFinished();
-  void MusicBrainzDiscIdLoaded(const QString &musicbrainz_discid);
+  void LoadTagsFromMusicBrainz(const QString &musicbrainz_discid);
 
  private Q_SLOTS:
 #ifdef HAVE_MUSICBRAINZ
-  void LoadMusicBrainzCDTags(const QString &musicbrainz_discid) const;
+  void LoadTagsFromMusicBrainzSlot(const QString &musicbrainz_discid) const;
-  void MusicBrainzCDTagsLoaded(const QString &artist, const QString &album, const MusicBrainzClient::ResultList &results);
+  void LoadTagsFromMusicBrainzFinished(const QString &artist, const QString &album, const MusicBrainzClient::ResultList &results, const QString &error);
 #endif
 
  private:

src/playlist/songloaderinserter.cpp

@@ -96,6 +96,7 @@ void SongLoaderInserter::Load(Playlist *destination, const int row, const bool p
   else {
     (void)QtConcurrent::run(&SongLoaderInserter::AsyncLoad, this);
   }
+
 }
 
 // Load audio CD tracks:
@@ -111,13 +112,15 @@ void SongLoaderInserter::LoadAudioCD(Playlist *destination, const int row, const
   enqueue_next_ = enqueue_next;
 
   SongLoader *loader = new SongLoader(url_handlers_, collection_backend_, tagreader_client_, this);
-  QObject::connect(loader, &SongLoader::AudioCDTracksLoadFinished, this, [this, loader]() { AudioCDTracksLoadFinished(loader); });
+  QObject::connect(loader, &SongLoader::AudioCDTracksLoaded, this, &SongLoaderInserter::AudioCDTracksLoadedSlot);
-  QObject::connect(loader, &SongLoader::LoadAudioCDFinished, this, &SongLoaderInserter::AudioCDTagsLoaded);
+  QObject::connect(loader, &SongLoader::AudioCDTracksUpdated, this, &SongLoaderInserter::AudioCDTracksUpdatedSlot);
+  QObject::connect(loader, &SongLoader::AudioCDLoadingFinished, this, &SongLoaderInserter::AudioCDLoadingFinishedSlot);
   qLog(Info) << "Loading audio CD...";
   const SongLoader::Result result = loader->LoadAudioCD();
   if (result == SongLoader::Result::Error) {
-    if (loader->errors().isEmpty())
+    if (loader->errors().isEmpty()) {
       Q_EMIT Error(tr("Error while loading audio CD."));
+    }
     else {
       const QStringList errors = loader->errors();
       for (const QString &error : errors) {
@@ -126,13 +129,17 @@ void SongLoaderInserter::LoadAudioCD(Playlist *destination, const int row, const
     }
     delete loader;
   }
+
   // Songs will be loaded later: see AudioCDTracksLoadFinished and AudioCDTagsLoaded slots
 
 }
 
 void SongLoaderInserter::DestinationDestroyed() { destination_ = nullptr; }
 
-void SongLoaderInserter::AudioCDTracksLoadFinished(SongLoader *loader) {
+void SongLoaderInserter::AudioCDTracksLoadedSlot() {
+
+  SongLoader *loader = qobject_cast<SongLoader*>(sender());
+  if (!loader) return;
 
   songs_ = loader->songs();
   if (songs_.isEmpty()) {
@@ -147,17 +154,18 @@ void SongLoaderInserter::AudioCDTracksLoadFinished(SongLoader *loader) {
 
 }
 
-void SongLoaderInserter::AudioCDTagsLoaded(const bool success) {
+void SongLoaderInserter::AudioCDTracksUpdatedSlot() {
 
   SongLoader *loader = qobject_cast<SongLoader*>(sender());
-  if (!loader || !destination_) return;
+  if (!loader || loader->songs().isEmpty() || !destination_) return;
 
-  if (success) {
+  destination_->UpdateItems(loader->songs());
-    destination_->UpdateItems(loader->songs());
+
-  }
+}
-  else {
+
-    qLog(Error) << "Error while getting audio CD metadata from MusicBrainz";
+void SongLoaderInserter::AudioCDLoadingFinishedSlot(const bool success) {
-  }
+
+  Q_UNUSED(success)
 
   deleteLater();
 

src/playlist/songloaderinserter.h

@@ -62,8 +62,9 @@ class SongLoaderInserter : public QObject {
 
  private Q_SLOTS:
   void DestinationDestroyed();
-  void AudioCDTracksLoadFinished(SongLoader *loader);
+  void AudioCDTracksLoadedSlot();
-  void AudioCDTagsLoaded(const bool success);
+  void AudioCDTracksUpdatedSlot();
+  void AudioCDLoadingFinishedSlot(const bool success);
   void InsertSongs();
 
  private: