From 6fed602f442822fa00b6991df1cb0dcd78688a0e Mon Sep 17 00:00:00 2001 From: krasko78 <154632626+krasko78@users.noreply.github.com> Date: Sat, 28 Sep 2024 00:36:28 +0200 Subject: [PATCH] #24908: Moved the first corruption check after the second one and only if unsuccessful to not degrade save times + code review changes --- .../io/devtools/allzerosfilecorruptor.cpp | 3 +-- src/project/internal/notationproject.cpp | 25 +++++++++---------- src/project/internal/notationproject.h | 6 ++--- .../internal/projectactionscontroller.cpp | 18 ++++++------- 4 files changed, 25 insertions(+), 27 deletions(-) diff --git a/src/framework/global/io/devtools/allzerosfilecorruptor.cpp b/src/framework/global/io/devtools/allzerosfilecorruptor.cpp index 80cc11d180587..e77c5727ec389 100644 --- a/src/framework/global/io/devtools/allzerosfilecorruptor.cpp +++ b/src/framework/global/io/devtools/allzerosfilecorruptor.cpp @@ -28,10 +28,9 @@ AllZerosFileCorruptor::AllZerosFileCorruptor(const path_t& filePath) { } -size_t AllZerosFileCorruptor::writeData(const uint8_t* data, size_t len) +size_t AllZerosFileCorruptor::writeData(const uint8_t*, size_t len) { // Ignore the actual data and write all zeros so as to corrupt the file. - Q_UNUSED(data); uint8_t* corruptData = new uint8_t[len]; memset(corruptData, 0, len); return File::writeData(corruptData, len); diff --git a/src/project/internal/notationproject.cpp b/src/project/internal/notationproject.cpp index 59a0a1327a247..3b350fe18e690 100644 --- a/src/project/internal/notationproject.cpp +++ b/src/project/internal/notationproject.cpp @@ -657,17 +657,7 @@ Ret NotationProject::doSave(const muse::io::path_t& path, engraving::MscIoMode i } else { Ret ret = muse::make_ok(); - if (!isAutosave) { - ret = checkSavedFileForCorruption(ioMode, savePath, targetMainFileName.toQString()); - if (!ret) { - if (ret.code() == (int)Err::CorruptionUponSavingError) { - ret.setData("corruptedAfterMove", false); - } - return ret; - } - } - - ret = fileSystem()->move(savePath, targetContainerPath, true); + ret = fileSystem()->copy(savePath, targetContainerPath, true); if (!ret) { return ret; } @@ -676,11 +666,19 @@ Ret NotationProject::doSave(const muse::io::path_t& path, engraving::MscIoMode i ret = checkSavedFileForCorruption(ioMode, targetContainerPath, targetMainFileName.toQString()); if (!ret) { if (ret.code() == (int)Err::CorruptionUponSavingError) { - ret.setData("corruptedAfterMove", true); + // Validate the temporary "saving" file too. + Ret ret2 = checkSavedFileForCorruption(ioMode, savePath, targetMainFileName.toQString()); + if (ret2.code() == (int)Err::CorruptionUponSavingError) { + ret2.setData("savingFileIsCorrupt", true); + return ret2; + } } return ret; } } + + // Remove the temp save file (not problematic if fails) + fileSystem()->remove(savePath); } } @@ -815,7 +813,8 @@ Ret NotationProject::saveSelectionOnScore(const muse::io::path_t& path) return ret; } -Ret NotationProject::checkSavedFileForCorruption(MscIoMode ioMode, const muse::io::path_t path, const muse::io::path_t scoreFileName) +Ret NotationProject::checkSavedFileForCorruption(MscIoMode ioMode, const muse::io::path_t& path, + const muse::io::path_t& scoreFileName) { TRACEFUNC; diff --git a/src/project/internal/notationproject.h b/src/project/internal/notationproject.h index 56e49e77622e6..e2a8a75323ea3 100644 --- a/src/project/internal/notationproject.h +++ b/src/project/internal/notationproject.h @@ -115,11 +115,11 @@ class NotationProject : public INotationProject, public muse::Injectable, public bool createThumbnail = true, bool isAutosave = false); muse::Ret saveSelectionOnScore(const muse::io::path_t& path = muse::io::path_t()); muse::Ret exportProject(const muse::io::path_t& path, const std::string& suffix); - muse::Ret doSave(const muse::io::path_t& path, engraving::MscIoMode ioMode, bool generateBackup = true, - bool createThumbnail = true, bool isAutosave = false); + muse::Ret doSave(const muse::io::path_t& path, engraving::MscIoMode ioMode, bool generateBackup = true, bool createThumbnail = true, + bool isAutosave = false); muse::Ret makeCurrentFileAsBackup(); muse::Ret writeProject(engraving::MscWriter& msczWriter, bool onlySelection, bool createThumbnail = true); - muse::Ret checkSavedFileForCorruption(engraving::MscIoMode ioMode, const muse::io::path_t path, const muse::io::path_t scoreFileName); + muse::Ret checkSavedFileForCorruption(engraving::MscIoMode ioMode, const muse::io::path_t& path, const muse::io::path_t& scoreFileName); void listenIfNeedSaveChanges(); void markAsSaved(const muse::io::path_t& path); diff --git a/src/project/internal/projectactionscontroller.cpp b/src/project/internal/projectactionscontroller.cpp index f4393892e2575..bbc55c7f5b6c0 100644 --- a/src/project/internal/projectactionscontroller.cpp +++ b/src/project/internal/projectactionscontroller.cpp @@ -1585,19 +1585,19 @@ void ProjectActionsController::warnScoreCorruptAfterSave(const Ret& ret) { std::string errMessage = ret.toString(); - std::any corruptedAfterMove = ret.data("corruptedAfterMove"); - std::string originalFileCorruptedMessage = corruptedAfterMove.has_value() && std::any_cast(corruptedAfterMove) - ? muse::trc("project/save", "Sadly, your file has become corrupt.\n\n") - : ""; + std::any savingFileIsCorrupt = ret.data("savingFileIsCorrupt"); + std::string savingFileIsCorruptMessage = savingFileIsCorrupt.has_value() && std::any_cast(savingFileIsCorrupt) + ? "" + : " " + muse::trc("project/save", "or from the _saving file"); // The Linux build doesn't like trailing and leading whitespace in translations std::string message = muse::qtrc("project/save", - "An error occurred while saving your file. This may be a one-off error. " - "Please retry saving the file in order not to lose your latest changes.\n\n" + "An error occurred while saving your file and sadly it has become corrupt. " + "This may be a one-off error. Please retry saving the file in order not to lose your latest changes.\n\n" "If it keeps happening, try saving the file to a different location or to MuseScore.com. " - "Go to the “Support and bug reports” forum at https://musescore.org/en/forum for help.\n\n" - "%1" + "If all else fails, restore the file from the most recent backup created by MuseScore Studio%1.\n\n" + "If you need help, go to the “Support and bug reports” forum at https://musescore.org/en/forum\n\n" "Error: %2") - .arg(originalFileCorruptedMessage.c_str(), errMessage.c_str()) + .arg(savingFileIsCorruptMessage.c_str(), errMessage.c_str()) .toStdString(); interactive()->warning(muse::trc("project/save", "Your score could not be saved"), message);