From 9d19bf85b1c4a38fb128b2e3f4f0ea5ef0f243cb Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Thu, 10 Mar 2016 10:40:20 -0800 Subject: [PATCH] add consistent error handling for asset request and upload --- interface/src/assets/ATPAssetMigrator.cpp | 30 +++---- interface/src/ui/AssetUploadDialogFactory.cpp | 20 ++--- libraries/networking/src/AssetClient.cpp | 86 ++++++++++--------- .../networking/src/AssetResourceRequest.cpp | 8 -- libraries/networking/src/AssetUpload.cpp | 2 +- .../src/AssetScriptingInterface.cpp | 8 -- 6 files changed, 63 insertions(+), 91 deletions(-) diff --git a/interface/src/assets/ATPAssetMigrator.cpp b/interface/src/assets/ATPAssetMigrator.cpp index 51b2ac558a..e9327924a9 100644 --- a/interface/src/assets/ATPAssetMigrator.cpp +++ b/interface/src/assets/ATPAssetMigrator.cpp @@ -143,25 +143,17 @@ void ATPAssetMigrator::migrateResource(ResourceRequest* request) { QFileInfo assetInfo { request->getUrl().fileName() }; auto upload = assetClient->createUpload(request->getData()); - - if (upload) { - // add this URL to our hash of AssetUpload to original URL - _originalURLs.insert(upload, request->getUrl()); - - qCDebug(asset_migrator) << "Starting upload of asset from" << request->getUrl(); - - // connect to the finished signal so we know when the AssetUpload is done - QObject::connect(upload, &AssetUpload::finished, this, &ATPAssetMigrator::assetUploadFinished); - - // start the upload now - upload->start(); - } else { - // show a QMessageBox to say that there is no local asset server - QString messageBoxText = QString("Could not upload \n\n%1\n\nbecause you are currently not connected" \ - " to a local asset-server.").arg(assetInfo.fileName()); - - QMessageBox::information(_dialogParent, "Failed to Upload", messageBoxText); - } + + // add this URL to our hash of AssetUpload to original URL + _originalURLs.insert(upload, request->getUrl()); + + qCDebug(asset_migrator) << "Starting upload of asset from" << request->getUrl(); + + // connect to the finished signal so we know when the AssetUpload is done + QObject::connect(upload, &AssetUpload::finished, this, &ATPAssetMigrator::assetUploadFinished); + + // start the upload now + upload->start(); } void ATPAssetMigrator::assetUploadFinished(AssetUpload *upload, const QString& hash) { diff --git a/interface/src/ui/AssetUploadDialogFactory.cpp b/interface/src/ui/AssetUploadDialogFactory.cpp index 46f07cb7df..7b2da4c83b 100644 --- a/interface/src/ui/AssetUploadDialogFactory.cpp +++ b/interface/src/ui/AssetUploadDialogFactory.cpp @@ -44,20 +44,12 @@ void AssetUploadDialogFactory::showDialog() { auto assetClient = DependencyManager::get(); auto upload = assetClient->createUpload(filename); - - if (upload) { - // connect to the finished signal so we know when the AssetUpload is done - QObject::connect(upload, &AssetUpload::finished, this, &AssetUploadDialogFactory::handleUploadFinished); - - // start the upload now - upload->start(); - } else { - // show a QMessageBox to say that there is no local asset server - QString messageBoxText = QString("Could not upload \n\n%1\n\nbecause you are currently not connected" \ - " to a local asset-server.").arg(QFileInfo(filename).fileName()); - - QMessageBox::information(_dialogParent, "Failed to Upload", messageBoxText); - } + + // connect to the finished signal so we know when the AssetUpload is done + QObject::connect(upload, &AssetUpload::finished, this, &AssetUploadDialogFactory::handleUploadFinished); + + // start the upload now + upload->start(); } } else { // we don't have permission to upload to asset server in this domain - show the permission denied error diff --git a/libraries/networking/src/AssetClient.cpp b/libraries/networking/src/AssetClient.cpp index 79828cc235..8cb1f31090 100644 --- a/libraries/networking/src/AssetClient.cpp +++ b/libraries/networking/src/AssetClient.cpp @@ -144,62 +144,60 @@ GetMappingRequest* AssetClient::createGetMappingRequest(const AssetPath& path) { } GetAllMappingsRequest* AssetClient::createGetAllMappingsRequest() { - return new GetAllMappingsRequest(); + auto request = new GetAllMappingsRequest(); + + request->moveToThread(thread()); + + return request; } DeleteMappingsRequest* AssetClient::createDeleteMappingsRequest(const AssetPathList& paths) { - return new DeleteMappingsRequest(paths); + auto request = new DeleteMappingsRequest(paths); + + request->moveToThread(thread()); + + return request; } SetMappingRequest* AssetClient::createSetMappingRequest(const AssetPath& path, const AssetHash& hash) { - return new SetMappingRequest(path, hash); + auto request = new SetMappingRequest(path, hash); + + request->moveToThread(thread()); + + return request; } RenameMappingRequest* AssetClient::createRenameMappingRequest(const AssetPath& oldPath, const AssetPath& newPath) { - return new RenameMappingRequest(oldPath, newPath); + auto request = new RenameMappingRequest(oldPath, newPath); + + request->moveToThread(thread()); + + return request; } AssetRequest* AssetClient::createRequest(const AssetHash& hash) { - if (isValidHash(hash)) { - qCWarning(asset_client) << "Invalid hash"; - return nullptr; - } + auto request = new AssetRequest(hash); - if (haveAssetServer()) { - auto request = new AssetRequest(hash); - - // Move to the AssetClient thread in case we are not currently on that thread (which will usually be the case) - request->moveToThread(thread()); - - return request; - } else { - return nullptr; - } + // Move to the AssetClient thread in case we are not currently on that thread (which will usually be the case) + request->moveToThread(thread()); + + return request; } AssetUpload* AssetClient::createUpload(const QString& filename) { - - if (haveAssetServer()) { - auto upload = new AssetUpload(filename); - - upload->moveToThread(thread()); - - return upload; - } else { - return nullptr; - } + auto upload = new AssetUpload(filename); + + upload->moveToThread(thread()); + + return upload; } AssetUpload* AssetClient::createUpload(const QByteArray& data) { - if (haveAssetServer()) { - auto upload = new AssetUpload(data); - - upload->moveToThread(thread()); - - return upload; - } else { - return nullptr; - } + auto upload = new AssetUpload(data); + + upload->moveToThread(thread()); + + return upload; } bool AssetClient::getAsset(const QString& hash, DataOffset start, DataOffset end, @@ -233,9 +231,12 @@ bool AssetClient::getAsset(const QString& hash, DataOffset start, DataOffset end _pendingRequests[assetServer][messageID] = { callback, progressCallback }; return true; + } else { + callback(false, AssetServerError::NoError, QByteArray()); + return false; } - return false; + } bool AssetClient::getAssetInfo(const QString& hash, GetInfoCallback callback) { @@ -256,9 +257,10 @@ bool AssetClient::getAssetInfo(const QString& hash, GetInfoCallback callback) { _pendingInfoRequests[assetServer][messageID] = callback; return true; + } else { + callback(false, AssetServerError::NoError, { "", 0 }); + return false; } - - return false; } void AssetClient::handleAssetGetInfoReply(QSharedPointer message, SharedNodePointer senderNode) { @@ -495,8 +497,10 @@ bool AssetClient::uploadAsset(const QByteArray& data, UploadResultCallback callb _pendingUploads[assetServer][messageID] = callback; return true; + } else { + callback(false, AssetServerError::NoError, QString()); + return false; } - return false; } void AssetClient::handleAssetUploadReply(QSharedPointer message, SharedNodePointer senderNode) { diff --git a/libraries/networking/src/AssetResourceRequest.cpp b/libraries/networking/src/AssetResourceRequest.cpp index 3cc7bdd4c7..a896f94c8c 100644 --- a/libraries/networking/src/AssetResourceRequest.cpp +++ b/libraries/networking/src/AssetResourceRequest.cpp @@ -122,14 +122,6 @@ void AssetResourceRequest::requestHash(const AssetHash& hash) { auto assetClient = DependencyManager::get(); _assetRequest = assetClient->createRequest(hash); - if (!_assetRequest) { - _result = ServerUnavailable; - _state = Finished; - - emit finished(); - return; - } - connect(_assetRequest, &AssetRequest::progress, this, &AssetResourceRequest::progress); connect(_assetRequest, &AssetRequest::finished, this, [this](AssetRequest* req) { Q_ASSERT(_state == InProgress); diff --git a/libraries/networking/src/AssetUpload.cpp b/libraries/networking/src/AssetUpload.cpp index 754d632cce..2840f6777c 100644 --- a/libraries/networking/src/AssetUpload.cpp +++ b/libraries/networking/src/AssetUpload.cpp @@ -41,7 +41,7 @@ QString AssetUpload::getErrorString() const { case AssetUpload::FileOpenError: return "The file could not be opened. Please check your permissions and try again."; case AssetUpload::NetworkError: - return "The file could not be opened. Please check your network connectivity."; + return "There was a problem reaching your Asset Server. Please check your network connectivity."; default: // not handled, do not show a message box return QString(); diff --git a/libraries/script-engine/src/AssetScriptingInterface.cpp b/libraries/script-engine/src/AssetScriptingInterface.cpp index e6a482ad27..aaa4f97fb3 100644 --- a/libraries/script-engine/src/AssetScriptingInterface.cpp +++ b/libraries/script-engine/src/AssetScriptingInterface.cpp @@ -27,10 +27,6 @@ AssetScriptingInterface::AssetScriptingInterface(QScriptEngine* engine) : void AssetScriptingInterface::uploadData(QString data, QScriptValue callback) { QByteArray dataByteArray = data.toUtf8(); auto upload = DependencyManager::get()->createUpload(dataByteArray); - if (!upload) { - qCWarning(asset_client) << "Error uploading file to asset server"; - return; - } QObject::connect(upload, &AssetUpload::finished, this, [this, callback](AssetUpload* upload, const QString& hash) mutable { if (callback.isFunction()) { @@ -61,10 +57,6 @@ void AssetScriptingInterface::downloadData(QString urlString, QScriptValue callb auto assetClient = DependencyManager::get(); auto assetRequest = assetClient->createRequest(hash); - if (!assetRequest) { - return; - } - _pendingRequests << assetRequest; connect(assetRequest, &AssetRequest::finished, this, [this, callback](AssetRequest* request) mutable {