Improve song loader error handling

src/core/song.cpp

@@ -205,6 +205,8 @@ struct Song::Private : public QSharedData {
   bool init_from_file_;		// Whether this song was loaded from a file using taglib.
   bool suspicious_tags_;	// Whether our encoding guesser thinks these tags might be incorrectly encoded.
 
+  QString error_;
+
 };
 
 Song::Private::Private(Song::Source source)
@@ -317,6 +319,7 @@ void Song::manually_unset_cover() { d->art_manual_ = kManuallyUnsetCover; }
 bool Song::has_embedded_cover() const { return d->art_automatic_ == kEmbeddedCover; }
 void Song::set_embedded_cover() { d->art_automatic_ = kEmbeddedCover; }
 const QImage &Song::image() const { return d->image_; }
+const QString &Song::error() const { return d->error_; }
 
 void Song::set_id(int id) { d->id_ = id; }
 void Song::set_album_id(int v) { d->album_id_ = v; }
@@ -850,7 +853,7 @@ void Song::InitFromFilePartial(const QString &filename) {
   }
   else {
     d->valid_ = false;
-    qLog(Error) << "File" << filename << "is not recognized by TagLib as a valid audio file.";
+    d->error_ = QObject::tr("File %1 is not recognized as a valid audio file.").arg(filename);
   }
 
 }

src/core/song.h

@@ -249,6 +249,8 @@ class Song {
 
   const QImage &image() const;
 
+  const QString &error() const;
+
   // Pretty accessors
   QString PrettyTitle() const;
   QString PrettyTitleWithArtist() const;

src/core/songloader.cpp

@@ -134,18 +134,27 @@ SongLoader::Result SongLoader::Load(const QUrl &url) {
     preload_func_ = std::bind(&SongLoader::LoadRemote, this);
     return BlockingLoadRequired;
 #else
+    errors_ << tr("You need GStreamer for this URL.");
     return Error;
 #endif
   }
+  else {
+    errors_ << tr("You need GStreamer for this URL.");
+    return Error;
+  }
 
   return Success;
 
 }
 
-void SongLoader::LoadFilenamesBlocking() {
+SongLoader::Result SongLoader::LoadFilenamesBlocking() {
 
   if (preload_func_) {
-    preload_func_();
+    return preload_func_();
+  }
+  else {
+    errors_ << tr("Preload function was not set for blocking operation.");
+    return Error;
   }
 
 }
@@ -160,21 +169,33 @@ SongLoader::Result SongLoader::LoadLocalPartial(const QString &filename) {
   }
   Song song;
   song.InitFromFilePartial(filename);
-  if (song.is_valid()) songs_ << song;
+  if (song.is_valid()) {
-  return Success;
+    songs_ << song;
+    return Success;
+  }
+  else {
+    errors_ << song.error();
+    return Error;
+  }
 
 }
 
 SongLoader::Result SongLoader::LoadAudioCD() {
 
 #if defined(HAVE_AUDIOCD) && defined(HAVE_GSTREAMER)
-  CddaSongLoader *cdda_song_loader = new CddaSongLoader;
+  if (player_->engine()->type() == Engine::GStreamer) {
-  connect(cdda_song_loader, SIGNAL(SongsDurationLoaded(SongList)), this, SLOT(AudioCDTracksLoadedSlot(SongList)));
+    CddaSongLoader *cdda_song_loader = new CddaSongLoader(QUrl(), this);
-  connect(cdda_song_loader, SIGNAL(SongsMetadataLoaded(SongList)), this, SLOT(AudioCDTracksTagsLoaded(SongList)));
+    connect(cdda_song_loader, SIGNAL(SongsDurationLoaded(SongList)), this, SLOT(AudioCDTracksLoadedSlot(SongList)));
-  cdda_song_loader->LoadSongs();
+    connect(cdda_song_loader, SIGNAL(SongsMetadataLoaded(SongList)), this, SLOT(AudioCDTracksTagsLoaded(SongList)));
-  return Success;
+    cdda_song_loader->LoadSongs();
-#else
+    return Success;
-  return Error;
+  }
+  else {
+#endif
+    errors_ << tr("CD playback is only available with the GStreamer engine.");
+    return Error;
+#if defined(HAVE_AUDIOCD) && defined(HAVE_GSTREAMER)
+  }
 #endif
 
 }
