From 226a6c50e08d84e444d1d2598cbe34e22d3319fa Mon Sep 17 00:00:00 2001 From: Jonas Kvinge Date: Wed, 24 Jan 2024 19:27:30 +0100 Subject: [PATCH] Add better error messages for device and organize Fixes #1364 --- src/core/deletefiles.cpp | 3 ++- src/core/filesystemmusicstorage.cpp | 15 ++++++++--- src/core/filesystemmusicstorage.h | 2 +- src/core/musicstorage.h | 6 ++--- src/device/cddadevice.h | 2 +- src/device/connecteddevice.cpp | 6 +++-- src/device/connecteddevice.h | 4 +-- src/device/gpoddevice.cpp | 40 +++++++++++++++++------------ src/device/gpoddevice.h | 8 +++--- src/device/mtpconnection.cpp | 39 ++++++++++++++++++++++++---- src/device/mtpconnection.h | 4 +++ src/device/mtpdevice.cpp | 21 ++++++++++----- src/device/mtpdevice.h | 6 ++--- src/device/mtploader.cpp | 7 ++++- src/organize/organize.cpp | 11 ++++++-- 15 files changed, 123 insertions(+), 51 deletions(-) diff --git a/src/core/deletefiles.cpp b/src/core/deletefiles.cpp index 1e94f22810..3983098b19 100644 --- a/src/core/deletefiles.cpp +++ b/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_); diff --git a/src/core/filesystemmusicstorage.cpp b/src/core/filesystemmusicstorage.cpp index 52141fc081..f613420eee 100644 --- a/src/core/filesystemmusicstorage.cpp +++ b/src/core/filesystemmusicstorage.cpp @@ -36,7 +36,7 @@ FilesystemMusicStorage::FilesystemMusicStorage(const Song::Source source, const QString &root, const std::optional 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()); diff --git a/src/core/filesystemmusicstorage.h b/src/core/filesystemmusicstorage.h index a4cfc29f52..c57aceb1fc 100644 --- a/src/core/filesystemmusicstorage.h +++ b/src/core/filesystemmusicstorage.h @@ -39,7 +39,7 @@ class FilesystemMusicStorage : public virtual MusicStorage { QString LocalPath() const override { return root_; } std::optional 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: diff --git a/src/core/musicstorage.h b/src/core/musicstorage.h index bf90c1d087..3d7e607481 100644 --- a/src/core/musicstorage.h +++ b/src/core/musicstorage.h @@ -89,12 +89,12 @@ class MusicStorage { virtual bool GetSupportedFiletypes(QList *ret) { Q_UNUSED(ret); return true; } virtual bool StartCopy(QList *supported_types) { Q_UNUSED(supported_types); return true; } - virtual bool CopyToStorage(const CopyJob &job) = 0; - virtual void FinishCopy(bool success) { Q_UNUSED(success); } + virtual bool CopyToStorage(const CopyJob &job, QString &error_text) = 0; + 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() {} diff --git a/src/device/cddadevice.h b/src/device/cddadevice.h index 11d602adb1..20d3296dbf 100644 --- a/src/device/cddadevice.h +++ b/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"; } diff --git a/src/device/connecteddevice.cpp b/src/device/connecteddevice.cpp index 9fdd98270e..fb156bffc8 100644 --- a/src/device/connecteddevice.cpp +++ b/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 { diff --git a/src/device/connecteddevice.h b/src/device/connecteddevice.h index f32472827b..bb5e5ae15f 100644 --- a/src/device/connecteddevice.h +++ b/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; - void FinishDelete(bool success) override; + bool FinishCopy(bool success, QString &error_text) override; + bool FinishDelete(bool success, QString &error_text) override; void Eject() override; virtual void Close(); diff --git a/src/device/gpoddevice.cpp b/src/device/gpoddevice.cpp index ffe9c540f9..77741729f1 100644 --- a/src/device/gpoddevice.cpp +++ b/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; - app_->AddError(QString::fromUtf8(error->message)); + error_text = tr("Could not copy %1 to %2: %3").arg(job.metadata_.url().toLocalFile(), 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) { - qLog(Error) << "Writing database failed:" << error->message; - app_->AddError(QString::fromUtf8(error->message)); - g_error_free(error); - return false; + if (!success) { + if (error) { + error_text = tr("Writing database failed: %1").arg(error->message); + g_error_free(error); + } + 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); } diff --git a/src/device/gpoddevice.h b/src/device/gpoddevice.h index 493896c07b..31783dece0 100644 --- a/src/device/gpoddevice.h +++ b/src/device/gpoddevice.h @@ -64,12 +64,12 @@ class GPodDevice : public ConnectedDevice, public virtual MusicStorage { bool GetSupportedFiletypes(QList *ret) override; bool StartCopy(QList *supported_filetypes) override; - bool CopyToStorage(const CopyJob &job) override; - void FinishCopy(bool success) override; + bool CopyToStorage(const CopyJob &job, QString &error_text) 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_; diff --git a/src/device/mtpconnection.cpp b/src/device/mtpconnection.cpp index a8eee69589..412f5484b4 100644 --- a/src/device/mtpconnection.cpp +++ b/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); - if (err != LIBMTP_ERROR_NONE) { - qLog(Warning) << "MTP error:" << err; + LIBMTP_error_number_t error_number = LIBMTP_Detect_Raw_Devices(&raw_devices, &count); + if (error_number != LIBMTP_ERROR_NONE) { + 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 *ret) { if (!device_) return false; diff --git a/src/device/mtpconnection.h b/src/device/mtpconnection.h index 2b1a0cfe99..ffde07594b 100644 --- a/src/device/mtpconnection.h +++ b/src/device/mtpconnection.h @@ -45,13 +45,17 @@ class MtpConnection : public QObject, public enable_shared_from_this *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 diff --git a/src/device/mtpdevice.cpp b/src/device/mtpdevice.cpp index bc6f72c3dd..15b05d6d74 100644 --- a/src/device/mtpdevice.cpp +++ b/src/device/mtpdevice.cpp @@ -143,7 +143,8 @@ bool MtpDevice::StartCopy(QList *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 *ret) { diff --git a/src/device/mtpdevice.h b/src/device/mtpdevice.h index d56f892531..8abad40b79 100644 --- a/src/device/mtpdevice.h +++ b/src/device/mtpdevice.h @@ -63,12 +63,12 @@ class MtpDevice : public ConnectedDevice { int GetCapacity(); bool StartCopy(QList *supported_types) override; - bool CopyToStorage(const CopyJob &job) override; - void FinishCopy(const bool success) override; + bool CopyToStorage(const CopyJob &job, QString &error_text) 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); diff --git a/src/device/mtploader.cpp b/src/device/mtploader.cpp index 64795d27b1..514c66ef75 100644 --- a/src/device/mtploader.cpp +++ b/src/device/mtploader.cpp @@ -68,11 +68,16 @@ bool MtpLoader::TryLoad() { connection_ = make_unique(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); diff --git a/src/organize/organize.cpp b/src/organize/organize.cpp index 9c99c915b2..73752b1f78 100644 --- a/src/organize/organize.cpp +++ b/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