Scrobbler: Simplify error handling

src/scrobbler/listenbrainzscrobbler.cpp

@@ -227,6 +227,45 @@ void ListenBrainzScrobbler::RedirectArrived() {
 
 }
 
+ListenBrainzScrobbler::ReplyResult ListenBrainzScrobbler::GetJsonObject(QNetworkReply *reply, QJsonObject &json_obj, QString &error_description) {
+
+  ReplyResult reply_error_type = ReplyResult::ServerError;
+
+  if (reply->error() == QNetworkReply::NoError) {
+    if (reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() == 200) {
+      reply_error_type = ReplyResult::Success;
+    }
+    else {
+      error_description = QString("Received HTTP code %1").arg(reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt());
+    }
+  }
+  else {
+    error_description = QString("%1 (%2)").arg(reply->errorString()).arg(reply->error());
+  }
+
+  // See if there is Json data containing "error" and "error_description" or "code" and "error" - then use that instead.
+  if (reply->error() == QNetworkReply::NoError || reply->error() >= 200) {
+    const QByteArray data = reply->readAll();
+    if (!data.isEmpty() && ExtractJsonObj(data, json_obj, error_description)) {
+      if (json_obj.contains("error") && json_obj.contains("error_description")) {
+        error_description = json_obj["error_description"].toString();
+        reply_error_type = ReplyResult::APIError;
+      }
+      else if (json_obj.contains("code") && json_obj.contains("error")) {
+        error_description = QString("%1 (%2)").arg(json_obj["error"].toString()).arg(json_obj["code"].toInt());
+        reply_error_type = ReplyResult::APIError;
+      }
+    }
+    if (reply->error() == QNetworkReply::ContentAccessDenied || reply->error() == QNetworkReply::ContentOperationNotPermittedError || reply->error() == QNetworkReply::AuthenticationRequiredError) {
+      // Session is probably expired
+      Logout();
+    }
+  }
+
+  return reply_error_type;
+
+}
+
 void ListenBrainzScrobbler::RequestAccessToken(const QUrl &redirect_url, const QString &code) {
 
   refresh_login_timer_.stop();
@@ -271,50 +310,10 @@ void ListenBrainzScrobbler::AuthenticateReplyFinished(QNetworkReply *reply) {
   QObject::disconnect(reply, nullptr, this, nullptr);
   reply->deleteLater();
 
-  QByteArray data;
+  QJsonObject json_obj;
-
+  QString error_message;
-  if (reply->error() == QNetworkReply::NoError && reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() == 200) {
+  if (GetJsonObject(reply, json_obj, error_message) != ReplyResult::Success) {
-    data = reply->readAll();
+    AuthError(error_message);
-  }
-  else {
-    if (reply->error() != QNetworkReply::NoError && reply->error() < 200) {
-      // This is a network error, there is nothing more to do.
-      AuthError(QString("%1 (%2)").arg(reply->errorString()).arg(reply->error()));
-    }
-    else {
-      // See if there is Json data containing "error" and "error_description" - then use that instead.
-      data = reply->readAll();
-      QString error;
-      QJsonParseError json_error;
-      QJsonDocument json_doc = QJsonDocument::fromJson(data, &json_error);
-      if (json_error.error == QJsonParseError::NoError && !json_doc.isEmpty() && json_doc.isObject()) {
-        QJsonObject json_obj = json_doc.object();
-        if (json_obj.contains("error") && json_obj.contains("error_description")) {
-          error = json_obj["error_description"].toString();
-        }
-      }
-      if (error.isEmpty()) {
-        if (reply->error() != QNetworkReply::NoError) {
-          error = QString("%1 (%2)").arg(reply->errorString()).arg(reply->error());
-        }
-        else {
-          error = QString("Received HTTP code %1").arg(reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt());
-        }
-      }
-      AuthError(error);
-    }
-    return;
-  }
-
-  QJsonObject json_obj = ExtractJsonObj(data);
-  if (json_obj.isEmpty()) {
-    AuthError("Json document from server was empty.");
-    return;
-  }
-
-  if (json_obj.contains("error") && json_obj.contains("error_description")) {
-    QString failure_reason = json_obj["error_description"].toString();
-    AuthError(failure_reason);
     return;
   }
 
@@ -368,56 +367,6 @@ QNetworkReply *ListenBrainzScrobbler::CreateRequest(const QUrl &url, const QJson
 
 }
 
-QByteArray ListenBrainzScrobbler::GetReplyData(QNetworkReply *reply) {
-
-  QByteArray data;
-
-  if (reply->error() == QNetworkReply::NoError && reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() == 200) {
-    data = reply->readAll();
-  }
-  else {
-    if (reply->error() < 200) {
-      // This is a network error, there is nothing more to do.
-      Error(QString("%1 (%2)").arg(reply->errorString()).arg(reply->error()));
-    }
-    else {
-      // See if there is Json data containing "code" and "error" - then use that instead.
-      data = reply->readAll();
-      QString error;
-      QJsonParseError json_error;
-      QJsonDocument json_doc = QJsonDocument::fromJson(data, &json_error);
-      if (json_error.error == QJsonParseError::NoError && !json_doc.isEmpty() && json_doc.isObject()) {
-        QJsonObject json_obj = json_doc.object();
-        if (json_obj.contains("code") && json_obj.contains("error")) {
-          int error_code = json_obj["code"].toInt();
-          QString error_message = json_obj["error"].toString();
-          error = QString("%1 (%2)").arg(error_message).arg(error_code);
-        }
-        else {
-          error = QString("%1 (%2)").arg(reply->errorString()).arg(reply->error());
-        }
-      }
-      if (error.isEmpty()) {
-        if (reply->error() != QNetworkReply::NoError) {
-          error = QString("%1 (%2)").arg(reply->errorString()).arg(reply->error());
-        }
-        else {
-          error = QString("Received HTTP code %1").arg(reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt());
-        }
-      }
-      if (reply->error() == QNetworkReply::ContentAccessDenied || reply->error() == QNetworkReply::ContentOperationNotPermittedError || reply->error() == QNetworkReply::AuthenticationRequiredError) {
-        // Session is probably expired
-        Logout();
-      }
-      Error(error);
-    }
-    return QByteArray();
-  }
-
-  return data;
-
-}
-
 QJsonObject ListenBrainzScrobbler::JsonTrackMetadata(const ScrobbleMetadata &metadata) const {
 
   QJsonObject object_track_metadata;
@@ -524,30 +473,21 @@ void ListenBrainzScrobbler::UpdateNowPlayingRequestFinished(QNetworkReply *reply
   QObject::disconnect(reply, nullptr, this, nullptr);
   reply->deleteLater();
 
-  QByteArray data = GetReplyData(reply);
+  QJsonObject json_obj;
-  if (data.isEmpty()) {
+  QString error_description;
-    return;
+  if (GetJsonObject(reply, json_obj, error_description) != ReplyResult::Success) {
-  }
+    Error(error_description);
-
-  QJsonObject json_obj = ExtractJsonObj(data);
-  if (json_obj.isEmpty()) {
-    return;
-  }
-
-  if (json_obj.contains("code") && json_obj.contains("error_description")) {
-    QString error_desc = json_obj["error_description"].toString();
-    Error(error_desc);
     return;
   }
 
   if (!json_obj.contains("status")) {
-    Error("Missing status from server.", json_obj);
+    Error("Now playing request is missing status from server.");
     return;
   }
 
   QString status = json_obj["status"].toString();
   if (status.compare("ok", Qt::CaseInsensitive) != 0) {
-    Error(status);
+    Error(QString("Received %1 status for now playing.").arg(status));
   }
 
 }
@@ -600,22 +540,21 @@ void ListenBrainzScrobbler::Submit() {
   if (!IsEnabled() || !IsAuthenticated() || app_->scrobbler()->IsOffline()) return;
 
   QJsonArray array;
-  int i = 0;
+  ScrobblerCacheItemPtrList cache_items_sent;
-  QList<quint64> list;
+  ScrobblerCacheItemPtrList all_cache_items = cache_->List();
-  ScrobblerCacheItemPtrList cache_items = cache_->List();
+  for (ScrobblerCacheItemPtr cache_item : all_cache_items) {
-  for (ScrobblerCacheItemPtr cache_item : cache_items) {
     if (cache_item->sent) continue;
+    if (cache_item->error && cache_items_sent.count() > 0) break;
     cache_item->sent = true;
-    ++i;
+    cache_items_sent << cache_item;
-    list << cache_item->timestamp;
     QJsonObject object_listen;
     object_listen.insert("listened_at", QJsonValue::fromVariant(cache_item->timestamp));
     object_listen.insert("track_metadata", JsonTrackMetadata(cache_item->metadata));
     array.append(QJsonValue::fromVariant(object_listen));
-    if (i >= kScrobblesPerRequest) break;
+    if (cache_items_sent.count() >= kScrobblesPerRequest || cache_item->error) break;
   }
 
-  if (i <= 0) return;
+  if (cache_items_sent.count() <= 0) return;
 
   submitted_ = true;
 
@@ -626,11 +565,11 @@ void ListenBrainzScrobbler::Submit() {
 
   QUrl url(QString("%1/1/submit-listens").arg(kApiUrl));
   QNetworkReply *reply = CreateRequest(url, doc);
-  QObject::connect(reply, &QNetworkReply::finished, this, [this, reply, list]() { ScrobbleRequestFinished(reply, list); });
+  QObject::connect(reply, &QNetworkReply::finished, this, [this, reply, cache_items_sent]() { ScrobbleRequestFinished(reply, cache_items_sent); });
 
 }
 
-void ListenBrainzScrobbler::ScrobbleRequestFinished(QNetworkReply *reply, const QList<quint64> &list) {
+void ListenBrainzScrobbler::ScrobbleRequestFinished(QNetworkReply *reply, ScrobblerCacheItemPtrList cache_items) {
 
   if (!replies_.contains(reply)) return;
   replies_.removeAll(reply);
@@ -639,38 +578,40 @@ void ListenBrainzScrobbler::ScrobbleRequestFinished(QNetworkReply *reply, const
 
   submitted_ = false;
 
-  QByteArray data = GetReplyData(reply);
+  QJsonObject json_obj;
-  if (data.isEmpty()) {
+  QString error_message;
-    cache_->ClearSent(list);
+  const ReplyResult reply_result = GetJsonObject(reply, json_obj, error_message);
+  if (reply_result == ReplyResult::Success) {
+    if (json_obj.contains("status")) {
+      QString status = json_obj["status"].toString();
+      qLog(Debug) << "ListenBrainz: Received scrobble status:" << status;
+    }
+    else {
+      qLog(Debug) << "ListenBrainz: Received scrobble reply without status.";
+    }
+    cache_->Flush(cache_items);
+    submit_error_ = false;
+  }
+  else {
     submit_error_ = true;
-    StartSubmit();
+    if (reply_result == ReplyResult::APIError) {
-    return;
+      if (cache_items.count() == 1) {
+        const ScrobbleMetadata &metadata = cache_items.first()->metadata;
+        Error(tr("Unable to scrobble %1 - %2 because of error: %3").arg(metadata.effective_albumartist()).arg(metadata.title).arg(error_message));
+        cache_->Flush(cache_items);
+      }
+      else {
+        Error(error_message);
+        cache_->SetError(cache_items);
+        cache_->ClearSent(cache_items);
+      }
+    }
+    else {
+      Error(error_message);
+      cache_->ClearSent(cache_items);
+    }
   }
 
-  QJsonObject json_obj = ExtractJsonObj(data);
-  if (json_obj.isEmpty()) {
-    cache_->ClearSent(list);
-    submit_error_ = true;
-    StartSubmit();
-    return;
-  }
-
-  if (json_obj.contains("code") && json_obj.contains("error_description")) {
-    QString error_desc = json_obj["error_description"].toString();
-    Error(error_desc);
-    cache_->ClearSent(list);
-    submit_error_ = true;
-    StartSubmit();
-    return;
-  }
-
-  if (json_obj.contains("status")) {
-    QString status = json_obj["status"].toString();
-    qLog(Debug) << "ListenBrainz: Received scrobble status:" << status;
-  }
-
-  cache_->Flush(list);
-  submit_error_ = false;
   StartSubmit();
 
 }
@@ -682,7 +623,7 @@ void ListenBrainzScrobbler::Love() {
   if (!IsAuthenticated()) app_->scrobbler()->ShowConfig();
 
   if (song_playing_.musicbrainz_recording_id().isEmpty()) {
-    qLog(Error) << "ListenBrainz: Missing MusicBrainz recording ID for" << song_playing_.artist() << song_playing_.album() << song_playing_.title();
+    Error(tr("Missing MusicBrainz recording ID for %1 %2 %3").arg(song_playing_.artist()).arg(song_playing_.album()).arg(song_playing_.title()));
     return;
   }
 
@@ -705,30 +646,24 @@ void ListenBrainzScrobbler::LoveRequestFinished(QNetworkReply *reply) {
   QObject::disconnect(reply, nullptr, this, nullptr);
   reply->deleteLater();
 
-  QByteArray data = GetReplyData(reply);
+  QJsonObject json_obj;
-  if (data.isEmpty()) {
+  QString error_message;
-    return;
+  if (GetJsonObject(reply, json_obj, error_message) != ReplyResult::Success) {
-  }
+    Error(error_message);
-
-  QJsonObject json_obj = ExtractJsonObj(data);
-  if (json_obj.isEmpty()) {
-    return;
-  }
-
-  if (json_obj.contains("code") && json_obj.contains("error_description")) {
-    Error(json_obj["error_description"].toString());
     return;
   }
 
   if (json_obj.contains("status")) {
-    QString status = json_obj["status"].toString();
+    qLog(Debug) << "ListenBrainz: Received recording-feedback status:" << json_obj["status"].toString();
-    qLog(Debug) << "ListenBrainz: Received recording-feedback status:" << status;
   }
 
 }
 
 void ListenBrainzScrobbler::AuthError(const QString &error) {
+
+  qLog(Error) << "ListenBrainz" << error;
   emit AuthenticationComplete(false, error);
+
 }
 
 void ListenBrainzScrobbler::Error(const QString &error, const QVariant &debug) {
@@ -736,6 +671,10 @@ void ListenBrainzScrobbler::Error(const QString &error, const QVariant &debug) {
   qLog(Error) << "ListenBrainz:" << error;
   if (debug.isValid()) qLog(Debug) << debug;
 
+  if (app_->scrobbler()->ShowErrorDialog()) {
+    emit ErrorMessage(tr("ListenBrainz error: %1").arg(error));
+  }
+
 }
 
 void ListenBrainzScrobbler::CheckScrobblePrevSong() {

src/scrobbler/listenbrainzscrobbler.h

@@ -71,6 +71,12 @@ class ListenBrainzScrobbler : public ScrobblerService {
   void Scrobble(const Song &song) override;
   void Love() override;
 
+  enum class ReplyResult {
+    Success,
+    ServerError,
+    APIError
+  };
+
  signals:
   void AuthenticationComplete(const bool success, const QString &error = QString());
 
@@ -82,15 +88,15 @@ class ListenBrainzScrobbler : public ScrobblerService {
   void AuthenticateReplyFinished(QNetworkReply *reply);
   void RequestNewAccessToken() { RequestAccessToken(); }
   void UpdateNowPlayingRequestFinished(QNetworkReply *reply);
-  void ScrobbleRequestFinished(QNetworkReply *reply, const QList<quint64> &list);
+  void ScrobbleRequestFinished(QNetworkReply *reply, ScrobblerCacheItemPtrList cache_items);
   void LoveRequestFinished(QNetworkReply *reply);
 
  private:
   QNetworkReply *CreateRequest(const QUrl &url, const QJsonDocument &json_doc);
-  QByteArray GetReplyData(QNetworkReply *reply);
+  ReplyResult GetJsonObject(QNetworkReply *reply, QJsonObject &json_obj, QString &error_description);
   QJsonObject JsonTrackMetadata(const ScrobbleMetadata &metadata) const;
   void AuthError(const QString &error);
-  void Error(const QString &error, const QVariant &debug = QVariant()) override;
+  void Error(const QString &error, const QVariant &debug = QVariant());
   void RequestAccessToken(const QUrl &redirect_url = QUrl(), const QString &code = QString());
   void StartSubmit(const bool initial = false) override;
   void CheckScrobblePrevSong();

src/scrobbler/scrobblercache.cpp

@@ -25,7 +25,6 @@
 
 #include <QObject>
 #include <QStandardPaths>
-#include <QHash>
 #include <QString>
 #include <QFile>
 #include <QIODevice>
@@ -183,9 +182,8 @@ void ScrobblerCache::ReadCache() {
       metadata.musicbrainz_work_id = json_obj_track["musicbrainz_work_id"].toString();
     }
 
-    if (scrobbler_cache_.contains(timestamp)) continue;
+    ScrobblerCacheItemPtr cache_item = std::make_shared<ScrobblerCacheItem>(metadata, timestamp);
-    std::shared_ptr<ScrobblerCacheItem> cache_item = std::make_shared<ScrobblerCacheItem>(metadata, timestamp);
+    scrobbler_cache_ << cache_item;
-    scrobbler_cache_.insert(timestamp, cache_item);
 
   }
 
@@ -204,8 +202,7 @@ void ScrobblerCache::WriteCache() {
   }
 
   QJsonArray array;
-  for (QHash <quint64, std::shared_ptr<ScrobblerCacheItem>> ::iterator i = scrobbler_cache_.begin(); i != scrobbler_cache_.end(); ++i) {
+  for (ScrobblerCacheItemPtr cache_item : scrobbler_cache_) {
-    ScrobblerCacheItemPtr cache_item = i.value();
     QJsonObject object;
     object.insert("timestamp", QJsonValue::fromVariant(cache_item->timestamp));
     object.insert("artist", QJsonValue::fromVariant(cache_item->metadata.artist));
@@ -251,11 +248,9 @@ void ScrobblerCache::WriteCache() {
 
 ScrobblerCacheItemPtr ScrobblerCache::Add(const Song &song, const quint64 timestamp) {
 
-  if (scrobbler_cache_.contains(timestamp)) return nullptr;
-
   ScrobblerCacheItemPtr cache_item = std::make_shared<ScrobblerCacheItem>(ScrobbleMetadata(song), timestamp);
 
-  scrobbler_cache_.insert(timestamp, cache_item);
+  scrobbler_cache_ << cache_item;
 
   if (loaded_ && !timer_flush_->isActive()) {
     timer_flush_->start();
@@ -265,43 +260,35 @@ ScrobblerCacheItemPtr ScrobblerCache::Add(const Song &song, const quint64 timest
 
 }
 
-ScrobblerCacheItemPtr ScrobblerCache::Get(const quint64 hash) {
+void ScrobblerCache::Remove(ScrobblerCacheItemPtr cache_item) {
 
-  if (scrobbler_cache_.contains(hash)) { return scrobbler_cache_.value(hash); }
+  if (scrobbler_cache_.contains(cache_item)) {
-  else return nullptr;
+    scrobbler_cache_.removeAll(cache_item);
-
-}
-
-void ScrobblerCache::Remove(const quint64 hash) {
-
-  if (!scrobbler_cache_.contains(hash)) {
-    qLog(Error) << "Tried to remove non-existing hash" << hash;
-    return;
   }
-
-  scrobbler_cache_.remove(hash);
-
 }
 
-void ScrobblerCache::Remove(ScrobblerCacheItemPtr item) {
+void ScrobblerCache::ClearSent(ScrobblerCacheItemPtrList cache_items) {
-  scrobbler_cache_.remove(item->timestamp);
-}
 
-void ScrobblerCache::ClearSent(const QList<quint64> &list) {
+  for (ScrobblerCacheItemPtr cache_item : cache_items) {
-
+    cache_item->sent = false;
-  for (const quint64 timestamp : list) {
-    if (!scrobbler_cache_.contains(timestamp)) continue;
-    ScrobblerCacheItemPtr item = scrobbler_cache_.value(timestamp);
-    item->sent = false;
   }
 
 }
 
-void ScrobblerCache::Flush(const QList<quint64> &list) {
+void ScrobblerCache::SetError(ScrobblerCacheItemPtrList cache_items) {
 
-  for (const quint64 timestamp : list) {
+  for (ScrobblerCacheItemPtr item : cache_items) {
-    if (!scrobbler_cache_.contains(timestamp)) continue;
+    item->error = true;
-    scrobbler_cache_.remove(timestamp);
+  }
+
+}
+
+void ScrobblerCache::Flush(ScrobblerCacheItemPtrList cache_items) {
+
+  for (ScrobblerCacheItemPtr cache_item : cache_items) {
+    if (scrobbler_cache_.contains(cache_item)) {
+      scrobbler_cache_.removeAll(cache_item);
+    }
   }
 
   if (!timer_flush_->isActive()) {

src/scrobbler/scrobblercache.h

@@ -27,7 +27,6 @@
 #include <QtGlobal>
 #include <QObject>
 #include <QList>
-#include <QHash>
 #include <QString>
 
 #include "scrobblercacheitem.h"
@@ -45,13 +44,12 @@ class ScrobblerCache : public QObject {
   void ReadCache();
 
   ScrobblerCacheItemPtr Add(const Song &song, const quint64 timestamp);
-  ScrobblerCacheItemPtr Get(const quint64 hash);
+  void Remove(ScrobblerCacheItemPtr cache_item);
-  void Remove(const quint64 hash);
-  void Remove(ScrobblerCacheItemPtr item);
   int Count() const { return scrobbler_cache_.size(); };
-  ScrobblerCacheItemPtrList List() const { return scrobbler_cache_.values(); }
+  ScrobblerCacheItemPtrList List() const { return scrobbler_cache_; }
-  void ClearSent(const QList<quint64> &list);
+  void ClearSent(ScrobblerCacheItemPtrList cache_items);
-  void Flush(const QList<quint64> &list);
+  void SetError(ScrobblerCacheItemPtrList cache_items);
+  void Flush(ScrobblerCacheItemPtrList cache_items);
 
  public slots:
   void WriteCache();
@@ -60,7 +58,7 @@ class ScrobblerCache : public QObject {
   QTimer *timer_flush_;
   QString filename_;
   bool loaded_;
-  QHash<quint64, ScrobblerCacheItemPtr> scrobbler_cache_;
+  QList<ScrobblerCacheItemPtr> scrobbler_cache_;
 
 };
 

src/scrobbler/scrobblercacheitem.cpp

@@ -27,4 +27,5 @@
 ScrobblerCacheItem::ScrobblerCacheItem(const ScrobbleMetadata &_metadata, const quint64 _timestamp)
     : metadata(_metadata),
       timestamp(_timestamp),
-      sent(false) {}
+      sent(false),
+      error(false) {}

src/scrobbler/scrobblercacheitem.h

@@ -36,6 +36,7 @@ class ScrobblerCacheItem {
   ScrobbleMetadata metadata;
   quint64 timestamp;
   bool sent;
+  bool error;
 };
 
 using ScrobblerCacheItemPtr = std::shared_ptr<ScrobblerCacheItem>;

src/scrobbler/scrobblerservice.cpp

@@ -36,32 +36,21 @@ ScrobblerService::ScrobblerService(const QString &name, Application *app, QObjec
 
 }
 
-QJsonObject ScrobblerService::ExtractJsonObj(const QByteArray &data, const bool ignore_empty) {
+bool ScrobblerService::ExtractJsonObj(const QByteArray &data, QJsonObject &json_obj, QString &error_description) {
 
-  QJsonParseError error;
+  QJsonParseError json_parse_error;
-  QJsonDocument json_doc = QJsonDocument::fromJson(data, &error);
+  const QJsonDocument json_doc = QJsonDocument::fromJson(data, &json_parse_error);
 
-  if (error.error != QJsonParseError::NoError) {
+  if (json_parse_error.error != QJsonParseError::NoError) {
-    Error("Reply from server missing Json data.", data);
+    error_description = json_parse_error.errorString();
-    return QJsonObject();
+    return false;
-  }
-  if (json_doc.isEmpty()) {
-    Error("Received empty Json document.", json_doc);
-    return QJsonObject();
-  }
-  if (!json_doc.isObject()) {
-    Error("Json document is not an object.", json_doc);
-    return QJsonObject();
-  }
-  QJsonObject json_obj = json_doc.object();
-  if (json_obj.isEmpty()) {
-    if (!ignore_empty) {
-      Error("Received empty Json object.", json_doc);
-    }
-    return QJsonObject();
   }
 
-  return json_obj;
+  if (json_doc.isObject()) {
+    json_obj = json_doc.object();
+  }
+
+  return true;
 
 }
 

src/scrobbler/scrobblerservice.h

@@ -61,8 +61,7 @@ class ScrobblerService : public QObject {
   using ParamList = QList<Param>;
   using EncodedParam = QPair<QByteArray, QByteArray>;
 
-  QJsonObject ExtractJsonObj(const QByteArray &data, const bool ignore_empty = false);
+  bool ExtractJsonObj(const QByteArray &data, QJsonObject &json_obj, QString &error_description);
-  virtual void Error(const QString &error, const QVariant &debug = QVariant()) = 0;
 
   QString StripAlbum(QString album) const;
   QString StripTitle(QString title) const;

src/scrobbler/scrobblingapi20.cpp

@@ -149,6 +149,51 @@ void ScrobblingAPI20::Logout() {
 
 }
 
+ScrobblingAPI20::ReplyResult ScrobblingAPI20::GetJsonObject(QNetworkReply *reply, QJsonObject &json_obj, QString &error_description) {
+
+  ReplyResult reply_error_type = ReplyResult::ServerError;
+
+  if (reply->error() == QNetworkReply::NoError) {
+    if (reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() == 200) {
+      reply_error_type = ReplyResult::Success;
+    }
+    else {
+      error_description = QString("Received HTTP code %1").arg(reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt());
+    }
+  }
+  else {
+    error_description = QString("%1 (%2)").arg(reply->errorString()).arg(reply->error());
+  }
+
+  // See if there is Json data containing "error" and "message" - then use that instead.
+  if (reply->error() == QNetworkReply::NoError || reply->error() >= 200) {
+    const QByteArray data = reply->readAll();
+    int error_code = 0;
+    if (!data.isEmpty() && ExtractJsonObj(data, json_obj, error_description) && json_obj.contains("error") && json_obj.contains("message")) {
+      error_code = json_obj["error"].toInt();
+      QString error_message = json_obj["message"].toString();
+      error_description = QString("%1 (%2)").arg(error_message).arg(error_code);
+      reply_error_type = ReplyResult::APIError;
+    }
+    const ScrobbleErrorCode lastfm_error_code = static_cast<ScrobbleErrorCode>(error_code);
+    if (reply->error() == QNetworkReply::ContentAccessDenied ||
+        reply->error() == QNetworkReply::ContentOperationNotPermittedError ||
+        reply->error() == QNetworkReply::AuthenticationRequiredError ||
+        lastfm_error_code == ScrobbleErrorCode::InvalidSessionKey ||
+        lastfm_error_code == ScrobbleErrorCode::UnauthorizedToken ||
+        lastfm_error_code == ScrobbleErrorCode::LoginRequired ||
+        lastfm_error_code == ScrobbleErrorCode::AuthenticationFailed ||
+        lastfm_error_code == ScrobbleErrorCode::APIKeySuspended
+    ) {
+      // Session is probably expired
+      Logout();
+    }
+  }
+
+  return reply_error_type;
+
+}
+
 void ScrobblingAPI20::Authenticate(const bool https) {
 
   if (!server_) {
@@ -262,57 +307,10 @@ void ScrobblingAPI20::AuthenticateReplyFinished(QNetworkReply *reply) {
   QObject::disconnect(reply, nullptr, this, nullptr);
   reply->deleteLater();
 
-  QByteArray data;
+  QJsonObject json_obj;
-
+  QString error_message;
-  if (reply->error() == QNetworkReply::NoError && reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() == 200) {
+  if (GetJsonObject(reply, json_obj, error_message) != ReplyResult::Success) {
-    data = reply->readAll();
+    AuthError(error_message);
-  }
-  else {
-    if (reply->error() < 200) {
-      // This is a network error, there is nothing more to do.
-      AuthError(QString("%1 (%2)").arg(reply->errorString()).arg(reply->error()));
-    }
-    else {
-      // See if there is Json data containing "error" and "message" - then use that instead.
-      data = reply->readAll();
-      QString error;
-      QJsonParseError json_error;
-      QJsonDocument json_doc = QJsonDocument::fromJson(data, &json_error);
-      if (json_error.error == QJsonParseError::NoError && !json_doc.isEmpty() && json_doc.isObject()) {
-        QJsonObject json_obj = json_doc.object();
-        if (json_obj.contains("error") && json_obj.contains("message")) {
-          int code = json_obj["error"].toInt();
-          QString message = json_obj["message"].toString();
-          error = "Error: " + QString::number(code) + ": " + message;
-        }
-        else {
-          error = QString("%1 (%2)").arg(reply->errorString()).arg(reply->error());
-        }
-      }
-      if (error.isEmpty()) {
-        if (reply->error() != QNetworkReply::NoError) {
-          error = QString("%1 (%2)").arg(reply->errorString()).arg(reply->error());
-        }
-        else {
-          error = QString("Received HTTP code %1").arg(reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt());
-        }
-      }
-      AuthError(error);
-    }
-    return;
-  }
-
-  QJsonObject json_obj = ExtractJsonObj(data);
-  if (json_obj.isEmpty()) {
-    AuthError("Json document from server was empty.");
-    return;
-  }
-
-  if (json_obj.contains("error") && json_obj.contains("message")) {
-    int error = json_obj["error"].toInt();
-    QString message = json_obj["message"].toString();
-    QString failure_reason = "Error: " + QString::number(error) + ": " + message;
-    AuthError(failure_reason);
     return;
   }
 
@@ -392,63 +390,6 @@ QNetworkReply *ScrobblingAPI20::CreateRequest(const ParamList &request_params) {
 
 }
 
-QByteArray ScrobblingAPI20::GetReplyData(QNetworkReply *reply) {
-
-  QByteArray data;
-
-  if (reply->error() == QNetworkReply::NoError && reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() == 200) {
-    data = reply->readAll();
-  }
-  else {
-    if (reply->error() != QNetworkReply::NoError && reply->error() < 200) {
-      // This is a network error, there is nothing more to do.
-      Error(QString("%1 (%2)").arg(reply->errorString()).arg(reply->error()));
-    }
-    else {
-      QString error;
-      // See if there is Json data containing "error" and "message" - then use that instead.
-      data = reply->readAll();
-      QJsonParseError json_error;
-      QJsonDocument json_doc = QJsonDocument::fromJson(data, &json_error);
-      int error_code = -1;
-      if (json_error.error == QJsonParseError::NoError && !json_doc.isEmpty() && json_doc.isObject()) {
-        QJsonObject json_obj = json_doc.object();
-        if (json_obj.contains("error") && json_obj.contains("message")) {
-          error_code = json_obj["error"].toInt();
-          QString error_message = json_obj["message"].toString();
-          error = QString("%1 (%2)").arg(error_message).arg(error_code);
-        }
-      }
-      if (error.isEmpty()) {
-        if (reply->error() != QNetworkReply::NoError) {
-          error = QString("%1 (%2)").arg(reply->errorString()).arg(reply->error());
-        }
-        else {
-          error = QString("Received HTTP code %1").arg(reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt());
-        }
-      }
-      const ScrobbleErrorCode lastfm_error_code = static_cast<ScrobbleErrorCode>(error_code);
-      if (reply->error() == QNetworkReply::ContentAccessDenied ||
-          reply->error() == QNetworkReply::ContentOperationNotPermittedError ||
-          reply->error() == QNetworkReply::AuthenticationRequiredError ||
-          lastfm_error_code == ScrobbleErrorCode::InvalidSessionKey ||
-          lastfm_error_code == ScrobbleErrorCode::UnauthorizedToken ||
-          lastfm_error_code == ScrobbleErrorCode::LoginRequired ||
-          lastfm_error_code == ScrobbleErrorCode::AuthenticationFailed ||
-          lastfm_error_code == ScrobbleErrorCode::APIKeySuspended
-        ){
-        // Session is probably expired
-        Logout();
-      }
-      Error(error);
-    }
-    return QByteArray();
-  }
-
-  return data;
-
-}
-
 void ScrobblingAPI20::UpdateNowPlaying(const Song &song) {
 
   CheckScrobblePrevSong();
@@ -484,21 +425,10 @@ void ScrobblingAPI20::UpdateNowPlayingRequestFinished(QNetworkReply *reply) {
   QObject::disconnect(reply, nullptr, this, nullptr);
   reply->deleteLater();
 
-  QByteArray data = GetReplyData(reply);
+  QJsonObject json_obj;
-  if (data.isEmpty()) {
+  QString error_message;
-    return;
+  if (GetJsonObject(reply, json_obj, error_message) != ReplyResult::Success) {
-  }
+    Error(error_message);
-
-  QJsonObject json_obj = ExtractJsonObj(data);
-  if (json_obj.isEmpty()) {
-    return;
-  }
-
-  if (json_obj.contains("error") && json_obj.contains("message")) {
-    int error_code = json_obj["error"].toInt();
-    QString error_message = json_obj["message"].toString();
-    QString error_reason = QString("%1 (%2)").arg(error_message).arg(error_code);
-    Error(error_reason);
     return;
   }
 
@@ -566,43 +496,43 @@ void ScrobblingAPI20::Submit() {
   ParamList params = ParamList() << Param("method", "track.scrobble");
 
   int i = 0;
-  QList<quint64> list;
+  ScrobblerCacheItemPtrList all_cache_items = cache_->List();
-  ScrobblerCacheItemPtrList items = cache_->List();
+  ScrobblerCacheItemPtrList cache_items_sent;
-  for (ScrobblerCacheItemPtr item : items) {
+  for (ScrobblerCacheItemPtr cache_item : all_cache_items) {
-    if (item->sent) continue;
+    if (cache_item->sent) continue;
-    item->sent = true;
+    cache_item->sent = true;
     if (!batch_) {
-      SendSingleScrobble(item);
+      SendSingleScrobble(cache_item);
       continue;
     }
-    list << item->timestamp;
+    cache_items_sent << cache_item;
-    params << Param(QString("%1[%2]").arg("artist").arg(i), prefer_albumartist_ ? item->metadata.effective_albumartist() : item->metadata.artist);
+    params << Param(QString("%1[%2]").arg("artist").arg(i), prefer_albumartist_ ? cache_item->metadata.effective_albumartist() : cache_item->metadata.artist);
-    params << Param(QString("%1[%2]").arg("track").arg(i), StripTitle(item->metadata.title));
+    params << Param(QString("%1[%2]").arg("track").arg(i), StripTitle(cache_item->metadata.title));
-    params << Param(QString("%1[%2]").arg("timestamp").arg(i), QString::number(item->timestamp));
+    params << Param(QString("%1[%2]").arg("timestamp").arg(i), QString::number(cache_item->timestamp));
-    params << Param(QString("%1[%2]").arg("duration").arg(i), QString::number(item->metadata.length_nanosec / kNsecPerSec));
+    params << Param(QString("%1[%2]").arg("duration").arg(i), QString::number(cache_item->metadata.length_nanosec / kNsecPerSec));
-    if (!item->metadata.album.isEmpty()) {
+    if (!cache_item->metadata.album.isEmpty()) {
-      params << Param(QString("%1[%2]").arg("album").arg(i), StripAlbum(item->metadata.album));
+      params << Param(QString("%1[%2]").arg("album").arg(i), StripAlbum(cache_item->metadata.album));
     }
-    if (!prefer_albumartist_ && !item->metadata.albumartist.isEmpty()) {
+    if (!prefer_albumartist_ && !cache_item->metadata.albumartist.isEmpty()) {
-      params << Param(QString("%1[%2]").arg("albumArtist").arg(i), item->metadata.albumartist);
+      params << Param(QString("%1[%2]").arg("albumArtist").arg(i), cache_item->metadata.albumartist);
     }
-    if (item->metadata.track > 0) {
+    if (cache_item->metadata.track > 0) {
-      params << Param(QString("%1[%2]").arg("trackNumber").arg(i), QString::number(item->metadata.track));
+      params << Param(QString("%1[%2]").arg("trackNumber").arg(i), QString::number(cache_item->metadata.track));
     }
     ++i;
-    if (i >= kScrobblesPerRequest) break;
+    if (cache_items_sent.count() >= kScrobblesPerRequest) break;
   }
 
-  if (!batch_ || i <= 0) return;
+  if (!batch_ || cache_items_sent.count() <= 0) return;
 
   submitted_ = true;
 
   QNetworkReply *reply = CreateRequest(params);
-  QObject::connect(reply, &QNetworkReply::finished, this, [this, reply, list]() { ScrobbleRequestFinished(reply, list); });
+  QObject::connect(reply, &QNetworkReply::finished, this, [this, reply, cache_items_sent]() { ScrobbleRequestFinished(reply, cache_items_sent); });
 
 }
 
-void ScrobblingAPI20::ScrobbleRequestFinished(QNetworkReply *reply, const QList<quint64> &list) {
+void ScrobblingAPI20::ScrobbleRequestFinished(QNetworkReply *reply, ScrobblerCacheItemPtrList cache_items) {
 
   if (!replies_.contains(reply)) return;
   replies_.removeAll(reply);
@@ -611,34 +541,17 @@ void ScrobblingAPI20::ScrobbleRequestFinished(QNetworkReply *reply, const QList<
 
   submitted_ = false;
 
-  QByteArray data = GetReplyData(reply);
+  QJsonObject json_obj;
-  if (data.isEmpty()) {
+  QString error_message;
-    cache_->ClearSent(list);
+  if (GetJsonObject(reply, json_obj, error_message) != ReplyResult::Success) {
+    Error(error_message);
+    cache_->ClearSent(cache_items);
     submit_error_ = true;
     StartSubmit();
     return;
   }
 
-  QJsonObject json_obj = ExtractJsonObj(data);
+  cache_->Flush(cache_items);
-  if (json_obj.isEmpty()) {
-    cache_->ClearSent(list);
-    submit_error_ = true;
-    StartSubmit();
-    return;
-  }
-
-  if (json_obj.contains("error") && json_obj.contains("message")) {
-    int error_code = json_obj["error"].toInt();
-    QString error_message = json_obj["message"].toString();
-    QString error_reason = QString("%1 (%2)").arg(error_message).arg(error_code);
-    Error(error_reason);
-    cache_->ClearSent(list);
-    submit_error_ = true;
-    StartSubmit();
-    return;
-  }
-
-  cache_->Flush(list);
   submit_error_ = false;
 
   if (!json_obj.contains("scrobbles")) {
@@ -799,52 +712,32 @@ void ScrobblingAPI20::SendSingleScrobble(ScrobblerCacheItemPtr item) {
   }
 
   QNetworkReply *reply = CreateRequest(params);
-  QObject::connect(reply, &QNetworkReply::finished, this, [this, reply, item]() { SingleScrobbleRequestFinished(reply, item->timestamp); });
+  QObject::connect(reply, &QNetworkReply::finished, this, [this, reply, item]() { SingleScrobbleRequestFinished(reply, item); });
 
 }
 
-void ScrobblingAPI20::SingleScrobbleRequestFinished(QNetworkReply *reply, const quint64 timestamp) {
+void ScrobblingAPI20::SingleScrobbleRequestFinished(QNetworkReply *reply, ScrobblerCacheItemPtr cache_item) {
 
   if (!replies_.contains(reply)) return;
   replies_.removeAll(reply);
   QObject::disconnect(reply, nullptr, this, nullptr);
   reply->deleteLater();
 
-  ScrobblerCacheItemPtr item = cache_->Get(timestamp);
+  QJsonObject json_obj;
-  if (!item) {
+  QString error_message;
-    Error(QString("Received reply for non-existing cache entry %1.").arg(timestamp));
+  if (GetJsonObject(reply, json_obj, error_message) != ReplyResult::Success) {
-    return;
+    Error(error_message);
-  }
+    cache_item->sent = false;
-
-  QByteArray data = GetReplyData(reply);
-  if (data.isEmpty()) {
-    item->sent = false;
-    return;
-  }
-
-  QJsonObject json_obj = ExtractJsonObj(data);
-  if (json_obj.isEmpty()) {
-    item->sent = false;
-    return;
-  }
-
-  if (json_obj.contains("error") && json_obj.contains("message")) {
-    int error_code = json_obj["error"].toInt();
-    QString error_message = json_obj["message"].toString();
-    QString error_reason = QString("%1 (%2)").arg(error_message).arg(error_code);
-    Error(error_reason);
-    item->sent = false;
     return;
   }
 
   if (!json_obj.contains("scrobbles")) {
     Error("Json reply from server is missing scrobbles.", json_obj);
-    item->sent = false;
+    cache_item->sent = false;
     return;
   }
 
-  cache_->Remove(timestamp);
+  cache_->Remove(cache_item);
-  item = nullptr;
 
   QJsonValue value_scrobbles = json_obj["scrobbles"];
   if (!value_scrobbles.isObject()) {
@@ -963,13 +856,10 @@ void ScrobblingAPI20::LoveRequestFinished(QNetworkReply *reply) {
   QObject::disconnect(reply, nullptr, this, nullptr);
   reply->deleteLater();
 
-  QByteArray data = GetReplyData(reply);
+  QJsonObject json_obj;
-  if (data.isEmpty()) {
+  QString error_message;
-    return;
+  if (GetJsonObject(reply, json_obj, error_message) != ReplyResult::Success) {
-  }
+    Error(error_message);
-
-  QJsonObject json_obj = ExtractJsonObj(data, true);
-  if (json_obj.isEmpty()) {
     return;
   }
 
@@ -1008,7 +898,10 @@ void ScrobblingAPI20::LoveRequestFinished(QNetworkReply *reply) {
 }
 
 void ScrobblingAPI20::AuthError(const QString &error) {
+
+  qLog(Error) << name_ << error;
   emit AuthenticationComplete(false, error);
+
 }
 
 void ScrobblingAPI20::Error(const QString &error, const QVariant &debug) {

src/scrobbler/scrobblingapi20.h

@@ -79,11 +79,16 @@ class ScrobblingAPI20 : public ScrobblerService {
   void RedirectArrived();
   void AuthenticateReplyFinished(QNetworkReply *reply);
   void UpdateNowPlayingRequestFinished(QNetworkReply *reply);
-  void ScrobbleRequestFinished(QNetworkReply *reply, const QList<quint64> &list);
+  void ScrobbleRequestFinished(QNetworkReply *reply, ScrobblerCacheItemPtrList cache_items);
-  void SingleScrobbleRequestFinished(QNetworkReply *reply, const quint64 timestamp);
+  void SingleScrobbleRequestFinished(QNetworkReply *reply, ScrobblerCacheItemPtr cache_item);
   void LoveRequestFinished(QNetworkReply *reply);
 
  private:
+  enum class ReplyResult {
+    Success,
+    ServerError,
+    APIError
+  };
 
   enum class ScrobbleErrorCode {
     NoError = 1,
@@ -120,12 +125,12 @@ class ScrobblingAPI20 : public ScrobblerService {
   static const int kScrobblesPerRequest;
 
   QNetworkReply *CreateRequest(const ParamList &request_params);
-  QByteArray GetReplyData(QNetworkReply *reply);
+  ReplyResult GetJsonObject(QNetworkReply *reply, QJsonObject &json_obj, QString &error_description);
 
   void RequestSession(const QString &token);
   void AuthError(const QString &error);
   void SendSingleScrobble(ScrobblerCacheItemPtr item);
-  void Error(const QString &error, const QVariant &debug = QVariant()) override;
+  void Error(const QString &error, const QVariant &debug = QVariant());
   static QString ErrorString(const ScrobbleErrorCode error);
   void StartSubmit(const bool initial = false) override;
   void CheckScrobblePrevSong();

src/scrobbler/subsonicscrobbler.cpp

@@ -109,10 +109,3 @@ void SubsonicScrobbler::Submit() {
   service_->Scrobble(song_playing_.song_id(), true, time_);
 
 }
-
-void SubsonicScrobbler::Error(const QString &error, const QVariant &debug) {
-
-  qLog(Error) << "SubsonicScrobbler:" << error;
-  if (debug.isValid()) qLog(Debug) << debug;
-
-}

src/scrobbler/subsonicscrobbler.h

@@ -52,7 +52,6 @@ class SubsonicScrobbler : public ScrobblerService {
   void UpdateNowPlaying(const Song &song) override;
   void ClearPlaying() override;
   void Scrobble(const Song &song) override;
-  void Error(const QString &error, const QVariant &debug = QVariant()) override;
 
   void StartSubmit(const bool initial = false) override { Q_UNUSED(initial) }
   void Submitted() override { submitted_ = true; }