@@ -225,17 +246,20 @@ SongLoader::Result SongLoader::LoadLocal(const QString &filename) {
 
 }
 
-void SongLoader::LoadLocalAsync(const QString &filename) {
+SongLoader::Result SongLoader::LoadLocalAsync(const QString &filename) {
 
   // First check to see if it's a directory - if so we will load all the songs inside right away.
   if (QFileInfo(filename).isDir()) {
     LoadLocalDirectory(filename);
-    return;
+    return Success;
   }
 
   // It's a local file, so check if it looks like a playlist. Read the first few bytes.
   QFile file(filename);
-  if (!file.open(QIODevice::ReadOnly)) return;
+  if (!file.open(QIODevice::ReadOnly)) {
+    errors_ << tr("Could not open file %1").arg(filename);
+    return Error;
+  }
   QByteArray data(file.read(PlaylistParser::kMagicSize));
 
   ParserBase *parser = playlist_parser_->ParserForMagic(data);
@@ -247,7 +271,7 @@ void SongLoader::LoadLocalAsync(const QString &filename) {
   if (parser) { // It's a playlist!
     qLog(Debug) << "Parsing using" << parser->name();
     LoadPlaylist(parser, filename);
-    return;
+    return Success;
   }
 
   // Check if it's a cue file
@@ -261,13 +285,20 @@ void SongLoader::LoadLocalAsync(const QString &filename) {
     for (const Song &song : song_list) {
       if (song.is_valid()) songs_ << song;
     }
-    return;
+    return Success;
   }
 
   // Assume it's just a normal file
   Song song;
   song.InitFromFilePartial(filename);
-  if (song.is_valid()) songs_ << song;
+  if (song.is_valid()) {
+    songs_ << song;
+    return Success;
+  }
+  else {
+    errors_ << song.error();
+    return Error;
+  }
 
 }
 
@@ -384,7 +415,7 @@ void SongLoader::StopTypefind() {
 }
 
 #ifdef HAVE_GSTREAMER
-void SongLoader::LoadRemote() {
+SongLoader::Result SongLoader::LoadRemote() {
 
   qLog(Debug) << "Loading remote file" << url_;
 
@@ -402,8 +433,8 @@ void SongLoader::LoadRemote() {
   // Create the source element automatically based on the URL
   GstElement *source = gst_element_make_from_uri(GST_URI_SRC, url_.toEncoded().constData(), nullptr, nullptr);
   if (!source) {
-    qLog(Warning) << "Couldn't create gstreamer source element for" << url_.toString();
+    errors_ << tr("Couldn't create gstreamer source element for %1").arg(url_.toString());
-    return;
+    return Error;
   }
 
   // Create the other elements and link them up
@@ -433,6 +464,9 @@ void SongLoader::LoadRemote() {
 
   // Wait until loading is finished
   loop.exec();
+
+  return Success;
+
 }
 #endif
 

src/core/songloader.h

@@ -38,6 +38,7 @@
 #include <QByteArray>
 #include <QSet>
 #include <QString>
+#include <QStringList>
 #include <QUrl>
 #include <QTimer>
 
@@ -77,13 +78,15 @@ class SongLoader : public QObject {
   Result Load(const QUrl &url);
   // Loads the files with only filenames. When finished, songs() contains a complete list of all Song objects, but without metadata.
   // This method is blocking, do not call it from the UI thread.
-  void LoadFilenamesBlocking();
+  SongLoader::Result LoadFilenamesBlocking();
   // Completely load songs previously loaded with LoadFilenamesBlocking().
   // When finished, the Song objects in songs() contain metadata now. This method is blocking, do not call it from the UI thread.
   void LoadMetadataBlocking();
   Result LoadAudioCD();
 
-signals:
+  QStringList errors() { return errors_; }
+
+ signals:
   void AudioCDTracksLoaded();
   void LoadAudioCDFinished(bool success);
   void LoadRemoteFinished();
@@ -100,7 +103,7 @@ signals:
   enum State { WaitingForType, WaitingForMagic, WaitingForData, Finished };
 
   Result LoadLocal(const QString &filename);
-  void LoadLocalAsync(const QString &filename);
+  SongLoader::Result LoadLocalAsync(const QString &filename);
   void EffectiveSongLoad(Song *song);
   Result LoadLocalPartial(const QString &filename);
   void LoadLocalDirectory(const QString &filename);
@@ -109,7 +112,7 @@ signals:
   void AddAsRawStream();
 
 #ifdef HAVE_GSTREAMER
-  void LoadRemote();
+  Result LoadRemote();
 
   // GStreamer callbacks
   static void TypeFound(GstElement *typefind, uint probability, GstCaps *caps, void *self);
@@ -135,7 +138,7 @@ signals:
   CueParser *cue_parser_;
 
   // For async loads
-  std::function<void()> preload_func_;
+  std::function<Result()> preload_func_;
   int timeout_;
   State state_;
   bool success_;
@@ -150,6 +153,9 @@ signals:
 #endif
 
   QThreadPool thread_pool_;
+
+  QStringList errors_;
+
 };
 
 #endif  // SONGLOADER_H

src/playlist/songloaderinserter.cpp

@@ -40,7 +40,8 @@ SongLoaderInserter::SongLoaderInserter(TaskManager *task_manager, CollectionBack
       enqueue_(false),
       enqueue_next_(false),
       collection_(collection),
-      player_(player) {}
+      player_(player) {
+    }
 
 SongLoaderInserter::~SongLoaderInserter() { qDeleteAll(pending_); }
 
@@ -66,10 +67,14 @@ void SongLoaderInserter::Load(Playlist *destination, int row, bool play_now, boo
       continue;
     }
 
-    if (ret == SongLoader::Success)
+    if (ret == SongLoader::Success) {
       songs_ << loader->songs();
-    else
+    }
-      emit Error(tr("Error loading %1").arg(url.toString()));
+    else {
+      for (const QString &error : loader->errors()) {
+        emit Error(error);
+      }
+    }
     delete loader;
   }
 
@@ -100,7 +105,13 @@ void SongLoaderInserter::LoadAudioCD(Playlist *destination, int row, bool play_n
   qLog(Info) << "Loading audio CD...";
   SongLoader::Result ret = loader->LoadAudioCD();
   if (ret == SongLoader::Error) {
-    emit Error(tr("Error while loading audio CD"));
+    if (loader->errors().isEmpty())
+      emit Error(tr("Error while loading audio CD."));
+    else {
+      for (const QString &error : loader->errors()) {
+        emit Error(error);
+      }
+    }
     delete loader;
   }
   // Songs will be loaded later: see AudioCDTracksLoaded and AudioCDTagsLoaded slots
@@ -141,16 +152,28 @@ void SongLoaderInserter::AsyncLoad() {
   int async_progress = 0;
   int async_load_id = task_manager_->StartTask(tr("Loading tracks"));
   task_manager_->SetTaskProgress(async_load_id, async_progress, pending_.count());
+  bool first_loaded = false;
   for (int i = 0; i < pending_.count(); ++i) {
     SongLoader *loader = pending_[i];
-    loader->LoadFilenamesBlocking();
+    SongLoader::Result res = loader->LoadFilenamesBlocking();
     task_manager_->SetTaskProgress(async_load_id, ++async_progress);
-    if (i == 0) {
+
+    if (res == SongLoader::Error) {
+      for (const QString &error : loader->errors()) {
+        emit Error(error);
+      }
+      continue;
+    }
+
+    if (!first_loaded) {
       // Load everything from the first song.
       // It'll start playing as soon as we emit PreloadFinished, so it needs to have the duration set to show properly in the UI.
       loader->LoadMetadataBlocking();
+      first_loaded = true;
     }
+
     songs_ << loader->songs();
+
   }
   task_manager_->SetTaskFinished(async_load_id);
   emit PreloadFinished();

src/playlist/songloaderinserter.h

@@ -47,7 +47,7 @@ class SongLoaderInserter : public QObject {
   void Load(Playlist *destination, int row, bool play_now, bool enqueue, bool enqueue_next, const QList<QUrl> &urls);
   void LoadAudioCD(Playlist *destination, int row, bool play_now, bool enqueue, bool enqueue_next);
 
-signals:
+ signals:
   void Error(const QString &message);
   void PreloadFinished();
   void EffectiveLoadFinished(const SongList &songs);
@@ -75,6 +75,7 @@ signals:
   QList<SongLoader*> pending_;
   CollectionBackendInterface *collection_;
   const Player *player_;
+
 };
 
 #endif  // SONGLOADERINSERTER_H