Add better error messages for device and organize

Fixes #1364

src/core/deletefiles.cpp

@@ -92,7 +92,8 @@ void DeleteFiles::ProcessSomeFiles() {
   if (progress_ >= songs_.count()) {
     task_manager_->SetTaskProgress(task_id_, progress_, songs_.count());
 
-    storage_->FinishCopy(songs_with_errors_.isEmpty());
+    QString error_text;
+    storage_->FinishCopy(songs_with_errors_.isEmpty(), error_text);
 
     task_manager_->SetTaskFinished(task_id_);
 

src/core/filesystemmusicstorage.cpp

@@ -36,7 +36,7 @@
 
 FilesystemMusicStorage::FilesystemMusicStorage(const Song::Source source, const QString &root, const std::optional<int> collection_directory_id) : source_(source), root_(root), collection_directory_id_(collection_directory_id) {}
 
-bool FilesystemMusicStorage::CopyToStorage(const CopyJob &job) {
+bool FilesystemMusicStorage::CopyToStorage(const CopyJob &job, QString &error_text) {
 
   const QFileInfo src = QFileInfo(job.source_);
   const QFileInfo dest = QFileInfo(root_ + "/" + job.destination_);
@@ -54,7 +54,8 @@ bool FilesystemMusicStorage::CopyToStorage(const CopyJob &job) {
   // Create directories as required
   QDir dir;
   if (!dir.mkpath(dest.absolutePath())) {
-    qLog(Warning) << "Failed to create directory" << dest.dir().absolutePath();
+    error_text = QObject::tr("Failed to create directory %1.").arg(dest.dir().absolutePath());
+    qLog(Error) << error_text;
     return false;
   }
 
@@ -65,10 +66,12 @@ bool FilesystemMusicStorage::CopyToStorage(const CopyJob &job) {
   }
 
   // Copy or move
-  bool result(true);
+  bool result = true;
   if (job.remove_original_) {
     if (dest.exists() && !job.overwrite_) {
       result = false;
+      error_text = QObject::tr("Destination file %1 exists, but not allowed to overwrite.").arg(dest.absoluteFilePath());
+      qLog(Error) << error_text;
     }
     else {
       result = QFile::rename(src.absoluteFilePath(), dest.absoluteFilePath());
@@ -86,9 +89,15 @@ bool FilesystemMusicStorage::CopyToStorage(const CopyJob &job) {
   else {
     if (dest.exists() && !job.overwrite_) {
       result = false;
+      error_text = QObject::tr("Destination file %1 exists, but not allowed to overwrite").arg(dest.absoluteFilePath());
+      qLog(Error) << error_text;
     }
     else {
       result = QFile::copy(src.absoluteFilePath(), dest.absoluteFilePath());
+      if (!result) {
+        error_text = QObject::tr("Could not copy file %1 to %2.").arg(src.absoluteFilePath(), dest.absoluteFilePath());
+        qLog(Error) << error_text;
+      }
     }
     if ((!cover_dest.exists() || job.overwrite_) && !cover_src.filePath().isEmpty() && !cover_dest.filePath().isEmpty()) {
       QFile::copy(cover_src.absoluteFilePath(), cover_dest.absoluteFilePath());

src/core/filesystemmusicstorage.h

@@ -39,7 +39,7 @@ class FilesystemMusicStorage : public virtual MusicStorage {
   QString LocalPath() const override { return root_; }
   std::optional<int> collection_directory_id() const override { return collection_directory_id_; }
 
-  bool CopyToStorage(const CopyJob &job) override;
+  bool CopyToStorage(const CopyJob &job, QString &error_text) override;
   bool DeleteFromStorage(const DeleteJob &job) override;
 
  private:

src/core/musicstorage.h

@@ -89,12 +89,12 @@ class MusicStorage {
   virtual bool GetSupportedFiletypes(QList<Song::FileType> *ret) { Q_UNUSED(ret); return true; }
 
   virtual bool StartCopy(QList<Song::FileType> *supported_types) { Q_UNUSED(supported_types); return true; }
-  virtual bool CopyToStorage(const CopyJob &job) = 0;
+  virtual bool CopyToStorage(const CopyJob &job, QString &error_text) = 0;
-  virtual void FinishCopy(bool success) { Q_UNUSED(success); }
+  virtual bool FinishCopy(bool success, QString &error_text) { Q_UNUSED(error_text); return success; }
 
   virtual void StartDelete() {}
   virtual bool DeleteFromStorage(const DeleteJob &job) = 0;
-  virtual void FinishDelete(bool success) { Q_UNUSED(success); }
+  virtual bool FinishDelete(bool success, QString &error_text) { Q_UNUSED(error_text); return success; }
 
   virtual void Eject() {}
 

src/device/cddadevice.h

@@ -51,7 +51,7 @@ class CddaDevice : public ConnectedDevice {
 
   bool Init() override;
   void Refresh() override;
-  bool CopyToStorage(const MusicStorage::CopyJob&) override { return false; }
+  bool CopyToStorage(const CopyJob&, QString&) { return false; }
   bool DeleteFromStorage(const MusicStorage::DeleteJob&) override { return false; }
 
   static QStringList url_schemes() { return QStringList() << "cdda"; }

src/device/connecteddevice.cpp

@@ -132,12 +132,14 @@ void ConnectedDevice::Eject() {
 
 }
 
-void ConnectedDevice::FinishCopy(bool) {
+bool ConnectedDevice::FinishCopy(bool success, QString&) {
   lister_->UpdateDeviceFreeSpace(unique_id_);
+  return success;
 }
 
-void ConnectedDevice::FinishDelete(bool) {
+bool ConnectedDevice::FinishDelete(bool success, QString&) {
   lister_->UpdateDeviceFreeSpace(unique_id_);
+  return success;
 }
 
 MusicStorage::TranscodeMode ConnectedDevice::GetTranscodeMode() const {

src/device/connecteddevice.h

@@ -67,8 +67,8 @@ class ConnectedDevice : public QObject, public virtual MusicStorage, public enab
   QUrl url() const { return url_; }
   qint64 song_count() const { return song_count_; }
 
-  void FinishCopy(bool success) override;
+  bool FinishCopy(bool success, QString &error_text) override;
-  void FinishDelete(bool success) override;
+  bool FinishDelete(bool success, QString &error_text) override;
 
   void Eject() override;
   virtual void Close();

src/device/gpoddevice.cpp

@@ -185,7 +185,7 @@ void GPodDevice::AddTrackToModel(Itdb_Track *track, const QString &prefix) {
 
 }
 
-bool GPodDevice::CopyToStorage(const CopyJob &job) {
+bool GPodDevice::CopyToStorage(const CopyJob &job, QString &error_text) {
 
   Q_ASSERT(db_);
 
@@ -238,9 +238,10 @@ bool GPodDevice::CopyToStorage(const CopyJob &job) {
   GError *error = nullptr;
   itdb_cp_track_to_ipod(track, QDir::toNativeSeparators(job.source_).toLocal8Bit().constData(), &error);
   if (error) {
-    qLog(Error) << "Copying failed:" << error->message;
+    error_text = tr("Could not copy %1 to %2: %3").arg(job.metadata_.url().toLocalFile(), QString::fromUtf8(error->message));
-    app_->AddError(QString::fromUtf8(error->message));
     g_error_free(error);
+    qLog(Error) << error_text;
+    app_->AddError(error_text);
 
     // Need to remove the track from the db again
     itdb_track_remove(track);
@@ -272,19 +273,24 @@ bool GPodDevice::CopyToStorage(const CopyJob &job) {
 
 }
 
-bool GPodDevice::WriteDatabase() {
+bool GPodDevice::WriteDatabase(QString &error_text) {
 
   // Write the itunes database
   GError *error = nullptr;
-  itdb_write(db_, &error);
+  const bool success = itdb_write(db_, &error);
   cover_files_.clear();
-  if (error) {
+  if (!success) {
-    qLog(Error) << "Writing database failed:" << error->message;
+    if (error) {
-    app_->AddError(QString::fromUtf8(error->message));
+      error_text = tr("Writing database failed: %1").arg(error->message);
-    g_error_free(error);
+      g_error_free(error);
-    return false;
+    }
+    else {
+      error_text = tr("Writing database failed.");
+    }
+    app_->AddError(error_text);
   }
-  else return true;
+
+  return success;
 
 }
 
@@ -307,11 +313,11 @@ void GPodDevice::Finish(const bool success) {
 
 }
 
-void GPodDevice::FinishCopy(bool success) {
+bool GPodDevice::FinishCopy(bool success, QString &error_text) {
 
-  if (success) success = WriteDatabase();
+  if (success) success = WriteDatabase(error_text);
   Finish(success);
-  ConnectedDevice::FinishCopy(success);
+  return ConnectedDevice::FinishCopy(success, error_text);
 
 }
 
@@ -378,11 +384,11 @@ bool GPodDevice::DeleteFromStorage(const DeleteJob &job) {
 
 }
 
-void GPodDevice::FinishDelete(bool success) {
+bool GPodDevice::FinishDelete(bool success, QString &error_text) {
 
-  if (success) success = WriteDatabase();
+  if (success) success = WriteDatabase(error_text);
   Finish(success);
-  ConnectedDevice::FinishDelete(success);
+  return ConnectedDevice::FinishDelete(success, error_text);
 
 }
 

src/device/gpoddevice.h

@@ -64,12 +64,12 @@ class GPodDevice : public ConnectedDevice, public virtual MusicStorage {
   bool GetSupportedFiletypes(QList<Song::FileType> *ret) override;
 
   bool StartCopy(QList<Song::FileType> *supported_filetypes) override;
-  bool CopyToStorage(const CopyJob &job) override;
+  bool CopyToStorage(const CopyJob &job, QString &error_text) override;
-  void FinishCopy(bool success) override;
+  bool FinishCopy(bool success, QString &error_text) override;
 
   void StartDelete() override;
   bool DeleteFromStorage(const DeleteJob &job) override;
-  void FinishDelete(bool success) override;
+  bool FinishDelete(bool success, QString &error_text) override;
 
  protected slots:
   void LoadFinished(Itdb_iTunesDB *db, const bool success);
@@ -83,7 +83,7 @@ class GPodDevice : public ConnectedDevice, public virtual MusicStorage {
  private:
   void Start();
   void Finish(const bool success);
-  bool WriteDatabase();
+  bool WriteDatabase(QString &error_text);
 
  protected:
   GPodLoader *loader_;

src/device/mtpconnection.cpp

@@ -54,7 +54,8 @@ MtpConnection::MtpConnection(const QUrl &url, QObject *parent) : QObject(parent)
     device_num = url_query.queryItemValue("devnum").toUInt();
   }
   else {
-    qLog(Warning) << "Invalid MTP device:" << hostname;
+    error_text_ = tr("Invalid MTP device: %1").arg(hostname);
+    qLog(Error) << error_text_;
     return;
   }
 
@@ -70,15 +71,20 @@ MtpConnection::MtpConnection(const QUrl &url, QObject *parent) : QObject(parent)
     raw_device->devnum = device_num;
 
     device_ = LIBMTP_Open_Raw_Device(raw_device);  // NOLINT(clang-analyzer-unix.Malloc)
+    if (!device_) {
+      error_text_ = tr("Could not open MTP device.");
+      qLog(Error) << error_text_;
+    }
     return;
   }
 
   // Get a list of devices from libmtp and figure out which one is ours
   int count = 0;
   LIBMTP_raw_device_t *raw_devices = nullptr;
-  LIBMTP_error_number_t err = LIBMTP_Detect_Raw_Devices(&raw_devices, &count);
+  LIBMTP_error_number_t error_number = LIBMTP_Detect_Raw_Devices(&raw_devices, &count);
-  if (err != LIBMTP_ERROR_NONE) {
+  if (error_number != LIBMTP_ERROR_NONE) {
-    qLog(Warning) << "MTP error:" << err;
+    error_text_ = tr("MTP error: %1").arg(ErrorString(error_number));
+    qLog(Error) << error_text_;
     return;
   }
 
@@ -91,13 +97,18 @@ MtpConnection::MtpConnection(const QUrl &url, QObject *parent) : QObject(parent)
   }
 
   if (!raw_device) {
-    qLog(Warning) << "MTP device not found";
+    error_text_ = tr("MTP device not found.");
+    qLog(Error) << error_text_;
     free(raw_devices);
     return;
   }
 
   // Connect to the device
   device_ = LIBMTP_Open_Raw_Device(raw_device);
+  if (!device_) {
+    error_text_ = tr("Could not open MTP device.");
+    qLog(Error) << error_text_;
+  }
 
   free(raw_devices);
 
@@ -107,6 +118,24 @@ MtpConnection::~MtpConnection() {
   if (device_) LIBMTP_Release_Device(device_);
 }
 
+QString MtpConnection::ErrorString(const LIBMTP_error_number_t error_number) {
+
+  switch(error_number) {
+    case LIBMTP_ERROR_NO_DEVICE_ATTACHED:
+      return "No Devices have been found.";
+    case LIBMTP_ERROR_CONNECTING:
+      return "There has been an error connecting.";
+    case LIBMTP_ERROR_MEMORY_ALLOCATION:
+      return "Memory Allocation Error.";
+    case LIBMTP_ERROR_GENERAL:
+    default:
+      return "Unknown error, please report this to the libmtp developers.";
+    case LIBMTP_ERROR_NONE:
+      return "Successfully connected.";
+  }
+
+}
+
 bool MtpConnection::GetSupportedFiletypes(QList<Song::FileType> *ret) {
 
   if (!device_) return false;

src/device/mtpconnection.h

@@ -45,13 +45,17 @@ class MtpConnection : public QObject, public enable_shared_from_this<MtpConnecti
   ~MtpConnection() override;
 
   bool is_valid() const { return device_; }
+  QString error_text() const { return error_text_; }
   LIBMTP_mtpdevice_t *device() const { return device_; }
   bool GetSupportedFiletypes(QList<Song::FileType> *ret);
 
+  static QString ErrorString(const LIBMTP_error_number_t error_number);
+
  private:
   Q_DISABLE_COPY(MtpConnection)
 
   LIBMTP_mtpdevice_t *device_;
+  QString error_text_;
 };
 
 #endif  // MTPCONNECTION_H

src/device/mtpdevice.cpp

@@ -143,7 +143,8 @@ bool MtpDevice::StartCopy(QList<Song::FileType> *supported_types) {
   // Did the caller want a list of supported types?
   if (supported_types) {
     if (!GetSupportedFiletypes(supported_types, connection_->device())) {
-      FinishCopy(false);
+      QString error_text;
+      FinishCopy(false, error_text);
       return false;
     }
   }
@@ -161,7 +162,7 @@ static int ProgressCallback(uint64_t const sent, uint64_t const total, void cons
 
 }
 
-bool MtpDevice::CopyToStorage(const CopyJob &job) {
+bool MtpDevice::CopyToStorage(const CopyJob &job, QString &error_text) {
 
   if (!connection_ || !connection_->is_valid()) return false;
 
@@ -171,7 +172,15 @@ bool MtpDevice::CopyToStorage(const CopyJob &job) {
 
   // Send the file
   int ret = LIBMTP_Send_Track_From_File(connection_->device(), job.source_.toUtf8().constData(), &track, ProgressCallback, &job);
-  if (ret != 0) return false;
+  if (ret != 0) {
+    LIBMTP_error_struct *error = LIBMTP_Get_Errorstack(connection_->device());
+    if (error) {
+      error_text = QString::fromUtf8(error->error_text);
+      qLog(Error) << error_text;
+      LIBMTP_Clear_Errorstack(connection_->device());
+    }
+    return false;
+  }
 
   // Add it to our CollectionModel
   Song metadata_on_device(Song::Source::Device);
@@ -190,7 +199,7 @@ bool MtpDevice::CopyToStorage(const CopyJob &job) {
 
 }
 
-void MtpDevice::FinishCopy(const bool success) {
+bool MtpDevice::FinishCopy(const bool success, QString &error_text) {
 
   if (success) {
     if (!songs_to_add_.isEmpty()) backend_->AddOrUpdateSongs(songs_to_add_);
@@ -205,7 +214,7 @@ void MtpDevice::FinishCopy(const bool success) {
 
   db_busy_.unlock();
 
-  ConnectedDevice::FinishCopy(success);
+  return ConnectedDevice::FinishCopy(success, error_text);
 
 }
 
@@ -234,7 +243,7 @@ bool MtpDevice::DeleteFromStorage(const DeleteJob &job) {
 
 }
 
-void MtpDevice::FinishDelete(const bool success) { FinishCopy(success); }
+bool MtpDevice::FinishDelete(const bool success, QString &error_text) { return FinishCopy(success, error_text); }
 
 bool MtpDevice::GetSupportedFiletypes(QList<Song::FileType> *ret) {
 

src/device/mtpdevice.h

@@ -63,12 +63,12 @@ class MtpDevice : public ConnectedDevice {
   int GetCapacity();
 
   bool StartCopy(QList<Song::FileType> *supported_types) override;
-  bool CopyToStorage(const CopyJob &job) override;
+  bool CopyToStorage(const CopyJob &job, QString &error_text) override;
-  void FinishCopy(const bool success) override;
+  bool FinishCopy(const bool success, QString &error_text) override;
 
   void StartDelete() override;
   bool DeleteFromStorage(const DeleteJob &job) override;
-  void FinishDelete(const bool success) override;
+  bool FinishDelete(const bool success, QString &error_text) override;
 
  private slots:
   void LoadFinished(bool success, MtpConnection *connection);

src/device/mtploader.cpp

@@ -68,11 +68,16 @@ bool MtpLoader::TryLoad() {
 
   connection_ = make_unique<MtpConnection>(url_);
 
-  if (!connection_ || !connection_->is_valid()) {
+  if (!connection_) {
     emit Error(tr("Error connecting MTP device %1").arg(url_.toString()));
     return false;
   }
 
+  if (!connection_->is_valid()) {
+    emit Error(tr("Error connecting MTP device %1: %2").arg(url_.toString(), connection_->error_text()));
+    return false;
+  }
+
   // Load the list of songs on the device
   SongList songs;
   LIBMTP_track_t *tracks = LIBMTP_Get_Tracklisting_With_Callback(connection_->device(), nullptr, nullptr);

src/organize/organize.cpp

@@ -146,7 +146,10 @@ void Organize::ProcessSomeFiles() {
 
     UpdateProgress();
 
-    destination_->FinishCopy(files_with_errors_.isEmpty());
+    QString error_text;
+    if (!destination_->FinishCopy(files_with_errors_.isEmpty(), error_text) && !error_text.isEmpty()) {
+      log_ << error_text;
+    }
     if (eject_after_) destination_->Eject();
 
     task_manager_->SetTaskFinished(task_id_);
@@ -250,7 +253,8 @@ void Organize::ProcessSomeFiles() {
 
     job.progress_ = std::bind(&Organize::SetSongProgress, this, std::placeholders::_1, !task.transcoded_filename_.isEmpty());
 
-    if (destination_->CopyToStorage(job)) {
+    QString error_text;
+    if (destination_->CopyToStorage(job, error_text)) {
       if (job.remove_original_ && song.is_collection_song() && destination_->source() == Song::Source::Collection) {
         // Notify other aspects of system that song has been invalidated
         QString root = destination_->LocalPath();
@@ -260,6 +264,9 @@ void Organize::ProcessSomeFiles() {
     }
     else {
       files_with_errors_ << task.song_info_.song_.basefilename();
+      if (!error_text.isEmpty()) {
+        log_ << error_text;
+      }
     }
 
     // Clean up the temporary transcoded file