RipCDDialog: no longer forces Cdda* to emit signals

- CddaDevice: Removed LoadSongs() method (then renamed ForceLoadSongs to LoadSongs)
- CddaDevice: added songs() method to get currently song list
- CddaSongLoader: cached_tracks is now thread-safe

src/devices/cddadevice.cpp

@@ -52,7 +52,7 @@ bool CddaDevice::Init() {
   if (!cdio_) {
     cdio_ = cdio_open(url_.path().toLocal8Bit().constData(), DRIVER_DEVICE);
     if (!cdio_) return false;
-    ForceLoadSongs();
+    LoadSongs();
     WatchForDiscChanges(true);
   }
   return true;
@@ -71,16 +71,11 @@ void CddaDevice::WatchForDiscChanges(bool watch) {
     disc_changed_timer_.stop();
 }
 
-void CddaDevice::ForceLoadSongs() {
+void CddaDevice::LoadSongs() {
   cdda_song_loader_.LoadSongs();
   disc_changed_timer_.stop();
 }
 
-void CddaDevice::LoadSongs() {
-  SongList songs = cdda_song_loader_.cached_tracks();
-  SongsLoaded(songs);
-}
-
 void CddaDevice::SongsLoaded(const SongList& songs) {
   model_->Reset();
   song_count_ = songs.size();
@@ -111,6 +106,8 @@ void CddaDevice::CheckDiscChanged() {
     song_count_ = 0;
     SongList no_songs;
     SongsLoaded(no_songs);
-    ForceLoadSongs();
+    LoadSongs();
   }
 }
+
+SongList CddaDevice::songs() const { return cdda_song_loader_.cached_tracks(); }

src/devices/cddadevice.h

@@ -54,7 +54,7 @@ class CddaDevice : public ConnectedDevice {
   // Check whether a valid device handle was opened.
   bool IsValid() const;
   void WatchForDiscChanges(bool watch);
-  void LoadSongs();
+  SongList songs() const;
 
   static QStringList url_schemes() { return QStringList() << "cdda"; }
 
@@ -79,7 +79,7 @@ class CddaDevice : public ConnectedDevice {
   void CheckDiscChanged();
 
  private:
-  void ForceLoadSongs();
+  void LoadSongs();
 
   CdIo_t* cdio_;
   QTimer disc_changed_timer_;

src/devices/cddasongloader.cpp

@@ -68,6 +68,7 @@ bool CddaSongLoader::IsActive() const { return loading_future_.isRunning(); }
 void CddaSongLoader::LoadSongs() {
   // only dispatch a new thread for loading tracks if not already running.
   if (!IsActive()) {
+    QMutexLocker lock(&disc_mutex_);
     disc_ = Disc();
     loading_future_ =
         QtConcurrent::run(this, &CddaSongLoader::LoadSongsFromCdda);
@@ -350,37 +351,45 @@ void CddaSongLoader::ProcessMusicBrainzResponse(
     return;
   }
 
-  if (disc_.tracks.length() != results.length()) {
-    qLog(Warning) << "Number of tracks in metadata does not match number of "
-                     "songs on disc!";
-    // no idea how to recover; signal that no further updates will follow now
-    emit Finished();
-    return;
+  {
+    QMutexLocker lock(&disc_mutex_);
+
+    if (disc_.tracks.length() != results.length()) {
+      qLog(Warning) << "Number of tracks in metadata does not match number of "
+                       "songs on disc!";
+      // no idea how to recover; signal that no further updates will follow now
+      emit Finished();
+      return;
+    }
+
+    for (int i = 0; i < results.length(); ++i) {
+      const MusicBrainzClient::Result& new_song_info = results[i];
+      Song& song = disc_.tracks[i];
+
+      if (!disc_.has_titles) song.set_title(new_song_info.title_);
+      if (song.album().isEmpty()) song.set_album(album);
+      if (song.artist().isEmpty()) song.set_artist(artist);
+
+      if (song.length_nanosec() == -1)
+        song.set_length_nanosec(new_song_info.duration_msec_ * kNsecPerMsec);
+      if (song.track() < 1) song.set_track(new_song_info.track_);
+      if (song.year() == -1) song.set_year(new_song_info.year_);
+    }
+    disc_.has_titles = true;
   }
 
-  for (int i = 0; i < results.length(); ++i) {
-    const MusicBrainzClient::Result& new_song_info = results[i];
-    Song& song = disc_.tracks[i];
-
-    if (!disc_.has_titles) song.set_title(new_song_info.title_);
-    if (song.album().isEmpty()) song.set_album(album);
-    if (song.artist().isEmpty()) song.set_artist(artist);
-
-    if (song.length_nanosec() == -1)
-      song.set_length_nanosec(new_song_info.duration_msec_ * kNsecPerMsec);
-    if (song.track() < 1) song.set_track(new_song_info.track_);
-    if (song.year() == -1) song.set_year(new_song_info.year_);
-  }
-  disc_.has_titles = true;
-
   emit SongsMetadataLoaded(disc_.tracks);
   emit Finished();  // no further updates will follow
 }
 
 void CddaSongLoader::SetDiscTracks(const SongList& songs, bool has_titles) {
+  QMutexLocker lock(&disc_mutex_);
   disc_.tracks = songs;
   disc_.has_titles = has_titles;
   emit SongsUpdated(disc_.tracks);
 }
 
-SongList CddaSongLoader::cached_tracks() const { return disc_.tracks; }
+SongList CddaSongLoader::cached_tracks() const {
+  QMutexLocker lock(&disc_mutex_);
+  return disc_.tracks;
+}

src/devices/cddasongloader.h

@@ -48,8 +48,8 @@ class CddaSongLoader : public QObject {
   void LoadSongs();
   bool IsActive() const;
 
-  // The list of currently cached tracks. This gets updated during calls
-  // LoadSongs() is called. Not thread-safe.
+  // The list of currently cached tracks. This gets updated when
+  // LoadSongs() is called.
   SongList cached_tracks() const;
 
  signals:
@@ -110,6 +110,7 @@ class CddaSongLoader : public QObject {
   QFuture<void> loading_future_;
   std::atomic<bool> may_load_;
   Disc disc_;
+  mutable QMutex disc_mutex_;
 };
 
 #endif  // CDDASONGLOADER_H

src/ripper/ripcddialog.cpp

@@ -335,10 +335,12 @@ void RipCDDialog::DeviceSelected(int device_index) {
     return;
   }
 
+  SongList songs = cdda_device_->songs();
+  SongsLoaded(songs);
+
   connect(cdda_device_.get(), SIGNAL(DiscChanged()), SLOT(DiscChanged()));
   connect(cdda_device_.get(), SIGNAL(SongsDiscovered(SongList)),
           SLOT(SongsLoaded(SongList)));
-  cdda_device_->LoadSongs();
 }
 
 void RipCDDialog::Finished(Ripper* ripper) {