From ae63ed4804192a8d1ead02a98f39cd84574f2437 Mon Sep 17 00:00:00 2001 From: Roman Pudashkin Date: Wed, 9 Sep 2026 19:59:11 +0300 Subject: [PATCH 01/10] Convert file to score: treat converted scores as cloud scores Instead of downloading them locally --- .../cloud/musescorecom/converttypes.h | 11 +- .../imusescorecomconvertservice.h | 17 +- .../musescorecom/musescorecomservice.cpp | 88 +-- .../cloud/musescorecom/musescorecomservice.h | 2 - .../mocks/musescorecomconvertservicemock.h | 2 - src/project/iconvertfiletoscorescenario.h | 7 +- src/project/iconvertfiletoscoreservice.h | 16 +- .../internal/convertfiletoscorescenario.cpp | 46 +- .../internal/convertfiletoscorescenario.h | 12 +- .../internal/convertfiletoscoreservice.cpp | 469 ++++------- .../internal/convertfiletoscoreservice.h | 76 +- src/project/internal/projectconfiguration.cpp | 5 - src/project/internal/projectconfiguration.h | 7 +- src/project/iprojectconfiguration.h | 4 - src/project/projecterrors.h | 1 - .../Project/ConvertFileToScoreDialog.qml | 12 +- .../ConvertFileToScore/LinkEntryPage.qml | 2 +- .../ConvertFileToScore/SelectedFilesPage.qml | 2 +- .../internal/ScoresPage/recentscoresmodel.cpp | 6 - .../convertfiletoscorescenario_tests.cpp | 88 ++- .../tests/convertfiletoscoreservice_tests.cpp | 748 ++++++------------ .../mocks/convertfiletoscoreservicemock.h | 8 +- .../tests/mocks/projectconfigurationmock.h | 4 - src/project/types/converttypes.h | 4 +- .../project/projectconfigurationstub.cpp | 10 - src/stubs/project/projectconfigurationstub.h | 4 - 26 files changed, 555 insertions(+), 1096 deletions(-) diff --git a/src/framework/cloud/musescorecom/converttypes.h b/src/framework/cloud/musescorecom/converttypes.h index d28383873d631..45d54ad411275 100644 --- a/src/framework/cloud/musescorecom/converttypes.h +++ b/src/framework/cloud/musescorecom/converttypes.h @@ -22,6 +22,7 @@ #pragma once +#include #include #include #include @@ -29,7 +30,6 @@ #include #include #include -#include #include "global/logstream.h" #include "global/types/flags.h" @@ -189,7 +189,7 @@ struct ConvertQueueItem { ConvertStatus status = ConvertStatus::Processing; QString filename; QString link; //! audio2score only - int scoreId = 0; + std::optional scoreId; //! set once the score is ready (AwaitingReview/Done) QDateTime createdAt; QDateTime updatedAt; ConvertErrorCode errorCode = ConvertErrorCode::Unknown; @@ -197,11 +197,6 @@ struct ConvertQueueItem { using ConvertQueueList = std::vector; -struct SignedMsczUrl { - QUrl url; - int expiresInSeconds = 0; -}; - //! NOTE: must be in sync with the musescore.com API enum class ReviewRating { Bad = 0, @@ -220,7 +215,7 @@ inline muse::logger::Stream& operator<<(muse::logger::Stream& s, const muse::clo << ", link: \"" << item.link << "\"" << ", type: " << muse::cloud::convertTypeToString(item.type) << ", status: " << muse::cloud::convertStatusToString(item.status) - << ", scoreId: " << item.scoreId + << ", scoreId: " << (item.scoreId ? QString::number(*item.scoreId) : QString("none")) << ", createdAt: " << dateTimeToString(item.createdAt) << ", updatedAt: " << dateTimeToString(item.updatedAt); return s; diff --git a/src/framework/cloud/musescorecom/imusescorecomconvertservice.h b/src/framework/cloud/musescorecom/imusescorecomconvertservice.h index 83ef1014b8e08..a7b42b562d3a0 100644 --- a/src/framework/cloud/musescorecom/imusescorecomconvertservice.h +++ b/src/framework/cloud/musescorecom/imusescorecomconvertservice.h @@ -29,9 +29,6 @@ #include "converttypes.h" -class QIODevice; -using DevicePtr = std::shared_ptr; - namespace muse::cloud { /// fetchConfig() can be called at any time (no authenticated user required) to get the /// upload limits (max file size, page/image counts, allowed types) for client-side validation @@ -39,13 +36,11 @@ namespace muse::cloud { /// /// Expected call order for a conversion (OMR or Audio2Score): /// 1. upload() to submit the file(s) and start processing -/// 2. Poll fetchQueue() and watch the item's status -/// 3. As soon as the status is AwaitingReview or Done, the MSCZ is already -/// available: call fetchMsczUrl() then downloadConvertedScore() to get the score -/// 4. Rating the recognition quality (submitReview(), once AwaitingReview) is optional -/// and does not gate the download above; submitReviewComment() may attach a -/// comment afterwards, once the review has been submitted -/// 5. Keep polling fetchQueue() until the status is Failed, or the item disappears +/// 2. Poll fetchQueue() and watch the item's status; once it's AwaitingReview or Done, its +/// scoreId identifies the resulting score, already available via IMuseScoreComService +/// 3. Rating the recognition quality (submitReview(), once AwaitingReview) is optional; +/// submitReviewComment() may attach a comment afterwards, once the review has been submitted +/// 4. Keep polling fetchQueue() until the status is Failed, or the item disappears /// from the queue (which should be treated the same as Done) class IMuseScoreComConvertService : MODULE_CONTEXT_INTERFACE { @@ -57,10 +52,8 @@ class IMuseScoreComConvertService : MODULE_CONTEXT_INTERFACE virtual async::Promise > fetchConfig() = 0; virtual ProgressPtr upload(const ConvertInput& input) = 0; - virtual ProgressPtr downloadConvertedScore(const SignedMsczUrl& urlInfo, DevicePtr scoreData) = 0; virtual async::Promise > fetchQueue() = 0; - virtual async::Promise > fetchMsczUrl(ConvertType type, int id) = 0; virtual async::Promise > submitReview(ConvertType type, int id, ReviewRating review, const QString& comment = QString()) = 0; diff --git a/src/framework/cloud/musescorecom/musescorecomservice.cpp b/src/framework/cloud/musescorecom/musescorecomservice.cpp index f61555066b6ea..3e294e68845f1 100644 --- a/src/framework/cloud/musescorecom/musescorecomservice.cpp +++ b/src/framework/cloud/musescorecom/musescorecomservice.cpp @@ -59,7 +59,6 @@ static const QUrl MUSESCORECOM_UPLOAD_AUDIO_API_URL(MUSESCORECOM_API_ROOT_URL + static const QUrl MUSESCORECOM_CONVERT_CONFIG_URL("https://musescore.com/static/musescore/studio/upload-config.json"); static const QUrl MUSESCORECOM_CONVERT_UPLOAD_API_URL(MUSESCORECOM_API_ROOT_URL + "/score/convert/convert"); static const QUrl MUSESCORECOM_CONVERT_QUEUE_API_URL(MUSESCORECOM_API_ROOT_URL + "/score/convert/queue"); -static const QUrl MUSESCORECOM_CONVERT_MSCZ_API_URL(MUSESCORECOM_API_ROOT_URL + "/score/convert/mscz"); static const QUrl MUSESCORECOM_CONVERT_REVIEW_API_URL(MUSESCORECOM_API_ROOT_URL + "/score/convert/review"); static const QUrl MUSESCORECOM_CONVERT_COMMENT_API_URL(MUSESCORECOM_API_ROOT_URL + "/score/convert/comment"); @@ -389,7 +388,10 @@ static RetVal parseConvertQueueList(const QByteArray& data) item.status = convertStatusFromApiString(itemObj.value("status").toString()); item.filename = itemObj.value("filename").toString(); item.link = itemObj.value("link").toString(); - item.scoreId = itemObj.value("score_id").toInt(); + const QJsonValue scoreIdVal = itemObj.value("score_id"); + if (scoreIdVal.isDouble()) { + item.scoreId = scoreIdVal.toInt(); + } item.createdAt = QDateTime::fromSecsSinceEpoch(itemObj.value("created_at").toInteger()); item.updatedAt = QDateTime::fromSecsSinceEpoch(itemObj.value("updated_at").toInteger()); item.errorCode = convertErrorCodeFromApiString(itemObj.value("error_code").toString()); @@ -400,23 +402,6 @@ static RetVal parseConvertQueueList(const QByteArray& data) return RetVal::make_ok(result); } -static RetVal parseSignedMsczUrl(const QByteArray& data) -{ - QJsonParseError err; - QJsonDocument doc = QJsonDocument::fromJson(data, &err); - if (err.error != QJsonParseError::NoError || !doc.isObject()) { - return RetVal::make_ret((int)Ret::Code::InternalError, err.errorString().toStdString()); - } - - QJsonObject obj = doc.object(); - - SignedMsczUrl result; - result.url = QUrl(obj.value("url").toString()); - result.expiresInSeconds = obj.value("expires_in").toInt(); - - return RetVal::make_ok(result); -} - static LinkSource linkSourceFromApiString(const QString& str) { if (str.compare("youtube", Qt::CaseInsensitive) == 0) { @@ -1110,37 +1095,6 @@ Promise MuseScoreComService::doUpload(const ConvertInput& input, ProgressPt }); } -ProgressPtr MuseScoreComService::downloadConvertedScore(const SignedMsczUrl& urlInfo, DevicePtr scoreData) -{ - TRACEFUNC; - - ProgressPtr progress = std::make_shared(); - progress->start(); - - IF_ASSERT_FAILED(urlInfo.url.isValid()) { - progress->finish(make_ret(Err::InvalidData)); - return progress; - } - - //! NOTE: urlInfo.url is already a signed URL, so it must be - //! requested as-is, without going through prepareUrlForRequest - RetVal getProgress = m_networkManager->get(urlInfo.url, scoreData, headers()); - if (!getProgress.ret) { - progress->finish(getProgress.ret); - return progress; - } - - getProgress.val.progressChanged().onReceive(this, [progress](int64_t current, int64_t total, const std::string& msg) { - progress->progress(current, total, msg); - }); - - getProgress.val.finished().onReceive(this, [this, progress](const ProgressResult& res) { - progress->finish(uploadingDownloadingRetFromRawRet(res.ret)); - }); - - return progress; -} - Promise > MuseScoreComService::fetchQueue() { return Promise >([this](auto resolve, auto) { @@ -1171,40 +1125,6 @@ Promise > MuseScoreComService::fetchQueue() }); } -Promise > MuseScoreComService::fetchMsczUrl(ConvertType type, int id) -{ - return Promise >([this, type, id](auto resolve, auto) { - QVariantMap params; - params["id"] = id; - params["type"] = convertTypeToApiString(type); - - RetVal msczUrl = prepareUrlForRequest(MUSESCORECOM_CONVERT_MSCZ_API_URL, params); - if (!msczUrl.ret) { - return resolve(RetVal::make_ret(msczUrl.ret)); - } - - auto receivedData = std::make_shared(); - RetVal progress = m_networkManager->get(msczUrl.val, receivedData, headers()); - if (!progress.ret) { - return resolve(RetVal::make_ret(progress.ret)); - } - - progress.val.finished().onReceive(this, [this, receivedData, resolve](const ProgressResult& res) { - if (!res.ret) { - printServerReply(*receivedData); - Ret ret = uploadingDownloadingRetFromRawRet(res.ret); - appendServerErrorCode(ret, receivedData->data()); - (void)resolve(RetVal::make_ret(ret)); - return; - } - - (void)resolve(parseSignedMsczUrl(receivedData->data())); - }); - - return Promise >::dummy_result(); - }); -} - Promise > MuseScoreComService::submitReview(ConvertType type, int id, ReviewRating review, const QString& comment) { return Promise >([this, type, id, review, comment](auto resolve, auto) { diff --git a/src/framework/cloud/musescorecom/musescorecomservice.h b/src/framework/cloud/musescorecom/musescorecomservice.h index defd95d412635..c8f2b422aeb8c 100644 --- a/src/framework/cloud/musescorecom/musescorecomservice.h +++ b/src/framework/cloud/musescorecom/musescorecomservice.h @@ -68,10 +68,8 @@ class MuseScoreComService : public IMuseScoreComService, public IMuseScoreComCon async::Promise > fetchConfig() override; ProgressPtr upload(const ConvertInput& input) override; - ProgressPtr downloadConvertedScore(const SignedMsczUrl& urlInfo, DevicePtr scoreData) override; async::Promise > fetchQueue() override; - async::Promise > fetchMsczUrl(ConvertType type, int id) override; async::Promise > submitReview(ConvertType type, int id, ReviewRating review, const QString& comment = QString()) override; diff --git a/src/framework/cloud/tests/mocks/musescorecomconvertservicemock.h b/src/framework/cloud/tests/mocks/musescorecomconvertservicemock.h index 3c945716da477..f4b264c5cbf51 100644 --- a/src/framework/cloud/tests/mocks/musescorecomconvertservicemock.h +++ b/src/framework/cloud/tests/mocks/musescorecomconvertservicemock.h @@ -32,10 +32,8 @@ class MuseScoreComConvertServiceMock : public IMuseScoreComConvertService MOCK_METHOD(async::Promise >, fetchConfig, (), (override)); MOCK_METHOD(ProgressPtr, upload, (const ConvertInput&), (override)); - MOCK_METHOD(ProgressPtr, downloadConvertedScore, (const SignedMsczUrl&, DevicePtr), (override)); MOCK_METHOD(async::Promise >, fetchQueue, (), (override)); - MOCK_METHOD(async::Promise >, fetchMsczUrl, (ConvertType, int), (override)); MOCK_METHOD(async::Promise >, submitReview, (ConvertType, int, ReviewRating, const QString&), (override)); MOCK_METHOD(async::Promise >, submitReviewComment, (ConvertType, int, const QString&), (override)); diff --git a/src/project/iconvertfiletoscorescenario.h b/src/project/iconvertfiletoscorescenario.h index 8af74d6d483fb..ff2e6ac9a4f10 100644 --- a/src/project/iconvertfiletoscorescenario.h +++ b/src/project/iconvertfiletoscorescenario.h @@ -35,7 +35,7 @@ class QUrl; namespace mu::project { struct ConvertSelection { ConvertInput input; - muse::String convertedFileName; + muse::String convertedScoreName; }; class IConvertFileToScoreScenario : MODULE_CONTEXT_INTERFACE @@ -62,8 +62,9 @@ class IConvertFileToScoreScenario : MODULE_CONTEXT_INTERFACE //! opens the picker with them pre-selected virtual void convertFiles(const muse::io::paths_t& paths = {}) = 0; - //! Emits the result once the server-side conversion completes - virtual muse::async::Channel convertFinished() const = 0; + //! Emits the result once the server-side conversion completes; on success, the converted score + //! is already available in the user's MuseScore.com account, described by ScoreInfo + virtual muse::async::Channel convertFinished() const = 0; }; using IConvertFileToScoreScenarioPtr = std::shared_ptr; diff --git a/src/project/iconvertfiletoscoreservice.h b/src/project/iconvertfiletoscoreservice.h index 90dd3c557f8c2..eed1851866392 100644 --- a/src/project/iconvertfiletoscoreservice.h +++ b/src/project/iconvertfiletoscoreservice.h @@ -53,12 +53,12 @@ class IConvertFileToScoreService : MODULE_CONTEXT_INTERFACE virtual muse::Ret validateLink(const QUrl& link) const = 0; //! Sends the conversion request to the server - virtual muse::Ret startConvert(const ConvertInput& input, const muse::String& convertedFileName) = 0; + virtual muse::Ret startConvert(const ConvertInput& input, const muse::String& convertedScoreName) = 0; - //! Emits the result once the server-side conversion completes - virtual muse::async::Channel convertFinished() const = 0; + //! Emits the final result of a conversion (upload or processing failure, or success with ScoreInfo) + virtual muse::async::Channel convertFinished() const = 0; - //! Names of the files currently being converted server-side (queued, processing, or downloading) + //! Names of the files currently being converted server-side (queued or processing) virtual muse::StringList fileNamesBeingConverted() const = 0; virtual muse::async::Notification fileNamesBeingConvertedChanged() const = 0; @@ -68,10 +68,10 @@ class IConvertFileToScoreService : MODULE_CONTEXT_INTERFACE //! Resumes polling for any still-pending items - e.g. in response to the user pressing "Retry" virtual void retryPolling() = 0; - //! Emitted once a converted score has been downloaded and is awaiting a quality review - virtual muse::async::Channel reviewRequested() const = 0; - virtual void submitReview(ConvertType type, int itemId, ReviewRating rating, const QString& comment = QString()) = 0; - virtual void submitReviewComment(ConvertType type, int itemId, const QString& comment) = 0; + //! Emitted once a converted score is ready and awaiting a quality review + virtual muse::async::Channel reviewRequested() const = 0; + virtual void submitReview(int scoreId, ReviewRating rating, const QString& comment = QString()) = 0; + virtual void submitReviewComment(int scoreId, const QString& comment) = 0; }; using IConvertFileToScoreServicePtr = std::shared_ptr; diff --git a/src/project/internal/convertfiletoscorescenario.cpp b/src/project/internal/convertfiletoscorescenario.cpp index d8e970425d363..1bb8e4957bbe5 100644 --- a/src/project/internal/convertfiletoscorescenario.cpp +++ b/src/project/internal/convertfiletoscorescenario.cpp @@ -21,7 +21,6 @@ */ #include "convertfiletoscorescenario.h" -#include #include #include @@ -53,7 +52,7 @@ static ConvertSelection toConvertSelection(const Val& val) } ConvertSelection selection; - selection.convertedFileName = map.value("convertedFileName").toString(); + selection.convertedScoreName = map.value("convertedScoreName").toString(); if (type == ConvertType::Audio2Score && !link.isEmpty()) { selection.input = Audio2ScoreConvertInput { link }; @@ -75,18 +74,18 @@ void ConvertFileToScoreScenario::init() { TRACEFUNC; - service()->convertFinished().onReceive(this, [this](const Ret& ret, const io::path_t& path) { + service()->convertFinished().onReceive(this, [this](const Ret& ret, const ScoreInfo& scoreInfo) { if (ret) { - showScoreReadyNotification(path); + showScoreReadyNotification(scoreInfo); } else { showConvertFailedNotification(ret); } - m_convertFinished.send(ret, path); + m_convertFinished.send(ret, scoreInfo); }); - service()->reviewRequested().onReceive(this, [this](ConvertType type, int queueId, const io::path_t& path) { - m_pendingReviews[path] = { type, queueId }; + service()->reviewRequested().onReceive(this, [this](int scoreId) { + m_pendingReviews[configuration()->cloudProjectPath(scoreId)] = scoreId; checkPendingReview(); }); @@ -107,11 +106,10 @@ void ConvertFileToScoreScenario::checkPendingReview() return; } - const ConvertType type = it->second.first; - const int queueId = it->second.second; + const int scoreId = it->second; const io::path_t path = it->first; - QTimer::singleShot(REVIEW_PROMPT_DELAY_MS, this, [this, type, queueId, path]() { + QTimer::singleShot(REVIEW_PROMPT_DELAY_MS, this, [this, scoreId, path]() { INotationProjectPtr currentProject = globalContext()->currentProject(); if (!currentProject || currentProject->path() != path) { return; @@ -121,7 +119,7 @@ void ConvertFileToScoreScenario::checkPendingReview() return; } - askReviewRating(type, queueId); + askReviewRating(scoreId); }); } @@ -166,7 +164,7 @@ void ConvertFileToScoreScenario::convertFiles(const io::paths_t& paths) if (paths.empty()) { selectFilesToConvert() .onResolve(this, [this](const ConvertSelection& selection) { - startConvert(selection.input, selection.convertedFileName); + startConvert(selection.input, selection.convertedScoreName); }); return; } @@ -180,7 +178,7 @@ void ConvertFileToScoreScenario::convertFiles(const io::paths_t& paths) }); } -async::Channel ConvertFileToScoreScenario::convertFinished() const +async::Channel ConvertFileToScoreScenario::convertFinished() const { return m_convertFinished; } @@ -277,14 +275,14 @@ void ConvertFileToScoreScenario::confirmConvert(const io::paths_t& paths, Conver selectFilesToConvert(paths, type) .onResolve(this, [this](const ConvertSelection& selection) { - startConvert(selection.input, selection.convertedFileName); + startConvert(selection.input, selection.convertedScoreName); }); }); } -Ret ConvertFileToScoreScenario::startConvert(const ConvertInput& input, const muse::String& convertedFileName) +Ret ConvertFileToScoreScenario::startConvert(const ConvertInput& input, const muse::String& convertedScoreName) { - Ret ret = service()->startConvert(input, convertedFileName); + Ret ret = service()->startConvert(input, convertedScoreName); if (!ret) { showUnknownError(); return ret; @@ -439,22 +437,22 @@ void ConvertFileToScoreScenario::showFileProcessingDialog() }); } -void ConvertFileToScoreScenario::showScoreReadyNotification(const io::path_t& path) +void ConvertFileToScoreScenario::showScoreReadyNotification(const ScoreInfo& scoreInfo) { constexpr int openScoreBtn = int(toast::ToastActionCode::Custom) + 1; - QString scoreName = QFileInfo(path.toQString()).completeBaseName(); std::string msg = muse::qtrc("project/convert", "‘%1’ has finished processing and is ready to open.") - .arg(scoreName).toStdString(); + .arg(scoreInfo.title).toStdString(); toastService()->show(muse::trc("project/convert", "Your score is ready!"), msg, muse::ui::IconCode::Code::TICK_FILLED, true, { { muse::trc("global", "Dismiss"), toast::ToastActionCode::Dismiss }, { muse::trc("project/convert", "Open score"), openScoreBtn, /*accent*/ true }, - }).onResolve(this, [this, path, openScoreBtn](const toast::ToastResult& result) { + }).onResolve(this, [this, scoreInfo, openScoreBtn](const toast::ToastResult& result) { if (result.isCode(openScoreBtn)) { - dispatcher()->dispatch("file-open", actions::ActionData::make_arg1(path.toQUrl())); + const QUrl url(QString("musescore://open-score/%1").arg(scoreInfo.id)); + dispatcher()->dispatch("file-open", actions::ActionData::make_arg1(url)); } }); } @@ -481,7 +479,7 @@ void ConvertFileToScoreScenario::showConvertFailedNotification(const Ret& ret) }); } -void ConvertFileToScoreScenario::askReviewRating(ConvertType type, int queueId) +void ConvertFileToScoreScenario::askReviewRating(int scoreId) { static constexpr int goodBtn = int(toast::ToastActionCode::Custom) + 1; static constexpr int badBtn = int(toast::ToastActionCode::Custom) + 2; @@ -495,8 +493,8 @@ void ConvertFileToScoreScenario::askReviewRating(ConvertType type, int queueId) { muse::trc("project/convert", "Good"), goodBtn, /*accent*/ true, muse::ui::IconCode::Code::LIKE }, //: Button to rate the quality of a converted score as bad { muse::trc("project/convert", "Bad"), badBtn, /*accent*/ false, muse::ui::IconCode::Code::DISLIKE }, - }).onResolve(this, [this, queueId, type](const toast::ToastResult& result) { + }).onResolve(this, [this, scoreId](const toast::ToastResult& result) { ReviewRating rating = result.isCode(goodBtn) ? ReviewRating::Good : ReviewRating::Bad; - service()->submitReview(type, queueId, rating); + service()->submitReview(scoreId, rating); }); } diff --git a/src/project/internal/convertfiletoscorescenario.h b/src/project/internal/convertfiletoscorescenario.h index 56f5de1b71689..9c23b137b0dca 100644 --- a/src/project/internal/convertfiletoscorescenario.h +++ b/src/project/internal/convertfiletoscorescenario.h @@ -66,7 +66,7 @@ class ConvertFileToScoreScenario : public QObject, public IConvertFileToScoreSce muse::Ret validateLink(const QUrl& link) override; void convertFiles(const muse::io::paths_t& paths = {}) override; - muse::async::Channel convertFinished() const override; + muse::async::Channel convertFinished() const override; private: muse::async::Promise checkConvertIsAllowed(); @@ -76,7 +76,7 @@ class ConvertFileToScoreScenario : public QObject, public IConvertFileToScoreSce void confirmConvert(const muse::io::paths_t& paths, ConvertType type); - muse::Ret startConvert(const ConvertInput& input, const muse::String& convertedFileName); + muse::Ret startConvert(const ConvertInput& input, const muse::String& convertedScoreName); void showValidationError(const muse::Ret& ret); @@ -92,13 +92,13 @@ class ConvertFileToScoreScenario : public QObject, public IConvertFileToScoreSce void showTooManyImagesError(int maxImages); void showFileProcessingDialog(); - void showScoreReadyNotification(const muse::io::path_t& path); + void showScoreReadyNotification(const ScoreInfo& scoreInfo); void showConvertFailedNotification(const muse::Ret& ret); - void askReviewRating(ConvertType type, int queueId); + void askReviewRating(int scoreId); void checkPendingReview(); - muse::async::Channel m_convertFinished; - std::map > m_pendingReviews; + muse::async::Channel m_convertFinished; + std::map m_pendingReviews; }; } diff --git a/src/project/internal/convertfiletoscoreservice.cpp b/src/project/internal/convertfiletoscoreservice.cpp index f5f4ed3fcd8de..2a2bf8772ba9f 100644 --- a/src/project/internal/convertfiletoscoreservice.cpp +++ b/src/project/internal/convertfiletoscoreservice.cpp @@ -24,7 +24,6 @@ #include #include -#include #include #include "project/types/filecategory.h" @@ -115,9 +114,9 @@ static std::string convertLogId(ConvertType type, int itemId) return std::to_string(itemId) + " (type: " + convertTypeToString(type) + ")"; } -static std::string convertLogId(const muse::String& convertedFileName, ConvertType type, int itemId) +static std::string convertLogId(const muse::String& convertedScoreName, ConvertType type, int itemId) { - return "\"" + convertedFileName.toStdString() + "\" (conversion " + convertLogId(type, itemId) + ")"; + return "\"" + convertedScoreName.toStdString() + "\" (conversion " + convertLogId(type, itemId) + ")"; } void ConvertFileToScoreService::init() @@ -159,14 +158,14 @@ void ConvertFileToScoreService::resumeConvert() return; } - LOGI() << "Resuming " << m_watchedItems.size() << " pending conversion(s)"; + LOGI() << "Resuming watching " << m_watchedItems.size() << " pending conversion(s)"; m_timer.start(); m_fileNamesBeingConvertedChanged.notify(); for (const WatchedItem& item : m_watchedItems) { - if (item.convertStatus == ConvertStatus::AwaitingReview && !item.downloadedScorePath.empty()) { - m_reviewRequested.send(item.type, item.id, item.downloadedScorePath); + if (item.status == ConvertStatus::AwaitingReview && item.scoreId) { + m_reviewRequested.send(*item.scoreId); } } @@ -276,52 +275,45 @@ Ret ConvertFileToScoreService::validateLink(const QUrl& link) const return make_ret(Err::ConvertUnsupportedLink); } -Ret ConvertFileToScoreService::startConvert(const ConvertInput& input, const muse::String& convertedFileName) +Ret ConvertFileToScoreService::startConvert(const ConvertInput& input, const muse::String& convertedScoreName) { IF_ASSERT_FAILED(!convertPathsOf(input).empty() || !convertLinkOf(input).isEmpty()) { return make_ret(Err::ConvertValidationFailed); } - IF_ASSERT_FAILED(io::isAllowedFileName(io::path_t(convertedFileName))) { + IF_ASSERT_FAILED(io::isAllowedFileName(io::path_t(convertedScoreName))) { return make_ret(Err::ConvertValidationFailed); } const ConvertType type = convertTypeOf(input); ProgressPtr progress = museScoreComService()->convert()->upload(input); - progress->progressChanged().onReceive(this, [convertedFileName](int64_t current, int64_t total, const std::string&) { - LOGI() << "Uploading for convert \"" << convertedFileName << "\": " << current << "/" << total; + progress->progressChanged().onReceive(this, [convertedScoreName](int64_t current, int64_t total, const std::string&) { + LOGI() << "Uploading for convert \"" << convertedScoreName << "\": " << current << "/" << total; }); - progress->finished().onReceive(this, [this, type, convertedFileName](const ProgressResult& res) { + progress->finished().onReceive(this, [this, type, convertedScoreName](const ProgressResult& res) { if (!res.ret) { - LOGE() << "Could not upload files for \"" << convertedFileName << "\" (type: " + LOGE() << "Could not upload files for \"" << convertedScoreName << "\" (type: " << convertTypeToString(type) << "): " << res.ret.toString(); Ret ret = res.ret; - ret.setData(CONVERT_FAILED_FILE_NAME_KEY, convertedFileName); + ret.setData(CONVERT_FAILED_FILE_NAME_KEY, convertedScoreName); finishConvert(ret); return; } const int itemId = res.val.toMap()["id"].toInt(); - watch(type, itemId, convertedFileName); + watch(type, itemId, convertedScoreName); }); return make_ok(); } -async::Channel ConvertFileToScoreService::convertFinished() const +async::Channel ConvertFileToScoreService::convertFinished() const { return m_convertFinished; } -bool ConvertFileToScoreService::isPending(const WatchedItem& item) -{ - //! NOTE: an AwaitingReview item that's already downloaded is just waiting on the user - //! to submit a review - not "being converted" anymore - return item.convertStatus != ConvertStatus::AwaitingReview || item.downloadedScorePath.empty(); -} - muse::StringList ConvertFileToScoreService::fileNamesBeingConverted() const { muse::StringList result; @@ -329,7 +321,7 @@ muse::StringList ConvertFileToScoreService::fileNamesBeingConverted() const for (const WatchedItem& item : m_watchedItems) { if (isPending(item)) { - result.push_back(item.convertedFileName); + result.push_back(item.convertedScoreName); } } @@ -353,17 +345,25 @@ void ConvertFileToScoreService::retryPolling() poll(); } -async::Channel ConvertFileToScoreService::reviewRequested() const +async::Channel ConvertFileToScoreService::reviewRequested() const { return m_reviewRequested; } -void ConvertFileToScoreService::submitReview(ConvertType type, int itemId, ReviewRating rating, const QString& comment) +void ConvertFileToScoreService::submitReview(int scoreId, ReviewRating rating, const QString& comment) { IF_ASSERT_FAILED(rating == ReviewRating::Bad || comment.isEmpty()) { return; } + const WatchedItem* item = findWatchedItemByScoreId(scoreId); + IF_ASSERT_FAILED(item) { + return; + } + + const ConvertType type = item->type; + const int itemId = item->id; + museScoreComService()->convert()->submitReview(type, itemId, rating, comment) .onResolve(this, [type, itemId](const RetVal& submitRes) { if (!submitRes.ret) { @@ -372,8 +372,16 @@ void ConvertFileToScoreService::submitReview(ConvertType type, int itemId, Revie }); } -void ConvertFileToScoreService::submitReviewComment(ConvertType type, int itemId, const QString& comment) +void ConvertFileToScoreService::submitReviewComment(int scoreId, const QString& comment) { + const WatchedItem* item = findWatchedItemByScoreId(scoreId); + IF_ASSERT_FAILED(item) { + return; + } + + const ConvertType type = item->type; + const int itemId = item->id; + museScoreComService()->convert()->submitReviewComment(type, itemId, comment) .onResolve(this, [type, itemId](const RetVal& submitRes) { if (!submitRes.ret) { @@ -382,11 +390,78 @@ void ConvertFileToScoreService::submitReviewComment(ConvertType type, int itemId }); } -void ConvertFileToScoreService::watch(ConvertType type, int itemId, const muse::String& convertedFileName) +void ConvertFileToScoreService::loadWatchedItems() { - LOGI() << "Watching conversion " << convertLogId(convertedFileName, type, itemId); + TRACEFUNC; + + m_watchedItems.clear(); + + RetVal data = fileSystem()->readFile(configuration()->pendingConvertsJsonPath()); + if (!data.ret || data.val.empty()) { + if (!data.ret && data.ret.code() != static_cast(io::Err::FSNotExist)) { + LOGE() << "Could not read the pending conversions file: " << data.ret; + } + return; + } + + std::string err; + const JsonDocument json = JsonDocument::fromJson(data.val, &err); + if (!err.empty() || !json.isArray()) { + if (!err.empty()) { + LOGE() << "Could not parse the pending conversions file: " << err; + } + return; + } + + const JsonArray array = json.rootArray(); + m_watchedItems.reserve(array.size()); + + for (size_t i = 0; i < array.size(); ++i) { + const JsonObject obj = array.at(i).toObject(); + const int itemId = obj.value("id").toInt(); + const ConvertType type = static_cast(obj.value("type").toInt()); + const muse::String convertedScoreName = muse::String::fromStdString(obj.value("convertedScoreName").toStdString()); + const ConvertStatus status = static_cast(obj.value("status").toInt()); + const std::optional scoreId = obj.contains("scoreId") ? std::optional(obj.value("scoreId").toInt()) : std::nullopt; + + m_watchedItems.push_back(WatchedItem { itemId, type, status, scoreId, convertedScoreName }); + } +} + +void ConvertFileToScoreService::saveWatchedItems() +{ + TRACEFUNC; + + JsonArray array; + for (const WatchedItem& item : m_watchedItems) { + JsonObject obj; + obj["id"] = item.id; + obj["type"] = static_cast(item.type); + obj["status"] = static_cast(item.status); - m_watchedItems.push_back(WatchedItem { itemId, type, convertedFileName }); + if (!item.convertedScoreName.isEmpty()) { + obj["convertedScoreName"] = item.convertedScoreName.toStdString(); + } + + if (item.scoreId) { + obj["scoreId"] = *item.scoreId; + } + + array << obj; + } + + JsonDocument json(array); + Ret ret = fileSystem()->writeFile(configuration()->pendingConvertsJsonPath(), json.toJson()); + if (!ret) { + LOGE() << "Could not save the pending conversions list: " << ret.toString(); + } +} + +void ConvertFileToScoreService::watch(ConvertType type, int itemId, const muse::String& convertedScoreName) +{ + LOGI() << "Start watching conversion " << convertLogId(convertedScoreName, type, itemId); + + m_watchedItems.push_back(WatchedItem { itemId, type, ConvertStatus::Unknown, std::nullopt, convertedScoreName }); saveWatchedItems(); m_fileNamesBeingConvertedChanged.notify(); @@ -474,15 +549,31 @@ void ConvertFileToScoreService::updateWatchedItems(const ConvertQueueList& queue return queueItem.id == item.id && queueItem.type == item.type; }); - //! NOTE: normally a Done item still reports its status while queued, but it may - //! be dropped from the queue automatically at some point afterwards - treat that as Done too - const ConvertStatus status = found != queue.end() ? found->status : ConvertStatus::Done; - const ConvertErrorCode errorCode = found != queue.end() ? found->errorCode : ConvertErrorCode::Unknown; + ConvertStatus status; + ConvertErrorCode errorCode; + std::optional scoreId; + + if (found != queue.end()) { + status = found->status; + errorCode = found->errorCode; + scoreId = found->scoreId; + } else if (item.scoreId) { + //! NOTE: a Done/AwaitingReview item still reports its status while queued, but it may be + //! dropped from the queue automatically at some point afterwards, once already reported ready + status = ConvertStatus::Done; + errorCode = ConvertErrorCode::Unknown; + scoreId = item.scoreId; + } else { + //! NOTE: dropped from the queue before ever reporting a scoreId - there's no way to + //! identify the resulting score anymore, so it can't be recovered as a success + status = ConvertStatus::Failed; + errorCode = ConvertErrorCode::Unknown; + } - handleItem(item, status, errorCode); + handleItem(item, status, errorCode, scoreId); if (status == ConvertStatus::Failed - || (status == ConvertStatus::Done && !item.downloadedScorePath.empty())) { + || (status == ConvertStatus::Done && item.scoreId)) { it = m_watchedItems.erase(it); } else { ++it; @@ -495,107 +586,25 @@ void ConvertFileToScoreService::updateWatchedItems(const ConvertQueueList& queue } } -void ConvertFileToScoreService::loadWatchedItems() -{ - TRACEFUNC; - - m_watchedItems.clear(); - - RetVal data = fileSystem()->readFile(configuration()->pendingConvertsJsonPath()); - if (!data.ret || data.val.empty()) { - if (!data.ret && data.ret.code() != static_cast(io::Err::FSNotExist)) { - LOGE() << "Could not read the pending conversions file: " << data.ret; - } - return; - } - - std::string err; - const JsonDocument json = JsonDocument::fromJson(data.val, &err); - if (!err.empty() || !json.isArray()) { - if (!err.empty()) { - LOGE() << "Could not parse the pending conversions file: " << err; - } - return; - } - - const JsonArray array = json.rootArray(); - m_watchedItems.reserve(array.size()); - - for (size_t i = 0; i < array.size(); ++i) { - const JsonObject obj = array.at(i).toObject(); - const int itemId = obj.value("id").toInt(); - const ConvertType type = static_cast(obj.value("type").toInt()); - const muse::String convertedFileName = muse::String::fromStdString(obj.value("convertedFileName").toStdString()); - const ConvertStatus convertStatus = static_cast(obj.value("convertStatus").toInt()); - const io::path_t downloadedScorePath = obj.value("downloadedScorePath").toStdString(); - - m_watchedItems.push_back(WatchedItem { itemId, type, convertedFileName, convertStatus, false, downloadedScorePath }); - } -} - -void ConvertFileToScoreService::saveWatchedItems() +void ConvertFileToScoreService::handleItem(WatchedItem& item, ConvertStatus status, ConvertErrorCode errorCode, + std::optional scoreId) { - TRACEFUNC; - - JsonArray array; - for (const WatchedItem& item : m_watchedItems) { - JsonObject obj; - obj["id"] = item.id; - obj["type"] = static_cast(item.type); - obj["convertStatus"] = static_cast(item.convertStatus); - - if (!item.convertedFileName.isEmpty()) { - obj["convertedFileName"] = item.convertedFileName.toStdString(); - } - - if (!item.downloadedScorePath.empty()) { - obj["downloadedScorePath"] = item.downloadedScorePath.toStdString(); - } - - array << obj; - } - - JsonDocument json(array); - Ret ret = fileSystem()->writeFile(configuration()->pendingConvertsJsonPath(), json.toJson()); - if (!ret) { - LOGE() << "Could not save the pending conversions list: " << ret.toString(); - } -} - -std::vector::iterator ConvertFileToScoreService::findWatchedItem(ConvertType type, int itemId) -{ - return std::find_if(m_watchedItems.begin(), m_watchedItems.end(), [type, itemId](const WatchedItem& item) { - return item.type == type && item.id == itemId; - }); -} - -void ConvertFileToScoreService::eraseWatchedItem(ConvertType type, int itemId) -{ - auto it = findWatchedItem(type, itemId); - if (it != m_watchedItems.end()) { - m_watchedItems.erase(it); - } -} - -void ConvertFileToScoreService::handleItem(WatchedItem& item, ConvertStatus status, ConvertErrorCode errorCode) -{ - const bool statusChanged = item.convertStatus != status; + const bool statusChanged = item.status != status; if (statusChanged) { - LOGI() << "Conversion status changed: " << convertLogId(item.convertedFileName, item.type, item.id) + LOGI() << "Conversion status changed: " << convertLogId(item.convertedScoreName, item.type, item.id) << " -> " << convertStatusToString(status); } - item.convertStatus = status; + item.status = status; switch (status) { case ConvertStatus::Processing: case ConvertStatus::Unknown: break; case ConvertStatus::AwaitingReview: - //! NOTE: the MSCZ is already available at this point; the review rating doesn't gate the download - downloadIfNotAlready(item); - break; case ConvertStatus::Done: - downloadIfNotAlready(item); + if (scoreId && !item.scoreId) { + reportReady(item, status, *scoreId); + } break; case ConvertStatus::Failed: { if (!statusChanged) { @@ -603,8 +612,8 @@ void ConvertFileToScoreService::handleItem(WatchedItem& item, ConvertStatus stat } Ret ret = make_ret(Err::ConvertProcessingFailed); - ret.setText("Conversion failed for \"" + item.convertedFileName.toStdString() + "\": " + errorCodeToString(errorCode)); - ret.setData(CONVERT_FAILED_FILE_NAME_KEY, item.convertedFileName); + ret.setText("Conversion failed for \"" + item.convertedScoreName.toStdString() + "\": " + errorCodeToString(errorCode)); + ret.setData(CONVERT_FAILED_FILE_NAME_KEY, item.convertedScoreName); LOGE() << ret.toString(); @@ -614,208 +623,40 @@ void ConvertFileToScoreService::handleItem(WatchedItem& item, ConvertStatus stat } } -void ConvertFileToScoreService::downloadIfNotAlready(WatchedItem& item) -{ - if (item.isDownloading || !item.downloadedScorePath.empty()) { - return; - } - - item.isDownloading = true; - fetchScoreUrlAndDownload(item.type, item.id, item.convertedFileName); -} - -void ConvertFileToScoreService::fetchScoreUrlAndDownload(ConvertType type, int itemId, const muse::String& convertedFileName) -{ - museScoreComService()->convert()->fetchMsczUrl(type, itemId) - .onResolve(this, [this, type, itemId, convertedFileName](const RetVal& urlInfo) { - if (!urlInfo.ret) { - if (isRetryableError(urlInfo.ret)) { - LOGW() << "Could not fetch the converted score " << convertLogId(convertedFileName, type, itemId) - << ", will retry on next poll: " << urlInfo.ret.toString(); - clearDownloading(type, itemId); - return; - } - - Ret ret = urlInfo.ret; - ret.setText("Could not fetch the converted score: " + ret.text()); - failConvert(ret, type, itemId, convertedFileName); - return; - } - - if (urlInfo.val.expiresInSeconds <= 0) { - Ret ret = make_ret(Err::DownloadLinkExpired, std::string("The download link has already expired")); - ret.setData(CONVERT_FAILED_FILE_NAME_KEY, convertedFileName); - LOGW() << "Could not download the converted score " << convertLogId(convertedFileName, type, itemId) - << ": " << ret.toString(); - eraseWatchedItem(type, itemId); - saveWatchedItems(); - m_fileNamesBeingConvertedChanged.notify(); - finishConvert(ret); - return; - } - - downloadScoreAndFinish(type, itemId, convertedFileName, urlInfo.val); - }); -} - -void ConvertFileToScoreService::downloadScoreAndFinish(ConvertType type, int itemId, const muse::String& convertedFileName, - const SignedMsczUrl& urlInfo) -{ - auto scoreData = std::make_shared(); - ProgressPtr progress = museScoreComService()->convert()->downloadConvertedScore(urlInfo, scoreData); - - progress->finished().onReceive(this, [this, type, itemId, convertedFileName, scoreData](const ProgressResult& res) { - if (!res.ret) { - if (isRetryableError(res.ret)) { - LOGW() << "Could not download the converted score " << convertLogId(convertedFileName, type, itemId) - << ", will retry on next poll: " << res.ret.toString(); - clearDownloading(type, itemId); - return; - } - - Ret ret = res.ret; - ret.setText("Could not download the converted score: " + ret.text()); - failConvert(ret, type, itemId, convertedFileName); - return; - } - - if (findWatchedItem(type, itemId) == m_watchedItems.end()) { - //! NOTE: the item was already removed (e.g. reported as Failed) while this download - //! was in progress - discard the result rather than reporting a contradictory outcome - LOGW() << "Conversion " << convertLogId(convertedFileName, type, itemId) - << " was already removed while its download was in progress, discarding the result"; - return; - } - - writeConvertedScore(convertedFileName, scoreData, [this, type, itemId, convertedFileName](const RetVal& writeResult) { - onWriteFinished(type, itemId, convertedFileName, writeResult); - }); - }); -} - -void ConvertFileToScoreService::writeConvertedScore(const muse::String& convertedFileName, const std::shared_ptr& scoreData, - std::function&)> onFinished) +void ConvertFileToScoreService::reportReady(WatchedItem& item, ConvertStatus status, int scoreId) { - const io::path_t dir = configuration()->convertedScoresPath(); - - makePathWithRetry(dir, 0, [this, dir, convertedFileName, scoreData, onFinished](const Ret& ret) { - if (!ret) { - onFinished(RetVal::make_ret(ret)); - return; - } - - const io::path_t baseName = io::escapeFileName(io::path_t(convertedFileName)); - const std::string addition = configuration()->uniqueFileNameAddition(baseName, dir, "mscz"); - const io::path_t path = dir.appendingComponent(baseName + addition).appendingSuffix("mscz"); - - writeFileWithRetry(path, scoreData, 0, [onFinished, path](const Ret& ret) { - onFinished(ret ? RetVal::make_ok(path) : RetVal::make_ret(ret)); - }); - }); -} - -void ConvertFileToScoreService::onWriteFinished(ConvertType type, int itemId, const muse::String& convertedFileName, - const RetVal& writeResult) -{ - if (!writeResult.ret) { - failConvert(writeResult.ret, type, itemId, convertedFileName); + const RetVal scoreInfo = museScoreComService()->downloadScoreInfo(scoreId); + if (!scoreInfo.ret) { + LOGW() << "Could not fetch score info for " << convertLogId(item.convertedScoreName, item.type, item.id) + << ", will retry on next poll: " << scoreInfo.ret.toString(); return; } - //! NOTE: re-lookup rather than reusing an iterator from before the (possibly retried) write - - //! the vector may have changed while the write was in progress - auto watched = findWatchedItem(type, itemId); - if (watched == m_watchedItems.end()) { - LOGW() << "Conversion " << convertLogId(convertedFileName, type, itemId) - << " was already removed while its write was in progress, discarding the result"; - return; - } - - watched->isDownloading = false; - watched->downloadedScorePath = writeResult.val; - const bool requestReview = watched->convertStatus == ConvertStatus::AwaitingReview; - - //! NOTE: keep watching an AwaitingReview item even after it's downloaded, so the - //! review can be re-requested on resume if the app closes before it's submitted - if (watched->convertStatus == ConvertStatus::Done) { - m_watchedItems.erase(watched); - } - - saveWatchedItems(); - - m_fileNamesBeingConvertedChanged.notify(); - finishConvert(make_ok(), writeResult.val); + item.scoreId = scoreId; + finishConvert(make_ok(), scoreInfo.val); - if (requestReview) { - m_reviewRequested.send(type, itemId, writeResult.val); + if (status == ConvertStatus::AwaitingReview) { + m_reviewRequested.send(scoreId); } } -void ConvertFileToScoreService::makePathWithRetry(const io::path_t& dir, int attempt, std::function onFinished) +void ConvertFileToScoreService::finishConvert(const Ret& ret, const ScoreInfo& scoreInfo) { - Ret ret = fileSystem()->makePath(dir); - if (ret) { - onFinished(ret); - return; - } - - if (attempt + 1 == MAX_FS_RETRY_ATTEMPTS) { - LOGE() << "Could not create the directory for converted scores \"" << dir << "\", giving up: " << ret.toString(); - onFinished(ret); - return; - } - - LOGW() << "Could not create the directory for converted scores \"" << dir - << "\", retrying (attempt " << (attempt + 1) << "/" << MAX_FS_RETRY_ATTEMPTS << "): " << ret.toString(); - - QTimer::singleShot(FS_RETRY_INTERVAL_MS, this, [this, dir, attempt, onFinished]() { - makePathWithRetry(dir, attempt + 1, onFinished); - }); + m_convertFinished.send(ret, scoreInfo); } -void ConvertFileToScoreService::writeFileWithRetry(const io::path_t& path, const std::shared_ptr& scoreData, int attempt, - std::function onFinished) +ConvertFileToScoreService::WatchedItem* ConvertFileToScoreService::findWatchedItemByScoreId(int scoreId) { - //! NOTE: a no-copy view - scoreData is kept alive via capture for as long as retries are needed - const ByteArray byteArray = ByteArray::fromQByteArrayNoCopy(scoreData->data()); - - Ret ret = fileSystem()->writeFile(path, byteArray); - if (ret) { - onFinished(ret); - return; - } - - if (attempt + 1 == MAX_FS_RETRY_ATTEMPTS) { - LOGE() << "Could not save the converted score \"" << path << "\", giving up: " << ret.toString(); - onFinished(ret); - return; - } - - LOGW() << "Could not save the converted score \"" << path - << "\", retrying (attempt " << (attempt + 1) << "/" << MAX_FS_RETRY_ATTEMPTS << "): " << ret.toString(); - - QTimer::singleShot(FS_RETRY_INTERVAL_MS, this, [this, path, scoreData, attempt, onFinished]() { - writeFileWithRetry(path, scoreData, attempt + 1, onFinished); + auto it = std::find_if(m_watchedItems.begin(), m_watchedItems.end(), [scoreId](const WatchedItem& item) { + return item.scoreId == scoreId; }); -} -void ConvertFileToScoreService::clearDownloading(ConvertType type, int itemId) -{ - auto it = findWatchedItem(type, itemId); - if (it != m_watchedItems.end()) { - it->isDownloading = false; - } -} - -void ConvertFileToScoreService::finishConvert(const Ret& ret, const io::path_t& path) -{ - m_convertFinished.send(ret, path); + return it != m_watchedItems.end() ? &*it : nullptr; } -void ConvertFileToScoreService::failConvert(Ret ret, ConvertType type, int itemId, const muse::String& convertedFileName) +bool ConvertFileToScoreService::isPending(const WatchedItem& item) { - LOGE() << ret.toString() << " " << convertLogId(convertedFileName, type, itemId); - ret.setData(CONVERT_FAILED_FILE_NAME_KEY, convertedFileName); - clearDownloading(type, itemId); - finishConvert(ret); + //! NOTE: an AwaitingReview item that's already been reported ready is just waiting on the user + //! to submit a review - not "being converted" anymore + return item.status != ConvertStatus::AwaitingReview || !item.scoreId; } diff --git a/src/project/internal/convertfiletoscoreservice.h b/src/project/internal/convertfiletoscoreservice.h index 8e46dc8c7f198..0eef418b2c11f 100644 --- a/src/project/internal/convertfiletoscoreservice.h +++ b/src/project/internal/convertfiletoscoreservice.h @@ -21,24 +21,21 @@ */ #pragma once -#include -#include +#include #include #include #include +#include "project/iconvertfiletoscoreservice.h" + #include "async/asyncable.h" #include "modularity/ioc.h" -#include "io/ifilesystem.h" #include "cloud/musescorecom/imusescorecomservice.h" - -#include "project/iconvertfiletoscoreservice.h" +#include "io/ifilesystem.h" #include "project/iprojectconfiguration.h" -class QBuffer; - namespace mu::project { class ConvertFileToScoreService : public QObject, public IConvertFileToScoreService, public muse::async::Asyncable, public muse::Contextable { @@ -61,8 +58,8 @@ class ConvertFileToScoreService : public QObject, public IConvertFileToScoreServ muse::RetVal validateFiles(const muse::io::paths_t& paths) const override; muse::Ret validateLink(const QUrl& link) const override; - muse::Ret startConvert(const ConvertInput& input, const muse::String& convertedFileName) override; - muse::async::Channel convertFinished() const override; + muse::Ret startConvert(const ConvertInput& input, const muse::String& convertedScoreName) override; + muse::async::Channel convertFinished() const override; muse::StringList fileNamesBeingConverted() const override; muse::async::Notification fileNamesBeingConvertedChanged() const override; @@ -70,67 +67,51 @@ class ConvertFileToScoreService : public QObject, public IConvertFileToScoreServ muse::async::Channel pollingFailed() const override; void retryPolling() override; - muse::async::Channel reviewRequested() const override; - void submitReview(ConvertType type, int itemId, ReviewRating rating, const QString& comment = QString()) override; - void submitReviewComment(ConvertType type, int itemId, const QString& comment) override; + muse::async::Channel reviewRequested() const override; + void submitReview(int scoreId, ReviewRating rating, const QString& comment = QString()) override; + void submitReviewComment(int scoreId, const QString& comment) override; private: static constexpr int MIN_RETRY_INTERVAL_MS = 60000; static constexpr int MAX_RETRY_INTERVAL_MS = 10 * 60000; static constexpr int MAX_POLL_RETRY_ATTEMPTS = 5; // gives up after ~15 minutes - static constexpr int MAX_FS_RETRY_ATTEMPTS = 5; - static constexpr int FS_RETRY_INTERVAL_MS = 100; - struct WatchedItem { int id = 0; ConvertType type = ConvertType::Omr; - muse::String convertedFileName; - muse::cloud::ConvertStatus convertStatus = muse::cloud::ConvertStatus::Unknown; - bool isDownloading = false; - muse::io::path_t downloadedScorePath; + muse::cloud::ConvertStatus status = muse::cloud::ConvertStatus::Unknown; + std::optional scoreId; //! NOTE: set once the score is ready and reported (Done/AwaitingReview) + muse::String convertedScoreName; bool operator==(const WatchedItem& other) const { return id == other.id && type == other.type - && convertedFileName == other.convertedFileName - && convertStatus == other.convertStatus - && isDownloading == other.isDownloading - && downloadedScorePath == other.downloadedScorePath; + && status == other.status + && scoreId == other.scoreId + && convertedScoreName == other.convertedScoreName; } }; - void watch(ConvertType type, int itemId, const muse::String& convertedFileName); + void loadWatchedItems(); + void saveWatchedItems(); + + void watch(ConvertType type, int itemId, const muse::String& convertedScoreName); void poll(); void resetPollState(); void handlePollFailure(const muse::Ret& ret); void giveUpPolling(const muse::Ret& ret); void updateWatchedItems(const muse::cloud::ConvertQueueList& queue); - void loadWatchedItems(); - void saveWatchedItems(); + void handleItem(WatchedItem& item, muse::cloud::ConvertStatus status, muse::cloud::ConvertErrorCode errorCode, + std::optional scoreId); + void reportReady(WatchedItem& item, muse::cloud::ConvertStatus status, int scoreId); - std::vector::iterator findWatchedItem(ConvertType type, int itemId); - void eraseWatchedItem(ConvertType type, int itemId); + void finishConvert(const muse::Ret& ret, const ScoreInfo& scoreInfo = ScoreInfo()); - void handleItem(WatchedItem& item, muse::cloud::ConvertStatus status, muse::cloud::ConvertErrorCode errorCode); - static bool isPending(const WatchedItem& item); + WatchedItem* findWatchedItemByScoreId(int scoreId); - void downloadIfNotAlready(WatchedItem& item); - void fetchScoreUrlAndDownload(ConvertType type, int itemId, const muse::String& convertedFileName); - void downloadScoreAndFinish(ConvertType type, int itemId, const muse::String& convertedFileName, - const muse::cloud::SignedMsczUrl& urlInfo); - void writeConvertedScore(const muse::String& convertedFileName, const std::shared_ptr& scoreData, - std::function&)> onFinished); - void onWriteFinished(ConvertType type, int itemId, const muse::String& convertedFileName, - const muse::RetVal& writeResult); - void makePathWithRetry(const muse::io::path_t& dir, int attempt, std::function onFinished); - void writeFileWithRetry(const muse::io::path_t& path, const std::shared_ptr& scoreData, int attempt, - std::function onFinished); - void clearDownloading(ConvertType type, int itemId); - void finishConvert(const muse::Ret& ret, const muse::io::path_t& path = muse::io::path_t()); - void failConvert(muse::Ret ret, ConvertType type, int itemId, const muse::String& convertedFileName); + static bool isPending(const WatchedItem& item); ConvertConfig m_config; @@ -139,9 +120,10 @@ class ConvertFileToScoreService : public QObject, public IConvertFileToScoreServ int m_pollFailureCount = 0; std::vector m_watchedItems; bool m_pollInProgress = false; - muse::async::Channel m_convertFinished; - muse::async::Channel m_reviewRequested; - muse::async::Notification m_fileNamesBeingConvertedChanged; + muse::async::Channel m_pollingFailed; + muse::async::Notification m_fileNamesBeingConvertedChanged; + muse::async::Channel m_convertFinished; + muse::async::Channel m_reviewRequested; }; } diff --git a/src/project/internal/projectconfiguration.cpp b/src/project/internal/projectconfiguration.cpp index af09743c3f40e..770eda111aa11 100644 --- a/src/project/internal/projectconfiguration.cpp +++ b/src/project/internal/projectconfiguration.cpp @@ -795,11 +795,6 @@ void ProjectConfiguration::setShowConvertFileProcessingDialog(bool show) settings()->setSharedValue(SHOW_CONVERT_FILE_PROCESSING_DIALOG, Val(show)); } -muse::io::path_t ProjectConfiguration::convertedScoresPath() const -{ - return globalConfiguration()->userAppDataPath() + "/converted_scores"; -} - muse::io::path_t ProjectConfiguration::pendingConvertsJsonPath() const { return globalConfiguration()->userAppDataPath().appendingComponent("pending_converts.json"); diff --git a/src/project/internal/projectconfiguration.h b/src/project/internal/projectconfiguration.h index c08da50856ccc..b40082d151010 100644 --- a/src/project/internal/projectconfiguration.h +++ b/src/project/internal/projectconfiguration.h @@ -174,17 +174,16 @@ class ProjectConfiguration : public IProjectConfiguration, public muse::Contexta bool showConvertFileProcessingDialog() const override; void setShowConvertFileProcessingDialog(bool show) override; - muse::io::path_t convertedScoresPath() const override; muse::io::path_t pendingConvertsJsonPath() const override; - std::string uniqueFileNameAddition(const muse::io::path_t& filename, const muse::io::path_t& folderPath, - const std::string& suffix = std::string()) const override; - private: muse::io::path_t appTemplatesPath() const; muse::io::path_t legacyCloudProjectsPath() const; muse::io::path_t cloudProjectsPath() const; + std::string uniqueFileNameAddition(const muse::io::path_t& filename, const muse::io::path_t& folderPath, + const std::string& suffix = std::string()) const; + muse::async::Channel m_userTemplatesPathChanged; muse::async::Channel m_userScoresPathChanged; diff --git a/src/project/iprojectconfiguration.h b/src/project/iprojectconfiguration.h index 35bfb466955c4..abfdf7f2ffe6f 100644 --- a/src/project/iprojectconfiguration.h +++ b/src/project/iprojectconfiguration.h @@ -180,11 +180,7 @@ class IProjectConfiguration : MODULE_GLOBAL_INTERFACE virtual bool showConvertFileProcessingDialog() const = 0; virtual void setShowConvertFileProcessingDialog(bool show) = 0; - virtual muse::io::path_t convertedScoresPath() const = 0; virtual muse::io::path_t pendingConvertsJsonPath() const = 0; - - virtual std::string uniqueFileNameAddition(const muse::io::path_t& filename, const muse::io::path_t& folderPath, - const std::string& suffix = std::string()) const = 0; }; } diff --git a/src/project/projecterrors.h b/src/project/projecterrors.h index ece45a99b273b..cff03a8c351f2 100644 --- a/src/project/projecterrors.h +++ b/src/project/projecterrors.h @@ -54,7 +54,6 @@ enum class Err { ConvertTooManyImages, ConvertUnsupportedLink, ConvertProcessingFailed, - DownloadLinkExpired, }; //! NOTE: key for the converted file name stored in Ret::data diff --git a/src/project/qml/MuseScore/Project/ConvertFileToScoreDialog.qml b/src/project/qml/MuseScore/Project/ConvertFileToScoreDialog.qml index 8073adbe2cac5..ee89bab956a6b 100644 --- a/src/project/qml/MuseScore/Project/ConvertFileToScoreDialog.qml +++ b/src/project/qml/MuseScore/Project/ConvertFileToScoreDialog.qml @@ -72,9 +72,9 @@ StyledDialogView { } } - function finish(type, paths, link, convertedFileName) { + function finish(type, paths, link, convertedScoreName) { root.skipCloseConfirmation = true - root.ret = { errcode: 0, value: { type: type, paths: paths, link: link, convertedFileName: convertedFileName } } + root.ret = { errcode: 0, value: { type: type, paths: paths, link: link, convertedScoreName: convertedScoreName } } root.hide() } @@ -194,8 +194,8 @@ StyledDialogView { onBackRequested: convertModel.confirmGoingBack() - onConvertRequested: function(paths, convertedFileName) { - root.finish(convertModel.convertType, paths, "", convertedFileName) + onConvertRequested: function(paths, convertedScoreName) { + root.finish(convertModel.convertType, paths, "", convertedScoreName) } onSelectMoreFilesRequested: function(existingPaths) { @@ -220,13 +220,13 @@ StyledDialogView { onBackRequested: convertModel.confirmGoingBack() - onConvertRequested: function(link, convertedFileName) { + onConvertRequested: function(link, convertedScoreName) { if (!convertModel.validateLink(link)) { return } convertModel.selectedLink = link - root.finish(convertModel.convertType, [], link, convertedFileName) + root.finish(convertModel.convertType, [], link, convertedScoreName) } } } diff --git a/src/project/qml/MuseScore/Project/internal/ConvertFileToScore/LinkEntryPage.qml b/src/project/qml/MuseScore/Project/internal/ConvertFileToScore/LinkEntryPage.qml index f197c8d5fd3fc..f5d712a062fe7 100644 --- a/src/project/qml/MuseScore/Project/internal/ConvertFileToScore/LinkEntryPage.qml +++ b/src/project/qml/MuseScore/Project/internal/ConvertFileToScore/LinkEntryPage.qml @@ -42,7 +42,7 @@ Item { signal cancelRequested() signal backRequested() - signal convertRequested(string link, string convertedFileName) + signal convertRequested(string link, string convertedScoreName) function focusOnDefault() { linkInputField.navigation.requestActive() diff --git a/src/project/qml/MuseScore/Project/internal/ConvertFileToScore/SelectedFilesPage.qml b/src/project/qml/MuseScore/Project/internal/ConvertFileToScore/SelectedFilesPage.qml index 3f700a21d6d73..2402330683647 100644 --- a/src/project/qml/MuseScore/Project/internal/ConvertFileToScore/SelectedFilesPage.qml +++ b/src/project/qml/MuseScore/Project/internal/ConvertFileToScore/SelectedFilesPage.qml @@ -43,7 +43,7 @@ Item { signal cancelRequested() signal backRequested() signal selectMoreFilesRequested(var existingPaths) - signal convertRequested(var paths, string convertedFileName) + signal convertRequested(var paths, string convertedScoreName) function focusOnDefault() { saveAsField.focusOnInput() diff --git a/src/project/qml/MuseScore/Project/internal/ScoresPage/recentscoresmodel.cpp b/src/project/qml/MuseScore/Project/internal/ScoresPage/recentscoresmodel.cpp index 5c3e5661c59fe..95241e884de6d 100644 --- a/src/project/qml/MuseScore/Project/internal/ScoresPage/recentscoresmodel.cpp +++ b/src/project/qml/MuseScore/Project/internal/ScoresPage/recentscoresmodel.cpp @@ -48,12 +48,6 @@ void RecentScoresModel::load() convertFileToScoreService()->fileNamesBeingConvertedChanged().onNotify(this, [this]() { updateRecentScores(); }); - - convertFileToScoreService()->convertFinished().onReceive(this, [this](const Ret& ret, const io::path_t& path) { - if (ret) { - recentFilesController()->prependRecentFile(RecentFile(path)); - } - }); } void RecentScoresModel::setRecentScores(const std::vector& items) diff --git a/src/project/tests/convertfiletoscorescenario_tests.cpp b/src/project/tests/convertfiletoscorescenario_tests.cpp index b492654219880..0df247ba8501f 100644 --- a/src/project/tests/convertfiletoscorescenario_tests.cpp +++ b/src/project/tests/convertfiletoscorescenario_tests.cpp @@ -144,7 +144,7 @@ QVariantMap pickedOmrFileSelection() return { { "type", int(ConvertType::Omr) }, { "paths", QStringList { "/some/file.xyz" } }, - { "convertedFileName", QString("file") } + { "convertedScoreName", QString("file") } }; } } @@ -251,13 +251,16 @@ class Project_ConvertFileToScoreScenarioTest : public ::testing::Test TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Success_ShowsScoreReadyNotificationAndForwards) { // [GIVEN] The service's channels, wired up via init() - async::Channel convertFinished; - async::Channel reviewRequested; + async::Channel convertFinished; + async::Channel reviewRequested; ON_CALL(*m_service, convertFinished()).WillByDefault(Return(convertFinished)); ON_CALL(*m_service, reviewRequested()).WillByDefault(Return(reviewRequested)); m_scenario->init(); - const io::path_t path = "/some/path/My Score.xyz"; + ScoreInfo scoreInfo; + scoreInfo.id = 555; + scoreInfo.title = "My Score"; + constexpr int openScoreBtn = int(toast::ToastActionCode::Custom) + 1; const std::string title = muse::trc("project/convert", "Your score is ready!"); @@ -275,24 +278,55 @@ TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Success_ShowsScoreReadyNotif bool forwarded = false; Ret forwardedRet; - m_scenario->convertFinished().onReceive(nullptr, [&](const Ret& ret, const io::path_t&) { + m_scenario->convertFinished().onReceive(nullptr, [&](const Ret& ret, const ScoreInfo&) { forwarded = true; forwardedRet = ret; }); // [WHEN] The service reports a successful conversion - convertFinished.send(make_ok(), path); + convertFinished.send(make_ok(), scoreInfo); // [THEN] The result is forwarded to the scenario's own convertFinished channel EXPECT_TRUE(forwarded); EXPECT_TRUE(forwardedRet); } +TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Success_OpenScoreButton_DispatchesOpenScoreUrl) +{ + // [GIVEN] The service's channels, wired up via init() + async::Channel convertFinished; + async::Channel reviewRequested; + ON_CALL(*m_service, convertFinished()).WillByDefault(Return(convertFinished)); + ON_CALL(*m_service, reviewRequested()).WillByDefault(Return(reviewRequested)); + m_scenario->init(); + + ScoreInfo scoreInfo; + scoreInfo.id = 555; + scoreInfo.title = "My Score"; + + // [GIVEN] The user clicks "Open score" on the ready notification + constexpr int openScoreBtn = int(toast::ToastActionCode::Custom) + 1; + ON_CALL(*m_toastService, show(_, _, _, _, _)) + .WillByDefault(Invoke([](auto&&...) { + return resolvedToastResultPromise(toast::ToastResult(openScoreBtn)); + })); + + // [THEN] The score is opened via the cloud open-score URL, not a local path + EXPECT_CALL(*m_dispatcher, dispatch(actions::ActionCode("file-open"), Truly([](const actions::ActionData& data) { + return data.arg(0) == QUrl("musescore://open-score/555"); + }))) + .Times(1); + + // [WHEN] The service reports a successful conversion + convertFinished.send(make_ok(), scoreInfo); + pumpEvents(); +} + TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Failure_ShowsConvertFailedNotificationAndForwards) { // [GIVEN] The service's channels, wired up via init() - async::Channel convertFinished; - async::Channel reviewRequested; + async::Channel convertFinished; + async::Channel reviewRequested; ON_CALL(*m_service, convertFinished()).WillByDefault(Return(convertFinished)); ON_CALL(*m_service, reviewRequested()).WillByDefault(Return(reviewRequested)); m_scenario->init(); @@ -315,13 +349,13 @@ TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Failure_ShowsConvertFailedNo bool forwarded = false; Ret forwardedRet; - m_scenario->convertFinished().onReceive(nullptr, [&](const Ret& r, const io::path_t&) { + m_scenario->convertFinished().onReceive(nullptr, [&](const Ret& r, const ScoreInfo&) { forwarded = true; forwardedRet = r; }); // [WHEN] The service reports a failed conversion - convertFinished.send(ret, io::path_t()); + convertFinished.send(ret, ScoreInfo()); // [THEN] The failure is still forwarded to the scenario's own convertFinished channel EXPECT_TRUE(forwarded); @@ -331,8 +365,8 @@ TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Failure_ShowsConvertFailedNo TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Failure_TryAgain_RestartsConvert) { // [GIVEN] The service's channels, wired up via init() - async::Channel convertFinished; - async::Channel reviewRequested; + async::Channel convertFinished; + async::Channel reviewRequested; ON_CALL(*m_service, convertFinished()).WillByDefault(Return(convertFinished)); ON_CALL(*m_service, reviewRequested()).WillByDefault(Return(reviewRequested)); m_scenario->init(); @@ -347,7 +381,7 @@ TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Failure_TryAgain_RestartsCon const QVariantMap selectionMap { { "type", int(ConvertType::Omr) }, { "paths", QStringList { "/some/file.xyz" } }, - { "convertedFileName", QString("file") } + { "convertedScoreName", QString("file") } }; ON_CALL(*m_interactive, open(UriQuery("musescore://project/convert/selectfiles"))) .WillByDefault(Invoke([selectionMap](auto&&...) { @@ -363,7 +397,7 @@ TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Failure_TryAgain_RestartsCon // [WHEN] The service reports a failed conversion Ret ret = make_ret(Err::ConvertProcessingFailed); ret.setData(CONVERT_FAILED_FILE_NAME_KEY, muse::String(u"My Score")); - convertFinished.send(ret, io::path_t()); + convertFinished.send(ret, ScoreInfo()); pumpEvents(); } @@ -371,8 +405,8 @@ TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Failure_TryAgain_RestartsCon TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Failure_Dismiss_DoesNotRestartConvert) { // [GIVEN] The service's channels, wired up via init() - async::Channel convertFinished; - async::Channel reviewRequested; + async::Channel convertFinished; + async::Channel reviewRequested; ON_CALL(*m_service, convertFinished()).WillByDefault(Return(convertFinished)); ON_CALL(*m_service, reviewRequested()).WillByDefault(Return(reviewRequested)); m_scenario->init(); @@ -388,7 +422,7 @@ TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Failure_Dismiss_DoesNotResta EXPECT_CALL(*m_service, startConvert(_, _)).Times(0); // [WHEN] The service reports a failed conversion - convertFinished.send(make_ret(Err::ConvertProcessingFailed), io::path_t()); + convertFinished.send(make_ret(Err::ConvertProcessingFailed), ScoreInfo()); pumpEvents(); } @@ -396,8 +430,8 @@ TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Failure_Dismiss_DoesNotResta TEST_F(Project_ConvertFileToScoreScenarioTest, DISABLED_Init_ReviewRequested_Good_SubmitsGoodRating) { // [GIVEN] The service's channels, wired up via init() - async::Channel convertFinished; - async::Channel reviewRequested; + async::Channel convertFinished; + async::Channel reviewRequested; ON_CALL(*m_service, convertFinished()).WillByDefault(Return(convertFinished)); ON_CALL(*m_service, reviewRequested()).WillByDefault(Return(reviewRequested)); m_scenario->init(); @@ -416,11 +450,11 @@ TEST_F(Project_ConvertFileToScoreScenarioTest, DISABLED_Init_ReviewRequested_Goo return resolvedToastResultPromise(toast::ToastResult(goodBtn)); })); - EXPECT_CALL(*m_service, submitReview(ConvertType::Omr, 42, ReviewRating::Good, QString())) + EXPECT_CALL(*m_service, submitReview(555, ReviewRating::Good, QString())) .Times(1); // [WHEN] The service requests a review for a finished conversion - reviewRequested.send(ConvertType::Omr, 42, io::path_t("/some/path/My Score.mscz")); + reviewRequested.send(555); pumpEvents(); } @@ -428,8 +462,8 @@ TEST_F(Project_ConvertFileToScoreScenarioTest, DISABLED_Init_ReviewRequested_Goo TEST_F(Project_ConvertFileToScoreScenarioTest, DISABLED_Init_ReviewRequested_Bad_SubmitsBadRating) { // [GIVEN] The service's channels, wired up via init() - async::Channel convertFinished; - async::Channel reviewRequested; + async::Channel convertFinished; + async::Channel reviewRequested; ON_CALL(*m_service, convertFinished()).WillByDefault(Return(convertFinished)); ON_CALL(*m_service, reviewRequested()).WillByDefault(Return(reviewRequested)); m_scenario->init(); @@ -448,11 +482,11 @@ TEST_F(Project_ConvertFileToScoreScenarioTest, DISABLED_Init_ReviewRequested_Bad return resolvedToastResultPromise(toast::ToastResult(badBtn)); })); - EXPECT_CALL(*m_service, submitReview(ConvertType::Audio2Score, 7, ReviewRating::Bad, QString())) + EXPECT_CALL(*m_service, submitReview(555, ReviewRating::Bad, QString())) .Times(1); // [WHEN] The service requests a review for a finished conversion - reviewRequested.send(ConvertType::Audio2Score, 7, io::path_t("/some/path/My Score.mscz")); + reviewRequested.send(555); pumpEvents(); } @@ -1068,7 +1102,7 @@ TEST_F(Project_ConvertFileToScoreScenarioTest, ConvertFiles_Proceeds_StartsOmrCo const QVariantMap selectionMap { { "type", int(ConvertType::Omr) }, { "paths", QStringList { "/some/path/file.xyz" } }, - { "convertedFileName", QString("file") } + { "convertedScoreName", QString("file") } }; EXPECT_CALL(*m_interactive, open(expectedQuery)) .WillOnce(Invoke([selectionMap](auto&&...) { @@ -1117,7 +1151,7 @@ TEST_F(Project_ConvertFileToScoreScenarioTest, ConvertFiles_Proceeds_StartsAudio const QVariantMap selectionMap { { "type", int(ConvertType::Audio2Score) }, { "paths", QStringList { "/some/path/song.xyz" } }, - { "convertedFileName", QString("song") } + { "convertedScoreName", QString("song") } }; EXPECT_CALL(*m_interactive, open(expectedQuery)) .WillOnce(Invoke([selectionMap](auto&&...) { diff --git a/src/project/tests/convertfiletoscoreservice_tests.cpp b/src/project/tests/convertfiletoscoreservice_tests.cpp index 58a4b72bb9302..7b610e3faeda1 100644 --- a/src/project/tests/convertfiletoscoreservice_tests.cpp +++ b/src/project/tests/convertfiletoscoreservice_tests.cpp @@ -21,11 +21,10 @@ */ #include -#include -#include +#include #include +#include -#include #include #include "project/internal/convertfiletoscoreservice.h" @@ -40,7 +39,6 @@ #include "global/types/val.h" #include "global/types/bytearray.h" #include "global/serialization/json.h" -#include "global/io/ioretcodes.h" #include "mocks/projectconfigurationmock.h" #include "global/tests/mocks/filesystemmock.h" @@ -63,17 +61,6 @@ void pumpEvents(int iterations = 10) } } -//! NOTE: FS write/makePath retries are scheduled via QTimer::singleShot, which (unlike -//! pumpEvents() above) needs the real Qt event loop pumped and real time to actually pass -void waitUntil(const std::function& pred, int timeoutMs = 2000) -{ - const std::chrono::steady_clock::time_point deadline = std::chrono::steady_clock::now() + std::chrono::milliseconds(timeoutMs); - while (!pred() && std::chrono::steady_clock::now() < deadline) { - QCoreApplication::processEvents(); - std::this_thread::sleep_for(std::chrono::milliseconds(5)); - } -} - //! NOTE: keep the body queued, don't run it eagerly //! otherwise resolve() can fire before onResolve/onReject are attached template @@ -85,6 +72,14 @@ async::Promise resolvedPromise(const T& val) }); } +RetVal okScoreInfo(int scoreId, const QString& title = "My Score") +{ + ScoreInfo info; + info.id = scoreId; + info.title = title; + return RetVal::make_ok(info); +} + //! NOTE: the hardcoded values ConvertFileToScoreService::init() falls back to void expectFallbackConfig(const ConvertConfig& config) { @@ -159,7 +154,7 @@ class Project_ConvertFileToScoreServiceTest : public ::testing::Test //! NOTE: uploads the given file, resolves the upload with queueId, and lets the resulting //! poll (mocked to return queueList) run to completion - void deliverQueueStatus(const ConvertQueueList& queueList, ConvertType type, int queueId, const QString& convertedFileName) + void deliverQueueStatus(const ConvertQueueList& queueList, ConvertType type, int queueId, const QString& convertedScoreName) { ON_CALL(*m_convertService, fetchQueue()) .WillByDefault(Invoke([queueList] { @@ -175,7 +170,7 @@ class Project_ConvertFileToScoreServiceTest : public ::testing::Test ? ConvertInput(OmrConvertInput { paths }) : ConvertInput(Audio2ScoreConvertInput { paths }); - m_service->startConvert(input, convertedFileName); + m_service->startConvert(input, convertedScoreName); uploadProgress->finish(ProgressResult::make_ok(Val(ValMap { { "id", Val(queueId) } }))); pumpEvents(); @@ -184,7 +179,7 @@ class Project_ConvertFileToScoreServiceTest : public ::testing::Test //! NOTE: uploads the given file and resolves with queueId, triggering a fresh poll //! (watch() always re-polls all watched items) without touching the fetchQueue mock, //! which the caller owns - lets a test drive N polls without waiting on the real QTimer - void uploadAndResolve(int queueId, const QString& convertedFileName, const io::paths_t& paths) + void uploadAndResolve(int queueId, const QString& convertedScoreName, const io::paths_t& paths) { auto uploadProgress = std::make_shared(); EXPECT_CALL(*m_convertService, upload(Truly([paths](const ConvertInput& input) { @@ -192,7 +187,7 @@ class Project_ConvertFileToScoreServiceTest : public ::testing::Test }))) .WillOnce(Return(uploadProgress)); - m_service->startConvert(OmrConvertInput { paths }, convertedFileName); + m_service->startConvert(OmrConvertInput { paths }, convertedScoreName); uploadProgress->finish(ProgressResult::make_ok(Val(ValMap { { "id", Val(queueId) } }))); pumpEvents(); @@ -562,7 +557,7 @@ TEST_F(Project_ConvertFileToScoreServiceTest, StartConvert_UploadFails_ForwardsF bool received = false; Ret receivedRet; - m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const io::path_t&) { + m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const ScoreInfo&) { received = true; receivedRet = ret; }); @@ -609,7 +604,7 @@ TEST_F(Project_ConvertFileToScoreServiceTest, StartConvert_UploadSucceeds_Persis JsonObject obj = json.rootArray().at(0).toObject(); savedExpectedEntry = obj.value("id").toInt() == TEST_QUEUE_ID && obj.value("type").toInt() == int(ConvertType::Omr) - && obj.value("convertedFileName").toStdString() == "My Score"; + && obj.value("convertedScoreName").toStdString() == "My Score"; } return make_ok(); })); @@ -699,49 +694,27 @@ TEST_F(Project_ConvertFileToScoreServiceTest, FileNamesBeingConverted_AfterStart EXPECT_EQ(m_service->fileNamesBeingConverted().front(), u"My Score"); } -TEST_F(Project_ConvertFileToScoreServiceTest, FileNamesBeingConverted_AfterSuccessfulDownload_NoLongerContainsFileName) +TEST_F(Project_ConvertFileToScoreServiceTest, FileNamesBeingConverted_AfterDone_NoLongerContainsFileName) { - // [GIVEN] The queue reports the conversion as done, and the download succeeds + // [GIVEN] The queue reports the conversion as done, with its scoreId ConvertQueueItem item; item.id = TEST_QUEUE_ID; item.type = ConvertType::Omr; item.status = ConvertStatus::Done; + item.scoreId = 555; - ON_CALL(*m_convertService, fetchMsczUrl(ConvertType::Omr, TEST_QUEUE_ID)) - .WillByDefault(Invoke([] { - SignedMsczUrl url; - url.url = QUrl("https://link.xyz/score.mscz"); - url.expiresInSeconds = 60; - return resolvedPromise >(RetVal::make_ok(url)); - })); - - auto downloadProgress = std::make_shared(); - ON_CALL(*m_convertService, downloadConvertedScore(_, _)) - .WillByDefault(Return(downloadProgress)); - - ON_CALL(*m_fileSystem, makePath(_)) - .WillByDefault(Return(make_ok())); - ON_CALL(*m_fileSystem, writeFile(_, _)) - .WillByDefault(Return(make_ok())); - ON_CALL(*m_configuration, convertedScoresPath()) - .WillByDefault(Return(io::path_t("/scores"))); - ON_CALL(*m_configuration, uniqueFileNameAddition(_, _, _)) - .WillByDefault(Return(std::string())); - - // [WHEN] Uploading and polling the status - deliverQueueStatus({ item }, ConvertType::Omr, TEST_QUEUE_ID, "My Score"); - - ASSERT_EQ(m_service->fileNamesBeingConverted().size(), 1u); + ON_CALL(*m_museScoreComService, downloadScoreInfo(555)) + .WillByDefault(Invoke([] { return okScoreInfo(555); })); bool changed = false; m_service->fileNamesBeingConvertedChanged().onNotify(nullptr, [&] { changed = true; }); - // [AND WHEN] The download completes - downloadProgress->finish(ProgressResult::make_ok(Val())); + // [WHEN] Uploading and polling the status + deliverQueueStatus({ item }, ConvertType::Omr, TEST_QUEUE_ID, "My Score"); - // [THEN] The file is no longer reported as being converted + // [THEN] The item is reported ready and immediately erased, so it's no longer being converted EXPECT_TRUE(changed); EXPECT_TRUE(m_service->fileNamesBeingConverted().empty()); } @@ -756,7 +729,7 @@ TEST_F(Project_ConvertFileToScoreServiceTest, ResumeConvert_LoadsPersistedWatche JsonObject obj; obj["id"] = TEST_QUEUE_ID; obj["type"] = int(ConvertType::Audio2Score); - obj["convertedFileName"] = "My Score"; + obj["convertedScoreName"] = "My Score"; JsonArray array; array << obj; @@ -786,15 +759,15 @@ TEST_F(Project_ConvertFileToScoreServiceTest, ResumeConvert_LoadsPersistedWatche EXPECT_EQ(m_service->fileNamesBeingConverted().front(), u"My Score"); } -TEST_F(Project_ConvertFileToScoreServiceTest, ResumeConvert_AwaitingReviewItemAlreadyDownloaded_SendsReviewRequested) +TEST_F(Project_ConvertFileToScoreServiceTest, ResumeConvert_AwaitingReviewItemWithScoreId_SendsReviewRequested) { - // [GIVEN] A persisted item that was already downloaded and awaiting review before the app closed + // [GIVEN] A persisted item that was already reported ready and awaiting review before the app closed JsonObject obj; obj["id"] = TEST_QUEUE_ID; obj["type"] = int(ConvertType::Omr); - obj["convertStatus"] = int(ConvertStatus::AwaitingReview); - obj["convertedFileName"] = "My Score"; - obj["downloadedScorePath"] = "/scores/My Score.mscz"; + obj["status"] = int(ConvertStatus::AwaitingReview); + obj["convertedScoreName"] = "My Score"; + obj["scoreId"] = 555; JsonArray array; array << obj; @@ -810,38 +783,29 @@ TEST_F(Project_ConvertFileToScoreServiceTest, ResumeConvert_AwaitingReviewItemAl return pendingPromise >(); })); - // [THEN] No download is attempted - the score is already on disk - EXPECT_CALL(*m_convertService, fetchMsczUrl(_, _)).Times(0); - bool reviewRequested = false; - ConvertType reviewType = ConvertType::Audio2Score; - int reviewQueueId = 0; - io::path_t reviewPath; - m_service->reviewRequested().onReceive(nullptr, [&](ConvertType type, int queueId, const io::path_t& path) { + int reviewScoreId = 0; + m_service->reviewRequested().onReceive(nullptr, [&](int scoreId) { reviewRequested = true; - reviewType = type; - reviewQueueId = queueId; - reviewPath = path; + reviewScoreId = scoreId; }); // [WHEN] Resuming m_service->resumeConvert(); - // [THEN] The review is requested immediately, carrying the previously downloaded score's path + // [THEN] The review is requested immediately, carrying the previously reported scoreId ASSERT_TRUE(reviewRequested); - EXPECT_EQ(reviewType, ConvertType::Omr); - EXPECT_EQ(reviewQueueId, TEST_QUEUE_ID); - EXPECT_EQ(reviewPath, io::path_t("/scores/My Score.mscz")); + EXPECT_EQ(reviewScoreId, 555); } -TEST_F(Project_ConvertFileToScoreServiceTest, ResumeConvert_AwaitingReviewItemNotYetDownloaded_DoesNotSendReviewRequested) +TEST_F(Project_ConvertFileToScoreServiceTest, ResumeConvert_AwaitingReviewItemWithoutScoreId_DoesNotSendReviewRequested) { - // [GIVEN] A persisted item that was awaiting review, but never got a chance to download before the app closed + // [GIVEN] A persisted item that was awaiting review, but never got a chance to report a scoreId before the app closed JsonObject obj; obj["id"] = TEST_QUEUE_ID; obj["type"] = int(ConvertType::Omr); - obj["convertStatus"] = int(ConvertStatus::AwaitingReview); - obj["convertedFileName"] = "My Score"; + obj["status"] = int(ConvertStatus::AwaitingReview); + obj["convertedScoreName"] = "My Score"; JsonArray array; array << obj; @@ -858,400 +822,182 @@ TEST_F(Project_ConvertFileToScoreServiceTest, ResumeConvert_AwaitingReviewItemNo })); bool reviewRequested = false; - m_service->reviewRequested().onReceive(nullptr, [&](ConvertType, int, const io::path_t&) { + m_service->reviewRequested().onReceive(nullptr, [&](int) { reviewRequested = true; }); // [WHEN] Resuming m_service->resumeConvert(); - // [THEN] No review is requested yet - there's no downloaded score to review + // [THEN] No review is requested yet - there's no scoreId to identify the score by EXPECT_FALSE(reviewRequested); } // ================================================== -// polling / download pipeline (via startConvert()) +// polling / score info fetch pipeline // ================================================== -TEST_F(Project_ConvertFileToScoreServiceTest, Poll_DoneStatus_DownloadsAndFinishesWithPath) +TEST_F(Project_ConvertFileToScoreServiceTest, Poll_DoneStatus_FetchesScoreInfoAndFinishes) { - // [GIVEN] The queue reports the conversion as done + // [GIVEN] The queue reports the conversion as done, with its scoreId ConvertQueueItem item; item.id = TEST_QUEUE_ID; item.type = ConvertType::Omr; item.status = ConvertStatus::Done; + item.scoreId = 555; - ON_CALL(*m_convertService, fetchMsczUrl(ConvertType::Omr, TEST_QUEUE_ID)) - .WillByDefault(Invoke([] { - SignedMsczUrl url; - url.url = QUrl("https://link.xyz/score.mscz"); - url.expiresInSeconds = 60; - return resolvedPromise >(RetVal::make_ok(url)); - })); - - auto downloadProgress = std::make_shared(); - ON_CALL(*m_convertService, downloadConvertedScore(_, _)) - .WillByDefault(Return(downloadProgress)); - - ON_CALL(*m_fileSystem, makePath(_)) - .WillByDefault(Return(make_ok())); - ON_CALL(*m_fileSystem, writeFile(_, _)) - .WillByDefault(Return(make_ok())); - ON_CALL(*m_configuration, convertedScoresPath()) - .WillByDefault(Return(io::path_t("/scores"))); - ON_CALL(*m_configuration, uniqueFileNameAddition(_, _, _)) - .WillByDefault(Return(std::string())); + ON_CALL(*m_museScoreComService, downloadScoreInfo(555)) + .WillByDefault(Invoke([] { return okScoreInfo(555, "My Score"); })); bool received = false; Ret receivedRet; - io::path_t receivedPath; - m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const io::path_t& path) { + ScoreInfo receivedInfo; + m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const ScoreInfo& info) { received = true; receivedRet = ret; - receivedPath = path; + receivedInfo = info; }); // [WHEN] Uploading and polling the status deliverQueueStatus({ item }, ConvertType::Omr, TEST_QUEUE_ID, "My Score"); - // [AND WHEN] The download completes - downloadProgress->finish(ProgressResult::make_ok(Val())); - - // [THEN] The conversion finishes successfully, with the downloaded score's path - ASSERT_TRUE(received); - EXPECT_TRUE(receivedRet); - EXPECT_TRUE(receivedPath.hasSuffix("mscz")); - EXPECT_NE(receivedPath.toStdString().find("scores"), std::string::npos); -} - -TEST_F(Project_ConvertFileToScoreServiceTest, Download_WriteFileFailsThenSucceeds_RetriesAndFinishesSuccessfully) -{ - // [GIVEN] The queue reports the conversion as done - ConvertQueueItem item; - item.id = TEST_QUEUE_ID; - item.type = ConvertType::Omr; - item.status = ConvertStatus::Done; - - ON_CALL(*m_convertService, fetchMsczUrl(ConvertType::Omr, TEST_QUEUE_ID)) - .WillByDefault(Invoke([] { - SignedMsczUrl url; - url.url = QUrl("https://link.xyz/score.mscz"); - url.expiresInSeconds = 60; - return resolvedPromise >(RetVal::make_ok(url)); - })); - - auto downloadProgress = std::make_shared(); - ON_CALL(*m_convertService, downloadConvertedScore(_, _)) - .WillByDefault(Return(downloadProgress)); - - ON_CALL(*m_fileSystem, makePath(_)) - .WillByDefault(Return(make_ok())); - - // [GIVEN] saveWatchedItems() persists to its own file, unrelated to the converted score itself - ON_CALL(*m_configuration, pendingConvertsJsonPath()) - .WillByDefault(Return(io::path_t("/pending.json"))); - EXPECT_CALL(*m_fileSystem, writeFile(io::path_t("/pending.json"), _)) - .Times(AnyNumber()) - .WillRepeatedly(Return(make_ok())); - - ON_CALL(*m_configuration, convertedScoresPath()) - .WillByDefault(Return(io::path_t("/scores"))); - ON_CALL(*m_configuration, uniqueFileNameAddition(_, _, _)) - .WillByDefault(Return(std::string())); - - // [GIVEN] Writing the file fails twice with a transient FS error, then succeeds - EXPECT_CALL(*m_fileSystem, writeFile(Truly([](const io::path_t& path) { return path.hasSuffix("mscz"); }), _)) - .Times(3) - .WillOnce(Return(make_ret(io::Err::FSWriteError))) - .WillOnce(Return(make_ret(io::Err::FSWriteError))) - .WillOnce(Return(make_ok())); - - bool received = false; - Ret receivedRet; - m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const io::path_t&) { - received = true; - receivedRet = ret; - }); - - // [WHEN] Uploading and polling the status, then letting the download complete - deliverQueueStatus({ item }, ConvertType::Omr, TEST_QUEUE_ID, "My Score"); - downloadProgress->finish(ProgressResult::make_ok(Val())); - - // [THEN] The transient FS failures are retried in place (via a QTimer, hence the wait), and the - // conversion still finishes successfully - waitUntil([&] { return received; }); + // [THEN] The conversion finishes successfully, carrying the score's info ASSERT_TRUE(received); EXPECT_TRUE(receivedRet); + EXPECT_EQ(receivedInfo.id, 555); + EXPECT_EQ(receivedInfo.title, "My Score"); } -TEST_F(Project_ConvertFileToScoreServiceTest, Download_WriteFileFailsPermanently_FailsConversionAfterMaxRetries) +TEST_F(Project_ConvertFileToScoreServiceTest, Poll_AwaitingReviewWithoutScoreId_DoesNotReportYet) { - // [GIVEN] The queue reports the conversion as done - ConvertQueueItem item; - item.id = TEST_QUEUE_ID; - item.type = ConvertType::Omr; - item.status = ConvertStatus::Done; - - ON_CALL(*m_convertService, fetchMsczUrl(ConvertType::Omr, TEST_QUEUE_ID)) - .WillByDefault(Invoke([] { - SignedMsczUrl url; - url.url = QUrl("https://link.xyz/score.mscz"); - url.expiresInSeconds = 60; - return resolvedPromise >(RetVal::make_ok(url)); - })); - - auto downloadProgress = std::make_shared(); - ON_CALL(*m_convertService, downloadConvertedScore(_, _)) - .WillByDefault(Return(downloadProgress)); - - ON_CALL(*m_fileSystem, makePath(_)) - .WillByDefault(Return(make_ok())); - - // [GIVEN] saveWatchedItems() persists to its own file, unrelated to the converted score itself - ON_CALL(*m_configuration, pendingConvertsJsonPath()) - .WillByDefault(Return(io::path_t("/pending.json"))); - EXPECT_CALL(*m_fileSystem, writeFile(io::path_t("/pending.json"), _)) - .Times(AnyNumber()) - .WillRepeatedly(Return(make_ok())); - - ON_CALL(*m_configuration, convertedScoresPath()) - .WillByDefault(Return(io::path_t("/scores"))); - ON_CALL(*m_configuration, uniqueFileNameAddition(_, _, _)) - .WillByDefault(Return(std::string())); - - // [GIVEN] Writing the file always fails. MAX_FS_RETRY_ATTEMPTS (see convertfiletoscoreservice.h) - // is 5, so the 5th attempt should be the last one - const int maxAttempts = 5; - EXPECT_CALL(*m_fileSystem, writeFile(Truly([](const io::path_t& path) { return path.hasSuffix("mscz"); }), _)) - .Times(maxAttempts) - .WillRepeatedly(Return(make_ret(io::Err::FSWriteError))); - - bool received = false; - Ret receivedRet; - m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const io::path_t&) { - received = true; - receivedRet = ret; - }); - - // [WHEN] Uploading and polling the status, then letting the download complete - deliverQueueStatus({ item }, ConvertType::Omr, TEST_QUEUE_ID, "My Score"); - downloadProgress->finish(ProgressResult::make_ok(Val())); - - // [THEN] The conversion is reported as failed once the retries (via QTimer, hence the wait) are exhausted - waitUntil([&] { return received; }); - ASSERT_TRUE(received); - EXPECT_FALSE(receivedRet); -} - -TEST_F(Project_ConvertFileToScoreServiceTest, Download_MakePathFailsPermanently_FailsConversionWithoutWritingFile) -{ - // [GIVEN] The queue reports the conversion as done - ConvertQueueItem item; - item.id = TEST_QUEUE_ID; - item.type = ConvertType::Omr; - item.status = ConvertStatus::Done; - - ON_CALL(*m_convertService, fetchMsczUrl(ConvertType::Omr, TEST_QUEUE_ID)) - .WillByDefault(Invoke([] { - SignedMsczUrl url; - url.url = QUrl("https://link.xyz/score.mscz"); - url.expiresInSeconds = 60; - return resolvedPromise >(RetVal::make_ok(url)); - })); - - auto downloadProgress = std::make_shared(); - ON_CALL(*m_convertService, downloadConvertedScore(_, _)) - .WillByDefault(Return(downloadProgress)); - - // [GIVEN] Creating the destination directory always fails. MAX_FS_RETRY_ATTEMPTS (see - // convertfiletoscoreservice.h) is 5, so the 5th attempt should be the last one - const int maxAttempts = 5; - EXPECT_CALL(*m_fileSystem, makePath(_)) - .Times(maxAttempts) - .WillRepeatedly(Return(make_ret(io::Err::FSMakingError))); - - // [GIVEN] saveWatchedItems() persists to its own file, unrelated to the converted score itself - ON_CALL(*m_configuration, pendingConvertsJsonPath()) - .WillByDefault(Return(io::path_t("/pending.json"))); - EXPECT_CALL(*m_fileSystem, writeFile(io::path_t("/pending.json"), _)) - .Times(AnyNumber()) - .WillRepeatedly(Return(make_ok())); - - ON_CALL(*m_configuration, convertedScoresPath()) - .WillByDefault(Return(io::path_t("/scores"))); - - // [THEN] The file is never written, since the directory could never be created - EXPECT_CALL(*m_fileSystem, writeFile(Truly([](const io::path_t& path) { return path.hasSuffix("mscz"); }), _)).Times(0); - - bool received = false; - Ret receivedRet; - m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const io::path_t&) { - received = true; - receivedRet = ret; - }); - - // [WHEN] Uploading and polling the status, then letting the download complete - deliverQueueStatus({ item }, ConvertType::Omr, TEST_QUEUE_ID, "My Score"); - downloadProgress->finish(ProgressResult::make_ok(Val())); - - // [THEN] The conversion is reported as failed once the retries (via QTimer, hence the wait) are exhausted - waitUntil([&] { return received; }); - ASSERT_TRUE(received); - EXPECT_FALSE(receivedRet); -} - -TEST_F(Project_ConvertFileToScoreServiceTest, Poll_AwaitingReview_DownloadsButDoesNotRequestReviewYet) -{ - // [GIVEN] The queue reports the conversion as awaiting review + // [GIVEN] The queue reports the conversion as awaiting review, but hasn't assigned a scoreId yet ConvertQueueItem item; item.id = TEST_QUEUE_ID; item.type = ConvertType::Omr; item.status = ConvertStatus::AwaitingReview; - // [THEN] The score is downloaded even though the rating hasn't been submitted yet - EXPECT_CALL(*m_convertService, fetchMsczUrl(ConvertType::Omr, TEST_QUEUE_ID)) - .Times(1) - .WillOnce(Invoke([] { - return pendingPromise >(); - })); + // [THEN] There's nothing to identify the score by yet, so nothing is fetched or reported + EXPECT_CALL(*m_museScoreComService, downloadScoreInfo(An())).Times(0); bool reviewRequested = false; - m_service->reviewRequested().onReceive(nullptr, [&](ConvertType, int, const io::path_t&) { + bool convertFinished = false; + m_service->reviewRequested().onReceive(nullptr, [&](int) { reviewRequested = true; }); + m_service->convertFinished().onReceive(nullptr, [&](const Ret&, const ScoreInfo&) { + convertFinished = true; + }); // [WHEN] Uploading and polling the status deliverQueueStatus({ item }, ConvertType::Omr, TEST_QUEUE_ID, "My Score"); - // [THEN] No review is requested yet - the score hasn't finished downloading + // [THEN] Neither signal fires yet EXPECT_FALSE(reviewRequested); + EXPECT_FALSE(convertFinished); } -TEST_F(Project_ConvertFileToScoreServiceTest, Poll_AwaitingReview_EmitsReviewRequestedOnlyAfterDownloadCompletes) +TEST_F(Project_ConvertFileToScoreServiceTest, Poll_AwaitingReviewWithScoreId_EmitsReviewRequestedAndConvertFinished) { - // [GIVEN] The queue reports the conversion as awaiting review + // [GIVEN] The queue reports the conversion as awaiting review, with its scoreId ConvertQueueItem item; item.id = TEST_QUEUE_ID; item.type = ConvertType::Omr; item.status = ConvertStatus::AwaitingReview; + item.scoreId = 555; - ON_CALL(*m_convertService, fetchMsczUrl(ConvertType::Omr, TEST_QUEUE_ID)) - .WillByDefault(Invoke([] { - SignedMsczUrl url; - url.url = QUrl("https://link.xyz/score.mscz"); - url.expiresInSeconds = 60; - return resolvedPromise >(RetVal::make_ok(url)); - })); - - auto downloadProgress = std::make_shared(); - ON_CALL(*m_convertService, downloadConvertedScore(_, _)) - .WillByDefault(Return(downloadProgress)); - - ON_CALL(*m_fileSystem, makePath(_)) - .WillByDefault(Return(make_ok())); - ON_CALL(*m_fileSystem, writeFile(_, _)) - .WillByDefault(Return(make_ok())); - ON_CALL(*m_configuration, convertedScoresPath()) - .WillByDefault(Return(io::path_t("/scores"))); - ON_CALL(*m_configuration, uniqueFileNameAddition(_, _, _)) - .WillByDefault(Return(std::string())); + ON_CALL(*m_museScoreComService, downloadScoreInfo(555)) + .WillByDefault(Invoke([] { return okScoreInfo(555); })); bool reviewRequested = false; - ConvertType reviewType = ConvertType::Audio2Score; - int reviewQueueId = 0; - io::path_t reviewPath; - m_service->reviewRequested().onReceive(nullptr, [&](ConvertType type, int queueId, const io::path_t& path) { + int reviewScoreId = 0; + m_service->reviewRequested().onReceive(nullptr, [&](int scoreId) { reviewRequested = true; - reviewType = type; - reviewQueueId = queueId; - reviewPath = path; + reviewScoreId = scoreId; + }); + + bool convertFinished = false; + Ret convertFinishedRet; + m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const ScoreInfo&) { + convertFinished = true; + convertFinishedRet = ret; }); - // [WHEN] Uploading and polling the status, then letting the download complete + // [WHEN] Uploading and polling the status deliverQueueStatus({ item }, ConvertType::Omr, TEST_QUEUE_ID, "My Score"); - downloadProgress->finish(ProgressResult::make_ok(Val())); - // [THEN] The review is now requested, carrying the downloaded score's path + // [THEN] The score is already usable, so both signals fire immediately + ASSERT_TRUE(convertFinished); + EXPECT_TRUE(convertFinishedRet); + ASSERT_TRUE(reviewRequested); - EXPECT_EQ(reviewType, ConvertType::Omr); - EXPECT_EQ(reviewQueueId, TEST_QUEUE_ID); - EXPECT_FALSE(reviewPath.empty()); + EXPECT_EQ(reviewScoreId, 555); } -TEST_F(Project_ConvertFileToScoreServiceTest, Poll_ItemDroppedFromQueue_TreatedAsDoneAndDownloads) +TEST_F(Project_ConvertFileToScoreServiceTest, Poll_ItemNeverInQueueWithoutScoreId_TreatedAsFailed) { - // [THEN] A watched item that disappears from the queue is treated the same as "Done" - EXPECT_CALL(*m_convertService, fetchMsczUrl(ConvertType::Omr, TEST_QUEUE_ID)) - .Times(1) - .WillOnce(Invoke([] { - return pendingPromise >(); - })); + // [GIVEN] The item never appears in the queue at all, and never reported a scoreId - there's + // no way to identify a resulting score, so it can't be recovered as a success + + bool received = false; + Ret receivedRet; + m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const ScoreInfo&) { + received = true; + receivedRet = ret; + }); // [WHEN] Uploading, then polling an empty queue deliverQueueStatus({}, ConvertType::Omr, TEST_QUEUE_ID, "My Score"); + + // [THEN] The conversion is reported as failed + ASSERT_TRUE(received); + EXPECT_FALSE(receivedRet); } -TEST_F(Project_ConvertFileToScoreServiceTest, Poll_ItemDroppedFromQueue_DownloadRetryableFailure_RetriesOnNextPoll) +TEST_F(Project_ConvertFileToScoreServiceTest, Poll_PreviouslyReportedItemDropsFromQueue_SilentlyErasedWithoutDuplicateReport) { - // [GIVEN] The item is never present in the queue (already finished server-side and removed - // before it was ever observed), while an unrelated item stays in the queue across both polls + // [GIVEN] The item was already reported ready (AwaitingReview, with its scoreId) on the first poll, + // then disappears from the queue entirely on the second poll const int otherQueueId = TEST_QUEUE_ID + 1; + + ConvertQueueItem awaitingItem; + awaitingItem.id = TEST_QUEUE_ID; + awaitingItem.type = ConvertType::Omr; + awaitingItem.status = ConvertStatus::AwaitingReview; + awaitingItem.scoreId = 555; + ConvertQueueItem otherItem; otherItem.id = otherQueueId; otherItem.type = ConvertType::Omr; otherItem.status = ConvertStatus::Processing; - ON_CALL(*m_convertService, fetchQueue()) - .WillByDefault(Invoke([otherItem] { + EXPECT_CALL(*m_convertService, fetchQueue()) + .Times(2) + .WillOnce(Invoke([awaitingItem] { + return resolvedPromise >(RetVal::make_ok(ConvertQueueList { awaitingItem })); + })) + .WillOnce(Invoke([otherItem] { + //! NOTE: awaitingItem has now dropped out of the queue entirely return resolvedPromise >(RetVal::make_ok(ConvertQueueList { otherItem })); })); - SignedMsczUrl url; - url.url = QUrl("https://link.xyz/score.mscz"); - url.expiresInSeconds = 60; - ON_CALL(*m_convertService, fetchMsczUrl(ConvertType::Omr, TEST_QUEUE_ID)) - .WillByDefault(Invoke([url] { - return resolvedPromise >(RetVal::make_ok(url)); - })); - - // [GIVEN] The actual download fails with a transient error the first time, succeeds the second - auto failingDownload = std::make_shared(); - auto succeedingDownload = std::make_shared(); - EXPECT_CALL(*m_convertService, downloadConvertedScore(_, _)) - .Times(2) - .WillOnce(Return(failingDownload)) - .WillOnce(Return(succeedingDownload)); - - ON_CALL(*m_fileSystem, makePath(_)) - .WillByDefault(Return(make_ok())); - ON_CALL(*m_fileSystem, writeFile(_, _)) - .WillByDefault(Return(make_ok())); - ON_CALL(*m_configuration, convertedScoresPath()) - .WillByDefault(Return(io::path_t("/scores"))); - ON_CALL(*m_configuration, uniqueFileNameAddition(_, _, _)) - .WillByDefault(Return(std::string())); + EXPECT_CALL(*m_museScoreComService, downloadScoreInfo(555)) + .Times(1) + .WillOnce(Invoke([] { return okScoreInfo(555); })); - bool received = false; - Ret receivedRet; - m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const io::path_t&) { - received = true; - receivedRet = ret; + int convertFinishedCount = 0; + m_service->convertFinished().onReceive(nullptr, [&](const Ret&, const ScoreInfo&) { + ++convertFinishedCount; }); - // [WHEN] Starting the conversion - the first poll already reports an empty queue for it, - // so it's immediately treated as done and its download starts, but fails transiently + // [WHEN] Starting the conversion - the first poll reports it as awaiting review, already reporting it once uploadAndResolve(TEST_QUEUE_ID, "My Score", { "/some/path/a.pdf" }); - failingDownload->finish(make_ret(muse::network::Err::NetworkError)); - EXPECT_FALSE(received); + EXPECT_EQ(convertFinishedCount, 1); - // [WHEN] Starting an unrelated conversion triggers a second poll; the original item is - // still absent from the queue, but must still be retried rather than forgotten + // [WHEN] Starting an unrelated conversion triggers a second poll; the original item has now dropped uploadAndResolve(otherQueueId, "Other Score", { "/some/path/b.pdf" }); - succeedingDownload->finish(ProgressResult::make_ok(Val())); - // [THEN] The retried download succeeds and the conversion finishes - ASSERT_TRUE(received); - EXPECT_TRUE(receivedRet); + // [THEN] No duplicate report, and downloadScoreInfo() was only ever called once + EXPECT_EQ(convertFinishedCount, 1); } TEST_F(Project_ConvertFileToScoreServiceTest, Poll_SameIdDifferentType_DoesNotCrossMatch) @@ -1261,12 +1007,12 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_SameIdDifferentType_DoesNotCr JsonObject omrObj; omrObj["id"] = TEST_QUEUE_ID; omrObj["type"] = int(ConvertType::Omr); - omrObj["convertedFileName"] = "Omr Score"; + omrObj["convertedScoreName"] = "Omr Score"; JsonObject audioObj; audioObj["id"] = TEST_QUEUE_ID; audioObj["type"] = int(ConvertType::Audio2Score); - audioObj["convertedFileName"] = "Audio Score"; + audioObj["convertedScoreName"] = "Audio Score"; JsonArray array; array << omrObj << audioObj; @@ -1279,44 +1025,48 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_SameIdDifferentType_DoesNotCr ON_CALL(*m_fileSystem, writeFile(_, _)) .WillByDefault(Return(make_ok())); - // [GIVEN] The queue reports the Omr item as failed; the Audio2Score item has already - // dropped out of the queue (finished) and must not be mistaken for the failed Omr one + // [GIVEN] The queue reports the Omr item as failed, and the Audio2Score item as done; they + // must not be mistaken for each other just because they share the same numeric id ConvertQueueItem failedOmrItem; failedOmrItem.id = TEST_QUEUE_ID; failedOmrItem.type = ConvertType::Omr; failedOmrItem.status = ConvertStatus::Failed; failedOmrItem.errorCode = ConvertErrorCode::FileTooLarge; + ConvertQueueItem doneAudioItem; + doneAudioItem.id = TEST_QUEUE_ID; + doneAudioItem.type = ConvertType::Audio2Score; + doneAudioItem.status = ConvertStatus::Done; + doneAudioItem.scoreId = 999; + ON_CALL(*m_convertService, fetchQueue()) - .WillByDefault(Invoke([failedOmrItem] { - return resolvedPromise >(RetVal::make_ok(ConvertQueueList { failedOmrItem })); + .WillByDefault(Invoke([failedOmrItem, doneAudioItem] { + return resolvedPromise >(RetVal::make_ok(ConvertQueueList { failedOmrItem, + doneAudioItem })); })); - // [THEN] Only the Audio2Score item's URL is fetched (correctly treated as done, dropped - // from the queue); the failed Omr item is never mistaken for it, or vice versa - EXPECT_CALL(*m_convertService, fetchMsczUrl(ConvertType::Audio2Score, TEST_QUEUE_ID)) - .Times(1) - .WillOnce(Invoke([] { - return pendingPromise >(); - })); - EXPECT_CALL(*m_convertService, fetchMsczUrl(ConvertType::Omr, TEST_QUEUE_ID)) - .Times(0); + ON_CALL(*m_museScoreComService, downloadScoreInfo(999)) + .WillByDefault(Invoke([] { return okScoreInfo(999, "Audio Score"); })); - Ret receivedRet; - int receivedCount = 0; - m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const io::path_t&) { - receivedRet = ret; - ++receivedCount; + std::vector receivedRets; + m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const ScoreInfo&) { + receivedRets.push_back(ret); }); // [WHEN] Resuming loads both items and triggers a poll m_service->resumeConvert(); pumpEvents(); - // [THEN] Exactly one failure is reported, and it's for the Omr file, not the Audio2Score one - ASSERT_EQ(receivedCount, 1); - EXPECT_FALSE(receivedRet); - EXPECT_EQ(receivedRet.data(CONVERT_FAILED_FILE_NAME_KEY, String()), u"Omr Score"); + // [THEN] Exactly one failure (the Omr one) and one success (the Audio2Score one) are reported - + // if type were ignored during matching, the two items could be mixed up with each other + ASSERT_EQ(receivedRets.size(), 2u); + + const auto failureIt = std::find_if(receivedRets.begin(), receivedRets.end(), [](const Ret& ret) { return !ret; }); + ASSERT_NE(failureIt, receivedRets.end()); + EXPECT_EQ(failureIt->data(CONVERT_FAILED_FILE_NAME_KEY, String()), u"Omr Score"); + + const auto successIt = std::find_if(receivedRets.begin(), receivedRets.end(), [](const Ret& ret) { return bool(ret); }); + ASSERT_NE(successIt, receivedRets.end()); } TEST_F(Project_ConvertFileToScoreServiceTest, Poll_FailedStatus_ForwardsProcessingFailure) @@ -1330,7 +1080,7 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_FailedStatus_ForwardsProcessi bool received = false; Ret receivedRet; - m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const io::path_t&) { + m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const ScoreInfo&) { received = true; receivedRet = ret; }); @@ -1345,39 +1095,50 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_FailedStatus_ForwardsProcessi EXPECT_EQ(receivedRet.data(CONVERT_FAILED_FILE_NAME_KEY, String()), u"My Score"); } -TEST_F(Project_ConvertFileToScoreServiceTest, Poll_ExpiredDownloadLink_ForwardsFailure) +TEST_F(Project_ConvertFileToScoreServiceTest, Poll_ScoreInfoFetchFails_RetriesOnNextPoll) { - // [GIVEN] The score is done, but its download link has already expired - ConvertQueueItem item; - item.id = TEST_QUEUE_ID; - item.type = ConvertType::Omr; - item.status = ConvertStatus::Done; + // [GIVEN] The queue reports the conversion as done, with its scoreId, on both polls + const int otherQueueId = TEST_QUEUE_ID + 1; - ON_CALL(*m_convertService, fetchMsczUrl(ConvertType::Omr, TEST_QUEUE_ID)) - .WillByDefault(Invoke([] { - SignedMsczUrl url; - url.url = QUrl("https://link.xyz/score.mscz"); - url.expiresInSeconds = 0; - return resolvedPromise >(RetVal::make_ok(url)); + ConvertQueueItem doneItem; + doneItem.id = TEST_QUEUE_ID; + doneItem.type = ConvertType::Omr; + doneItem.status = ConvertStatus::Done; + doneItem.scoreId = 555; + + ConvertQueueItem otherItem; + otherItem.id = otherQueueId; + otherItem.type = ConvertType::Omr; + otherItem.status = ConvertStatus::Processing; + + ON_CALL(*m_convertService, fetchQueue()) + .WillByDefault(Invoke([doneItem, otherItem] { + return resolvedPromise >(RetVal::make_ok(ConvertQueueList { doneItem, otherItem })); })); - // [THEN] No download is attempted - EXPECT_CALL(*m_convertService, downloadConvertedScore(_, _)).Times(0); + // [GIVEN] Fetching the score's info fails transiently the first time, succeeds the second + EXPECT_CALL(*m_museScoreComService, downloadScoreInfo(555)) + .Times(2) + .WillOnce(Return(RetVal::make_ret(make_ret(muse::cloud::Err::NetworkError)))) + .WillOnce(Invoke([] { return okScoreInfo(555); })); bool received = false; Ret receivedRet; - m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const io::path_t&) { + m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const ScoreInfo&) { received = true; receivedRet = ret; }); - // [WHEN] Uploading and polling the status - deliverQueueStatus({ item }, ConvertType::Omr, TEST_QUEUE_ID, "My Score"); + // [WHEN] Starting the conversion - the first poll's score info fetch fails transiently + uploadAndResolve(TEST_QUEUE_ID, "My Score", { "/some/path/a.pdf" }); + EXPECT_FALSE(received); + + // [WHEN] Starting an unrelated conversion triggers a second poll, retrying the fetch + uploadAndResolve(otherQueueId, "Other Score", { "/some/path/b.pdf" }); - // [THEN] The failure is forwarded as an expired download link + // [THEN] The retried fetch succeeds and the conversion finishes ASSERT_TRUE(received); - EXPECT_FALSE(receivedRet); - EXPECT_EQ(receivedRet.code(), int(mu::project::Err::DownloadLinkExpired)); + EXPECT_TRUE(receivedRet); } // ================================================== @@ -1411,13 +1172,14 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_NonRetryableFetchFailure_Fini TEST_F(Project_ConvertFileToScoreServiceTest, Poll_RetryableFetchFailure_KeepsWatchingItemForNextPoll) { // [GIVEN] The first status check fails with a transient network error; - // the second succeeds, reporting the originally watched item as awaiting review + // the second succeeds, reporting the originally watched item as done const int otherQueueId = TEST_QUEUE_ID + 1; ConvertQueueItem watchedItem; watchedItem.id = TEST_QUEUE_ID; watchedItem.type = ConvertType::Omr; - watchedItem.status = ConvertStatus::AwaitingReview; + watchedItem.status = ConvertStatus::Done; + watchedItem.scoreId = 555; ConvertQueueItem otherItem; otherItem.id = otherQueueId; @@ -1435,15 +1197,11 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_RetryableFetchFailure_KeepsWa otherItem })); })); - // [THEN] The originally watched item's download is attempted once the retried poll processes it - EXPECT_CALL(*m_convertService, fetchMsczUrl(ConvertType::Omr, TEST_QUEUE_ID)) - .Times(1) - .WillOnce(Invoke([] { - return pendingPromise >(); - })); + ON_CALL(*m_museScoreComService, downloadScoreInfo(555)) + .WillByDefault(Invoke([] { return okScoreInfo(555); })); bool received = false; - m_service->convertFinished().onReceive(nullptr, [&](const Ret&, const io::path_t&) { + m_service->convertFinished().onReceive(nullptr, [&](const Ret&, const ScoreInfo&) { received = true; }); @@ -1453,8 +1211,11 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_RetryableFetchFailure_KeepsWa // [THEN] Nothing is finished yet, the item was not dropped from the watch list EXPECT_FALSE(received); - // [AND WHEN] Starting an unrelated conversion triggers a second poll, batching the original item in again + // [AND WHEN] Starting an unrelated conversion triggers a second poll, checking the original item again too uploadAndResolve(otherQueueId, "Other Score", { "/some/path/b.pdf" }); + + // [THEN] The item survived the transient failure and was processed once the queue succeeded + EXPECT_TRUE(received); } TEST_F(Project_ConvertFileToScoreServiceTest, Poll_ConsecutiveRetryableFetchFailures_GivesUpAfterMaxAttempts) @@ -1569,7 +1330,7 @@ TEST_F(Project_ConvertFileToScoreServiceTest, RetryPolling_NoPendingItems_DoesNo TEST_F(Project_ConvertFileToScoreServiceTest, Poll_SuccessBetweenFetchFailures_ResetsConsecutiveFailureCount) { - // [GIVEN] A pattern of failures with an intervening success: 3 failures, then a success, then + // [GIVEN] A pattern of failures with a success in between: 3 failures, then a success, then // 4 more failures - never 5 CONSECUTIVE failures, so polling should never give up ConvertQueueItem processingItem; processingItem.id = TEST_QUEUE_ID; @@ -1610,107 +1371,78 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_SuccessBetweenFetchFailures_R EXPECT_FALSE(gaveUp); } -TEST_F(Project_ConvertFileToScoreServiceTest, DownloadScore_RetryableFailure_RetriesAndSucceedsOnNextPoll) -{ - // [GIVEN] The queue reports the conversion as done on both polls, and fetching the download URL always succeeds - ConvertQueueItem doneItem; - doneItem.id = TEST_QUEUE_ID; - doneItem.type = ConvertType::Omr; - doneItem.status = ConvertStatus::Done; - - const int otherQueueId = TEST_QUEUE_ID + 1; - ConvertQueueItem otherItem; - otherItem.id = otherQueueId; - otherItem.type = ConvertType::Omr; - otherItem.status = ConvertStatus::Processing; - - ON_CALL(*m_convertService, fetchQueue()) - .WillByDefault(Invoke([doneItem, otherItem] { - return resolvedPromise >(RetVal::make_ok(ConvertQueueList { doneItem, otherItem })); - })); - - SignedMsczUrl url; - url.url = QUrl("https://link.xyz/score.mscz"); - url.expiresInSeconds = 60; - ON_CALL(*m_convertService, fetchMsczUrl(ConvertType::Omr, TEST_QUEUE_ID)) - .WillByDefault(Invoke([url] { - return resolvedPromise >(RetVal::make_ok(url)); - })); - - // [AND GIVEN] The actual download fails with a transient error the first time, succeeds the second - auto failingDownload = std::make_shared(); - auto succeedingDownload = std::make_shared(); - EXPECT_CALL(*m_convertService, downloadConvertedScore(_, _)) - .Times(2) - .WillOnce(Return(failingDownload)) - .WillOnce(Return(succeedingDownload)); - - ON_CALL(*m_fileSystem, makePath(_)) - .WillByDefault(Return(make_ok())); - ON_CALL(*m_fileSystem, writeFile(_, _)) - .WillByDefault(Return(make_ok())); - ON_CALL(*m_configuration, convertedScoresPath()) - .WillByDefault(Return(io::path_t("/scores"))); - ON_CALL(*m_configuration, uniqueFileNameAddition(_, _, _)) - .WillByDefault(Return(std::string())); - - bool received = false; - Ret receivedRet; - m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const io::path_t&) { - received = true; - receivedRet = ret; - }); - - // [WHEN] Starting the conversion - the first poll's download fails partway through - uploadAndResolve(TEST_QUEUE_ID, "My Score", { "/some/path/a.pdf" }); - failingDownload->finish(make_ret(muse::network::Err::NetworkError)); - EXPECT_FALSE(received); - - // [AND WHEN] Starting an unrelated conversion triggers a second poll, retrying the download - uploadAndResolve(otherQueueId, "Other Score", { "/some/path/b.pdf" }); - succeedingDownload->finish(ProgressResult::make_ok(Val())); - - // [THEN] The retried download succeeds and the conversion finishes - ASSERT_TRUE(received); - EXPECT_TRUE(receivedRet); -} - // ================================================== // submitReview() / submitReviewComment() // ================================================== TEST_F(Project_ConvertFileToScoreServiceTest, SubmitReview_Good_DelegatesToConvertService) { - // [THEN] The rating is delegated to the convert service + // [GIVEN] A watched item already reported ready and awaiting review, identified by its scoreId + ConvertQueueItem item; + item.id = TEST_QUEUE_ID; + item.type = ConvertType::Omr; + item.status = ConvertStatus::AwaitingReview; + item.scoreId = 555; + + ON_CALL(*m_museScoreComService, downloadScoreInfo(555)) + .WillByDefault(Invoke([] { return okScoreInfo(555); })); + + deliverQueueStatus({ item }, ConvertType::Omr, TEST_QUEUE_ID, "My Score"); + + // [THEN] The rating is delegated to the convert service, resolving the scoreId back to its conversion EXPECT_CALL(*m_convertService, submitReview(ConvertType::Omr, TEST_QUEUE_ID, ReviewRating::Good, QString())) .WillOnce(Invoke([](auto, auto, auto, auto) { return resolvedPromise >(RetVal::make_ok(ConvertResult {})); })); // [WHEN] Submitting a "Good" review with no comment - m_service->submitReview(ConvertType::Omr, TEST_QUEUE_ID, ReviewRating::Good); + m_service->submitReview(555, ReviewRating::Good); } TEST_F(Project_ConvertFileToScoreServiceTest, SubmitReview_BadWithComment_DelegatesToConvertService) { - // [THEN] The rating and comment are delegated to the convert service + // [GIVEN] A watched item already reported ready and awaiting review, identified by its scoreId + ConvertQueueItem item; + item.id = 7; + item.type = ConvertType::Audio2Score; + item.status = ConvertStatus::AwaitingReview; + item.scoreId = 555; + + ON_CALL(*m_museScoreComService, downloadScoreInfo(555)) + .WillByDefault(Invoke([] { return okScoreInfo(555); })); + + deliverQueueStatus({ item }, ConvertType::Audio2Score, 7, "My Score"); + + // [THEN] The rating and comment are delegated to the convert service, resolving the scoreId back to its conversion EXPECT_CALL(*m_convertService, submitReview(ConvertType::Audio2Score, 7, ReviewRating::Bad, QString("Too many wrong notes"))) .WillOnce(Invoke([](auto, auto, auto, auto) { return resolvedPromise >(RetVal::make_ok(ConvertResult {})); })); // [WHEN] Submitting a "Bad" review with a comment - m_service->submitReview(ConvertType::Audio2Score, 7, ReviewRating::Bad, "Too many wrong notes"); + m_service->submitReview(555, ReviewRating::Bad, "Too many wrong notes"); } TEST_F(Project_ConvertFileToScoreServiceTest, SubmitReviewComment_DelegatesToConvertService) { - // [THEN] The comment is delegated to the convert service + // [GIVEN] A watched item already reported ready and awaiting review, identified by its scoreId + ConvertQueueItem item; + item.id = 7; + item.type = ConvertType::Audio2Score; + item.status = ConvertStatus::AwaitingReview; + item.scoreId = 555; + + ON_CALL(*m_museScoreComService, downloadScoreInfo(555)) + .WillByDefault(Invoke([] { return okScoreInfo(555); })); + + deliverQueueStatus({ item }, ConvertType::Audio2Score, 7, "My Score"); + + // [THEN] The comment is delegated to the convert service, resolving the scoreId back to its conversion EXPECT_CALL(*m_convertService, submitReviewComment(ConvertType::Audio2Score, 7, QString("Great job"))) .WillOnce(Invoke([](auto, auto, auto) { return resolvedPromise >(RetVal::make_ok(ConvertResult {})); })); // [WHEN] Submitting a follow-up comment - m_service->submitReviewComment(ConvertType::Audio2Score, 7, "Great job"); + m_service->submitReviewComment(555, "Great job"); } diff --git a/src/project/tests/mocks/convertfiletoscoreservicemock.h b/src/project/tests/mocks/convertfiletoscoreservicemock.h index 11509d96322c7..a72c8690e999e 100644 --- a/src/project/tests/mocks/convertfiletoscoreservicemock.h +++ b/src/project/tests/mocks/convertfiletoscoreservicemock.h @@ -36,7 +36,7 @@ class ConvertFileToScoreServiceMock : public IConvertFileToScoreService MOCK_METHOD(muse::Ret, validateLink, (const QUrl&), (const, override)); MOCK_METHOD(muse::Ret, startConvert, (const ConvertInput&, const muse::String&), (override)); - MOCK_METHOD((muse::async::Channel), convertFinished, (), (const, override)); + MOCK_METHOD((muse::async::Channel), convertFinished, (), (const, override)); MOCK_METHOD(muse::StringList, fileNamesBeingConverted, (), (const, override)); MOCK_METHOD(muse::async::Notification, fileNamesBeingConvertedChanged, (), (const, override)); @@ -44,8 +44,8 @@ class ConvertFileToScoreServiceMock : public IConvertFileToScoreService MOCK_METHOD((muse::async::Channel), pollingFailed, (), (const, override)); MOCK_METHOD(void, retryPolling, (), (override)); - MOCK_METHOD((muse::async::Channel), reviewRequested, (), (const, override)); - MOCK_METHOD(void, submitReview, (ConvertType, int, ReviewRating, const QString&), (override)); - MOCK_METHOD(void, submitReviewComment, (ConvertType, int, const QString&), (override)); + MOCK_METHOD((muse::async::Channel), reviewRequested, (), (const, override)); + MOCK_METHOD(void, submitReview, (int, ReviewRating, const QString&), (override)); + MOCK_METHOD(void, submitReviewComment, (int, const QString&), (override)); }; } diff --git a/src/project/tests/mocks/projectconfigurationmock.h b/src/project/tests/mocks/projectconfigurationmock.h index 169795ebd9275..8b1cf59b94216 100644 --- a/src/project/tests/mocks/projectconfigurationmock.h +++ b/src/project/tests/mocks/projectconfigurationmock.h @@ -154,11 +154,7 @@ class ProjectConfigurationMock : public project::IProjectConfiguration MOCK_METHOD(bool, showConvertFileProcessingDialog, (), (const, override)); MOCK_METHOD(void, setShowConvertFileProcessingDialog, (bool), (override)); - MOCK_METHOD(muse::io::path_t, convertedScoresPath, (), (const, override)); MOCK_METHOD(muse::io::path_t, pendingConvertsJsonPath, (), (const, override)); - - MOCK_METHOD(std::string, uniqueFileNameAddition, (const muse::io::path_t&, const muse::io::path_t&, const std::string&), - (const, override)); }; } diff --git a/src/project/types/converttypes.h b/src/project/types/converttypes.h index b049bf45b5108..deeec27a2ed1c 100644 --- a/src/project/types/converttypes.h +++ b/src/project/types/converttypes.h @@ -23,6 +23,7 @@ #pragma once #include "cloud/musescorecom/converttypes.h" +#include "cloud/cloudtypes.h" #include "filecategory.h" @@ -36,6 +37,7 @@ using ConvertInput = muse::cloud::ConvertInput; using ReviewRating = muse::cloud::ReviewRating; using LinkSource = muse::cloud::LinkSource; using LinkSources = muse::cloud::LinkSources; +using ScoreInfo = muse::cloud::ScoreInfo; struct ConvertFilesValidation { ConvertType type = ConvertType::Omr; @@ -46,7 +48,7 @@ struct PollingFailure { muse::Ret ret; int attempt = 0; int maxAttempts = 0; - muse::secs_t nextInterval; + muse::secs_t nextInterval = 0.; bool gaveUp = false; }; } diff --git a/src/stubs/project/projectconfigurationstub.cpp b/src/stubs/project/projectconfigurationstub.cpp index c7494a2b55c3e..0111f932a0720 100644 --- a/src/stubs/project/projectconfigurationstub.cpp +++ b/src/stubs/project/projectconfigurationstub.cpp @@ -419,17 +419,7 @@ void ProjectConfigurationStub::setShowConvertFileProcessingDialog(bool) { } -muse::io::path_t ProjectConfigurationStub::convertedScoresPath() const -{ - return muse::io::path_t(); -} - muse::io::path_t ProjectConfigurationStub::pendingConvertsJsonPath() const { return muse::io::path_t(); } - -std::string ProjectConfigurationStub::uniqueFileNameAddition(const muse::io::path_t&, const muse::io::path_t&, const std::string&) const -{ - return std::string(); -} diff --git a/src/stubs/project/projectconfigurationstub.h b/src/stubs/project/projectconfigurationstub.h index c926d257476b6..5e8a924caed78 100644 --- a/src/stubs/project/projectconfigurationstub.h +++ b/src/stubs/project/projectconfigurationstub.h @@ -154,10 +154,6 @@ class ProjectConfigurationStub : public IProjectConfiguration bool showConvertFileProcessingDialog() const override; void setShowConvertFileProcessingDialog(bool show) override; - muse::io::path_t convertedScoresPath() const override; muse::io::path_t pendingConvertsJsonPath() const override; - - std::string uniqueFileNameAddition(const muse::io::path_t& filename, const muse::io::path_t& folderPath, - const std::string& suffix = std::string()) const override; }; } From ac6644269c691190f4ec74865599664ea739dbc2 Mon Sep 17 00:00:00 2001 From: Roman Pudashkin Date: Thu, 10 Sep 2026 09:41:26 +0300 Subject: [PATCH 02/10] Convert file to score: upload from bytes instead of file paths --- .../cloud/musescorecom/converttypes.h | 55 ++++++------------- .../imusescorecomconvertservice.h | 2 +- .../musescorecom/musescorecomservice.cpp | 54 +++++------------- .../cloud/musescorecom/musescorecomservice.h | 4 +- .../mocks/musescorecomconvertservicemock.h | 2 +- .../internal/convertfiletoscorescenario.cpp | 2 +- .../internal/convertfiletoscoreservice.cpp | 20 ++++++- .../tests/convertfiletoscoreservice_tests.cpp | 26 +++++++-- src/project/types/converttypes.h | 50 +++++++++++++++-- 9 files changed, 121 insertions(+), 94 deletions(-) diff --git a/src/framework/cloud/musescorecom/converttypes.h b/src/framework/cloud/musescorecom/converttypes.h index 45d54ad411275..f0bc2d75060dc 100644 --- a/src/framework/cloud/musescorecom/converttypes.h +++ b/src/framework/cloud/musescorecom/converttypes.h @@ -22,18 +22,20 @@ #pragma once +#include #include #include -#include #include #include #include #include +#include -#include "global/logstream.h" +#include "global/types/bytearray.h" #include "global/types/flags.h" -#include "io/path.h" +#include "global/io/path.h" +#include "global/logstream.h" namespace muse::cloud { enum class ConvertType { @@ -92,8 +94,6 @@ enum class ConvertErrorCode { //! NOTE: key for ConvertErrorCode stored in Ret::data static const std::string CONVERT_ERROR_CODE_KEY("errorCode"); -static const qint64 MAX_CONVERT_FILE_SIZE_BYTES = 1024LL * 1024 * 1024; // 1 GB - enum class LinkSource { NoSources = 0x0, YouTube = 0x1, @@ -141,52 +141,29 @@ struct ConvertConfig { Audio2ScoreConfig audio2score; }; -struct OmrConvertInput { - muse::io::paths_t paths; +struct ConvertFileData { + muse::ByteArray data; + muse::io::path_t fileName; // basename, with extension }; +using ConvertFileDataList = std::vector; -struct Audio2ScoreConvertInput { - std::variant data; // paths or link +struct ConvertUploadData { + ConvertType type = ConvertType::Omr; + ConvertFileDataList files; + QUrl link; // Audio2Score only }; - -using ConvertInput = std::variant; - -inline ConvertType convertTypeOf(const ConvertInput& input) -{ - return std::holds_alternative(input) ? ConvertType::Omr : ConvertType::Audio2Score; -} - -inline muse::io::paths_t convertPathsOf(const ConvertInput& input) -{ - if (const OmrConvertInput* omr = std::get_if(&input)) { - return omr->paths; - } - - const muse::io::paths_t* paths = std::get_if(&std::get(input).data); - return paths ? *paths : muse::io::paths_t(); -} - -inline QString convertLinkOf(const ConvertInput& input) -{ - const Audio2ScoreConvertInput* a2s = std::get_if(&input); - if (!a2s) { - return QString(); - } - - const QString* link = std::get_if(&a2s->data); - return link ? *link : QString(); -} +using ConvertUploadDataPtr = std::shared_ptr; struct ConvertResult { int id = 0; ConvertType type = ConvertType::Omr; - ConvertStatus status = ConvertStatus::Processing; + ConvertStatus status = ConvertStatus::Unknown; }; struct ConvertQueueItem { int id = 0; ConvertType type = ConvertType::Omr; - ConvertStatus status = ConvertStatus::Processing; + ConvertStatus status = ConvertStatus::Unknown; QString filename; QString link; //! audio2score only std::optional scoreId; //! set once the score is ready (AwaitingReview/Done) diff --git a/src/framework/cloud/musescorecom/imusescorecomconvertservice.h b/src/framework/cloud/musescorecom/imusescorecomconvertservice.h index a7b42b562d3a0..09dcb5195775a 100644 --- a/src/framework/cloud/musescorecom/imusescorecomconvertservice.h +++ b/src/framework/cloud/musescorecom/imusescorecomconvertservice.h @@ -51,7 +51,7 @@ class IMuseScoreComConvertService : MODULE_CONTEXT_INTERFACE virtual async::Promise > fetchConfig() = 0; - virtual ProgressPtr upload(const ConvertInput& input) = 0; + virtual ProgressPtr upload(const ConvertUploadDataPtr& data) = 0; virtual async::Promise > fetchQueue() = 0; diff --git a/src/framework/cloud/musescorecom/musescorecomservice.cpp b/src/framework/cloud/musescorecom/musescorecomservice.cpp index 3e294e68845f1..f1f9df67e2f37 100644 --- a/src/framework/cloud/musescorecom/musescorecomservice.cpp +++ b/src/framework/cloud/musescorecom/musescorecomservice.cpp @@ -23,8 +23,6 @@ #include "musescorecomservice.h" #include -#include -#include #include #include #include @@ -478,9 +476,7 @@ static QString sanitizeContentDispositionFilename(const QString& fileName) return sanitized; } -using ConvertFileList = std::vector >; - -static QHttpMultiPartPtr makeMultiPartForConvertUpload(ConvertType type, const ConvertFileList& files, const QString& link) +static QHttpMultiPartPtr makeMultiPartForConvertUpload(ConvertType type, const ConvertFileDataList& files, const QUrl& link) { auto multiPart = std::make_shared(QHttpMultiPart::FormDataType); @@ -492,20 +488,21 @@ static QHttpMultiPartPtr makeMultiPartForConvertUpload(ConvertType type, const C if (!link.isEmpty()) { QHttpPart linkPart; linkPart.setHeader(QNetworkRequest::ContentDispositionHeader, QVariant("form-data; name=\"link\"")); - linkPart.setBody(link.toUtf8()); + linkPart.setBody(link.toString().toUtf8()); multiPart->append(linkPart); } QMimeDatabase mimeDb; - for (const std::shared_ptr& file : files) { - const QString fileName = file->fileName(); - const QString baseName = QFileInfo(fileName).fileName(); + for (const ConvertFileData& file : files) { + const QString fileName = file.fileName.toQString(); QHttpPart filePart; filePart.setHeader(QNetworkRequest::ContentTypeHeader, QVariant(mimeDb.mimeTypeForFile(fileName).name())); QString contentDisposition - = QString("form-data; name=\"files[]\"; filename=\"%1\"").arg(sanitizeContentDispositionFilename(baseName)); + = QString("form-data; name=\"files[]\"; filename=\"%1\"").arg(sanitizeContentDispositionFilename(fileName)); filePart.setHeader(QNetworkRequest::ContentDispositionHeader, QVariant(contentDisposition)); - filePart.setBodyDevice(file.get()); + // NOTE: safe to avoid copying the bytes since the caller keeps ConvertUploadData alive + // (via a shared_ptr) for the whole async upload, not just this synchronous setup + filePart.setBody(file.data.toQByteArrayNoCopy()); multiPart->append(filePart); } @@ -1008,13 +1005,13 @@ Promise > MuseScoreComService::fetchConfig() }); } -ProgressPtr MuseScoreComService::upload(const ConvertInput& input) +ProgressPtr MuseScoreComService::upload(const ConvertUploadDataPtr& data) { ProgressPtr progress = std::make_shared(); progress->start(); - executeAsyncRequest([this, input, progress]() { - return doUpload(input, progress); + executeAsyncRequest([this, data, progress]() { + return doUpload(data, progress); }).onResolve(this, [progress](const Ret& ret) { if (progress->isStarted()) { progress->finish(ret); @@ -1024,36 +1021,17 @@ ProgressPtr MuseScoreComService::upload(const ConvertInput& input) return progress; } -Promise MuseScoreComService::doUpload(const ConvertInput& input, ProgressPtr progress) +Promise MuseScoreComService::doUpload(const ConvertUploadDataPtr& data, ProgressPtr progress) { TRACEFUNC; - return make_promise([this, input, progress](auto resolve, auto) { + return make_promise([this, data, progress](auto resolve, auto) { RetVal uploadUrl = prepareUrlForRequest(MUSESCORECOM_CONVERT_UPLOAD_API_URL); if (!uploadUrl.ret) { return resolve(uploadUrl.ret); } - const ConvertType type = convertTypeOf(input); - const QString link = convertLinkOf(input); - - ConvertFileList files; - for (const io::path_t& path : convertPathsOf(input)) { - auto file = std::make_shared(path.toQString()); - if (!file->open(QIODevice::ReadOnly)) { - return resolve(make_ret(Err::InvalidData)); - } - - if (file->size() > MAX_CONVERT_FILE_SIZE_BYTES) { - Ret ret = make_ret(Err::Status422_ValidationFailed); - ret.setData(CONVERT_ERROR_CODE_KEY, ConvertErrorCode::FileTooLarge); - return resolve(ret); - } - - files.push_back(file); - } - - auto multiPart = makeMultiPartForConvertUpload(type, files, link); + auto multiPart = makeMultiPartForConvertUpload(data->type, data->files, data->link); auto receivedData = std::make_shared(); RetVal uploadProgress = m_networkManager->post(uploadUrl.val, multiPart, receivedData, headers()); @@ -1065,9 +1043,7 @@ Promise MuseScoreComService::doUpload(const ConvertInput& input, ProgressPt progress->progress(current, total, msg); }); - //! NOTE: files must stay alive (and open) until the request finishes, - //! since multiPart's file parts hold raw pointers into them - uploadProgress.val.finished().onReceive(this, [this, files, receivedData, resolve, progress](const ProgressResult& res) { + uploadProgress.val.finished().onReceive(this, [this, data, receivedData, resolve, progress](const ProgressResult& res) { if (!res.ret) { printServerReply(*receivedData); Ret ret = uploadingDownloadingRetFromRawRet(res.ret); diff --git a/src/framework/cloud/musescorecom/musescorecomservice.h b/src/framework/cloud/musescorecom/musescorecomservice.h index c8f2b422aeb8c..0a9b8749f835a 100644 --- a/src/framework/cloud/musescorecom/musescorecomservice.h +++ b/src/framework/cloud/musescorecom/musescorecomservice.h @@ -67,7 +67,7 @@ class MuseScoreComService : public IMuseScoreComService, public IMuseScoreComCon // IMuseScoreComConvertService async::Promise > fetchConfig() override; - ProgressPtr upload(const ConvertInput& input) override; + ProgressPtr upload(const ConvertUploadDataPtr& data) override; async::Promise > fetchQueue() override; @@ -94,6 +94,6 @@ class MuseScoreComService : public IMuseScoreComService, public IMuseScoreComCon async::Promise doUploadAudio(DevicePtr audioData, const QString& audioFormat, const QUrl& sourceUrl, ProgressPtr progress); - async::Promise doUpload(const ConvertInput& input, ProgressPtr progress); + async::Promise doUpload(const ConvertUploadDataPtr& data, ProgressPtr progress); }; } diff --git a/src/framework/cloud/tests/mocks/musescorecomconvertservicemock.h b/src/framework/cloud/tests/mocks/musescorecomconvertservicemock.h index f4b264c5cbf51..4bab8a51bdced 100644 --- a/src/framework/cloud/tests/mocks/musescorecomconvertservicemock.h +++ b/src/framework/cloud/tests/mocks/musescorecomconvertservicemock.h @@ -31,7 +31,7 @@ class MuseScoreComConvertServiceMock : public IMuseScoreComConvertService public: MOCK_METHOD(async::Promise >, fetchConfig, (), (override)); - MOCK_METHOD(ProgressPtr, upload, (const ConvertInput&), (override)); + MOCK_METHOD(ProgressPtr, upload, (const ConvertUploadDataPtr&), (override)); MOCK_METHOD(async::Promise >, fetchQueue, (), (override)); diff --git a/src/project/internal/convertfiletoscorescenario.cpp b/src/project/internal/convertfiletoscorescenario.cpp index 1bb8e4957bbe5..ce999a090b890 100644 --- a/src/project/internal/convertfiletoscorescenario.cpp +++ b/src/project/internal/convertfiletoscorescenario.cpp @@ -55,7 +55,7 @@ static ConvertSelection toConvertSelection(const Val& val) selection.convertedScoreName = map.value("convertedScoreName").toString(); if (type == ConvertType::Audio2Score && !link.isEmpty()) { - selection.input = Audio2ScoreConvertInput { link }; + selection.input = Audio2ScoreConvertInput { QUrl(link) }; } else if (type == ConvertType::Audio2Score) { selection.input = Audio2ScoreConvertInput { paths }; } else { diff --git a/src/project/internal/convertfiletoscoreservice.cpp b/src/project/internal/convertfiletoscoreservice.cpp index 2a2bf8772ba9f..64b74a85e1b46 100644 --- a/src/project/internal/convertfiletoscoreservice.cpp +++ b/src/project/internal/convertfiletoscoreservice.cpp @@ -22,6 +22,7 @@ #include "convertfiletoscoreservice.h" #include +#include #include #include @@ -277,7 +278,10 @@ Ret ConvertFileToScoreService::validateLink(const QUrl& link) const Ret ConvertFileToScoreService::startConvert(const ConvertInput& input, const muse::String& convertedScoreName) { - IF_ASSERT_FAILED(!convertPathsOf(input).empty() || !convertLinkOf(input).isEmpty()) { + const io::paths_t paths = convertPathsOf(input); + const QUrl link = convertLinkOf(input); + + IF_ASSERT_FAILED(!paths.empty() || link.isValid()) { return make_ret(Err::ConvertValidationFailed); } @@ -285,8 +289,20 @@ Ret ConvertFileToScoreService::startConvert(const ConvertInput& input, const mus return make_ret(Err::ConvertValidationFailed); } + ConvertFileDataList files; + files.reserve(paths.size()); + for (const io::path_t& path : paths) { + RetVal fileData = fileSystem()->readFile(path); + if (!fileData.ret) { + return fileData.ret; + } + + files.push_back(ConvertFileData { std::move(fileData.val), io::filename(path) }); + } + const ConvertType type = convertTypeOf(input); - ProgressPtr progress = museScoreComService()->convert()->upload(input); + auto data = std::make_shared(ConvertUploadData { type, std::move(files), link }); + ProgressPtr progress = museScoreComService()->convert()->upload(data); progress->progressChanged().onReceive(this, [convertedScoreName](int64_t current, int64_t total, const std::string&) { LOGI() << "Uploading for convert \"" << convertedScoreName << "\": " << current << "/" << total; diff --git a/src/project/tests/convertfiletoscoreservice_tests.cpp b/src/project/tests/convertfiletoscoreservice_tests.cpp index 7b610e3faeda1..2cf27e6fddaaa 100644 --- a/src/project/tests/convertfiletoscoreservice_tests.cpp +++ b/src/project/tests/convertfiletoscoreservice_tests.cpp @@ -107,6 +107,21 @@ async::Promise pendingPromise() return async::Promise::dummy_result(); }); } + +bool uploadDataMatchesPaths(const ConvertUploadData& data, const io::paths_t& paths) +{ + if (data.files.size() != paths.size()) { + return false; + } + + for (size_t i = 0; i < paths.size(); ++i) { + if (data.files[i].fileName != io::filename(paths[i])) { + return false; + } + } + + return true; +} } namespace mu::project { @@ -131,6 +146,9 @@ class Project_ConvertFileToScoreServiceTest : public ::testing::Test ON_CALL(*m_fileSystem, fileSize(_)) .WillByDefault(Return(RetVal::make_ok(1024))); + + ON_CALL(*m_fileSystem, readFile(_)) + .WillByDefault(Return(RetVal::make_ok(ByteArray()))); } void setConfig(const ConvertConfig& config) @@ -182,8 +200,8 @@ class Project_ConvertFileToScoreServiceTest : public ::testing::Test void uploadAndResolve(int queueId, const QString& convertedScoreName, const io::paths_t& paths) { auto uploadProgress = std::make_shared(); - EXPECT_CALL(*m_convertService, upload(Truly([paths](const ConvertInput& input) { - return convertPathsOf(input) == paths; + EXPECT_CALL(*m_convertService, upload(Truly([paths](const ConvertUploadDataPtr& data) { + return uploadDataMatchesPaths(*data, paths); }))) .WillOnce(Return(uploadProgress)); @@ -583,8 +601,8 @@ TEST_F(Project_ConvertFileToScoreServiceTest, StartConvert_UploadSucceeds_Persis auto uploadProgress = std::make_shared(); const io::paths_t paths { "/some/path/file.pdf" }; - EXPECT_CALL(*m_convertService, upload(Truly([&](const ConvertInput& input) { - return convertTypeOf(input) == ConvertType::Omr && convertPathsOf(input) == paths; + EXPECT_CALL(*m_convertService, upload(Truly([&](const ConvertUploadDataPtr& data) { + return data->type == ConvertType::Omr && uploadDataMatchesPaths(*data, paths); }))) .WillOnce(Return(uploadProgress)); diff --git a/src/project/types/converttypes.h b/src/project/types/converttypes.h index deeec27a2ed1c..59579bb55eee1 100644 --- a/src/project/types/converttypes.h +++ b/src/project/types/converttypes.h @@ -22,23 +22,63 @@ #pragma once -#include "cloud/musescorecom/converttypes.h" -#include "cloud/cloudtypes.h" +#include + +#include #include "filecategory.h" -#include "types/ret.h" -#include "types/secs.h" +#include "cloud/musescorecom/converttypes.h" +#include "cloud/cloudtypes.h" + +#include "global/io/path.h" +#include "global/types/secs.h" +#include "global/types/ret.h" namespace mu::project { using ConvertConfig = muse::cloud::ConvertConfig; using ConvertType = muse::cloud::ConvertType; -using ConvertInput = muse::cloud::ConvertInput; using ReviewRating = muse::cloud::ReviewRating; using LinkSource = muse::cloud::LinkSource; using LinkSources = muse::cloud::LinkSources; using ScoreInfo = muse::cloud::ScoreInfo; +struct OmrConvertInput { + muse::io::paths_t paths; +}; + +struct Audio2ScoreConvertInput { + std::variant data; // paths or link +}; + +using ConvertInput = std::variant; + +inline ConvertType convertTypeOf(const ConvertInput& input) +{ + return std::holds_alternative(input) ? ConvertType::Omr : ConvertType::Audio2Score; +} + +inline muse::io::paths_t convertPathsOf(const ConvertInput& input) +{ + if (const OmrConvertInput* omr = std::get_if(&input)) { + return omr->paths; + } + + const muse::io::paths_t* paths = std::get_if(&std::get(input).data); + return paths ? *paths : muse::io::paths_t(); +} + +inline QUrl convertLinkOf(const ConvertInput& input) +{ + const Audio2ScoreConvertInput* a2s = std::get_if(&input); + if (!a2s) { + return QUrl(); + } + + const QUrl* link = std::get_if(&a2s->data); + return link ? *link : QUrl(); +} + struct ConvertFilesValidation { ConvertType type = ConvertType::Omr; FileCategory category = FileCategory::Unknown; From 02f72c770b28fc1e2b2eb7f5af57e1543ed0705d Mon Sep 17 00:00:00 2001 From: Roman Pudashkin Date: Thu, 10 Sep 2026 12:15:12 +0300 Subject: [PATCH 03/10] Convert file to score: expose all watched scores instead of just file names --- src/project/iconvertfiletoscoreservice.h | 5 +- .../internal/convertfiletoscoreservice.cpp | 169 ++++++++---------- .../internal/convertfiletoscoreservice.h | 38 +--- src/project/internal/projectconfiguration.cpp | 4 +- src/project/internal/projectconfiguration.h | 2 +- src/project/iprojectconfiguration.h | 2 +- .../internal/ScoresPage/recentscoresmodel.cpp | 14 +- .../tests/convertfiletoscoreservice_tests.cpp | 74 ++++---- .../mocks/convertfiletoscoreservicemock.h | 3 +- .../tests/mocks/projectconfigurationmock.h | 2 +- src/project/types/converttypes.h | 22 +++ .../project/projectconfigurationstub.cpp | 2 +- src/stubs/project/projectconfigurationstub.h | 2 +- 13 files changed, 164 insertions(+), 175 deletions(-) diff --git a/src/project/iconvertfiletoscoreservice.h b/src/project/iconvertfiletoscoreservice.h index eed1851866392..87220a70e4ea3 100644 --- a/src/project/iconvertfiletoscoreservice.h +++ b/src/project/iconvertfiletoscoreservice.h @@ -58,9 +58,8 @@ class IConvertFileToScoreService : MODULE_CONTEXT_INTERFACE //! Emits the final result of a conversion (upload or processing failure, or success with ScoreInfo) virtual muse::async::Channel convertFinished() const = 0; - //! Names of the files currently being converted server-side (queued or processing) - virtual muse::StringList fileNamesBeingConverted() const = 0; - virtual muse::async::Notification fileNamesBeingConvertedChanged() const = 0; + //! All pending/reviewable conversions from the server's convert queue + virtual muse::ValNt watchedScores() const = 0; //! Emitted whenever checking the conversion status fails virtual muse::async::Channel pollingFailed() const = 0; diff --git a/src/project/internal/convertfiletoscoreservice.cpp b/src/project/internal/convertfiletoscoreservice.cpp index 64b74a85e1b46..0fc3027471492 100644 --- a/src/project/internal/convertfiletoscoreservice.cpp +++ b/src/project/internal/convertfiletoscoreservice.cpp @@ -153,20 +153,20 @@ void ConvertFileToScoreService::init() void ConvertFileToScoreService::resumeConvert() { - loadWatchedItems(); + loadWatchedScores(); - if (m_watchedItems.empty()) { + if (m_watchedScores.empty()) { return; } - LOGI() << "Resuming watching " << m_watchedItems.size() << " pending conversion(s)"; + LOGI() << "Resuming watching " << m_watchedScores.size() << " pending conversion(s)"; m_timer.start(); - m_fileNamesBeingConvertedChanged.notify(); + m_watchedScoresChanged.notify(); - for (const WatchedItem& item : m_watchedItems) { - if (item.status == ConvertStatus::AwaitingReview && item.scoreId) { - m_reviewRequested.send(*item.scoreId); + for (const WatchedScore& watched : m_watchedScores) { + if (watched.convertStatus == ConvertStatus::AwaitingReview && watched.scoreId) { + m_reviewRequested.send(*watched.scoreId); } } @@ -330,25 +330,15 @@ async::Channel ConvertFileToScoreService::convertFinished() cons return m_convertFinished; } -muse::StringList ConvertFileToScoreService::fileNamesBeingConverted() const +ValNt ConvertFileToScoreService::watchedScores() const { - muse::StringList result; - result.reserve(m_watchedItems.size()); - - for (const WatchedItem& item : m_watchedItems) { - if (isPending(item)) { - result.push_back(item.convertedScoreName); - } - } + ValNt result; + result.val = m_watchedScores; + result.notification = m_watchedScoresChanged; return result; } -async::Notification ConvertFileToScoreService::fileNamesBeingConvertedChanged() const -{ - return m_fileNamesBeingConvertedChanged; -} - async::Channel ConvertFileToScoreService::pollingFailed() const { return m_pollingFailed; @@ -372,47 +362,47 @@ void ConvertFileToScoreService::submitReview(int scoreId, ReviewRating rating, c return; } - const WatchedItem* item = findWatchedItemByScoreId(scoreId); - IF_ASSERT_FAILED(item) { + const WatchedScore* watched = findWatchedScoreByScoreId(scoreId); + IF_ASSERT_FAILED(watched) { return; } - const ConvertType type = item->type; - const int itemId = item->id; + const ConvertType type = watched->convertType; + const int convertId = watched->convertId; - museScoreComService()->convert()->submitReview(type, itemId, rating, comment) - .onResolve(this, [type, itemId](const RetVal& submitRes) { + museScoreComService()->convert()->submitReview(type, convertId, rating, comment) + .onResolve(this, [type, convertId](const RetVal& submitRes) { if (!submitRes.ret) { - LOGE() << "Could not submit the review for conversion " << convertLogId(type, itemId) << ": " << submitRes.ret.toString(); + LOGE() << "Could not submit the review for conversion " << convertLogId(type, convertId) << ": " << submitRes.ret.toString(); } }); } void ConvertFileToScoreService::submitReviewComment(int scoreId, const QString& comment) { - const WatchedItem* item = findWatchedItemByScoreId(scoreId); - IF_ASSERT_FAILED(item) { + const WatchedScore* watched = findWatchedScoreByScoreId(scoreId); + IF_ASSERT_FAILED(watched) { return; } - const ConvertType type = item->type; - const int itemId = item->id; + const ConvertType type = watched->convertType; + const int convertId = watched->convertId; - museScoreComService()->convert()->submitReviewComment(type, itemId, comment) - .onResolve(this, [type, itemId](const RetVal& submitRes) { + museScoreComService()->convert()->submitReviewComment(type, convertId, comment) + .onResolve(this, [type, convertId](const RetVal& submitRes) { if (!submitRes.ret) { - LOGE() << "Could not submit the comment for conversion " << convertLogId(type, itemId) << ": " << submitRes.ret.toString(); + LOGE() << "Could not submit the comment for conversion " << convertLogId(type, convertId) << ": " << submitRes.ret.toString(); } }); } -void ConvertFileToScoreService::loadWatchedItems() +void ConvertFileToScoreService::loadWatchedScores() { TRACEFUNC; - m_watchedItems.clear(); + m_watchedScores.clear(); - RetVal data = fileSystem()->readFile(configuration()->pendingConvertsJsonPath()); + RetVal data = fileSystem()->readFile(configuration()->watchedConvertsJsonPath()); if (!data.ret || data.val.empty()) { if (!data.ret && data.ret.code() != static_cast(io::Err::FSNotExist)) { LOGE() << "Could not read the pending conversions file: " << data.ret; @@ -430,44 +420,44 @@ void ConvertFileToScoreService::loadWatchedItems() } const JsonArray array = json.rootArray(); - m_watchedItems.reserve(array.size()); + m_watchedScores.reserve(array.size()); for (size_t i = 0; i < array.size(); ++i) { const JsonObject obj = array.at(i).toObject(); - const int itemId = obj.value("id").toInt(); + const int convertId = obj.value("id").toInt(); const ConvertType type = static_cast(obj.value("type").toInt()); const muse::String convertedScoreName = muse::String::fromStdString(obj.value("convertedScoreName").toStdString()); const ConvertStatus status = static_cast(obj.value("status").toInt()); const std::optional scoreId = obj.contains("scoreId") ? std::optional(obj.value("scoreId").toInt()) : std::nullopt; - m_watchedItems.push_back(WatchedItem { itemId, type, status, scoreId, convertedScoreName }); + m_watchedScores.push_back(WatchedScore { convertId, type, status, scoreId, convertedScoreName }); } } -void ConvertFileToScoreService::saveWatchedItems() +void ConvertFileToScoreService::saveWatchedScores() { TRACEFUNC; JsonArray array; - for (const WatchedItem& item : m_watchedItems) { + for (const WatchedScore& watched : m_watchedScores) { JsonObject obj; - obj["id"] = item.id; - obj["type"] = static_cast(item.type); - obj["status"] = static_cast(item.status); + obj["id"] = watched.convertId; + obj["type"] = static_cast(watched.convertType); + obj["status"] = static_cast(watched.convertStatus); - if (!item.convertedScoreName.isEmpty()) { - obj["convertedScoreName"] = item.convertedScoreName.toStdString(); + if (!watched.name.isEmpty()) { + obj["convertedScoreName"] = watched.name.toStdString(); } - if (item.scoreId) { - obj["scoreId"] = *item.scoreId; + if (watched.scoreId) { + obj["scoreId"] = *watched.scoreId; } array << obj; } JsonDocument json(array); - Ret ret = fileSystem()->writeFile(configuration()->pendingConvertsJsonPath(), json.toJson()); + Ret ret = fileSystem()->writeFile(configuration()->watchedConvertsJsonPath(), json.toJson()); if (!ret) { LOGE() << "Could not save the pending conversions list: " << ret.toString(); } @@ -477,9 +467,9 @@ void ConvertFileToScoreService::watch(ConvertType type, int itemId, const muse:: { LOGI() << "Start watching conversion " << convertLogId(convertedScoreName, type, itemId); - m_watchedItems.push_back(WatchedItem { itemId, type, ConvertStatus::Unknown, std::nullopt, convertedScoreName }); - saveWatchedItems(); - m_fileNamesBeingConvertedChanged.notify(); + m_watchedScores.push_back(WatchedScore { itemId, type, ConvertStatus::Unknown, std::nullopt, convertedScoreName }); + saveWatchedScores(); + m_watchedScoresChanged.notify(); if (!m_timer.isActive()) { m_timer.start(); @@ -490,7 +480,7 @@ void ConvertFileToScoreService::watch(ConvertType type, int itemId, const muse:: void ConvertFileToScoreService::poll() { - if (m_watchedItems.empty()) { + if (m_watchedScores.empty()) { m_timer.stop(); return; } @@ -510,7 +500,7 @@ void ConvertFileToScoreService::poll() } resetPollState(); - updateWatchedItems(result.val); + updateWatchedScores(result.val); }); } @@ -544,7 +534,7 @@ void ConvertFileToScoreService::handlePollFailure(const Ret& ret) void ConvertFileToScoreService::giveUpPolling(const Ret& ret) { LOGE() << "Could not check the conversion status, stopping polling for now, " - << m_watchedItems.size() << " pending conversion(s) remain watched: " << ret.toString(); + << m_watchedScores.size() << " pending conversion(s) remain watched: " << ret.toString(); const int count = m_pollFailureCount; @@ -554,15 +544,15 @@ void ConvertFileToScoreService::giveUpPolling(const Ret& ret) m_pollingFailed.send(PollingFailure { ret, count, MAX_POLL_RETRY_ATTEMPTS, secs_t(0), true }); } -void ConvertFileToScoreService::updateWatchedItems(const ConvertQueueList& queue) +void ConvertFileToScoreService::updateWatchedScores(const ConvertQueueList& queue) { - const std::vector previousWatchedItems = m_watchedItems; + const std::vector previousWatchedScores = m_watchedScores; - for (auto it = m_watchedItems.begin(); it != m_watchedItems.end();) { - WatchedItem& item = *it; + for (auto it = m_watchedScores.begin(); it != m_watchedScores.end();) { + WatchedScore& watched = *it; - auto found = std::find_if(queue.begin(), queue.end(), [&item](const ConvertQueueItem& queueItem) { - return queueItem.id == item.id && queueItem.type == item.type; + auto found = std::find_if(queue.begin(), queue.end(), [&watched](const ConvertQueueItem& queueItem) { + return queueItem.id == watched.convertId && queueItem.type == watched.convertType; }); ConvertStatus status; @@ -573,12 +563,12 @@ void ConvertFileToScoreService::updateWatchedItems(const ConvertQueueList& queue status = found->status; errorCode = found->errorCode; scoreId = found->scoreId; - } else if (item.scoreId) { + } else if (watched.scoreId) { //! NOTE: a Done/AwaitingReview item still reports its status while queued, but it may be //! dropped from the queue automatically at some point afterwards, once already reported ready status = ConvertStatus::Done; errorCode = ConvertErrorCode::Unknown; - scoreId = item.scoreId; + scoreId = watched.scoreId; } else { //! NOTE: dropped from the queue before ever reporting a scoreId - there's no way to //! identify the resulting score anymore, so it can't be recovered as a success @@ -586,31 +576,31 @@ void ConvertFileToScoreService::updateWatchedItems(const ConvertQueueList& queue errorCode = ConvertErrorCode::Unknown; } - handleItem(item, status, errorCode, scoreId); + handleItem(watched, status, errorCode, scoreId); if (status == ConvertStatus::Failed - || (status == ConvertStatus::Done && item.scoreId)) { - it = m_watchedItems.erase(it); + || (status == ConvertStatus::Done && watched.scoreId)) { + it = m_watchedScores.erase(it); } else { ++it; } } - if (m_watchedItems != previousWatchedItems) { - saveWatchedItems(); - m_fileNamesBeingConvertedChanged.notify(); + if (m_watchedScores != previousWatchedScores) { + saveWatchedScores(); + m_watchedScoresChanged.notify(); } } -void ConvertFileToScoreService::handleItem(WatchedItem& item, ConvertStatus status, ConvertErrorCode errorCode, +void ConvertFileToScoreService::handleItem(WatchedScore& watched, ConvertStatus status, ConvertErrorCode errorCode, std::optional scoreId) { - const bool statusChanged = item.status != status; + const bool statusChanged = watched.convertStatus != status; if (statusChanged) { - LOGI() << "Conversion status changed: " << convertLogId(item.convertedScoreName, item.type, item.id) + LOGI() << "Conversion status changed: " << convertLogId(watched.name, watched.convertType, watched.convertId) << " -> " << convertStatusToString(status); } - item.status = status; + watched.convertStatus = status; switch (status) { case ConvertStatus::Processing: @@ -618,8 +608,8 @@ void ConvertFileToScoreService::handleItem(WatchedItem& item, ConvertStatus stat break; case ConvertStatus::AwaitingReview: case ConvertStatus::Done: - if (scoreId && !item.scoreId) { - reportReady(item, status, *scoreId); + if (scoreId && !watched.scoreId) { + reportReady(watched, status, *scoreId); } break; case ConvertStatus::Failed: { @@ -628,8 +618,8 @@ void ConvertFileToScoreService::handleItem(WatchedItem& item, ConvertStatus stat } Ret ret = make_ret(Err::ConvertProcessingFailed); - ret.setText("Conversion failed for \"" + item.convertedScoreName.toStdString() + "\": " + errorCodeToString(errorCode)); - ret.setData(CONVERT_FAILED_FILE_NAME_KEY, item.convertedScoreName); + ret.setText("Conversion failed for \"" + watched.name.toStdString() + "\": " + errorCodeToString(errorCode)); + ret.setData(CONVERT_FAILED_FILE_NAME_KEY, watched.name); LOGE() << ret.toString(); @@ -639,16 +629,16 @@ void ConvertFileToScoreService::handleItem(WatchedItem& item, ConvertStatus stat } } -void ConvertFileToScoreService::reportReady(WatchedItem& item, ConvertStatus status, int scoreId) +void ConvertFileToScoreService::reportReady(WatchedScore& watched, ConvertStatus status, int scoreId) { const RetVal scoreInfo = museScoreComService()->downloadScoreInfo(scoreId); if (!scoreInfo.ret) { - LOGW() << "Could not fetch score info for " << convertLogId(item.convertedScoreName, item.type, item.id) + LOGW() << "Could not fetch score info for " << convertLogId(watched.name, watched.convertType, watched.convertId) << ", will retry on next poll: " << scoreInfo.ret.toString(); return; } - item.scoreId = scoreId; + watched.scoreId = scoreId; finishConvert(make_ok(), scoreInfo.val); if (status == ConvertStatus::AwaitingReview) { @@ -661,18 +651,11 @@ void ConvertFileToScoreService::finishConvert(const Ret& ret, const ScoreInfo& s m_convertFinished.send(ret, scoreInfo); } -ConvertFileToScoreService::WatchedItem* ConvertFileToScoreService::findWatchedItemByScoreId(int scoreId) +WatchedScore* ConvertFileToScoreService::findWatchedScoreByScoreId(int scoreId) { - auto it = std::find_if(m_watchedItems.begin(), m_watchedItems.end(), [scoreId](const WatchedItem& item) { - return item.scoreId == scoreId; + auto it = std::find_if(m_watchedScores.begin(), m_watchedScores.end(), [scoreId](const WatchedScore& watched) { + return watched.scoreId == scoreId; }); - return it != m_watchedItems.end() ? &*it : nullptr; -} - -bool ConvertFileToScoreService::isPending(const WatchedItem& item) -{ - //! NOTE: an AwaitingReview item that's already been reported ready is just waiting on the user - //! to submit a review - not "being converted" anymore - return item.status != ConvertStatus::AwaitingReview || !item.scoreId; + return it != m_watchedScores.end() ? &*it : nullptr; } diff --git a/src/project/internal/convertfiletoscoreservice.h b/src/project/internal/convertfiletoscoreservice.h index 0eef418b2c11f..d4ab176b9bd48 100644 --- a/src/project/internal/convertfiletoscoreservice.h +++ b/src/project/internal/convertfiletoscoreservice.h @@ -61,8 +61,7 @@ class ConvertFileToScoreService : public QObject, public IConvertFileToScoreServ muse::Ret startConvert(const ConvertInput& input, const muse::String& convertedScoreName) override; muse::async::Channel convertFinished() const override; - muse::StringList fileNamesBeingConverted() const override; - muse::async::Notification fileNamesBeingConvertedChanged() const override; + muse::ValNt watchedScores() const override; muse::async::Channel pollingFailed() const override; void retryPolling() override; @@ -76,53 +75,34 @@ class ConvertFileToScoreService : public QObject, public IConvertFileToScoreServ static constexpr int MAX_RETRY_INTERVAL_MS = 10 * 60000; static constexpr int MAX_POLL_RETRY_ATTEMPTS = 5; // gives up after ~15 minutes - struct WatchedItem { - int id = 0; - ConvertType type = ConvertType::Omr; - muse::cloud::ConvertStatus status = muse::cloud::ConvertStatus::Unknown; - std::optional scoreId; //! NOTE: set once the score is ready and reported (Done/AwaitingReview) - muse::String convertedScoreName; - - bool operator==(const WatchedItem& other) const - { - return id == other.id - && type == other.type - && status == other.status - && scoreId == other.scoreId - && convertedScoreName == other.convertedScoreName; - } - }; - - void loadWatchedItems(); - void saveWatchedItems(); + void loadWatchedScores(); + void saveWatchedScores(); void watch(ConvertType type, int itemId, const muse::String& convertedScoreName); void poll(); void resetPollState(); void handlePollFailure(const muse::Ret& ret); void giveUpPolling(const muse::Ret& ret); - void updateWatchedItems(const muse::cloud::ConvertQueueList& queue); + void updateWatchedScores(const muse::cloud::ConvertQueueList& queue); - void handleItem(WatchedItem& item, muse::cloud::ConvertStatus status, muse::cloud::ConvertErrorCode errorCode, + void handleItem(WatchedScore& watched, muse::cloud::ConvertStatus status, muse::cloud::ConvertErrorCode errorCode, std::optional scoreId); - void reportReady(WatchedItem& item, muse::cloud::ConvertStatus status, int scoreId); + void reportReady(WatchedScore& watched, muse::cloud::ConvertStatus status, int scoreId); void finishConvert(const muse::Ret& ret, const ScoreInfo& scoreInfo = ScoreInfo()); - WatchedItem* findWatchedItemByScoreId(int scoreId); - - static bool isPending(const WatchedItem& item); + WatchedScore* findWatchedScoreByScoreId(int scoreId); ConvertConfig m_config; QTimer m_timer; int m_pollIntervalMs = MIN_RETRY_INTERVAL_MS; int m_pollFailureCount = 0; - std::vector m_watchedItems; + std::vector m_watchedScores; bool m_pollInProgress = false; muse::async::Channel m_pollingFailed; - muse::async::Notification m_fileNamesBeingConvertedChanged; + muse::async::Notification m_watchedScoresChanged; muse::async::Channel m_convertFinished; muse::async::Channel m_reviewRequested; }; diff --git a/src/project/internal/projectconfiguration.cpp b/src/project/internal/projectconfiguration.cpp index 770eda111aa11..eec8d9431b846 100644 --- a/src/project/internal/projectconfiguration.cpp +++ b/src/project/internal/projectconfiguration.cpp @@ -795,7 +795,7 @@ void ProjectConfiguration::setShowConvertFileProcessingDialog(bool show) settings()->setSharedValue(SHOW_CONVERT_FILE_PROCESSING_DIALOG, Val(show)); } -muse::io::path_t ProjectConfiguration::pendingConvertsJsonPath() const +muse::io::path_t ProjectConfiguration::watchedConvertsJsonPath() const { - return globalConfiguration()->userAppDataPath().appendingComponent("pending_converts.json"); + return globalConfiguration()->userAppDataPath().appendingComponent("watched_converts.json"); } diff --git a/src/project/internal/projectconfiguration.h b/src/project/internal/projectconfiguration.h index b40082d151010..5d7798f05a4e9 100644 --- a/src/project/internal/projectconfiguration.h +++ b/src/project/internal/projectconfiguration.h @@ -174,7 +174,7 @@ class ProjectConfiguration : public IProjectConfiguration, public muse::Contexta bool showConvertFileProcessingDialog() const override; void setShowConvertFileProcessingDialog(bool show) override; - muse::io::path_t pendingConvertsJsonPath() const override; + muse::io::path_t watchedConvertsJsonPath() const override; private: muse::io::path_t appTemplatesPath() const; diff --git a/src/project/iprojectconfiguration.h b/src/project/iprojectconfiguration.h index abfdf7f2ffe6f..742e8956308dd 100644 --- a/src/project/iprojectconfiguration.h +++ b/src/project/iprojectconfiguration.h @@ -180,7 +180,7 @@ class IProjectConfiguration : MODULE_GLOBAL_INTERFACE virtual bool showConvertFileProcessingDialog() const = 0; virtual void setShowConvertFileProcessingDialog(bool show) = 0; - virtual muse::io::path_t pendingConvertsJsonPath() const = 0; + virtual muse::io::path_t watchedConvertsJsonPath() const = 0; }; } diff --git a/src/project/qml/MuseScore/Project/internal/ScoresPage/recentscoresmodel.cpp b/src/project/qml/MuseScore/Project/internal/ScoresPage/recentscoresmodel.cpp index 95241e884de6d..2e9f7a622cf9c 100644 --- a/src/project/qml/MuseScore/Project/internal/ScoresPage/recentscoresmodel.cpp +++ b/src/project/qml/MuseScore/Project/internal/ScoresPage/recentscoresmodel.cpp @@ -45,7 +45,7 @@ void RecentScoresModel::load() updateRecentScores(); }); - convertFileToScoreService()->fileNamesBeingConvertedChanged().onNotify(this, [this]() { + convertFileToScoreService()->watchedScores().notification.onNotify(this, [this]() { updateRecentScores(); }); } @@ -64,10 +64,10 @@ void RecentScoresModel::setRecentScores(const std::vector& items) void RecentScoresModel::updateRecentScores() { const RecentFilesList& recentScores = recentFilesController()->recentFilesList(); - const StringList processingFiles = convertFileToScoreService()->fileNamesBeingConverted(); + const WatchedScoreList watchedScores = convertFileToScoreService()->watchedScores().val; std::vector items; - items.reserve(recentScores.size() + processingFiles.size() + 2); + items.reserve(recentScores.size() + watchedScores.size() + 2); QVariantMap addItem; addItem[NAME_KEY] = muse::qtrc("project", "New score"); @@ -77,9 +77,13 @@ void RecentScoresModel::updateRecentScores() addItem[IS_CLOUD_KEY] = false; items.push_back(addItem); - for (const String& fileName : processingFiles) { + for (const WatchedScore& watchedScore : watchedScores) { + if (watchedScore.convertStatus != ConvertStatus::Processing) { + continue; + } + QVariantMap obj; - obj[NAME_KEY] = fileName.toQString(); + obj[NAME_KEY] = watchedScore.name.toQString(); obj[IS_CREATE_NEW_KEY] = false; obj[IS_NO_RESULTS_FOUND_KEY] = false; obj[IS_PROCESSING_KEY] = true; diff --git a/src/project/tests/convertfiletoscoreservice_tests.cpp b/src/project/tests/convertfiletoscoreservice_tests.cpp index 2cf27e6fddaaa..78720c63b9d7a 100644 --- a/src/project/tests/convertfiletoscoreservice_tests.cpp +++ b/src/project/tests/convertfiletoscoreservice_tests.cpp @@ -595,8 +595,8 @@ TEST_F(Project_ConvertFileToScoreServiceTest, StartConvert_UploadFails_ForwardsF TEST_F(Project_ConvertFileToScoreServiceTest, StartConvert_UploadSucceeds_PersistsWatchedItemAndPolls) { // [GIVEN] The upload succeeds with queue id TEST_QUEUE_ID - ON_CALL(*m_configuration, pendingConvertsJsonPath()) - .WillByDefault(Return(io::path_t("/pending.json"))); + ON_CALL(*m_configuration, watchedConvertsJsonPath()) + .WillByDefault(Return(io::path_t("/watched.json"))); auto uploadProgress = std::make_shared(); const io::paths_t paths { "/some/path/file.pdf" }; @@ -614,7 +614,7 @@ TEST_F(Project_ConvertFileToScoreServiceTest, StartConvert_UploadSucceeds_Persis })); bool savedExpectedEntry = false; - EXPECT_CALL(*m_fileSystem, writeFile(io::path_t("/pending.json"), _)) + EXPECT_CALL(*m_fileSystem, writeFile(io::path_t("/watched.json"), _)) .WillOnce(Invoke([&](const io::path_t&, const ByteArray& data) { std::string err; JsonDocument json = JsonDocument::fromJson(data, &err); @@ -639,8 +639,8 @@ TEST_F(Project_ConvertFileToScoreServiceTest, StartConvert_UploadSucceeds_Persis TEST_F(Project_ConvertFileToScoreServiceTest, StartConvert_UploadSucceeds_PersistsAudio2ScoreType) { // [GIVEN] The upload succeeds for an Audio2Score conversion - ON_CALL(*m_configuration, pendingConvertsJsonPath()) - .WillByDefault(Return(io::path_t("/pending.json"))); + ON_CALL(*m_configuration, watchedConvertsJsonPath()) + .WillByDefault(Return(io::path_t("/watched.json"))); auto uploadProgress = std::make_shared(); ON_CALL(*m_convertService, upload(_)) @@ -651,7 +651,7 @@ TEST_F(Project_ConvertFileToScoreServiceTest, StartConvert_UploadSucceeds_Persis })); bool savedExpectedType = false; - EXPECT_CALL(*m_fileSystem, writeFile(io::path_t("/pending.json"), _)) + EXPECT_CALL(*m_fileSystem, writeFile(io::path_t("/watched.json"), _)) .WillOnce(Invoke([&](const io::path_t&, const ByteArray& data) { std::string err; JsonDocument json = JsonDocument::fromJson(data, &err); @@ -672,19 +672,19 @@ TEST_F(Project_ConvertFileToScoreServiceTest, StartConvert_UploadSucceeds_Persis } // ================================================== -// fileNamesBeingConverted() / fileNamesBeingConvertedChanged() +// watchedScores() // ================================================== -TEST_F(Project_ConvertFileToScoreServiceTest, FileNamesBeingConverted_Initially_Empty) +TEST_F(Project_ConvertFileToScoreServiceTest, WatchedScores_Initially_Empty) { - EXPECT_TRUE(m_service->fileNamesBeingConverted().empty()); + EXPECT_TRUE(m_service->watchedScores().val.empty()); } -TEST_F(Project_ConvertFileToScoreServiceTest, FileNamesBeingConverted_AfterStartConvert_ContainsFileNameAndFiresChanged) +TEST_F(Project_ConvertFileToScoreServiceTest, WatchedScores_AfterStartConvert_ContainsScoreAndFiresChanged) { // [GIVEN] The upload succeeds, and polling is left pending (the item stays watched) - ON_CALL(*m_configuration, pendingConvertsJsonPath()) - .WillByDefault(Return(io::path_t("/pending.json"))); + ON_CALL(*m_configuration, watchedConvertsJsonPath()) + .WillByDefault(Return(io::path_t("/watched.json"))); ON_CALL(*m_fileSystem, writeFile(_, _)) .WillByDefault(Return(make_ok())); @@ -698,7 +698,7 @@ TEST_F(Project_ConvertFileToScoreServiceTest, FileNamesBeingConverted_AfterStart })); bool changed = false; - m_service->fileNamesBeingConvertedChanged().onNotify(nullptr, [&] { + m_service->watchedScores().notification.onNotify(nullptr, [&] { changed = true; }); @@ -706,13 +706,14 @@ TEST_F(Project_ConvertFileToScoreServiceTest, FileNamesBeingConverted_AfterStart m_service->startConvert(OmrConvertInput { paths }, u"My Score"); uploadProgress->finish(ProgressResult::make_ok(Val(ValMap { { "id", Val(TEST_QUEUE_ID) } }))); - // [THEN] The file being converted is reported, and the change is signaled + // [THEN] The score being converted is reported, and the change is signaled EXPECT_TRUE(changed); - ASSERT_EQ(m_service->fileNamesBeingConverted().size(), 1u); - EXPECT_EQ(m_service->fileNamesBeingConverted().front(), u"My Score"); + const WatchedScoreList watchedScores = m_service->watchedScores().val; + ASSERT_EQ(watchedScores.size(), 1u); + EXPECT_EQ(watchedScores.front().name, u"My Score"); } -TEST_F(Project_ConvertFileToScoreServiceTest, FileNamesBeingConverted_AfterDone_NoLongerContainsFileName) +TEST_F(Project_ConvertFileToScoreServiceTest, WatchedScores_AfterDone_NoLongerContainsScore) { // [GIVEN] The queue reports the conversion as done, with its scoreId ConvertQueueItem item; @@ -725,7 +726,7 @@ TEST_F(Project_ConvertFileToScoreServiceTest, FileNamesBeingConverted_AfterDone_ .WillByDefault(Invoke([] { return okScoreInfo(555); })); bool changed = false; - m_service->fileNamesBeingConvertedChanged().onNotify(nullptr, [&] { + m_service->watchedScores().notification.onNotify(nullptr, [&] { changed = true; }); @@ -734,11 +735,11 @@ TEST_F(Project_ConvertFileToScoreServiceTest, FileNamesBeingConverted_AfterDone_ // [THEN] The item is reported ready and immediately erased, so it's no longer being converted EXPECT_TRUE(changed); - EXPECT_TRUE(m_service->fileNamesBeingConverted().empty()); + EXPECT_TRUE(m_service->watchedScores().val.empty()); } // ================================================== -// resumeConvert() / loadWatchedItems() +// resumeConvert() // ================================================== TEST_F(Project_ConvertFileToScoreServiceTest, ResumeConvert_LoadsPersistedWatchedItem) @@ -753,9 +754,9 @@ TEST_F(Project_ConvertFileToScoreServiceTest, ResumeConvert_LoadsPersistedWatche array << obj; JsonDocument json(array); - ON_CALL(*m_configuration, pendingConvertsJsonPath()) - .WillByDefault(Return(io::path_t("/pending.json"))); - ON_CALL(*m_fileSystem, readFile(io::path_t("/pending.json"))) + ON_CALL(*m_configuration, watchedConvertsJsonPath()) + .WillByDefault(Return(io::path_t("/watched.json"))); + ON_CALL(*m_fileSystem, readFile(io::path_t("/watched.json"))) .WillByDefault(Return(RetVal::make_ok(json.toJson()))); ON_CALL(*m_convertService, fetchQueue()) @@ -764,7 +765,7 @@ TEST_F(Project_ConvertFileToScoreServiceTest, ResumeConvert_LoadsPersistedWatche })); bool changed = false; - m_service->fileNamesBeingConvertedChanged().onNotify(nullptr, [&] { + m_service->watchedScores().notification.onNotify(nullptr, [&] { changed = true; }); @@ -773,8 +774,9 @@ TEST_F(Project_ConvertFileToScoreServiceTest, ResumeConvert_LoadsPersistedWatche // [THEN] The persisted item is restored and reported as being converted, and polling resumes EXPECT_TRUE(changed); - ASSERT_EQ(m_service->fileNamesBeingConverted().size(), 1u); - EXPECT_EQ(m_service->fileNamesBeingConverted().front(), u"My Score"); + const WatchedScoreList watchedScores = m_service->watchedScores().val; + ASSERT_EQ(watchedScores.size(), 1u); + EXPECT_EQ(watchedScores.front().name, u"My Score"); } TEST_F(Project_ConvertFileToScoreServiceTest, ResumeConvert_AwaitingReviewItemWithScoreId_SendsReviewRequested) @@ -791,9 +793,9 @@ TEST_F(Project_ConvertFileToScoreServiceTest, ResumeConvert_AwaitingReviewItemWi array << obj; JsonDocument json(array); - ON_CALL(*m_configuration, pendingConvertsJsonPath()) - .WillByDefault(Return(io::path_t("/pending.json"))); - ON_CALL(*m_fileSystem, readFile(io::path_t("/pending.json"))) + ON_CALL(*m_configuration, watchedConvertsJsonPath()) + .WillByDefault(Return(io::path_t("/watched.json"))); + ON_CALL(*m_fileSystem, readFile(io::path_t("/watched.json"))) .WillByDefault(Return(RetVal::make_ok(json.toJson()))); ON_CALL(*m_convertService, fetchQueue()) @@ -829,9 +831,9 @@ TEST_F(Project_ConvertFileToScoreServiceTest, ResumeConvert_AwaitingReviewItemWi array << obj; JsonDocument json(array); - ON_CALL(*m_configuration, pendingConvertsJsonPath()) - .WillByDefault(Return(io::path_t("/pending.json"))); - ON_CALL(*m_fileSystem, readFile(io::path_t("/pending.json"))) + ON_CALL(*m_configuration, watchedConvertsJsonPath()) + .WillByDefault(Return(io::path_t("/watched.json"))); + ON_CALL(*m_fileSystem, readFile(io::path_t("/watched.json"))) .WillByDefault(Return(RetVal::make_ok(json.toJson()))); ON_CALL(*m_convertService, fetchQueue()) @@ -1036,9 +1038,9 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_SameIdDifferentType_DoesNotCr array << omrObj << audioObj; JsonDocument json(array); - ON_CALL(*m_configuration, pendingConvertsJsonPath()) - .WillByDefault(Return(io::path_t("/pending.json"))); - ON_CALL(*m_fileSystem, readFile(io::path_t("/pending.json"))) + ON_CALL(*m_configuration, watchedConvertsJsonPath()) + .WillByDefault(Return(io::path_t("/watched.json"))); + ON_CALL(*m_fileSystem, readFile(io::path_t("/watched.json"))) .WillByDefault(Return(RetVal::make_ok(json.toJson()))); ON_CALL(*m_fileSystem, writeFile(_, _)) .WillByDefault(Return(make_ok())); @@ -1060,7 +1062,7 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_SameIdDifferentType_DoesNotCr ON_CALL(*m_convertService, fetchQueue()) .WillByDefault(Invoke([failedOmrItem, doneAudioItem] { return resolvedPromise >(RetVal::make_ok(ConvertQueueList { failedOmrItem, - doneAudioItem })); + doneAudioItem })); })); ON_CALL(*m_museScoreComService, downloadScoreInfo(999)) diff --git a/src/project/tests/mocks/convertfiletoscoreservicemock.h b/src/project/tests/mocks/convertfiletoscoreservicemock.h index a72c8690e999e..836c99bf58fba 100644 --- a/src/project/tests/mocks/convertfiletoscoreservicemock.h +++ b/src/project/tests/mocks/convertfiletoscoreservicemock.h @@ -38,8 +38,7 @@ class ConvertFileToScoreServiceMock : public IConvertFileToScoreService MOCK_METHOD(muse::Ret, startConvert, (const ConvertInput&, const muse::String&), (override)); MOCK_METHOD((muse::async::Channel), convertFinished, (), (const, override)); - MOCK_METHOD(muse::StringList, fileNamesBeingConverted, (), (const, override)); - MOCK_METHOD(muse::async::Notification, fileNamesBeingConvertedChanged, (), (const, override)); + MOCK_METHOD(muse::ValNt, watchedScores, (), (const, override)); MOCK_METHOD((muse::async::Channel), pollingFailed, (), (const, override)); MOCK_METHOD(void, retryPolling, (), (override)); diff --git a/src/project/tests/mocks/projectconfigurationmock.h b/src/project/tests/mocks/projectconfigurationmock.h index 8b1cf59b94216..ca86d67fdf45a 100644 --- a/src/project/tests/mocks/projectconfigurationmock.h +++ b/src/project/tests/mocks/projectconfigurationmock.h @@ -154,7 +154,7 @@ class ProjectConfigurationMock : public project::IProjectConfiguration MOCK_METHOD(bool, showConvertFileProcessingDialog, (), (const, override)); MOCK_METHOD(void, setShowConvertFileProcessingDialog, (bool), (override)); - MOCK_METHOD(muse::io::path_t, pendingConvertsJsonPath, (), (const, override)); + MOCK_METHOD(muse::io::path_t, watchedConvertsJsonPath, (), (const, override)); }; } diff --git a/src/project/types/converttypes.h b/src/project/types/converttypes.h index 59579bb55eee1..40624cfe40fd3 100644 --- a/src/project/types/converttypes.h +++ b/src/project/types/converttypes.h @@ -22,7 +22,9 @@ #pragma once +#include #include +#include #include @@ -32,12 +34,14 @@ #include "cloud/cloudtypes.h" #include "global/io/path.h" +#include "global/types/string.h" #include "global/types/secs.h" #include "global/types/ret.h" namespace mu::project { using ConvertConfig = muse::cloud::ConvertConfig; using ConvertType = muse::cloud::ConvertType; +using ConvertStatus = muse::cloud::ConvertStatus; using ReviewRating = muse::cloud::ReviewRating; using LinkSource = muse::cloud::LinkSource; using LinkSources = muse::cloud::LinkSources; @@ -91,4 +95,22 @@ struct PollingFailure { muse::secs_t nextInterval = 0.; bool gaveUp = false; }; + +struct WatchedScore { + int convertId = 0; + ConvertType convertType = ConvertType::Omr; + ConvertStatus convertStatus = ConvertStatus::Unknown; + std::optional scoreId; //! set once the score is ready and reported (Done/AwaitingReview) + muse::String name; + + bool operator==(const WatchedScore& other) const + { + return convertId == other.convertId + && convertType == other.convertType + && convertStatus == other.convertStatus + && scoreId == other.scoreId + && name == other.name; + } +}; +using WatchedScoreList = std::vector; } diff --git a/src/stubs/project/projectconfigurationstub.cpp b/src/stubs/project/projectconfigurationstub.cpp index 0111f932a0720..92092f9a09bcd 100644 --- a/src/stubs/project/projectconfigurationstub.cpp +++ b/src/stubs/project/projectconfigurationstub.cpp @@ -419,7 +419,7 @@ void ProjectConfigurationStub::setShowConvertFileProcessingDialog(bool) { } -muse::io::path_t ProjectConfigurationStub::pendingConvertsJsonPath() const +muse::io::path_t ProjectConfigurationStub::watchedConvertsJsonPath() const { return muse::io::path_t(); } diff --git a/src/stubs/project/projectconfigurationstub.h b/src/stubs/project/projectconfigurationstub.h index 5e8a924caed78..e5c27b409294d 100644 --- a/src/stubs/project/projectconfigurationstub.h +++ b/src/stubs/project/projectconfigurationstub.h @@ -154,6 +154,6 @@ class ProjectConfigurationStub : public IProjectConfiguration bool showConvertFileProcessingDialog() const override; void setShowConvertFileProcessingDialog(bool show) override; - muse::io::path_t pendingConvertsJsonPath() const override; + muse::io::path_t watchedConvertsJsonPath() const override; }; } From 455f3a81bda5932967bce97998e20ac3e8425636 Mon Sep 17 00:00:00 2001 From: Roman Pudashkin Date: Thu, 10 Sep 2026 14:19:19 +0300 Subject: [PATCH 04/10] Show connectivity toast after repeated conversion polling failures --- .../internal/convertfiletoscorescenario.cpp | 21 ++++++ .../internal/convertfiletoscorescenario.h | 3 + .../convertfiletoscorescenario_tests.cpp | 67 +++++-------------- 3 files changed, 42 insertions(+), 49 deletions(-) diff --git a/src/project/internal/convertfiletoscorescenario.cpp b/src/project/internal/convertfiletoscorescenario.cpp index ce999a090b890..c174adc90108c 100644 --- a/src/project/internal/convertfiletoscorescenario.cpp +++ b/src/project/internal/convertfiletoscorescenario.cpp @@ -38,6 +38,9 @@ using namespace muse::cloud; //! NOTE: gives the user a moment to land on the score before prompting for a review static constexpr int REVIEW_PROMPT_DELAY_MS = 10000; +//! NOTE: attempt 4 is ~5 minutes into retrying +static constexpr int RETRY_TOAST_ATTEMPT_THRESHOLD = 4; + static ConvertSelection toConvertSelection(const Val& val) { const QVariantMap map = val.toQVariant().toMap(); @@ -89,6 +92,17 @@ void ConvertFileToScoreScenario::init() checkPendingReview(); }); + service()->pollingFailed().onReceive(this, [this](const PollingFailure& failure) { + if (failure.attempt == 1) { + m_retryToastShown = false; + } + + if (!m_retryToastShown && failure.attempt >= RETRY_TOAST_ATTEMPT_THRESHOLD) { + m_retryToastShown = true; + showPollingFailureNotification(); + } + }); + globalContext()->currentProjectChanged().onNotify(this, [this]() { checkPendingReview(); }); @@ -479,6 +493,13 @@ void ConvertFileToScoreScenario::showConvertFailedNotification(const Ret& ret) }); } +void ConvertFileToScoreScenario::showPollingFailureNotification() +{ + toastService()->showWarning( + muse::trc("project/convert", "We’re having trouble connecting to the internet."), + muse::trc("project/convert", "We’ll keep trying intermittently.")); +} + void ConvertFileToScoreScenario::askReviewRating(int scoreId) { static constexpr int goodBtn = int(toast::ToastActionCode::Custom) + 1; diff --git a/src/project/internal/convertfiletoscorescenario.h b/src/project/internal/convertfiletoscorescenario.h index 9c23b137b0dca..676a896a72d1a 100644 --- a/src/project/internal/convertfiletoscorescenario.h +++ b/src/project/internal/convertfiletoscorescenario.h @@ -94,11 +94,14 @@ class ConvertFileToScoreScenario : public QObject, public IConvertFileToScoreSce void showFileProcessingDialog(); void showScoreReadyNotification(const ScoreInfo& scoreInfo); void showConvertFailedNotification(const muse::Ret& ret); + void showPollingFailureNotification(); void askReviewRating(int scoreId); void checkPendingReview(); muse::async::Channel m_convertFinished; std::map m_pendingReviews; + + bool m_retryToastShown = false; }; } diff --git a/src/project/tests/convertfiletoscorescenario_tests.cpp b/src/project/tests/convertfiletoscorescenario_tests.cpp index 0df247ba8501f..ac694dd83ef3b 100644 --- a/src/project/tests/convertfiletoscorescenario_tests.cpp +++ b/src/project/tests/convertfiletoscorescenario_tests.cpp @@ -427,66 +427,35 @@ TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Failure_Dismiss_DoesNotResta pumpEvents(); } -TEST_F(Project_ConvertFileToScoreScenarioTest, DISABLED_Init_ReviewRequested_Good_SubmitsGoodRating) -{ - // [GIVEN] The service's channels, wired up via init() - async::Channel convertFinished; - async::Channel reviewRequested; - ON_CALL(*m_service, convertFinished()).WillByDefault(Return(convertFinished)); - ON_CALL(*m_service, reviewRequested()).WillByDefault(Return(reviewRequested)); - m_scenario->init(); - - constexpr int goodBtn = int(toast::ToastActionCode::Custom) + 1; - constexpr int badBtn = int(toast::ToastActionCode::Custom) + 2; - - const std::string title = muse::trc("project/convert", "How does your score look?"); - const std::string text = muse::trc("project/convert", - "We’re always improving our score conversion accuracy. Let us know how we did with this one."); - - // [THEN] The review rating toast is shown, and the user's pick of "Good" is submitted - EXPECT_CALL(*m_toastService, - show(title, text, muse::ui::IconCode::Code::NONE, true, ToastActionCodesAre({ goodBtn, badBtn }))) - .WillOnce(Invoke([goodBtn](auto&&...) { - return resolvedToastResultPromise(toast::ToastResult(goodBtn)); - })); - - EXPECT_CALL(*m_service, submitReview(555, ReviewRating::Good, QString())) - .Times(1); - - // [WHEN] The service requests a review for a finished conversion - reviewRequested.send(555); - - pumpEvents(); -} +// ================================================== +// init() -- pollingFailed() +// ================================================== -TEST_F(Project_ConvertFileToScoreScenarioTest, DISABLED_Init_ReviewRequested_Bad_SubmitsBadRating) +TEST_F(Project_ConvertFileToScoreScenarioTest, Init_PollingFailed_ShowsToastOnceAfterThreshold) { // [GIVEN] The service's channels, wired up via init() async::Channel convertFinished; async::Channel reviewRequested; + async::Channel pollingFailed; ON_CALL(*m_service, convertFinished()).WillByDefault(Return(convertFinished)); ON_CALL(*m_service, reviewRequested()).WillByDefault(Return(reviewRequested)); + ON_CALL(*m_service, pollingFailed()).WillByDefault(Return(pollingFailed)); m_scenario->init(); - constexpr int goodBtn = int(toast::ToastActionCode::Custom) + 1; - constexpr int badBtn = int(toast::ToastActionCode::Custom) + 2; + const std::string title = muse::trc("project/convert", "We’re having trouble connecting to the internet."); + const std::string text = muse::trc("project/convert", "We’ll keep trying intermittently."); - const std::string title = muse::trc("project/convert", "How does your score look?"); - const std::string text = muse::trc("project/convert", - "We’re always improving our score conversion accuracy. Let us know how we did with this one."); - - // [THEN] The review rating toast is shown, and the user's pick of "Bad" is submitted - EXPECT_CALL(*m_toastService, - show(title, text, muse::ui::IconCode::Code::NONE, true, ToastActionCodesAre({ goodBtn, badBtn }))) - .WillOnce(Invoke([badBtn](auto&&...) { - return resolvedToastResultPromise(toast::ToastResult(badBtn)); - })); - - EXPECT_CALL(*m_service, submitReview(555, ReviewRating::Bad, QString())) - .Times(1); + // [THEN] The connectivity toast is shown exactly once + EXPECT_CALL(*m_toastService, showWarning(title, text)).Times(1); - // [WHEN] The service requests a review for a finished conversion - reviewRequested.send(555); + // [WHEN] Polling fails below the attempt threshold (4), then reaches and passes it, and + // eventually gives up + pollingFailed.send(PollingFailure { Ret(), 1, 5, secs_t(0), false }); + pollingFailed.send(PollingFailure { Ret(), 2, 5, secs_t(0), false }); + pollingFailed.send(PollingFailure { Ret(), 3, 5, secs_t(0), false }); + pollingFailed.send(PollingFailure { Ret(), 4, 5, secs_t(0), false }); + pollingFailed.send(PollingFailure { Ret(), 5, 5, secs_t(0), false }); + pollingFailed.send(PollingFailure { Ret(), 5, 5, secs_t(0), true }); pumpEvents(); } From dde1ec2f60395feb8ba9aed91a2c7c8ecafc240c Mon Sep 17 00:00:00 2001 From: Roman Pudashkin Date: Thu, 10 Sep 2026 20:04:59 +0300 Subject: [PATCH 05/10] Add missing resource guards --- .../tests/mocks/multiwindowsprovidermock.h | 58 +++++++++++++++++++ .../internal/convertfiletoscoreservice.cpp | 34 +++++++++-- .../internal/convertfiletoscoreservice.h | 3 + .../tests/convertfiletoscoreservice_tests.cpp | 4 ++ 4 files changed, 95 insertions(+), 4 deletions(-) create mode 100644 src/framework/multiwindows/tests/mocks/multiwindowsprovidermock.h diff --git a/src/framework/multiwindows/tests/mocks/multiwindowsprovidermock.h b/src/framework/multiwindows/tests/mocks/multiwindowsprovidermock.h new file mode 100644 index 0000000000000..08ad15803ce4f --- /dev/null +++ b/src/framework/multiwindows/tests/mocks/multiwindowsprovidermock.h @@ -0,0 +1,58 @@ +/* + * SPDX-License-Identifier: GPL-3.0-only + * MuseScore-Studio-CLA-applies + * + * MuseScore Studio + * Music Composition & Notation + * + * Copyright (C) 2026 MuseScore Limited and others + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 3 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +#pragma once + +#include + +#include "multiwindows/imultiwindowsprovider.h" + +namespace muse::mi { +class MultiWindowsProviderMock : public IMultiWindowsProvider +{ +public: + MOCK_METHOD(int, windowCount, (), (const, override)); + + MOCK_METHOD(bool, isProjectAlreadyOpened, (const io::path_t&), (const, override)); + MOCK_METHOD(void, activateWindowWithProject, (const io::path_t&), (override)); + MOCK_METHOD(bool, isHasWindowWithoutProject, (), (const, override)); + MOCK_METHOD(void, activateWindowWithoutProject, (const QStringList&), (override)); + MOCK_METHOD(bool, openNewWindow, (const QStringList&), (override)); + + MOCK_METHOD(bool, isPreferencesAlreadyOpened, (), (const, override)); + MOCK_METHOD(void, activateWindowWithOpenedPreferences, (), (const, override)); + MOCK_METHOD(void, settingsBeginTransaction, (), (override)); + MOCK_METHOD(void, settingsCommitTransaction, (), (override)); + MOCK_METHOD(void, settingsRollbackTransaction, (), (override)); + MOCK_METHOD(void, settingsReset, (), (override)); + MOCK_METHOD(void, settingsSetValue, (const std::string&, const Val&), (override)); + + MOCK_METHOD(bool, lockResource, (const std::string&), (override)); + MOCK_METHOD(bool, unlockResource, (const std::string&), (override)); + MOCK_METHOD(void, notifyAboutResourceChanged, (const std::string&), (override)); + MOCK_METHOD(async::Channel, resourceChanged, (), (override)); + + MOCK_METHOD(void, notifyAboutWindowWasQuited, (), (override)); + MOCK_METHOD(void, quitForAll, (), (override)); + MOCK_METHOD(void, quitAllAndRestartLast, (), (override)); + MOCK_METHOD(void, quitAllAndRunInstallation, (const io::path_t&), (override)); +}; +} diff --git a/src/project/internal/convertfiletoscoreservice.cpp b/src/project/internal/convertfiletoscoreservice.cpp index 0fc3027471492..5d84b558e825f 100644 --- a/src/project/internal/convertfiletoscoreservice.cpp +++ b/src/project/internal/convertfiletoscoreservice.cpp @@ -32,6 +32,7 @@ #include "network/networkerrors.h" #include "cloud/clouderrors.h" +#include "multiwindows/resourcelockguard.h" #include "global/serialization/json.h" #include "global/types/bytearray.h" @@ -110,6 +111,8 @@ static std::string errorCodeToString(ConvertErrorCode code) return std::string(); } +static const std::string WATCHED_CONVERTS_RESOURCE_NAME("WATCHED_CONVERTS"); + static std::string convertLogId(ConvertType type, int itemId) { return std::to_string(itemId) + " (type: " + convertTypeToString(type) + ")"; @@ -149,6 +152,18 @@ void ConvertFileToScoreService::init() m_config = config.val; } }); + + multiwindowsProvider()->resourceChanged().onReceive(this, [this](const std::string& resourceName) { + if (resourceName == WATCHED_CONVERTS_RESOURCE_NAME && !m_isSaving) { + loadWatchedScores(); + + if (!m_watchedScores.empty() && !m_timer.isActive()) { + m_timer.start(); + } + + m_watchedScoresChanged.notify(); + } + }); } void ConvertFileToScoreService::resumeConvert() @@ -402,7 +417,12 @@ void ConvertFileToScoreService::loadWatchedScores() m_watchedScores.clear(); - RetVal data = fileSystem()->readFile(configuration()->watchedConvertsJsonPath()); + RetVal data; + { + muse::mi::ReadResourceLockGuard resource_guard(multiwindowsProvider(), WATCHED_CONVERTS_RESOURCE_NAME); + data = fileSystem()->readFile(configuration()->watchedConvertsJsonPath()); + } + if (!data.ret || data.val.empty()) { if (!data.ret && data.ret.code() != static_cast(io::Err::FSNotExist)) { LOGE() << "Could not read the pending conversions file: " << data.ret; @@ -457,10 +477,16 @@ void ConvertFileToScoreService::saveWatchedScores() } JsonDocument json(array); - Ret ret = fileSystem()->writeFile(configuration()->watchedConvertsJsonPath(), json.toJson()); - if (!ret) { - LOGE() << "Could not save the pending conversions list: " << ret.toString(); + + m_isSaving = true; + { + muse::mi::WriteResourceLockGuard resource_guard(multiwindowsProvider(), WATCHED_CONVERTS_RESOURCE_NAME); + Ret ret = fileSystem()->writeFile(configuration()->watchedConvertsJsonPath(), json.toJson()); + if (!ret) { + LOGE() << "Could not save the pending conversions list: " << ret.toString(); + } } + m_isSaving = false; } void ConvertFileToScoreService::watch(ConvertType type, int itemId, const muse::String& convertedScoreName) diff --git a/src/project/internal/convertfiletoscoreservice.h b/src/project/internal/convertfiletoscoreservice.h index d4ab176b9bd48..1fe2983545c56 100644 --- a/src/project/internal/convertfiletoscoreservice.h +++ b/src/project/internal/convertfiletoscoreservice.h @@ -34,6 +34,7 @@ #include "cloud/musescorecom/imusescorecomservice.h" #include "io/ifilesystem.h" +#include "multiwindows/imultiwindowsprovider.h" #include "project/iprojectconfiguration.h" namespace mu::project { @@ -45,6 +46,7 @@ class ConvertFileToScoreService : public QObject, public IConvertFileToScoreServ muse::ContextInject museScoreComService = { this }; muse::GlobalInject fileSystem; muse::GlobalInject configuration; + muse::GlobalInject multiwindowsProvider; explicit ConvertFileToScoreService(const muse::modularity::ContextPtr& iocCtx, QObject* parent = nullptr) : QObject(parent), muse::Contextable(iocCtx) {} @@ -100,6 +102,7 @@ class ConvertFileToScoreService : public QObject, public IConvertFileToScoreServ int m_pollFailureCount = 0; std::vector m_watchedScores; bool m_pollInProgress = false; + bool m_isSaving = false; muse::async::Channel m_pollingFailed; muse::async::Notification m_watchedScoresChanged; diff --git a/src/project/tests/convertfiletoscoreservice_tests.cpp b/src/project/tests/convertfiletoscoreservice_tests.cpp index 78720c63b9d7a..40096e2e686ef 100644 --- a/src/project/tests/convertfiletoscoreservice_tests.cpp +++ b/src/project/tests/convertfiletoscoreservice_tests.cpp @@ -44,6 +44,7 @@ #include "global/tests/mocks/filesystemmock.h" #include "cloud/tests/mocks/musescorecomservicemock.h" #include "cloud/tests/mocks/musescorecomconvertservicemock.h" +#include "multiwindows/tests/mocks/multiwindowsprovidermock.h" using namespace ::testing; using namespace mu::project; @@ -136,10 +137,12 @@ class Project_ConvertFileToScoreServiceTest : public ::testing::Test m_convertService = std::make_shared >(); m_fileSystem = std::make_shared >(); m_configuration = std::make_shared >(); + m_multiWindowsProvider = std::make_shared >(); m_service->museScoreComService.set(m_museScoreComService); m_service->fileSystem.set(m_fileSystem); m_service->configuration.set(m_configuration); + m_service->multiwindowsProvider.set(m_multiWindowsProvider); ON_CALL(*m_museScoreComService, convert()) .WillByDefault(Return(m_convertService)); @@ -216,6 +219,7 @@ class Project_ConvertFileToScoreServiceTest : public ::testing::Test std::shared_ptr m_convertService; std::shared_ptr m_fileSystem; std::shared_ptr m_configuration; + std::shared_ptr m_multiWindowsProvider; }; } From ec04b909e612ec3cf422fb611553c02b22b9b1eb Mon Sep 17 00:00:00 2001 From: Roman Pudashkin Date: Thu, 10 Sep 2026 20:53:37 +0300 Subject: [PATCH 06/10] Make convert queue the source of truth for watched scores --- .../cloud/musescorecom/converttypes.h | 6 +- .../internal/convertfiletoscoreservice.cpp | 167 ++++++++++++------ .../internal/convertfiletoscoreservice.h | 1 - .../tests/convertfiletoscoreservice_tests.cpp | 34 ++++ src/project/types/converttypes.h | 2 + 5 files changed, 154 insertions(+), 56 deletions(-) diff --git a/src/framework/cloud/musescorecom/converttypes.h b/src/framework/cloud/musescorecom/converttypes.h index f0bc2d75060dc..8c72cc28cc32e 100644 --- a/src/framework/cloud/musescorecom/converttypes.h +++ b/src/framework/cloud/musescorecom/converttypes.h @@ -39,8 +39,10 @@ namespace muse::cloud { enum class ConvertType { - Omr, - Audio2Score + Omr = 0, + Audio2Score, + + Last = Audio2Score }; enum class ConvertStatus { diff --git a/src/project/internal/convertfiletoscoreservice.cpp b/src/project/internal/convertfiletoscoreservice.cpp index 5d84b558e825f..6bab2d2e00614 100644 --- a/src/project/internal/convertfiletoscoreservice.cpp +++ b/src/project/internal/convertfiletoscoreservice.cpp @@ -22,8 +22,10 @@ #include "convertfiletoscoreservice.h" #include +#include #include #include +#include #include @@ -444,13 +446,19 @@ void ConvertFileToScoreService::loadWatchedScores() for (size_t i = 0; i < array.size(); ++i) { const JsonObject obj = array.at(i).toObject(); - const int convertId = obj.value("id").toInt(); - const ConvertType type = static_cast(obj.value("type").toInt()); - const muse::String convertedScoreName = muse::String::fromStdString(obj.value("convertedScoreName").toStdString()); - const ConvertStatus status = static_cast(obj.value("status").toInt()); - const std::optional scoreId = obj.contains("scoreId") ? std::optional(obj.value("scoreId").toInt()) : std::nullopt; + const int typeInt = obj.value("type").toInt(); + if (typeInt < 0 || typeInt > static_cast(ConvertType::Last)) { + LOGW() << "Skipping conversion with unknown type: " << typeInt; + continue; + } - m_watchedScores.push_back(WatchedScore { convertId, type, status, scoreId, convertedScoreName }); + WatchedScore& watched = m_watchedScores.emplace_back(); + watched.convertId = obj.value("id").toInt(); + watched.convertType = static_cast(typeInt); + watched.convertStatus = static_cast(obj.value("status").toInt()); + watched.scoreId = obj.contains("scoreId") ? std::optional(obj.value("scoreId").toInt()) : std::nullopt; + watched.startedLocally = obj.value("startedLocally").toBool(); + watched.name = muse::String::fromStdString(obj.value("convertedScoreName").toStdString()); } } @@ -464,6 +472,7 @@ void ConvertFileToScoreService::saveWatchedScores() obj["id"] = watched.convertId; obj["type"] = static_cast(watched.convertType); obj["status"] = static_cast(watched.convertStatus); + obj["startedLocally"] = watched.startedLocally; if (!watched.name.isEmpty()) { obj["convertedScoreName"] = watched.name.toStdString(); @@ -493,7 +502,16 @@ void ConvertFileToScoreService::watch(ConvertType type, int itemId, const muse:: { LOGI() << "Start watching conversion " << convertLogId(convertedScoreName, type, itemId); - m_watchedScores.push_back(WatchedScore { itemId, type, ConvertStatus::Unknown, std::nullopt, convertedScoreName }); + const auto it = std::find_if(m_watchedScores.begin(), m_watchedScores.end(), [type, itemId](const WatchedScore& watched) { + return watched.convertType == type && watched.convertId == itemId; + }); + WatchedScore& watched = it != m_watchedScores.end() ? *it : m_watchedScores.emplace_back(); + watched.convertId = itemId; + watched.convertType = type; + watched.convertStatus = ConvertStatus::Processing; + watched.startedLocally = true; + watched.name = convertedScoreName; + saveWatchedScores(); m_watchedScoresChanged.notify(); @@ -572,47 +590,86 @@ void ConvertFileToScoreService::giveUpPolling(const Ret& ret) void ConvertFileToScoreService::updateWatchedScores(const ConvertQueueList& queue) { - const std::vector previousWatchedScores = m_watchedScores; + TRACEFUNC; + + constexpr size_t CONVERT_TYPE_COUNT = static_cast(ConvertType::Last) + 1; + std::array, CONVERT_TYPE_COUNT> oldByTypeAndId; + for (size_t i = 0; i < m_watchedScores.size(); ++i) { + const WatchedScore& watched = m_watchedScores[i]; + oldByTypeAndId[static_cast(watched.convertType)][watched.convertId] = i; + } + + std::vector seen(m_watchedScores.size(), false); + + //! NOTE: the queue is the source of truth - rebuild m_watchedScores from it every time, + //! since it may also contain conversions started outside MuseScore + std::vector newWatchedScores; + newWatchedScores.reserve(queue.size()); + + for (const ConvertQueueItem& queueItem : queue) { + const std::unordered_map& oldIndexById = oldByTypeAndId[static_cast(queueItem.type)]; + const auto it = oldIndexById.find(queueItem.id); + + if (it != oldIndexById.end()) { + //! NOTE: already watched - update it (status, scoreId) + seen[it->second] = true; + WatchedScore watched = m_watchedScores.at(it->second); + if (!queueItem.filename.isEmpty()) { + watched.name = queueItem.filename; + } + handleItem(watched, queueItem.status, queueItem.errorCode, queueItem.scoreId); - for (auto it = m_watchedScores.begin(); it != m_watchedScores.end();) { - WatchedScore& watched = *it; + if (watched.convertStatus != ConvertStatus::Done) { + newWatchedScores.push_back(watched); + } + continue; + } + + if (queueItem.status != ConvertStatus::Processing && queueItem.status != ConvertStatus::AwaitingReview) { + //! NOTE: not previously watched, and already terminal - nothing to watch for anymore + continue; + } + + LOGI() << "New external conversion: " << convertLogId(queueItem.type, queueItem.id); + + WatchedScore watched; + watched.convertId = queueItem.id; + watched.convertType = queueItem.type; + watched.name = queueItem.filename; - auto found = std::find_if(queue.begin(), queue.end(), [&watched](const ConvertQueueItem& queueItem) { - return queueItem.id == watched.convertId && queueItem.type == watched.convertType; - }); + handleItem(watched, queueItem.status, queueItem.errorCode, queueItem.scoreId); + + if (watched.convertStatus != ConvertStatus::Done) { + newWatchedScores.push_back(watched); + } + } + + for (size_t i = 0; i < m_watchedScores.size(); ++i) { + if (seen.at(i)) { + continue; + } - ConvertStatus status; - ConvertErrorCode errorCode; + WatchedScore dropped = m_watchedScores.at(i); + ConvertStatus status = ConvertStatus::Unknown; std::optional scoreId; - if (found != queue.end()) { - status = found->status; - errorCode = found->errorCode; - scoreId = found->scoreId; - } else if (watched.scoreId) { + if (dropped.scoreId) { //! NOTE: a Done/AwaitingReview item still reports its status while queued, but it may be //! dropped from the queue automatically at some point afterwards, once already reported ready status = ConvertStatus::Done; - errorCode = ConvertErrorCode::Unknown; - scoreId = watched.scoreId; + scoreId = dropped.scoreId; } else { //! NOTE: dropped from the queue before ever reporting a scoreId - there's no way to //! identify the resulting score anymore, so it can't be recovered as a success status = ConvertStatus::Failed; - errorCode = ConvertErrorCode::Unknown; } - handleItem(watched, status, errorCode, scoreId); - - if (status == ConvertStatus::Failed - || (status == ConvertStatus::Done && watched.scoreId)) { - it = m_watchedScores.erase(it); - } else { - ++it; - } + //! NOTE: always terminal, so never added back to newWatchedScores + handleItem(dropped, status, ConvertErrorCode::Unknown, scoreId); } - if (m_watchedScores != previousWatchedScores) { + if (m_watchedScores != newWatchedScores) { + m_watchedScores = std::move(newWatchedScores); saveWatchedScores(); m_watchedScoresChanged.notify(); } @@ -621,12 +678,12 @@ void ConvertFileToScoreService::updateWatchedScores(const ConvertQueueList& queu void ConvertFileToScoreService::handleItem(WatchedScore& watched, ConvertStatus status, ConvertErrorCode errorCode, std::optional scoreId) { - const bool statusChanged = watched.convertStatus != status; + const ConvertStatus previousStatus = watched.convertStatus; + const bool statusChanged = previousStatus != status; if (statusChanged) { LOGI() << "Conversion status changed: " << convertLogId(watched.name, watched.convertType, watched.convertId) << " -> " << convertStatusToString(status); } - watched.convertStatus = status; switch (status) { case ConvertStatus::Processing: @@ -634,8 +691,25 @@ void ConvertFileToScoreService::handleItem(WatchedScore& watched, ConvertStatus break; case ConvertStatus::AwaitingReview: case ConvertStatus::Done: - if (scoreId && !watched.scoreId) { - reportReady(watched, status, *scoreId); + if (!scoreId || watched.scoreId) { + break; + } + + if (watched.startedLocally) { + const RetVal scoreInfo = museScoreComService()->downloadScoreInfo(*scoreId); + if (!scoreInfo.ret) { + LOGW() << "Could not fetch score info for " << convertLogId(watched.name, watched.convertType, watched.convertId) + << ", will retry on next poll: " << scoreInfo.ret.toString(); + return; //! NOTE: watched.convertStatus stays at previousStatus - retried next poll + } + + finishConvert(make_ok(), scoreInfo.val); + } + + watched.scoreId = *scoreId; + + if (status == ConvertStatus::AwaitingReview) { + m_reviewRequested.send(*scoreId); } break; case ConvertStatus::Failed: { @@ -649,27 +723,14 @@ void ConvertFileToScoreService::handleItem(WatchedScore& watched, ConvertStatus LOGE() << ret.toString(); - finishConvert(ret); + if (watched.startedLocally) { + finishConvert(ret); + } break; } } -} - -void ConvertFileToScoreService::reportReady(WatchedScore& watched, ConvertStatus status, int scoreId) -{ - const RetVal scoreInfo = museScoreComService()->downloadScoreInfo(scoreId); - if (!scoreInfo.ret) { - LOGW() << "Could not fetch score info for " << convertLogId(watched.name, watched.convertType, watched.convertId) - << ", will retry on next poll: " << scoreInfo.ret.toString(); - return; - } - - watched.scoreId = scoreId; - finishConvert(make_ok(), scoreInfo.val); - if (status == ConvertStatus::AwaitingReview) { - m_reviewRequested.send(scoreId); - } + watched.convertStatus = status; } void ConvertFileToScoreService::finishConvert(const Ret& ret, const ScoreInfo& scoreInfo) diff --git a/src/project/internal/convertfiletoscoreservice.h b/src/project/internal/convertfiletoscoreservice.h index 1fe2983545c56..a26f658bdb1ac 100644 --- a/src/project/internal/convertfiletoscoreservice.h +++ b/src/project/internal/convertfiletoscoreservice.h @@ -89,7 +89,6 @@ class ConvertFileToScoreService : public QObject, public IConvertFileToScoreServ void handleItem(WatchedScore& watched, muse::cloud::ConvertStatus status, muse::cloud::ConvertErrorCode errorCode, std::optional scoreId); - void reportReady(WatchedScore& watched, muse::cloud::ConvertStatus status, int scoreId); void finishConvert(const muse::Ret& ret, const ScoreInfo& scoreInfo = ScoreInfo()); diff --git a/src/project/tests/convertfiletoscoreservice_tests.cpp b/src/project/tests/convertfiletoscoreservice_tests.cpp index 40096e2e686ef..38bb1f276866e 100644 --- a/src/project/tests/convertfiletoscoreservice_tests.cpp +++ b/src/project/tests/convertfiletoscoreservice_tests.cpp @@ -742,6 +742,38 @@ TEST_F(Project_ConvertFileToScoreServiceTest, WatchedScores_AfterDone_NoLongerCo EXPECT_TRUE(m_service->watchedScores().val.empty()); } +TEST_F(Project_ConvertFileToScoreServiceTest, WatchedScores_ExternalProcessingItem_AddedToWatchedScores) +{ + // [GIVEN] The queue reports an item that was never started via startConvert() locally, + // alongside the one that was + const int externalId = TEST_QUEUE_ID + 1; + + ConvertQueueItem ownItem; + ownItem.id = TEST_QUEUE_ID; + ownItem.type = ConvertType::Omr; + ownItem.status = ConvertStatus::Processing; + + ConvertQueueItem externalItem; + externalItem.id = externalId; + externalItem.type = ConvertType::Omr; + externalItem.status = ConvertStatus::Processing; + externalItem.filename = "Externally Started Score"; + + // [WHEN] Uploading and polling the status + deliverQueueStatus({ ownItem, externalItem }, ConvertType::Omr, TEST_QUEUE_ID, "My Score"); + + // [THEN] Both items are watched - the one we started, and the one discovered via the queue + const WatchedScoreList watchedScores = m_service->watchedScores().val; + ASSERT_EQ(watchedScores.size(), 2u); + + const auto externalIt = std::find_if(watchedScores.begin(), watchedScores.end(), [externalId](const WatchedScore& watched) { + return watched.convertId == externalId; + }); + ASSERT_NE(externalIt, watchedScores.end()); + EXPECT_EQ(externalIt->name, u"Externally Started Score"); + EXPECT_FALSE(externalIt->scoreId.has_value()); +} + // ================================================== // resumeConvert() // ================================================== @@ -1031,11 +1063,13 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_SameIdDifferentType_DoesNotCr JsonObject omrObj; omrObj["id"] = TEST_QUEUE_ID; omrObj["type"] = int(ConvertType::Omr); + omrObj["startedLocally"] = true; omrObj["convertedScoreName"] = "Omr Score"; JsonObject audioObj; audioObj["id"] = TEST_QUEUE_ID; audioObj["type"] = int(ConvertType::Audio2Score); + audioObj["startedLocally"] = true; audioObj["convertedScoreName"] = "Audio Score"; JsonArray array; diff --git a/src/project/types/converttypes.h b/src/project/types/converttypes.h index 40624cfe40fd3..3d6e9d98d32b4 100644 --- a/src/project/types/converttypes.h +++ b/src/project/types/converttypes.h @@ -101,6 +101,7 @@ struct WatchedScore { ConvertType convertType = ConvertType::Omr; ConvertStatus convertStatus = ConvertStatus::Unknown; std::optional scoreId; //! set once the score is ready and reported (Done/AwaitingReview) + bool startedLocally = false; //! true if started in MuseScore muse::String name; bool operator==(const WatchedScore& other) const @@ -109,6 +110,7 @@ struct WatchedScore { && convertType == other.convertType && convertStatus == other.convertStatus && scoreId == other.scoreId + && startedLocally == other.startedLocally && name == other.name; } }; From 5ef4425d3aa694a6dcec34d7cebc001ab4ce9a5f Mon Sep 17 00:00:00 2001 From: Roman Pudashkin Date: Fri, 11 Sep 2026 17:29:02 +0300 Subject: [PATCH 07/10] Convert file to score: update to the latest API spec --- src/framework/cloud/cloudtypes.h | 17 ++ .../cloud/musescorecom/converttypes.h | 2 + .../imusescorecomconvertservice.h | 3 + .../musescorecom/musescorecomservice.cpp | 218 +++++++++++------- .../cloud/musescorecom/musescorecomservice.h | 2 + .../mocks/musescorecomconvertservicemock.h | 2 + src/project/iconvertfiletoscoreservice.h | 3 + .../internal/convertfiletoscoreservice.cpp | 79 ++++--- .../internal/convertfiletoscoreservice.h | 2 + .../ConvertFileToScore/SaveAsField.qml | 2 + .../internal/ScoresPage/recentscoresmodel.cpp | 2 +- .../tests/convertfiletoscoreservice_tests.cpp | 59 ++++- .../mocks/convertfiletoscoreservicemock.h | 2 + src/project/types/converttypes.h | 9 +- 14 files changed, 290 insertions(+), 112 deletions(-) diff --git a/src/framework/cloud/cloudtypes.h b/src/framework/cloud/cloudtypes.h index 61b3079d91183..2e40802a92b27 100644 --- a/src/framework/cloud/cloudtypes.h +++ b/src/framework/cloud/cloudtypes.h @@ -22,6 +22,7 @@ #ifndef MUSE_CLOUD_CLOUDTYPES_H #define MUSE_CLOUD_CLOUDTYPES_H +#include #include #include @@ -30,6 +31,8 @@ #include "types/id.h" +#include "musescorecom/converttypes.h" + namespace muse::cloud { static const QString MUSESCORE_COM_CLOUD_CODE = "musescorecom"; static const QString AUDIO_COM_CLOUD_CODE = "audiocom"; @@ -126,6 +129,19 @@ struct ScoreInfo { } }; +struct ScoreConversionInfo { + int id = 0; + ConvertType type = ConvertType::Omr; + ConvertStatus status = ConvertStatus::Unknown; + + bool operator==(const ScoreConversionInfo& other) const + { + return id == other.id + && type == other.type + && status == other.status; + } +}; + struct ScoresList { struct Item { int id = 0; @@ -135,6 +151,7 @@ struct ScoresList { QString thumbnailUrl; Visibility visibility = Visibility::Private; int viewCount = 0; + std::optional conversion; //! set if the score originated from a conversion }; std::vector items; diff --git a/src/framework/cloud/musescorecom/converttypes.h b/src/framework/cloud/musescorecom/converttypes.h index 8c72cc28cc32e..3abf69174ff64 100644 --- a/src/framework/cloud/musescorecom/converttypes.h +++ b/src/framework/cloud/musescorecom/converttypes.h @@ -91,6 +91,7 @@ enum class ConvertErrorCode { TooComplex, DontRecognizeNotes, GeneralFailure, + BadParams, }; //! NOTE: key for ConvertErrorCode stored in Ret::data @@ -153,6 +154,7 @@ struct ConvertUploadData { ConvertType type = ConvertType::Omr; ConvertFileDataList files; QUrl link; // Audio2Score only + QString filename; // desired name for the converted score, no extension }; using ConvertUploadDataPtr = std::shared_ptr; diff --git a/src/framework/cloud/musescorecom/imusescorecomconvertservice.h b/src/framework/cloud/musescorecom/imusescorecomconvertservice.h index 09dcb5195775a..2819ae5202c46 100644 --- a/src/framework/cloud/musescorecom/imusescorecomconvertservice.h +++ b/src/framework/cloud/musescorecom/imusescorecomconvertservice.h @@ -42,6 +42,7 @@ namespace muse::cloud { /// submitReviewComment() may attach a comment afterwards, once the review has been submitted /// 4. Keep polling fetchQueue() until the status is Failed, or the item disappears /// from the queue (which should be treated the same as Done) +/// 5. deleteConversion() may be called at any point to remove an item from the queue class IMuseScoreComConvertService : MODULE_CONTEXT_INTERFACE { INTERFACE_ID(IMuseScoreComConvertService) @@ -58,6 +59,8 @@ class IMuseScoreComConvertService : MODULE_CONTEXT_INTERFACE virtual async::Promise > submitReview(ConvertType type, int id, ReviewRating review, const QString& comment = QString()) = 0; virtual async::Promise > submitReviewComment(ConvertType type, int id, const QString& comment) = 0; + + virtual async::Promise deleteConversion(ConvertType type, int id) = 0; }; using IMuseScoreComConvertServicePtr = std::shared_ptr; } diff --git a/src/framework/cloud/musescorecom/musescorecomservice.cpp b/src/framework/cloud/musescorecom/musescorecomservice.cpp index f1f9df67e2f37..7792d681a703e 100644 --- a/src/framework/cloud/musescorecom/musescorecomservice.cpp +++ b/src/framework/cloud/musescorecom/musescorecomservice.cpp @@ -44,11 +44,12 @@ using namespace muse::async; static const QString MUSESCORECOM_CLOUD_TITLE("MuseScore.com"); static const QString MUSESCORECOM_CLOUD_URL("https://musescore.com"); static const QString MUSESCORECOM_API_ROOT_URL("https://desktop.musescore.com/editor/v1"); +static const QString MUSESCORECOM_API_ROOT_URL_V2("https://desktop.musescore.com/editor/v2"); static const QUrl MUSESCORECOM_SCORE_MANAGER_URL(MUSESCORECOM_CLOUD_URL + "/my-scores"); static const QUrl MUSESCORECOM_USER_INFO_API_URL(MUSESCORECOM_API_ROOT_URL + "/me"); static const QUrl MUSESCORECOM_SCORE_INFO_API_URL(MUSESCORECOM_API_ROOT_URL + "/score/info"); -static const QUrl MUSESCORECOM_SCORES_LIST_API_URL(MUSESCORECOM_API_ROOT_URL + "/collection/scores"); +static const QUrl MUSESCORECOM_SCORES_LIST_API_URL(MUSESCORECOM_API_ROOT_URL_V2 + "/collection/scores"); static const QUrl MUSESCORECOM_SCORE_DOWNLOAD_API_URL(MUSESCORECOM_API_ROOT_URL + "/score/download"); static const QUrl MUSESCORECOM_SCORE_DOWNLOAD_SHARED_API_URL(MUSESCORECOM_API_ROOT_URL + "/score/download-shared"); static const QUrl MUSESCORECOM_UPLOAD_SCORE_API_URL(MUSESCORECOM_API_ROOT_URL + "/score/upload"); @@ -56,6 +57,8 @@ static const QUrl MUSESCORECOM_UPLOAD_AUDIO_API_URL(MUSESCORECOM_API_ROOT_URL + static const QUrl MUSESCORECOM_CONVERT_CONFIG_URL("https://musescore.com/static/musescore/studio/upload-config.json"); static const QUrl MUSESCORECOM_CONVERT_UPLOAD_API_URL(MUSESCORECOM_API_ROOT_URL + "/score/convert/convert"); +//! NOTE: same path as the upload endpoint, DELETE instead of POST +static const QUrl MUSESCORECOM_CONVERT_DELETE_API_URL = MUSESCORECOM_CONVERT_UPLOAD_API_URL; static const QUrl MUSESCORECOM_CONVERT_QUEUE_API_URL(MUSESCORECOM_API_ROOT_URL + "/score/convert/queue"); static const QUrl MUSESCORECOM_CONVERT_REVIEW_API_URL(MUSESCORECOM_API_ROOT_URL + "/score/convert/review"); static const QUrl MUSESCORECOM_CONVERT_COMMENT_API_URL(MUSESCORECOM_API_ROOT_URL + "/score/convert/comment"); @@ -93,6 +96,98 @@ static RetVal parseMuseScoreComAccountInfo(const QByteArray& data) return RetVal::make_ok(info); } +static QString convertTypeToApiString(ConvertType type) +{ + switch (type) { + case ConvertType::Omr: return "omr"; + case ConvertType::Audio2Score: return "audio2score"; + } + + return QString(); +} + +static ConvertType convertTypeFromApiString(const QString& str) +{ + if (str == "omr") { + return ConvertType::Omr; + } + + if (str == "audio2score") { + return ConvertType::Audio2Score; + } + + LOGW() << "Unknown convert type: \"" << str << "\", falling back to Omr"; + return ConvertType::Omr; +} + +static ConvertStatus convertStatusFromApiString(const QString& str) +{ + if (str == "processing") { + return ConvertStatus::Processing; + } else if (str == "awaiting_review") { + return ConvertStatus::AwaitingReview; + } else if (str == "done") { + return ConvertStatus::Done; + } else if (str == "failed") { + return ConvertStatus::Failed; + } + + return ConvertStatus::Unknown; +} + +static ConvertErrorCode convertErrorCodeFromApiString(const QString& str) +{ + if (str == "unsupported_format") { + return ConvertErrorCode::UnsupportedFormat; + } else if (str == "file_too_large") { + return ConvertErrorCode::FileTooLarge; + } else if (str == "too_many_files") { + return ConvertErrorCode::TooManyFiles; + } else if (str == "file_or_link_required") { + return ConvertErrorCode::FileOrLinkRequired; + } else if (str == "invalid_link") { + return ConvertErrorCode::InvalidLink; + } else if (str == "rate_limited") { + return ConvertErrorCode::RateLimited; + } else if (str == "mscz_not_ready") { + return ConvertErrorCode::MsczNotReady; + } else if (str == "no_need_review") { + return ConvertErrorCode::NoNeedReview; + } else if (str == "review_required") { + return ConvertErrorCode::ReviewRequired; + } else if (str == "comment_required") { + return ConvertErrorCode::CommentRequired; + } else if (str == "mu_status_various_file_issues") { + return ConvertErrorCode::VariousFileIssues; + } else if (str == "mu_status_too_complex") { + return ConvertErrorCode::TooComplex; + } else if (str == "mu_status_dont_recognize_notes") { + return ConvertErrorCode::DontRecognizeNotes; + } else if (str == "mu_status_general_failure") { + return ConvertErrorCode::GeneralFailure; + } else if (str == "bad_params") { + return ConvertErrorCode::BadParams; + } + + return ConvertErrorCode::Unknown; +} + +static std::optional parseScoreConversionInfo(const QJsonObject& itemObj) +{ + if (!itemObj.value("conversion").isObject()) { + return std::nullopt; + } + + QJsonObject conversionObj = itemObj.value("conversion").toObject(); + + ScoreConversionInfo result; + result.id = conversionObj.value("id").toInt(); + result.type = convertTypeFromApiString(conversionObj.value("type").toString()); + result.status = convertStatusFromApiString(conversionObj.value("status").toString()); + + return result; +} + static RetVal parseScoreList(const QByteArray& data, int batchNumber) { QJsonParseError err; @@ -131,6 +226,7 @@ static RetVal parseScoreList(const QByteArray& data, int batchNumber item.thumbnailUrl = itemObj.value("thumbnails").toObject().value("small").toString(); item.visibility = static_cast(itemObj.value("privacy").toInt()); item.viewCount = itemObj.value("view_count").toInt(); + item.conversion = parseScoreConversionInfo(itemObj); result.items.push_back(item); } @@ -256,80 +352,6 @@ static QHttpMultiPartPtr makeMultiPartForAudioUpload(QIODevice* audioData, const return multiPart; } -static QString convertTypeToApiString(ConvertType type) -{ - switch (type) { - case ConvertType::Omr: return "omr"; - case ConvertType::Audio2Score: return "audio2score"; - } - - return QString(); -} - -static ConvertType convertTypeFromApiString(const QString& str) -{ - if (str == "omr") { - return ConvertType::Omr; - } - - if (str == "audio2score") { - return ConvertType::Audio2Score; - } - - LOGW() << "Unknown convert type: \"" << str << "\", falling back to Omr"; - return ConvertType::Omr; -} - -static ConvertStatus convertStatusFromApiString(const QString& str) -{ - if (str == "processing") { - return ConvertStatus::Processing; - } else if (str == "awaiting_review") { - return ConvertStatus::AwaitingReview; - } else if (str == "done") { - return ConvertStatus::Done; - } else if (str == "failed") { - return ConvertStatus::Failed; - } - - return ConvertStatus::Unknown; -} - -static ConvertErrorCode convertErrorCodeFromApiString(const QString& str) -{ - if (str == "unsupported_format") { - return ConvertErrorCode::UnsupportedFormat; - } else if (str == "file_too_large") { - return ConvertErrorCode::FileTooLarge; - } else if (str == "too_many_files") { - return ConvertErrorCode::TooManyFiles; - } else if (str == "file_or_link_required") { - return ConvertErrorCode::FileOrLinkRequired; - } else if (str == "invalid_link") { - return ConvertErrorCode::InvalidLink; - } else if (str == "rate_limited") { - return ConvertErrorCode::RateLimited; - } else if (str == "mscz_not_ready") { - return ConvertErrorCode::MsczNotReady; - } else if (str == "no_need_review") { - return ConvertErrorCode::NoNeedReview; - } else if (str == "review_required") { - return ConvertErrorCode::ReviewRequired; - } else if (str == "comment_required") { - return ConvertErrorCode::CommentRequired; - } else if (str == "mu_status_various_file_issues") { - return ConvertErrorCode::VariousFileIssues; - } else if (str == "mu_status_too_complex") { - return ConvertErrorCode::TooComplex; - } else if (str == "mu_status_dont_recognize_notes") { - return ConvertErrorCode::DontRecognizeNotes; - } else if (str == "mu_status_general_failure") { - return ConvertErrorCode::GeneralFailure; - } - - return ConvertErrorCode::Unknown; -} - static void appendServerErrorCode(Ret& ret, const QByteArray& data) { QJsonParseError err; @@ -476,7 +498,8 @@ static QString sanitizeContentDispositionFilename(const QString& fileName) return sanitized; } -static QHttpMultiPartPtr makeMultiPartForConvertUpload(ConvertType type, const ConvertFileDataList& files, const QUrl& link) +static QHttpMultiPartPtr makeMultiPartForConvertUpload(ConvertType type, const ConvertFileDataList& files, const QUrl& link, + const QString& filename) { auto multiPart = std::make_shared(QHttpMultiPart::FormDataType); @@ -485,6 +508,13 @@ static QHttpMultiPartPtr makeMultiPartForConvertUpload(ConvertType type, const C typePart.setBody(convertTypeToApiString(type).toUtf8()); multiPart->append(typePart); + if (!filename.isEmpty()) { + QHttpPart filenamePart; + filenamePart.setHeader(QNetworkRequest::ContentDispositionHeader, QVariant("form-data; name=\"filename\"")); + filenamePart.setBody(filename.toUtf8()); + multiPart->append(filenamePart); + } + if (!link.isEmpty()) { QHttpPart linkPart; linkPart.setHeader(QNetworkRequest::ContentDispositionHeader, QVariant("form-data; name=\"link\"")); @@ -1031,7 +1061,7 @@ Promise MuseScoreComService::doUpload(const ConvertUploadDataPtr& data, Pro return resolve(uploadUrl.ret); } - auto multiPart = makeMultiPartForConvertUpload(data->type, data->files, data->link); + auto multiPart = makeMultiPartForConvertUpload(data->type, data->files, data->link, data->filename); auto receivedData = std::make_shared(); RetVal uploadProgress = m_networkManager->post(uploadUrl.val, multiPart, receivedData, headers()); @@ -1132,6 +1162,40 @@ Promise > MuseScoreComService::submitReview(ConvertType ty }); } +Promise MuseScoreComService::deleteConversion(ConvertType type, int id) +{ + return Promise([this, type, id](auto resolve, auto) { + QVariantMap params; + params["type"] = convertTypeToApiString(type); + params["id"] = id; + + RetVal url = prepareUrlForRequest(MUSESCORECOM_CONVERT_DELETE_API_URL, params); + if (!url.ret) { + return resolve(url.ret); + } + + auto receivedData = std::make_shared(); + RetVal progress = m_networkManager->del(url.val, receivedData, headers()); + if (!progress.ret) { + return resolve(progress.ret); + } + + progress.val.finished().onReceive(this, [this, receivedData, resolve](const ProgressResult& res) { + if (!res.ret) { + printServerReply(*receivedData); + Ret ret = uploadingDownloadingRetFromRawRet(res.ret); + appendServerErrorCode(ret, receivedData->data()); + (void)resolve(ret); + return; + } + + (void)resolve(make_ok()); + }); + + return Promise::dummy_result(); + }); +} + Promise > MuseScoreComService::submitReviewComment(ConvertType type, int id, const QString& comment) { return Promise >([this, type, id, comment](auto resolve, auto) { diff --git a/src/framework/cloud/musescorecom/musescorecomservice.h b/src/framework/cloud/musescorecom/musescorecomservice.h index 0a9b8749f835a..fec4ef9d8fbbd 100644 --- a/src/framework/cloud/musescorecom/musescorecomservice.h +++ b/src/framework/cloud/musescorecom/musescorecomservice.h @@ -75,6 +75,8 @@ class MuseScoreComService : public IMuseScoreComService, public IMuseScoreComCon const QString& comment = QString()) override; async::Promise > submitReviewComment(ConvertType type, int id, const QString& comment) override; + async::Promise deleteConversion(ConvertType type, int id) override; + private: ServerConfig serverConfig() const override; diff --git a/src/framework/cloud/tests/mocks/musescorecomconvertservicemock.h b/src/framework/cloud/tests/mocks/musescorecomconvertservicemock.h index 4bab8a51bdced..90346f7e9f1cd 100644 --- a/src/framework/cloud/tests/mocks/musescorecomconvertservicemock.h +++ b/src/framework/cloud/tests/mocks/musescorecomconvertservicemock.h @@ -37,5 +37,7 @@ class MuseScoreComConvertServiceMock : public IMuseScoreComConvertService MOCK_METHOD(async::Promise >, submitReview, (ConvertType, int, ReviewRating, const QString&), (override)); MOCK_METHOD(async::Promise >, submitReviewComment, (ConvertType, int, const QString&), (override)); + + MOCK_METHOD(async::Promise, deleteConversion, (ConvertType, int), (override)); }; } diff --git a/src/project/iconvertfiletoscoreservice.h b/src/project/iconvertfiletoscoreservice.h index 87220a70e4ea3..79575672477ba 100644 --- a/src/project/iconvertfiletoscoreservice.h +++ b/src/project/iconvertfiletoscoreservice.h @@ -71,6 +71,9 @@ class IConvertFileToScoreService : MODULE_CONTEXT_INTERFACE virtual muse::async::Channel reviewRequested() const = 0; virtual void submitReview(int scoreId, ReviewRating rating, const QString& comment = QString()) = 0; virtual void submitReviewComment(int scoreId, const QString& comment) = 0; + + //! Deletes a watched conversion, both server-side and from watchedScores() + virtual void deleteConversion(ConvertType type, int convertId) = 0; }; using IConvertFileToScoreServicePtr = std::shared_ptr; diff --git a/src/project/internal/convertfiletoscoreservice.cpp b/src/project/internal/convertfiletoscoreservice.cpp index 6bab2d2e00614..343a091370270 100644 --- a/src/project/internal/convertfiletoscoreservice.cpp +++ b/src/project/internal/convertfiletoscoreservice.cpp @@ -109,6 +109,7 @@ static std::string errorCodeToString(ConvertErrorCode code) case ConvertErrorCode::TooComplex: return "The file is too large or there is a problem with access to this file"; case ConvertErrorCode::DontRecognizeNotes: return "Invalid file, could not recognize notes"; case ConvertErrorCode::GeneralFailure: return "Something went wrong"; + case ConvertErrorCode::BadParams: return "Invalid conversion parameters"; } return std::string(); } @@ -182,7 +183,7 @@ void ConvertFileToScoreService::resumeConvert() m_watchedScoresChanged.notify(); for (const WatchedScore& watched : m_watchedScores) { - if (watched.convertStatus == ConvertStatus::AwaitingReview && watched.scoreId) { + if (watched.conversion.status == ConvertStatus::AwaitingReview && watched.scoreId) { m_reviewRequested.send(*watched.scoreId); } } @@ -318,7 +319,8 @@ Ret ConvertFileToScoreService::startConvert(const ConvertInput& input, const mus } const ConvertType type = convertTypeOf(input); - auto data = std::make_shared(ConvertUploadData { type, std::move(files), link }); + auto data = std::make_shared(ConvertUploadData { type, std::move(files), link, + convertedScoreName.toQString() }); ProgressPtr progress = museScoreComService()->convert()->upload(data); progress->progressChanged().onReceive(this, [convertedScoreName](int64_t current, int64_t total, const std::string&) { @@ -384,8 +386,8 @@ void ConvertFileToScoreService::submitReview(int scoreId, ReviewRating rating, c return; } - const ConvertType type = watched->convertType; - const int convertId = watched->convertId; + const ConvertType type = watched->conversion.type; + const int convertId = watched->conversion.id; museScoreComService()->convert()->submitReview(type, convertId, rating, comment) .onResolve(this, [type, convertId](const RetVal& submitRes) { @@ -402,8 +404,8 @@ void ConvertFileToScoreService::submitReviewComment(int scoreId, const QString& return; } - const ConvertType type = watched->convertType; - const int convertId = watched->convertId; + const ConvertType type = watched->conversion.type; + const int convertId = watched->conversion.id; museScoreComService()->convert()->submitReviewComment(type, convertId, comment) .onResolve(this, [type, convertId](const RetVal& submitRes) { @@ -413,6 +415,29 @@ void ConvertFileToScoreService::submitReviewComment(int scoreId, const QString& }); } +void ConvertFileToScoreService::deleteConversion(ConvertType type, int convertId) +{ + museScoreComService()->convert()->deleteConversion(type, convertId) + .onResolve(this, [this, type, convertId](const Ret& ret) { + if (!ret) { + LOGE() << "Could not delete conversion " << convertLogId(type, convertId) << ": " << ret.toString(); + return; + } + + const auto it = std::find_if(m_watchedScores.begin(), m_watchedScores.end(), [type, convertId](const WatchedScore& watched) { + return watched.conversion.type == type && watched.conversion.id == convertId; + }); + + if (it == m_watchedScores.end()) { + return; + } + + m_watchedScores.erase(it); + saveWatchedScores(); + m_watchedScoresChanged.notify(); + }); +} + void ConvertFileToScoreService::loadWatchedScores() { TRACEFUNC; @@ -453,9 +478,9 @@ void ConvertFileToScoreService::loadWatchedScores() } WatchedScore& watched = m_watchedScores.emplace_back(); - watched.convertId = obj.value("id").toInt(); - watched.convertType = static_cast(typeInt); - watched.convertStatus = static_cast(obj.value("status").toInt()); + watched.conversion.id = obj.value("id").toInt(); + watched.conversion.type = static_cast(typeInt); + watched.conversion.status = static_cast(obj.value("status").toInt()); watched.scoreId = obj.contains("scoreId") ? std::optional(obj.value("scoreId").toInt()) : std::nullopt; watched.startedLocally = obj.value("startedLocally").toBool(); watched.name = muse::String::fromStdString(obj.value("convertedScoreName").toStdString()); @@ -469,9 +494,9 @@ void ConvertFileToScoreService::saveWatchedScores() JsonArray array; for (const WatchedScore& watched : m_watchedScores) { JsonObject obj; - obj["id"] = watched.convertId; - obj["type"] = static_cast(watched.convertType); - obj["status"] = static_cast(watched.convertStatus); + obj["id"] = watched.conversion.id; + obj["type"] = static_cast(watched.conversion.type); + obj["status"] = static_cast(watched.conversion.status); obj["startedLocally"] = watched.startedLocally; if (!watched.name.isEmpty()) { @@ -503,12 +528,12 @@ void ConvertFileToScoreService::watch(ConvertType type, int itemId, const muse:: LOGI() << "Start watching conversion " << convertLogId(convertedScoreName, type, itemId); const auto it = std::find_if(m_watchedScores.begin(), m_watchedScores.end(), [type, itemId](const WatchedScore& watched) { - return watched.convertType == type && watched.convertId == itemId; + return watched.conversion.type == type && watched.conversion.id == itemId; }); WatchedScore& watched = it != m_watchedScores.end() ? *it : m_watchedScores.emplace_back(); - watched.convertId = itemId; - watched.convertType = type; - watched.convertStatus = ConvertStatus::Processing; + watched.conversion.id = itemId; + watched.conversion.type = type; + watched.conversion.status = ConvertStatus::Processing; watched.startedLocally = true; watched.name = convertedScoreName; @@ -596,7 +621,7 @@ void ConvertFileToScoreService::updateWatchedScores(const ConvertQueueList& queu std::array, CONVERT_TYPE_COUNT> oldByTypeAndId; for (size_t i = 0; i < m_watchedScores.size(); ++i) { const WatchedScore& watched = m_watchedScores[i]; - oldByTypeAndId[static_cast(watched.convertType)][watched.convertId] = i; + oldByTypeAndId[static_cast(watched.conversion.type)][watched.conversion.id] = i; } std::vector seen(m_watchedScores.size(), false); @@ -611,7 +636,7 @@ void ConvertFileToScoreService::updateWatchedScores(const ConvertQueueList& queu const auto it = oldIndexById.find(queueItem.id); if (it != oldIndexById.end()) { - //! NOTE: already watched - update it (status, scoreId) + //! NOTE: already watched - update it (name, status, scoreId) seen[it->second] = true; WatchedScore watched = m_watchedScores.at(it->second); if (!queueItem.filename.isEmpty()) { @@ -619,7 +644,7 @@ void ConvertFileToScoreService::updateWatchedScores(const ConvertQueueList& queu } handleItem(watched, queueItem.status, queueItem.errorCode, queueItem.scoreId); - if (watched.convertStatus != ConvertStatus::Done) { + if (watched.conversion.status != ConvertStatus::Done) { newWatchedScores.push_back(watched); } continue; @@ -633,13 +658,13 @@ void ConvertFileToScoreService::updateWatchedScores(const ConvertQueueList& queu LOGI() << "New external conversion: " << convertLogId(queueItem.type, queueItem.id); WatchedScore watched; - watched.convertId = queueItem.id; - watched.convertType = queueItem.type; + watched.conversion.id = queueItem.id; + watched.conversion.type = queueItem.type; watched.name = queueItem.filename; handleItem(watched, queueItem.status, queueItem.errorCode, queueItem.scoreId); - if (watched.convertStatus != ConvertStatus::Done) { + if (watched.conversion.status != ConvertStatus::Done) { newWatchedScores.push_back(watched); } } @@ -678,10 +703,10 @@ void ConvertFileToScoreService::updateWatchedScores(const ConvertQueueList& queu void ConvertFileToScoreService::handleItem(WatchedScore& watched, ConvertStatus status, ConvertErrorCode errorCode, std::optional scoreId) { - const ConvertStatus previousStatus = watched.convertStatus; + const ConvertStatus previousStatus = watched.conversion.status; const bool statusChanged = previousStatus != status; if (statusChanged) { - LOGI() << "Conversion status changed: " << convertLogId(watched.name, watched.convertType, watched.convertId) + LOGI() << "Conversion status changed: " << convertLogId(watched.name, watched.conversion.type, watched.conversion.id) << " -> " << convertStatusToString(status); } @@ -698,9 +723,9 @@ void ConvertFileToScoreService::handleItem(WatchedScore& watched, ConvertStatus if (watched.startedLocally) { const RetVal scoreInfo = museScoreComService()->downloadScoreInfo(*scoreId); if (!scoreInfo.ret) { - LOGW() << "Could not fetch score info for " << convertLogId(watched.name, watched.convertType, watched.convertId) + LOGW() << "Could not fetch score info for " << convertLogId(watched.name, watched.conversion.type, watched.conversion.id) << ", will retry on next poll: " << scoreInfo.ret.toString(); - return; //! NOTE: watched.convertStatus stays at previousStatus - retried next poll + return; //! NOTE: watched.conversion.status stays at previousStatus - retried next poll } finishConvert(make_ok(), scoreInfo.val); @@ -730,7 +755,7 @@ void ConvertFileToScoreService::handleItem(WatchedScore& watched, ConvertStatus } } - watched.convertStatus = status; + watched.conversion.status = status; } void ConvertFileToScoreService::finishConvert(const Ret& ret, const ScoreInfo& scoreInfo) diff --git a/src/project/internal/convertfiletoscoreservice.h b/src/project/internal/convertfiletoscoreservice.h index a26f658bdb1ac..ae634d43405aa 100644 --- a/src/project/internal/convertfiletoscoreservice.h +++ b/src/project/internal/convertfiletoscoreservice.h @@ -72,6 +72,8 @@ class ConvertFileToScoreService : public QObject, public IConvertFileToScoreServ void submitReview(int scoreId, ReviewRating rating, const QString& comment = QString()) override; void submitReviewComment(int scoreId, const QString& comment) override; + void deleteConversion(ConvertType type, int convertId) override; + private: static constexpr int MIN_RETRY_INTERVAL_MS = 60000; static constexpr int MAX_RETRY_INTERVAL_MS = 10 * 60000; diff --git a/src/project/qml/MuseScore/Project/internal/ConvertFileToScore/SaveAsField.qml b/src/project/qml/MuseScore/Project/internal/ConvertFileToScore/SaveAsField.qml index 39ea13dbd2a96..7e93defb39c55 100644 --- a/src/project/qml/MuseScore/Project/internal/ConvertFileToScore/SaveAsField.qml +++ b/src/project/qml/MuseScore/Project/internal/ConvertFileToScore/SaveAsField.qml @@ -51,6 +51,8 @@ Column { width: parent.width + maximumLength: 255 + navigation.panel: root.navigationPanel navigation.order: root.navigationOrder navigation.accessible.name: label.text + " " + input.currentText diff --git a/src/project/qml/MuseScore/Project/internal/ScoresPage/recentscoresmodel.cpp b/src/project/qml/MuseScore/Project/internal/ScoresPage/recentscoresmodel.cpp index 2e9f7a622cf9c..1faf6d8852520 100644 --- a/src/project/qml/MuseScore/Project/internal/ScoresPage/recentscoresmodel.cpp +++ b/src/project/qml/MuseScore/Project/internal/ScoresPage/recentscoresmodel.cpp @@ -78,7 +78,7 @@ void RecentScoresModel::updateRecentScores() items.push_back(addItem); for (const WatchedScore& watchedScore : watchedScores) { - if (watchedScore.convertStatus != ConvertStatus::Processing) { + if (watchedScore.conversion.status != ConvertStatus::Processing) { continue; } diff --git a/src/project/tests/convertfiletoscoreservice_tests.cpp b/src/project/tests/convertfiletoscoreservice_tests.cpp index 38bb1f276866e..2bbe8403009f0 100644 --- a/src/project/tests/convertfiletoscoreservice_tests.cpp +++ b/src/project/tests/convertfiletoscoreservice_tests.cpp @@ -767,7 +767,7 @@ TEST_F(Project_ConvertFileToScoreServiceTest, WatchedScores_ExternalProcessingIt ASSERT_EQ(watchedScores.size(), 2u); const auto externalIt = std::find_if(watchedScores.begin(), watchedScores.end(), [externalId](const WatchedScore& watched) { - return watched.convertId == externalId; + return watched.conversion.id == externalId; }); ASSERT_NE(externalIt, watchedScores.end()); EXPECT_EQ(externalIt->name, u"Externally Started Score"); @@ -1090,12 +1090,14 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_SameIdDifferentType_DoesNotCr failedOmrItem.type = ConvertType::Omr; failedOmrItem.status = ConvertStatus::Failed; failedOmrItem.errorCode = ConvertErrorCode::FileTooLarge; + failedOmrItem.filename = "Omr Score"; ConvertQueueItem doneAudioItem; doneAudioItem.id = TEST_QUEUE_ID; doneAudioItem.type = ConvertType::Audio2Score; doneAudioItem.status = ConvertStatus::Done; doneAudioItem.scoreId = 999; + doneAudioItem.filename = "Audio Score"; ON_CALL(*m_convertService, fetchQueue()) .WillByDefault(Invoke([failedOmrItem, doneAudioItem] { @@ -1135,6 +1137,7 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_FailedStatus_ForwardsProcessi item.type = ConvertType::Omr; item.status = ConvertStatus::Failed; item.errorCode = ConvertErrorCode::FileTooLarge; + item.filename = "My Score"; bool received = false; Ret receivedRet; @@ -1504,3 +1507,57 @@ TEST_F(Project_ConvertFileToScoreServiceTest, SubmitReviewComment_DelegatesToCon // [WHEN] Submitting a follow-up comment m_service->submitReviewComment(555, "Great job"); } + +// ================================================== +// deleteConversion() +// ================================================== + +TEST_F(Project_ConvertFileToScoreServiceTest, DeleteConversion_Success_RemovesFromWatchedScores) +{ + // [GIVEN] A watched, still-processing conversion + ConvertQueueItem item; + item.id = TEST_QUEUE_ID; + item.type = ConvertType::Omr; + item.status = ConvertStatus::Processing; + + deliverQueueStatus({ item }, ConvertType::Omr, TEST_QUEUE_ID, "My Score"); + ASSERT_EQ(m_service->watchedScores().val.size(), 1u); + + // [THEN] The deletion is delegated to the convert service + EXPECT_CALL(*m_convertService, deleteConversion(ConvertType::Omr, TEST_QUEUE_ID)) + .WillOnce(Invoke([](auto, auto) { + return resolvedPromise(make_ok()); + })); + + // [WHEN] Deleting the conversion + m_service->deleteConversion(ConvertType::Omr, TEST_QUEUE_ID); + pumpEvents(); + + // [THEN] It is no longer watched + EXPECT_TRUE(m_service->watchedScores().val.empty()); +} + +TEST_F(Project_ConvertFileToScoreServiceTest, DeleteConversion_Fails_KeepsWatching) +{ + // [GIVEN] A watched, still-processing conversion + ConvertQueueItem item; + item.id = TEST_QUEUE_ID; + item.type = ConvertType::Omr; + item.status = ConvertStatus::Processing; + + deliverQueueStatus({ item }, ConvertType::Omr, TEST_QUEUE_ID, "My Score"); + ASSERT_EQ(m_service->watchedScores().val.size(), 1u); + + // [THEN] The deletion is delegated to the convert service, but fails + EXPECT_CALL(*m_convertService, deleteConversion(ConvertType::Omr, TEST_QUEUE_ID)) + .WillOnce(Invoke([](auto, auto) { + return resolvedPromise(make_ret(muse::cloud::Err::UnknownError)); + })); + + // [WHEN] Deleting the conversion + m_service->deleteConversion(ConvertType::Omr, TEST_QUEUE_ID); + pumpEvents(); + + // [THEN] It is still watched + EXPECT_EQ(m_service->watchedScores().val.size(), 1u); +} diff --git a/src/project/tests/mocks/convertfiletoscoreservicemock.h b/src/project/tests/mocks/convertfiletoscoreservicemock.h index 836c99bf58fba..de04559c77f31 100644 --- a/src/project/tests/mocks/convertfiletoscoreservicemock.h +++ b/src/project/tests/mocks/convertfiletoscoreservicemock.h @@ -46,5 +46,7 @@ class ConvertFileToScoreServiceMock : public IConvertFileToScoreService MOCK_METHOD((muse::async::Channel), reviewRequested, (), (const, override)); MOCK_METHOD(void, submitReview, (int, ReviewRating, const QString&), (override)); MOCK_METHOD(void, submitReviewComment, (int, const QString&), (override)); + + MOCK_METHOD(void, deleteConversion, (ConvertType, int), (override)); }; } diff --git a/src/project/types/converttypes.h b/src/project/types/converttypes.h index 3d6e9d98d32b4..76918f449a688 100644 --- a/src/project/types/converttypes.h +++ b/src/project/types/converttypes.h @@ -46,6 +46,7 @@ using ReviewRating = muse::cloud::ReviewRating; using LinkSource = muse::cloud::LinkSource; using LinkSources = muse::cloud::LinkSources; using ScoreInfo = muse::cloud::ScoreInfo; +using ScoreConversionInfo = muse::cloud::ScoreConversionInfo; struct OmrConvertInput { muse::io::paths_t paths; @@ -97,18 +98,14 @@ struct PollingFailure { }; struct WatchedScore { - int convertId = 0; - ConvertType convertType = ConvertType::Omr; - ConvertStatus convertStatus = ConvertStatus::Unknown; + ScoreConversionInfo conversion; std::optional scoreId; //! set once the score is ready and reported (Done/AwaitingReview) bool startedLocally = false; //! true if started in MuseScore muse::String name; bool operator==(const WatchedScore& other) const { - return convertId == other.convertId - && convertType == other.convertType - && convertStatus == other.convertStatus + return conversion == other.conversion && scoreId == other.scoreId && startedLocally == other.startedLocally && name == other.name; From 5a758b3953afef95ba0654f96cf208244c158fe3 Mon Sep 17 00:00:00 2001 From: Roman Pudashkin Date: Mon, 14 Sep 2026 10:41:15 +0300 Subject: [PATCH 08/10] Fix date created / updated and improve logs --- .../cloud/musescorecom/converttypes.h | 8 +++--- .../musescorecom/musescorecomservice.cpp | 4 +-- .../internal/convertfiletoscoreservice.cpp | 28 +++++++++---------- 3 files changed, 19 insertions(+), 21 deletions(-) diff --git a/src/framework/cloud/musescorecom/converttypes.h b/src/framework/cloud/musescorecom/converttypes.h index 3abf69174ff64..4026fe78ed72f 100644 --- a/src/framework/cloud/musescorecom/converttypes.h +++ b/src/framework/cloud/musescorecom/converttypes.h @@ -171,8 +171,8 @@ struct ConvertQueueItem { QString filename; QString link; //! audio2score only std::optional scoreId; //! set once the score is ready (AwaitingReview/Done) - QDateTime createdAt; - QDateTime updatedAt; + QDateTime dataCreated; + QDateTime dataUpdated; ConvertErrorCode errorCode = ConvertErrorCode::Unknown; }; @@ -197,7 +197,7 @@ inline muse::logger::Stream& operator<<(muse::logger::Stream& s, const muse::clo << ", type: " << muse::cloud::convertTypeToString(item.type) << ", status: " << muse::cloud::convertStatusToString(item.status) << ", scoreId: " << (item.scoreId ? QString::number(*item.scoreId) : QString("none")) - << ", createdAt: " << dateTimeToString(item.createdAt) - << ", updatedAt: " << dateTimeToString(item.updatedAt); + << ", dataCreated: " << dateTimeToString(item.dataCreated) + << ", dataUpdated: " << dateTimeToString(item.dataUpdated); return s; } diff --git a/src/framework/cloud/musescorecom/musescorecomservice.cpp b/src/framework/cloud/musescorecom/musescorecomservice.cpp index 7792d681a703e..0befcc40ade4a 100644 --- a/src/framework/cloud/musescorecom/musescorecomservice.cpp +++ b/src/framework/cloud/musescorecom/musescorecomservice.cpp @@ -412,8 +412,8 @@ static RetVal parseConvertQueueList(const QByteArray& data) if (scoreIdVal.isDouble()) { item.scoreId = scoreIdVal.toInt(); } - item.createdAt = QDateTime::fromSecsSinceEpoch(itemObj.value("created_at").toInteger()); - item.updatedAt = QDateTime::fromSecsSinceEpoch(itemObj.value("updated_at").toInteger()); + item.dataCreated = QDateTime::fromSecsSinceEpoch(itemObj.value("date_created").toInteger()); + item.dataUpdated = QDateTime::fromSecsSinceEpoch(itemObj.value("date_updated").toInteger()); item.errorCode = convertErrorCodeFromApiString(itemObj.value("error_code").toString()); result.push_back(item); diff --git a/src/project/internal/convertfiletoscoreservice.cpp b/src/project/internal/convertfiletoscoreservice.cpp index 343a091370270..59a9c24b1633b 100644 --- a/src/project/internal/convertfiletoscoreservice.cpp +++ b/src/project/internal/convertfiletoscoreservice.cpp @@ -116,14 +116,9 @@ static std::string errorCodeToString(ConvertErrorCode code) static const std::string WATCHED_CONVERTS_RESOURCE_NAME("WATCHED_CONVERTS"); -static std::string convertLogId(ConvertType type, int itemId) +static std::string convertIdAndType(ConvertType type, int itemId) { - return std::to_string(itemId) + " (type: " + convertTypeToString(type) + ")"; -} - -static std::string convertLogId(const muse::String& convertedScoreName, ConvertType type, int itemId) -{ - return "\"" + convertedScoreName.toStdString() + "\" (conversion " + convertLogId(type, itemId) + ")"; + return "id: " + std::to_string(itemId) + ", type: " + convertTypeToString(type); } void ConvertFileToScoreService::init() @@ -392,7 +387,8 @@ void ConvertFileToScoreService::submitReview(int scoreId, ReviewRating rating, c museScoreComService()->convert()->submitReview(type, convertId, rating, comment) .onResolve(this, [type, convertId](const RetVal& submitRes) { if (!submitRes.ret) { - LOGE() << "Could not submit the review for conversion " << convertLogId(type, convertId) << ": " << submitRes.ret.toString(); + LOGE() << "Could not submit the review for conversion (" << convertIdAndType(type, + convertId) << "): " << submitRes.ret.toString(); } }); } @@ -410,7 +406,8 @@ void ConvertFileToScoreService::submitReviewComment(int scoreId, const QString& museScoreComService()->convert()->submitReviewComment(type, convertId, comment) .onResolve(this, [type, convertId](const RetVal& submitRes) { if (!submitRes.ret) { - LOGE() << "Could not submit the comment for conversion " << convertLogId(type, convertId) << ": " << submitRes.ret.toString(); + LOGE() << "Could not submit the comment for conversion (" << convertIdAndType(type, + convertId) << "): " << submitRes.ret.toString(); } }); } @@ -420,7 +417,7 @@ void ConvertFileToScoreService::deleteConversion(ConvertType type, int convertId museScoreComService()->convert()->deleteConversion(type, convertId) .onResolve(this, [this, type, convertId](const Ret& ret) { if (!ret) { - LOGE() << "Could not delete conversion " << convertLogId(type, convertId) << ": " << ret.toString(); + LOGE() << "Could not delete conversion (" << convertIdAndType(type, convertId) << "): " << ret.toString(); return; } @@ -525,7 +522,7 @@ void ConvertFileToScoreService::saveWatchedScores() void ConvertFileToScoreService::watch(ConvertType type, int itemId, const muse::String& convertedScoreName) { - LOGI() << "Start watching conversion " << convertLogId(convertedScoreName, type, itemId); + LOGI() << "Start watching conversion of \"" << convertedScoreName << "\" (" << convertIdAndType(type, itemId) << ")"; const auto it = std::find_if(m_watchedScores.begin(), m_watchedScores.end(), [type, itemId](const WatchedScore& watched) { return watched.conversion.type == type && watched.conversion.id == itemId; @@ -655,7 +652,7 @@ void ConvertFileToScoreService::updateWatchedScores(const ConvertQueueList& queu continue; } - LOGI() << "New external conversion: " << convertLogId(queueItem.type, queueItem.id); + LOGI() << "Found new external conversion (" << convertIdAndType(queueItem.type, queueItem.id) << ")"; WatchedScore watched; watched.conversion.id = queueItem.id; @@ -706,8 +703,8 @@ void ConvertFileToScoreService::handleItem(WatchedScore& watched, ConvertStatus const ConvertStatus previousStatus = watched.conversion.status; const bool statusChanged = previousStatus != status; if (statusChanged) { - LOGI() << "Conversion status changed: " << convertLogId(watched.name, watched.conversion.type, watched.conversion.id) - << " -> " << convertStatusToString(status); + LOGI() << "Conversion of \"" << watched.name << "\" (" << convertIdAndType(watched.conversion.type, watched.conversion.id) << ")" + << " status changed: " << convertStatusToString(previousStatus) << " -> " << convertStatusToString(status); } switch (status) { @@ -723,7 +720,8 @@ void ConvertFileToScoreService::handleItem(WatchedScore& watched, ConvertStatus if (watched.startedLocally) { const RetVal scoreInfo = museScoreComService()->downloadScoreInfo(*scoreId); if (!scoreInfo.ret) { - LOGW() << "Could not fetch score info for " << convertLogId(watched.name, watched.conversion.type, watched.conversion.id) + LOGW() << "Could not fetch score info for \"" << watched.name << "\" (" + << convertIdAndType(watched.conversion.type, watched.conversion.id) << ")" << ", will retry on next poll: " << scoreInfo.ret.toString(); return; //! NOTE: watched.conversion.status stays at previousStatus - retried next poll } From 5d28071dfc6b22f4b213e1626ca1867d56ca4f11 Mon Sep 17 00:00:00 2001 From: Roman Pudashkin Date: Mon, 14 Sep 2026 16:41:24 +0300 Subject: [PATCH 09/10] Fix successful conversions incorrectly reported as failed --- src/project/iconvertfiletoscorescenario.h | 4 +- src/project/iconvertfiletoscoreservice.h | 4 +- .../internal/convertfiletoscorescenario.cpp | 17 ++- .../internal/convertfiletoscorescenario.h | 6 +- .../internal/convertfiletoscoreservice.cpp | 84 +++++------ .../internal/convertfiletoscoreservice.h | 9 +- .../convertfiletoscorescenario_tests.cpp | 45 +++--- .../tests/convertfiletoscoreservice_tests.cpp | 138 ++++-------------- .../mocks/convertfiletoscoreservicemock.h | 2 +- 9 files changed, 108 insertions(+), 201 deletions(-) diff --git a/src/project/iconvertfiletoscorescenario.h b/src/project/iconvertfiletoscorescenario.h index ff2e6ac9a4f10..c770567cb1908 100644 --- a/src/project/iconvertfiletoscorescenario.h +++ b/src/project/iconvertfiletoscorescenario.h @@ -63,8 +63,8 @@ class IConvertFileToScoreScenario : MODULE_CONTEXT_INTERFACE virtual void convertFiles(const muse::io::paths_t& paths = {}) = 0; //! Emits the result once the server-side conversion completes; on success, the converted score - //! is already available in the user's MuseScore.com account, described by ScoreInfo - virtual muse::async::Channel convertFinished() const = 0; + //! is already available in the user's MuseScore.com account, described by the WatchedScore + virtual muse::async::Channel convertFinished() const = 0; }; using IConvertFileToScoreScenarioPtr = std::shared_ptr; diff --git a/src/project/iconvertfiletoscoreservice.h b/src/project/iconvertfiletoscoreservice.h index 79575672477ba..0b7379edb83e7 100644 --- a/src/project/iconvertfiletoscoreservice.h +++ b/src/project/iconvertfiletoscoreservice.h @@ -55,8 +55,8 @@ class IConvertFileToScoreService : MODULE_CONTEXT_INTERFACE //! Sends the conversion request to the server virtual muse::Ret startConvert(const ConvertInput& input, const muse::String& convertedScoreName) = 0; - //! Emits the final result of a conversion (upload or processing failure, or success with ScoreInfo) - virtual muse::async::Channel convertFinished() const = 0; + //! Emits the final result of a conversion (upload or processing failure, or success with the WatchedScore) + virtual muse::async::Channel convertFinished() const = 0; //! All pending/reviewable conversions from the server's convert queue virtual muse::ValNt watchedScores() const = 0; diff --git a/src/project/internal/convertfiletoscorescenario.cpp b/src/project/internal/convertfiletoscorescenario.cpp index c174adc90108c..d5f5c7d6da660 100644 --- a/src/project/internal/convertfiletoscorescenario.cpp +++ b/src/project/internal/convertfiletoscorescenario.cpp @@ -77,14 +77,14 @@ void ConvertFileToScoreScenario::init() { TRACEFUNC; - service()->convertFinished().onReceive(this, [this](const Ret& ret, const ScoreInfo& scoreInfo) { + service()->convertFinished().onReceive(this, [this](const Ret& ret, const WatchedScore& watched) { if (ret) { - showScoreReadyNotification(scoreInfo); + showScoreReadyNotification(watched); } else { showConvertFailedNotification(ret); } - m_convertFinished.send(ret, scoreInfo); + m_convertFinished.send(ret, watched); }); service()->reviewRequested().onReceive(this, [this](int scoreId) { @@ -192,7 +192,7 @@ void ConvertFileToScoreScenario::convertFiles(const io::paths_t& paths) }); } -async::Channel ConvertFileToScoreScenario::convertFinished() const +async::Channel ConvertFileToScoreScenario::convertFinished() const { return m_convertFinished; } @@ -451,21 +451,22 @@ void ConvertFileToScoreScenario::showFileProcessingDialog() }); } -void ConvertFileToScoreScenario::showScoreReadyNotification(const ScoreInfo& scoreInfo) +void ConvertFileToScoreScenario::showScoreReadyNotification(const WatchedScore& watched) { constexpr int openScoreBtn = int(toast::ToastActionCode::Custom) + 1; + const int scoreId = watched.scoreId ? *watched.scoreId : 0; std::string msg = muse::qtrc("project/convert", "‘%1’ has finished processing and is ready to open.") - .arg(scoreInfo.title).toStdString(); + .arg(watched.name.toQString()).toStdString(); toastService()->show(muse::trc("project/convert", "Your score is ready!"), msg, muse::ui::IconCode::Code::TICK_FILLED, true, { { muse::trc("global", "Dismiss"), toast::ToastActionCode::Dismiss }, { muse::trc("project/convert", "Open score"), openScoreBtn, /*accent*/ true }, - }).onResolve(this, [this, scoreInfo, openScoreBtn](const toast::ToastResult& result) { + }).onResolve(this, [this, scoreId, openScoreBtn](const toast::ToastResult& result) { if (result.isCode(openScoreBtn)) { - const QUrl url(QString("musescore://open-score/%1").arg(scoreInfo.id)); + const QUrl url(QString("musescore://open-score/%1").arg(scoreId)); dispatcher()->dispatch("file-open", actions::ActionData::make_arg1(url)); } }); diff --git a/src/project/internal/convertfiletoscorescenario.h b/src/project/internal/convertfiletoscorescenario.h index 676a896a72d1a..58a6e03c93791 100644 --- a/src/project/internal/convertfiletoscorescenario.h +++ b/src/project/internal/convertfiletoscorescenario.h @@ -66,7 +66,7 @@ class ConvertFileToScoreScenario : public QObject, public IConvertFileToScoreSce muse::Ret validateLink(const QUrl& link) override; void convertFiles(const muse::io::paths_t& paths = {}) override; - muse::async::Channel convertFinished() const override; + muse::async::Channel convertFinished() const override; private: muse::async::Promise checkConvertIsAllowed(); @@ -92,14 +92,14 @@ class ConvertFileToScoreScenario : public QObject, public IConvertFileToScoreSce void showTooManyImagesError(int maxImages); void showFileProcessingDialog(); - void showScoreReadyNotification(const ScoreInfo& scoreInfo); + void showScoreReadyNotification(const WatchedScore& watched); void showConvertFailedNotification(const muse::Ret& ret); void showPollingFailureNotification(); void askReviewRating(int scoreId); void checkPendingReview(); - muse::async::Channel m_convertFinished; + muse::async::Channel m_convertFinished; std::map m_pendingReviews; bool m_retryToastShown = false; diff --git a/src/project/internal/convertfiletoscoreservice.cpp b/src/project/internal/convertfiletoscoreservice.cpp index 59a9c24b1633b..739a578e2df6c 100644 --- a/src/project/internal/convertfiletoscoreservice.cpp +++ b/src/project/internal/convertfiletoscoreservice.cpp @@ -339,7 +339,7 @@ Ret ConvertFileToScoreService::startConvert(const ConvertInput& input, const mus return make_ok(); } -async::Channel ConvertFileToScoreService::convertFinished() const +async::Channel ConvertFileToScoreService::convertFinished() const { return m_convertFinished; } @@ -639,7 +639,8 @@ void ConvertFileToScoreService::updateWatchedScores(const ConvertQueueList& queu if (!queueItem.filename.isEmpty()) { watched.name = queueItem.filename; } - handleItem(watched, queueItem.status, queueItem.errorCode, queueItem.scoreId); + watched.scoreId = queueItem.scoreId; + updateStatus(watched, queueItem.status, queueItem.errorCode); if (watched.conversion.status != ConvertStatus::Done) { newWatchedScores.push_back(watched); @@ -658,8 +659,9 @@ void ConvertFileToScoreService::updateWatchedScores(const ConvertQueueList& queu watched.conversion.id = queueItem.id; watched.conversion.type = queueItem.type; watched.name = queueItem.filename; + watched.scoreId = queueItem.scoreId; - handleItem(watched, queueItem.status, queueItem.errorCode, queueItem.scoreId); + updateStatus(watched, queueItem.status, queueItem.errorCode); if (watched.conversion.status != ConvertStatus::Done) { newWatchedScores.push_back(watched); @@ -671,23 +673,18 @@ void ConvertFileToScoreService::updateWatchedScores(const ConvertQueueList& queu continue; } - WatchedScore dropped = m_watchedScores.at(i); - ConvertStatus status = ConvertStatus::Unknown; - std::optional scoreId; + WatchedScore& dropped = m_watchedScores.at(i); if (dropped.scoreId) { - //! NOTE: a Done/AwaitingReview item still reports its status while queued, but it may be - //! dropped from the queue automatically at some point afterwards, once already reported ready - status = ConvertStatus::Done; - scoreId = dropped.scoreId; + LOGI() << "Conversion of \"" << dropped.name << "\" (" << convertIdAndType(dropped.conversion.type, dropped.conversion.id) + << ") was dropped from the queue, recovering as Done with scoreId " << *dropped.scoreId; + + //! NOTE: always terminal, so never added back to newWatchedScores + updateStatus(dropped, ConvertStatus::Done, ConvertErrorCode::Unknown); } else { - //! NOTE: dropped from the queue before ever reporting a scoreId - there's no way to - //! identify the resulting score anymore, so it can't be recovered as a success - status = ConvertStatus::Failed; + LOGW() << "Conversion of \"" << dropped.name << "\" (" << convertIdAndType(dropped.conversion.type, dropped.conversion.id) + << ") was dropped from the queue without ever reporting a scoreId"; } - - //! NOTE: always terminal, so never added back to newWatchedScores - handleItem(dropped, status, ConvertErrorCode::Unknown, scoreId); } if (m_watchedScores != newWatchedScores) { @@ -697,49 +694,36 @@ void ConvertFileToScoreService::updateWatchedScores(const ConvertQueueList& queu } } -void ConvertFileToScoreService::handleItem(WatchedScore& watched, ConvertStatus status, ConvertErrorCode errorCode, - std::optional scoreId) +void ConvertFileToScoreService::updateStatus(WatchedScore& watched, ConvertStatus newStatus, ConvertErrorCode errorCode) { const ConvertStatus previousStatus = watched.conversion.status; - const bool statusChanged = previousStatus != status; - if (statusChanged) { - LOGI() << "Conversion of \"" << watched.name << "\" (" << convertIdAndType(watched.conversion.type, watched.conversion.id) << ")" - << " status changed: " << convertStatusToString(previousStatus) << " -> " << convertStatusToString(status); + if (previousStatus == newStatus) { + return; } - switch (status) { + LOGI() << "Conversion of \"" << watched.name << "\" (" << convertIdAndType(watched.conversion.type, watched.conversion.id) << ")" + << " status changed: " << convertStatusToString(previousStatus) << " -> " << convertStatusToString(newStatus); + + const bool wasDone = previousStatus == ConvertStatus::Done + || previousStatus == ConvertStatus::AwaitingReview; + + watched.conversion.status = newStatus; + + switch (newStatus) { case ConvertStatus::Processing: case ConvertStatus::Unknown: break; case ConvertStatus::AwaitingReview: - case ConvertStatus::Done: - if (!scoreId || watched.scoreId) { - break; - } - - if (watched.startedLocally) { - const RetVal scoreInfo = museScoreComService()->downloadScoreInfo(*scoreId); - if (!scoreInfo.ret) { - LOGW() << "Could not fetch score info for \"" << watched.name << "\" (" - << convertIdAndType(watched.conversion.type, watched.conversion.id) << ")" - << ", will retry on next poll: " << scoreInfo.ret.toString(); - return; //! NOTE: watched.conversion.status stays at previousStatus - retried next poll - } - - finishConvert(make_ok(), scoreInfo.val); + case ConvertStatus::Done: { + if (!wasDone && watched.scoreId && watched.startedLocally) { + finishConvert(make_ok(), watched); } - watched.scoreId = *scoreId; - - if (status == ConvertStatus::AwaitingReview) { - m_reviewRequested.send(*scoreId); + if (newStatus == ConvertStatus::AwaitingReview && watched.scoreId) { + m_reviewRequested.send(*watched.scoreId); } - break; + } break; case ConvertStatus::Failed: { - if (!statusChanged) { - return; - } - Ret ret = make_ret(Err::ConvertProcessingFailed); ret.setText("Conversion failed for \"" + watched.name.toStdString() + "\": " + errorCodeToString(errorCode)); ret.setData(CONVERT_FAILED_FILE_NAME_KEY, watched.name); @@ -752,13 +736,11 @@ void ConvertFileToScoreService::handleItem(WatchedScore& watched, ConvertStatus break; } } - - watched.conversion.status = status; } -void ConvertFileToScoreService::finishConvert(const Ret& ret, const ScoreInfo& scoreInfo) +void ConvertFileToScoreService::finishConvert(const Ret& ret, const WatchedScore& watched) { - m_convertFinished.send(ret, scoreInfo); + m_convertFinished.send(ret, watched); } WatchedScore* ConvertFileToScoreService::findWatchedScoreByScoreId(int scoreId) diff --git a/src/project/internal/convertfiletoscoreservice.h b/src/project/internal/convertfiletoscoreservice.h index ae634d43405aa..0da0c41acc63a 100644 --- a/src/project/internal/convertfiletoscoreservice.h +++ b/src/project/internal/convertfiletoscoreservice.h @@ -61,7 +61,7 @@ class ConvertFileToScoreService : public QObject, public IConvertFileToScoreServ muse::Ret validateLink(const QUrl& link) const override; muse::Ret startConvert(const ConvertInput& input, const muse::String& convertedScoreName) override; - muse::async::Channel convertFinished() const override; + muse::async::Channel convertFinished() const override; muse::ValNt watchedScores() const override; @@ -89,10 +89,9 @@ class ConvertFileToScoreService : public QObject, public IConvertFileToScoreServ void giveUpPolling(const muse::Ret& ret); void updateWatchedScores(const muse::cloud::ConvertQueueList& queue); - void handleItem(WatchedScore& watched, muse::cloud::ConvertStatus status, muse::cloud::ConvertErrorCode errorCode, - std::optional scoreId); + void updateStatus(WatchedScore& watched, muse::cloud::ConvertStatus status, muse::cloud::ConvertErrorCode errorCode); - void finishConvert(const muse::Ret& ret, const ScoreInfo& scoreInfo = ScoreInfo()); + void finishConvert(const muse::Ret& ret, const WatchedScore& watched = WatchedScore()); WatchedScore* findWatchedScoreByScoreId(int scoreId); @@ -107,7 +106,7 @@ class ConvertFileToScoreService : public QObject, public IConvertFileToScoreServ muse::async::Channel m_pollingFailed; muse::async::Notification m_watchedScoresChanged; - muse::async::Channel m_convertFinished; + muse::async::Channel m_convertFinished; muse::async::Channel m_reviewRequested; }; } diff --git a/src/project/tests/convertfiletoscorescenario_tests.cpp b/src/project/tests/convertfiletoscorescenario_tests.cpp index ac694dd83ef3b..e4d255effc7b2 100644 --- a/src/project/tests/convertfiletoscorescenario_tests.cpp +++ b/src/project/tests/convertfiletoscorescenario_tests.cpp @@ -251,15 +251,15 @@ class Project_ConvertFileToScoreScenarioTest : public ::testing::Test TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Success_ShowsScoreReadyNotificationAndForwards) { // [GIVEN] The service's channels, wired up via init() - async::Channel convertFinished; + async::Channel convertFinished; async::Channel reviewRequested; ON_CALL(*m_service, convertFinished()).WillByDefault(Return(convertFinished)); ON_CALL(*m_service, reviewRequested()).WillByDefault(Return(reviewRequested)); m_scenario->init(); - ScoreInfo scoreInfo; - scoreInfo.id = 555; - scoreInfo.title = "My Score"; + WatchedScore watched; + watched.scoreId = 555; + watched.name = u"My Score"; constexpr int openScoreBtn = int(toast::ToastActionCode::Custom) + 1; @@ -278,31 +278,36 @@ TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Success_ShowsScoreReadyNotif bool forwarded = false; Ret forwardedRet; - m_scenario->convertFinished().onReceive(nullptr, [&](const Ret& ret, const ScoreInfo&) { + WatchedScore forwardedWatched; + m_scenario->convertFinished().onReceive(nullptr, [&](const Ret& ret, const WatchedScore& w) { forwarded = true; forwardedRet = ret; + forwardedWatched = w; }); // [WHEN] The service reports a successful conversion - convertFinished.send(make_ok(), scoreInfo); + convertFinished.send(make_ok(), watched); - // [THEN] The result is forwarded to the scenario's own convertFinished channel + // [THEN] The result, including the WatchedScore payload, is forwarded to the scenario's own convertFinished channel EXPECT_TRUE(forwarded); EXPECT_TRUE(forwardedRet); + ASSERT_TRUE(forwardedWatched.scoreId.has_value()); + EXPECT_EQ(*forwardedWatched.scoreId, 555); + EXPECT_EQ(forwardedWatched.name, u"My Score"); } TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Success_OpenScoreButton_DispatchesOpenScoreUrl) { // [GIVEN] The service's channels, wired up via init() - async::Channel convertFinished; + async::Channel convertFinished; async::Channel reviewRequested; ON_CALL(*m_service, convertFinished()).WillByDefault(Return(convertFinished)); ON_CALL(*m_service, reviewRequested()).WillByDefault(Return(reviewRequested)); m_scenario->init(); - ScoreInfo scoreInfo; - scoreInfo.id = 555; - scoreInfo.title = "My Score"; + WatchedScore watched; + watched.scoreId = 555; + watched.name = u"My Score"; // [GIVEN] The user clicks "Open score" on the ready notification constexpr int openScoreBtn = int(toast::ToastActionCode::Custom) + 1; @@ -318,14 +323,14 @@ TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Success_OpenScoreButton_Disp .Times(1); // [WHEN] The service reports a successful conversion - convertFinished.send(make_ok(), scoreInfo); + convertFinished.send(make_ok(), watched); pumpEvents(); } TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Failure_ShowsConvertFailedNotificationAndForwards) { // [GIVEN] The service's channels, wired up via init() - async::Channel convertFinished; + async::Channel convertFinished; async::Channel reviewRequested; ON_CALL(*m_service, convertFinished()).WillByDefault(Return(convertFinished)); ON_CALL(*m_service, reviewRequested()).WillByDefault(Return(reviewRequested)); @@ -349,13 +354,13 @@ TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Failure_ShowsConvertFailedNo bool forwarded = false; Ret forwardedRet; - m_scenario->convertFinished().onReceive(nullptr, [&](const Ret& r, const ScoreInfo&) { + m_scenario->convertFinished().onReceive(nullptr, [&](const Ret& r, const WatchedScore&) { forwarded = true; forwardedRet = r; }); // [WHEN] The service reports a failed conversion - convertFinished.send(ret, ScoreInfo()); + convertFinished.send(ret, WatchedScore()); // [THEN] The failure is still forwarded to the scenario's own convertFinished channel EXPECT_TRUE(forwarded); @@ -365,7 +370,7 @@ TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Failure_ShowsConvertFailedNo TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Failure_TryAgain_RestartsConvert) { // [GIVEN] The service's channels, wired up via init() - async::Channel convertFinished; + async::Channel convertFinished; async::Channel reviewRequested; ON_CALL(*m_service, convertFinished()).WillByDefault(Return(convertFinished)); ON_CALL(*m_service, reviewRequested()).WillByDefault(Return(reviewRequested)); @@ -397,7 +402,7 @@ TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Failure_TryAgain_RestartsCon // [WHEN] The service reports a failed conversion Ret ret = make_ret(Err::ConvertProcessingFailed); ret.setData(CONVERT_FAILED_FILE_NAME_KEY, muse::String(u"My Score")); - convertFinished.send(ret, ScoreInfo()); + convertFinished.send(ret, WatchedScore()); pumpEvents(); } @@ -405,7 +410,7 @@ TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Failure_TryAgain_RestartsCon TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Failure_Dismiss_DoesNotRestartConvert) { // [GIVEN] The service's channels, wired up via init() - async::Channel convertFinished; + async::Channel convertFinished; async::Channel reviewRequested; ON_CALL(*m_service, convertFinished()).WillByDefault(Return(convertFinished)); ON_CALL(*m_service, reviewRequested()).WillByDefault(Return(reviewRequested)); @@ -422,7 +427,7 @@ TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Failure_Dismiss_DoesNotResta EXPECT_CALL(*m_service, startConvert(_, _)).Times(0); // [WHEN] The service reports a failed conversion - convertFinished.send(make_ret(Err::ConvertProcessingFailed), ScoreInfo()); + convertFinished.send(make_ret(Err::ConvertProcessingFailed), WatchedScore()); pumpEvents(); } @@ -434,7 +439,7 @@ TEST_F(Project_ConvertFileToScoreScenarioTest, Init_Failure_Dismiss_DoesNotResta TEST_F(Project_ConvertFileToScoreScenarioTest, Init_PollingFailed_ShowsToastOnceAfterThreshold) { // [GIVEN] The service's channels, wired up via init() - async::Channel convertFinished; + async::Channel convertFinished; async::Channel reviewRequested; async::Channel pollingFailed; ON_CALL(*m_service, convertFinished()).WillByDefault(Return(convertFinished)); diff --git a/src/project/tests/convertfiletoscoreservice_tests.cpp b/src/project/tests/convertfiletoscoreservice_tests.cpp index 2bbe8403009f0..35ee0c32c73fb 100644 --- a/src/project/tests/convertfiletoscoreservice_tests.cpp +++ b/src/project/tests/convertfiletoscoreservice_tests.cpp @@ -73,14 +73,6 @@ async::Promise resolvedPromise(const T& val) }); } -RetVal okScoreInfo(int scoreId, const QString& title = "My Score") -{ - ScoreInfo info; - info.id = scoreId; - info.title = title; - return RetVal::make_ok(info); -} - //! NOTE: the hardcoded values ConvertFileToScoreService::init() falls back to void expectFallbackConfig(const ConvertConfig& config) { @@ -579,7 +571,7 @@ TEST_F(Project_ConvertFileToScoreServiceTest, StartConvert_UploadFails_ForwardsF bool received = false; Ret receivedRet; - m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const ScoreInfo&) { + m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const WatchedScore&) { received = true; receivedRet = ret; }); @@ -726,9 +718,6 @@ TEST_F(Project_ConvertFileToScoreServiceTest, WatchedScores_AfterDone_NoLongerCo item.status = ConvertStatus::Done; item.scoreId = 555; - ON_CALL(*m_museScoreComService, downloadScoreInfo(555)) - .WillByDefault(Invoke([] { return okScoreInfo(555); })); - bool changed = false; m_service->watchedScores().notification.onNotify(nullptr, [&] { changed = true; @@ -893,7 +882,7 @@ TEST_F(Project_ConvertFileToScoreServiceTest, ResumeConvert_AwaitingReviewItemWi // polling / score info fetch pipeline // ================================================== -TEST_F(Project_ConvertFileToScoreServiceTest, Poll_DoneStatus_FetchesScoreInfoAndFinishes) +TEST_F(Project_ConvertFileToScoreServiceTest, Poll_DoneStatus_FinishesImmediatelyWithWatchedScore) { // [GIVEN] The queue reports the conversion as done, with its scoreId ConvertQueueItem item; @@ -902,26 +891,25 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_DoneStatus_FetchesScoreInfoAn item.status = ConvertStatus::Done; item.scoreId = 555; - ON_CALL(*m_museScoreComService, downloadScoreInfo(555)) - .WillByDefault(Invoke([] { return okScoreInfo(555, "My Score"); })); - bool received = false; Ret receivedRet; - ScoreInfo receivedInfo; - m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const ScoreInfo& info) { + WatchedScore receivedWatched; + m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const WatchedScore& watched) { received = true; receivedRet = ret; - receivedInfo = info; + receivedWatched = watched; }); // [WHEN] Uploading and polling the status deliverQueueStatus({ item }, ConvertType::Omr, TEST_QUEUE_ID, "My Score"); - // [THEN] The conversion finishes successfully, carrying the score's info + // [THEN] The conversion finishes successfully immediately - no separate fetch is needed, since + // the queue already carries everything needed to identify the resulting score ASSERT_TRUE(received); EXPECT_TRUE(receivedRet); - EXPECT_EQ(receivedInfo.id, 555); - EXPECT_EQ(receivedInfo.title, "My Score"); + ASSERT_TRUE(receivedWatched.scoreId.has_value()); + EXPECT_EQ(*receivedWatched.scoreId, 555); + EXPECT_EQ(receivedWatched.name, u"My Score"); } TEST_F(Project_ConvertFileToScoreServiceTest, Poll_AwaitingReviewWithoutScoreId_DoesNotReportYet) @@ -931,16 +919,14 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_AwaitingReviewWithoutScoreId_ item.id = TEST_QUEUE_ID; item.type = ConvertType::Omr; item.status = ConvertStatus::AwaitingReview; - - // [THEN] There's nothing to identify the score by yet, so nothing is fetched or reported - EXPECT_CALL(*m_museScoreComService, downloadScoreInfo(An())).Times(0); + item.scoreId = std::nullopt; // no scoreId bool reviewRequested = false; bool convertFinished = false; m_service->reviewRequested().onReceive(nullptr, [&](int) { reviewRequested = true; }); - m_service->convertFinished().onReceive(nullptr, [&](const Ret&, const ScoreInfo&) { + m_service->convertFinished().onReceive(nullptr, [&](const Ret&, const WatchedScore&) { convertFinished = true; }); @@ -961,9 +947,6 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_AwaitingReviewWithScoreId_Emi item.status = ConvertStatus::AwaitingReview; item.scoreId = 555; - ON_CALL(*m_museScoreComService, downloadScoreInfo(555)) - .WillByDefault(Invoke([] { return okScoreInfo(555); })); - bool reviewRequested = false; int reviewScoreId = 0; m_service->reviewRequested().onReceive(nullptr, [&](int scoreId) { @@ -973,9 +956,11 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_AwaitingReviewWithScoreId_Emi bool convertFinished = false; Ret convertFinishedRet; - m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const ScoreInfo&) { + WatchedScore convertFinishedWatched; + m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const WatchedScore& watched) { convertFinished = true; convertFinishedRet = ret; + convertFinishedWatched = watched; }); // [WHEN] Uploading and polling the status @@ -984,29 +969,29 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_AwaitingReviewWithScoreId_Emi // [THEN] The score is already usable, so both signals fire immediately ASSERT_TRUE(convertFinished); EXPECT_TRUE(convertFinishedRet); + ASSERT_TRUE(convertFinishedWatched.scoreId.has_value()); + EXPECT_EQ(*convertFinishedWatched.scoreId, 555); + EXPECT_EQ(convertFinishedWatched.name, u"My Score"); ASSERT_TRUE(reviewRequested); EXPECT_EQ(reviewScoreId, 555); } -TEST_F(Project_ConvertFileToScoreServiceTest, Poll_ItemNeverInQueueWithoutScoreId_TreatedAsFailed) +TEST_F(Project_ConvertFileToScoreServiceTest, Poll_ItemNeverInQueueWithoutScoreId_SilentlyDropped) { // [GIVEN] The item never appears in the queue at all, and never reported a scoreId - there's - // no way to identify a resulting score, so it can't be recovered as a success + // no way to identify a resulting score, so it's silently dropped rather than reported as failed bool received = false; - Ret receivedRet; - m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const ScoreInfo&) { + m_service->convertFinished().onReceive(nullptr, [&](const Ret&, const WatchedScore&) { received = true; - receivedRet = ret; }); // [WHEN] Uploading, then polling an empty queue deliverQueueStatus({}, ConvertType::Omr, TEST_QUEUE_ID, "My Score"); - // [THEN] The conversion is reported as failed - ASSERT_TRUE(received); - EXPECT_FALSE(receivedRet); + // [THEN] Nothing is reported - not failed, not succeeded + EXPECT_FALSE(received); } TEST_F(Project_ConvertFileToScoreServiceTest, Poll_PreviouslyReportedItemDropsFromQueue_SilentlyErasedWithoutDuplicateReport) @@ -1036,12 +1021,8 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_PreviouslyReportedItemDropsFr return resolvedPromise >(RetVal::make_ok(ConvertQueueList { otherItem })); })); - EXPECT_CALL(*m_museScoreComService, downloadScoreInfo(555)) - .Times(1) - .WillOnce(Invoke([] { return okScoreInfo(555); })); - int convertFinishedCount = 0; - m_service->convertFinished().onReceive(nullptr, [&](const Ret&, const ScoreInfo&) { + m_service->convertFinished().onReceive(nullptr, [&](const Ret&, const WatchedScore&) { ++convertFinishedCount; }); @@ -1052,7 +1033,7 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_PreviouslyReportedItemDropsFr // [WHEN] Starting an unrelated conversion triggers a second poll; the original item has now dropped uploadAndResolve(otherQueueId, "Other Score", { "/some/path/b.pdf" }); - // [THEN] No duplicate report, and downloadScoreInfo() was only ever called once + // [THEN] No duplicate report EXPECT_EQ(convertFinishedCount, 1); } @@ -1105,11 +1086,8 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_SameIdDifferentType_DoesNotCr doneAudioItem })); })); - ON_CALL(*m_museScoreComService, downloadScoreInfo(999)) - .WillByDefault(Invoke([] { return okScoreInfo(999, "Audio Score"); })); - std::vector receivedRets; - m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const ScoreInfo&) { + m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const WatchedScore&) { receivedRets.push_back(ret); }); @@ -1141,7 +1119,7 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_FailedStatus_ForwardsProcessi bool received = false; Ret receivedRet; - m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const ScoreInfo&) { + m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const WatchedScore&) { received = true; receivedRet = ret; }); @@ -1156,54 +1134,8 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_FailedStatus_ForwardsProcessi EXPECT_EQ(receivedRet.data(CONVERT_FAILED_FILE_NAME_KEY, String()), u"My Score"); } -TEST_F(Project_ConvertFileToScoreServiceTest, Poll_ScoreInfoFetchFails_RetriesOnNextPoll) -{ - // [GIVEN] The queue reports the conversion as done, with its scoreId, on both polls - const int otherQueueId = TEST_QUEUE_ID + 1; - - ConvertQueueItem doneItem; - doneItem.id = TEST_QUEUE_ID; - doneItem.type = ConvertType::Omr; - doneItem.status = ConvertStatus::Done; - doneItem.scoreId = 555; - - ConvertQueueItem otherItem; - otherItem.id = otherQueueId; - otherItem.type = ConvertType::Omr; - otherItem.status = ConvertStatus::Processing; - - ON_CALL(*m_convertService, fetchQueue()) - .WillByDefault(Invoke([doneItem, otherItem] { - return resolvedPromise >(RetVal::make_ok(ConvertQueueList { doneItem, otherItem })); - })); - - // [GIVEN] Fetching the score's info fails transiently the first time, succeeds the second - EXPECT_CALL(*m_museScoreComService, downloadScoreInfo(555)) - .Times(2) - .WillOnce(Return(RetVal::make_ret(make_ret(muse::cloud::Err::NetworkError)))) - .WillOnce(Invoke([] { return okScoreInfo(555); })); - - bool received = false; - Ret receivedRet; - m_service->convertFinished().onReceive(nullptr, [&](const Ret& ret, const ScoreInfo&) { - received = true; - receivedRet = ret; - }); - - // [WHEN] Starting the conversion - the first poll's score info fetch fails transiently - uploadAndResolve(TEST_QUEUE_ID, "My Score", { "/some/path/a.pdf" }); - EXPECT_FALSE(received); - - // [WHEN] Starting an unrelated conversion triggers a second poll, retrying the fetch - uploadAndResolve(otherQueueId, "Other Score", { "/some/path/b.pdf" }); - - // [THEN] The retried fetch succeeds and the conversion finishes - ASSERT_TRUE(received); - EXPECT_TRUE(receivedRet); -} - // ================================================== -// retry logic (poll / download failures) +// retry logic (poll failures) // ================================================== TEST_F(Project_ConvertFileToScoreServiceTest, Poll_NonRetryableFetchFailure_FinishesImmediatelyWithError) @@ -1258,11 +1190,8 @@ TEST_F(Project_ConvertFileToScoreServiceTest, Poll_RetryableFetchFailure_KeepsWa otherItem })); })); - ON_CALL(*m_museScoreComService, downloadScoreInfo(555)) - .WillByDefault(Invoke([] { return okScoreInfo(555); })); - bool received = false; - m_service->convertFinished().onReceive(nullptr, [&](const Ret&, const ScoreInfo&) { + m_service->convertFinished().onReceive(nullptr, [&](const Ret&, const WatchedScore&) { received = true; }); @@ -1445,9 +1374,6 @@ TEST_F(Project_ConvertFileToScoreServiceTest, SubmitReview_Good_DelegatesToConve item.status = ConvertStatus::AwaitingReview; item.scoreId = 555; - ON_CALL(*m_museScoreComService, downloadScoreInfo(555)) - .WillByDefault(Invoke([] { return okScoreInfo(555); })); - deliverQueueStatus({ item }, ConvertType::Omr, TEST_QUEUE_ID, "My Score"); // [THEN] The rating is delegated to the convert service, resolving the scoreId back to its conversion @@ -1469,9 +1395,6 @@ TEST_F(Project_ConvertFileToScoreServiceTest, SubmitReview_BadWithComment_Delega item.status = ConvertStatus::AwaitingReview; item.scoreId = 555; - ON_CALL(*m_museScoreComService, downloadScoreInfo(555)) - .WillByDefault(Invoke([] { return okScoreInfo(555); })); - deliverQueueStatus({ item }, ConvertType::Audio2Score, 7, "My Score"); // [THEN] The rating and comment are delegated to the convert service, resolving the scoreId back to its conversion @@ -1493,9 +1416,6 @@ TEST_F(Project_ConvertFileToScoreServiceTest, SubmitReviewComment_DelegatesToCon item.status = ConvertStatus::AwaitingReview; item.scoreId = 555; - ON_CALL(*m_museScoreComService, downloadScoreInfo(555)) - .WillByDefault(Invoke([] { return okScoreInfo(555); })); - deliverQueueStatus({ item }, ConvertType::Audio2Score, 7, "My Score"); // [THEN] The comment is delegated to the convert service, resolving the scoreId back to its conversion diff --git a/src/project/tests/mocks/convertfiletoscoreservicemock.h b/src/project/tests/mocks/convertfiletoscoreservicemock.h index de04559c77f31..225f26fb1c10c 100644 --- a/src/project/tests/mocks/convertfiletoscoreservicemock.h +++ b/src/project/tests/mocks/convertfiletoscoreservicemock.h @@ -36,7 +36,7 @@ class ConvertFileToScoreServiceMock : public IConvertFileToScoreService MOCK_METHOD(muse::Ret, validateLink, (const QUrl&), (const, override)); MOCK_METHOD(muse::Ret, startConvert, (const ConvertInput&, const muse::String&), (override)); - MOCK_METHOD((muse::async::Channel), convertFinished, (), (const, override)); + MOCK_METHOD((muse::async::Channel), convertFinished, (), (const, override)); MOCK_METHOD(muse::ValNt, watchedScores, (), (const, override)); From 9b0e0dce98141da34b1e0ccabeca560120aa92f0 Mon Sep 17 00:00:00 2001 From: Roman Pudashkin Date: Mon, 14 Sep 2026 18:17:54 +0300 Subject: [PATCH 10/10] Fix duplicate "Unable to connect" dialog on cloud availability check timeout --- .../cloud/internal/abstractcloudservice.cpp | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/framework/cloud/internal/abstractcloudservice.cpp b/src/framework/cloud/internal/abstractcloudservice.cpp index 374b32d8871ee..a6af98efbd030 100644 --- a/src/framework/cloud/internal/abstractcloudservice.cpp +++ b/src/framework/cloud/internal/abstractcloudservice.cpp @@ -358,14 +358,21 @@ Promise AbstractCloudService::checkCloudIsAvailableAsync() const } Progress progressVal = progress.val; + std::shared_ptr finished = std::make_shared(false); QTimer* timer = new QTimer(); timer->setSingleShot(true); - QObject::connect(timer, &QTimer::timeout, [progressVal]() mutable { - progressVal.cancel(); + QObject::connect(timer, &QTimer::timeout, [progressVal, finished]() mutable { + if (!*finished) { + progressVal.cancel(); + } }); - progressVal.finished().onReceive(this, [resolve, timer](const ProgressResult& res) { + progressVal.finished().onReceive(this, [resolve, timer, finished](const ProgressResult& res) { + if (*finished) { + return; + } + *finished = true; timer->stop(); timer->deleteLater(); (void)resolve(res.ret);