File 0001-Add-error-handling-path-for-async-song-loa... of Package clementine

Overview Repositories Revisions Requests Users Attributes Meta

File 0001-Add-error-handling-path-for-async-song-loading.patch of Package clementine (Revision 641bcecffe390f0a684c11fcc314408d)

Currently displaying revision 641bcecffe390f0a684c11fcc314408d , Show latest

From babff78025cba7e2f82cf905f14eb0db8090cb2a Mon Sep 17 00:00:00 2001
From: Jim Broadus <jbroadus@gmail.com>
Date: Sun, 7 Apr 2019 14:18:11 -0700
Subject: [PATCH] Add error handling path for async song loading.

Async song loading can fail without user feedback. This change adds return codes
to these async load functions. It will now produce an error dialog in simple
scenarios (test case is user selecting a file that is not readable). Other cases,
such as directories and playlists, aren't yet covered.
---
 src/core/songloader.cpp             | 28 +++++++++++++++++++---------
 src/core/songloader.h               |  8 ++++----
 src/playlist/songloaderinserter.cpp | 12 ++++++++++--
 3 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/src/core/songloader.cpp b/src/core/songloader.cpp
index 677fc26b5..0a28149c9 100644
--- a/src/core/songloader.cpp
+++ b/src/core/songloader.cpp
@@ -130,9 +130,12 @@ SongLoader::Result SongLoader::Load(const QUrl& url) {
   return BlockingLoadRequired;
 }
 
-void SongLoader::LoadFilenamesBlocking() {
+SongLoader::Result SongLoader::LoadFilenamesBlocking() {
   if (preload_func_) {
-    preload_func_();
+    return preload_func_();
+  } else {
+    qLog(Error) << "Preload function was not set for blocking operation";
+    return Error;
   }
 }
 
@@ -207,18 +210,21 @@ SongLoader::Result SongLoader::LoadLocal(const QString& filename) {
   return BlockingLoadRequired;
 }
 
-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)) {
+    qLog(Error) << "Could not open file " << filename;
+    return Error;
+  }
   QByteArray data(file.read(PlaylistParser::kMagicSize));
 
   ParserBase* parser = playlist_parser_->ParserForMagic(data);
@@ -234,7 +240,7 @@ void SongLoader::LoadLocalAsync(const QString& filename) {
 
     // It's a playlist!
     LoadPlaylist(parser, filename);
-    return;
+    return Success;
   }
 
   // Check if it's a cue file
@@ -249,7 +255,7 @@ void SongLoader::LoadLocalAsync(const QString& filename) {
     for (Song song : song_list) {
       if (song.is_valid()) songs_ << song;
     }
-    return;
+    return Success;
   }
 
   // Assume it's just a normal file
@@ -257,6 +263,9 @@ void SongLoader::LoadLocalAsync(const QString& filename) {
   song.InitFromFilePartial(filename);
   if (song.is_valid()) {
     songs_ << song;
+    return Success;
+  } else {
+    return Error;
   }
 }
 
@@ -374,7 +383,7 @@ void SongLoader::StopTypefind() {
   emit LoadRemoteFinished();
 }
 
-void SongLoader::LoadRemote() {
+SongLoader::Result SongLoader::LoadRemote() {
   qLog(Debug) << "Loading remote file" << url_;
 
   // It's not a local file so we have to fetch it to see what it is.  We use
@@ -399,7 +408,7 @@ void SongLoader::LoadRemote() {
   if (!source) {
     qLog(Warning) << "Couldn't create gstreamer source element for"
                   << url_.toString();
-    return;
+    return Error;
   }
 
   // Create the other elements and link them up
@@ -430,6 +439,7 @@ void SongLoader::LoadRemote() {
 
   // Wait until loading is finished
   loop.exec();
+  return Success;
 }
 
 void SongLoader::TypeFound(GstElement*, uint, GstCaps* caps, void* self) {
diff --git a/src/core/songloader.h b/src/core/songloader.h
index 494a4fc9d..0f32378e4 100644
--- a/src/core/songloader.h
+++ b/src/core/songloader.h
@@ -72,7 +72,7 @@ class SongLoader : public QObject {
   // 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();
+  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.
@@ -101,7 +101,7 @@ signals:
   };
 
   Result LoadLocal(const QString& filename);
-  void LoadLocalAsync(const QString& filename);
+  Result LoadLocalAsync(const QString& filename);
   void EffectiveSongLoad(Song* song);
   Result LoadLocalPartial(const QString& filename);
   void LoadLocalDirectory(const QString& filename);
@@ -109,7 +109,7 @@ signals:
 
   void AddAsRawStream();
 
-  void LoadRemote();
+  Result LoadRemote();
   bool LoadRemotePlaylist(const QUrl& url);
 
   // GStreamer callbacks
@@ -138,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_;
diff --git a/src/playlist/songloaderinserter.cpp b/src/playlist/songloaderinserter.cpp
index d6ebca740..ffa83e59a 100644
--- a/src/playlist/songloaderinserter.cpp
+++ b/src/playlist/songloaderinserter.cpp
@@ -134,15 +134,23 @@ void SongLoaderInserter::AsyncLoad() {
   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) {
+      emit Error(tr("Error loading %1").arg(loader->url().toString()));
+      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();
   }
-- 
2.16.4

Places

File 0001-Add-error-handling-path-for-async-song-loading.patch of Package clementine (Revision 641bcecffe390f0a684c11fcc314408d)

Places