From 4909061e59ab7408bd295b8801bbd5e30e8507cb Mon Sep 17 00:00:00 2001 From: Nils Schimmelmann Date: Mon, 20 Apr 2026 08:07:05 -0500 Subject: [PATCH 01/12] add atomic saving via .tmp files and ReplaceFileW for Windows --- src/global/io.cpp | 40 +++++++++++++++++++++++++++++++++++- src/global/io.h | 2 ++ src/mapstorage/filesaver.cpp | 15 ++++---------- 3 files changed, 45 insertions(+), 12 deletions(-) diff --git a/src/global/io.cpp b/src/global/io.cpp index db56d9749..acbd5d64c 100644 --- a/src/global/io.cpp +++ b/src/global/io.cpp @@ -21,6 +21,9 @@ #ifdef Q_OS_WIN #include "WinSock.h" + +#include +#include #endif namespace io { @@ -61,7 +64,9 @@ bool fsync(QFile &file) CAN_THROW { const int handle = file.handle(); #ifdef Q_OS_WIN - return false; + if (::FlushFileBuffers(reinterpret_cast(::_get_osfhandle(handle))) == 0) { + throw IOException::withErrorNumber(static_cast(::GetLastError())); + } #elif defined(Q_OS_MAC) if (::fcntl(handle, F_FULLFSYNC) == -1) { throw IOException::withCurrentErrno(); @@ -74,6 +79,39 @@ bool fsync(QFile &file) CAN_THROW return true; } +void rename(const QString &from, const QString &to) CAN_THROW +{ +#ifdef Q_OS_WIN + const std::wstring fromW = from.toStdWString(); + const std::wstring toW = to.toStdWString(); + if (::ReplaceFileW(toW.c_str(), + fromW.c_str(), + nullptr, + REPLACEFILE_IGNORE_MERGE_ERRORS, + nullptr, + nullptr) + == 0) { + const auto err = ::GetLastError(); + if (err == ERROR_FILE_NOT_FOUND) { + if (::MoveFileExW(fromW.c_str(), + toW.c_str(), + MOVEFILE_REPLACE_EXISTING | MOVEFILE_COPY_ALLOWED) + == 0) { + throw IOException::withErrorNumber(static_cast(::GetLastError())); + } + } else { + throw IOException::withErrorNumber(static_cast(err)); + } + } +#else + const auto fromEncoded = QFile::encodeName(from); + const auto toEncoded = QFile::encodeName(to); + if (::rename(fromEncoded.data(), toEncoded.data()) == -1) { + throw IOException::withCurrentErrno(); + } +#endif +} + IOResultEnum fsyncNoexcept(QFile &file) noexcept { try { diff --git a/src/global/io.h b/src/global/io.h index f4051d0d9..b6db8e60d 100644 --- a/src/global/io.h +++ b/src/global/io.h @@ -135,6 +135,8 @@ static_assert(sizeof(ErrorNumberMessage) == 1024); NODISCARD extern bool fsync(QFile &) CAN_THROW; +extern void rename(const QString &from, const QString &to) CAN_THROW; + NODISCARD extern IOResultEnum fsyncNoexcept(QFile &) noexcept; NODISCARD extern bool tuneKeepAlive(qintptr socketDescriptor, diff --git a/src/mapstorage/filesaver.cpp b/src/mapstorage/filesaver.cpp index 59200caca..6f7d1c2e7 100644 --- a/src/mapstorage/filesaver.cpp +++ b/src/mapstorage/filesaver.cpp @@ -14,25 +14,18 @@ #include -static constexpr const bool USE_TMP_SUFFIX = CURRENT_PLATFORM != PlatformEnum::Windows; - static const char *const TMP_FILE_SUFFIX = ".tmp"; NODISCARD static auto maybe_add_suffix(const QString &filename) { - return USE_TMP_SUFFIX ? (filename + TMP_FILE_SUFFIX) : filename; + return filename + TMP_FILE_SUFFIX; } static void remove_tmp_suffix(const QString &filename) CAN_THROW { - if (!USE_TMP_SUFFIX) { - return; - } + const QString from = filename + TMP_FILE_SUFFIX; + const QString to = filename; - const auto from = QFile::encodeName(filename + TMP_FILE_SUFFIX); - const auto to = QFile::encodeName(filename); - if (::rename(from.data(), to.data()) == -1) { - throw io::IOException::withCurrentErrno(); - } + io::rename(from, to); } FileSaver::~FileSaver() From 765550dd78e1429e3036374d5bd8cf6b12babe1a Mon Sep 17 00:00:00 2001 From: Nils Schimmelmann Date: Sun, 19 Apr 2026 21:18:07 -0500 Subject: [PATCH 02/12] fix Mac teardown crashes by correcting QObject ownership hierarchy The application was crashing on macOS during shutdown due to improper parenting of UI components. When objects like delegates or models are parented to the main Widget instead of the View, they can be destroyed out of order, leading to use-after-free errors when the View attempts to access them during its own destruction. - Parent GroupModel and GroupProxyModel to m_table instead of 'this'. - Parent GroupDelegate and TimerDelegate to their respective views. - Ensure the lifecycle of the delegate matches the lifecycle of the view to prevent segmentation faults during signal disconnection. - Restore heap allocation for GroupModel to ensure Qt's ownership system manages its cleanup correctly. --- src/group/groupwidget.cpp | 36 +++++++++++++++++------------------- src/group/groupwidget.h | 6 +++--- src/timers/TimerWidget.cpp | 4 ++-- 3 files changed, 22 insertions(+), 24 deletions(-) diff --git a/src/group/groupwidget.cpp b/src/group/groupwidget.cpp index 0878ee206..3cdb32d67 100644 --- a/src/group/groupwidget.cpp +++ b/src/group/groupwidget.cpp @@ -829,14 +829,7 @@ GroupWidget::GroupWidget(Mmapper2Group *const group, MapData *const md, QWidget : QWidget(parent) , m_group(group) , m_map(md) - , m_model(this) { - if (m_group) { - m_model.setCharacters(m_group->selectAll()); - } else { - m_model.setCharacters({}); - } - auto *layout = new QVBoxLayout(this); layout->setAlignment(Qt::AlignTop); layout->setContentsMargins(0, 0, 0, 0); @@ -849,8 +842,15 @@ GroupWidget::GroupWidget(Mmapper2Group *const group, MapData *const md, QWidget m_table->horizontalHeader()->setStretchLastSection(true); m_table->horizontalHeader()->setSectionResizeMode(QHeaderView::ResizeToContents); - m_proxyModel = new GroupProxyModel(this); - m_proxyModel->setSourceModel(&m_model); + m_model = new GroupModel(m_table); + if (m_group) { + m_model->setCharacters(m_group->selectAll()); + } else { + m_model->setCharacters({}); + } + + m_proxyModel = new GroupProxyModel(m_table); + m_proxyModel->setSourceModel(m_model); m_table->setModel(m_proxyModel); m_table->setDragEnabled(true); @@ -859,7 +859,7 @@ GroupWidget::GroupWidget(Mmapper2Group *const group, MapData *const md, QWidget m_table->setDefaultDropAction(Qt::MoveAction); m_table->setDropIndicatorShown(true); - m_table->setItemDelegate(new GroupDelegate(this)); + m_table->setItemDelegate(new GroupDelegate(m_table)); layout->addWidget(m_table); m_pulseTimer = new QTimer(this); @@ -912,7 +912,7 @@ GroupWidget::GroupWidget(Mmapper2Group *const group, MapData *const md, QWidget return; } - selectedCharacter = m_model.getCharacter(sourceIndex.row()); + selectedCharacter = deref(m_model).getCharacter(sourceIndex.row()); if (selectedCharacter) { // Build Context menu m_center->setText( @@ -947,8 +947,6 @@ GroupWidget::GroupWidget(Mmapper2Group *const group, MapData *const md, QWidget GroupWidget::~GroupWidget() { m_pulseTimer->stop(); - delete m_table; - delete m_recolor; } QSize GroupWidget::sizeHint() const @@ -964,7 +962,7 @@ void GroupWidget::updateColumnVisibility() { // Hide unnecessary columns like mana if everyone is a zorc/troll const auto one_character_had_mana = [this]() -> bool { - for (const auto &character : m_model.getCharacters()) { + for (const auto &character : deref(m_model).getCharacters()) { if (character && (character->getMana() > 0 || character->getMaxMana() > 0)) { return true; } @@ -978,7 +976,7 @@ void GroupWidget::updateColumnVisibility() void GroupWidget::updatePulseTimer() { const auto needs_pulse = [this]() -> bool { - for (const auto &character : m_model.getCharacters()) { + for (const auto &character : deref(m_model).getCharacters()) { if (!character) { continue; } @@ -1012,7 +1010,7 @@ void GroupWidget::updatePulseTimer() void GroupWidget::slot_onCharacterAdded(SharedGroupChar character) { assert(character); - m_model.insertCharacter(character); + deref(m_model).insertCharacter(character); updateColumnVisibility(); updatePulseTimer(); } @@ -1020,7 +1018,7 @@ void GroupWidget::slot_onCharacterAdded(SharedGroupChar character) void GroupWidget::slot_onCharacterRemoved(const GroupId characterId) { assert(characterId != INVALID_GROUPID); - m_model.removeCharacterById(characterId); + deref(m_model).removeCharacterById(characterId); updateColumnVisibility(); updatePulseTimer(); } @@ -1028,13 +1026,13 @@ void GroupWidget::slot_onCharacterRemoved(const GroupId characterId) void GroupWidget::slot_onCharacterUpdated(SharedGroupChar character) { assert(character); - m_model.updateCharacter(character); + deref(m_model).updateCharacter(character); updatePulseTimer(); } void GroupWidget::slot_onGroupReset(const GroupVector &newCharacterList) { - m_model.setCharacters(newCharacterList); + deref(m_model).setCharacters(newCharacterList); updateColumnVisibility(); updatePulseTimer(); } diff --git a/src/group/groupwidget.h b/src/group/groupwidget.h index 2f19fae25..5abe9db08 100644 --- a/src/group/groupwidget.h +++ b/src/group/groupwidget.h @@ -151,7 +151,7 @@ class NODISCARD_QOBJECT GroupWidget final : public QWidget Mmapper2Group *m_group = nullptr; MapData *m_map = nullptr; GroupProxyModel *m_proxyModel = nullptr; - GroupModel m_model; + GroupModel *m_model = nullptr; QTimer *m_pulseTimer = nullptr; void updateColumnVisibility(); @@ -174,8 +174,8 @@ class NODISCARD_QOBJECT GroupWidget final : public QWidget void sig_center(glm::vec2); public slots: - void slot_mapUnloaded() { m_model.setMapLoaded(false); } - void slot_mapLoaded() { m_model.setMapLoaded(true); } + void slot_mapUnloaded() { deref(m_model).setMapLoaded(false); } + void slot_mapLoaded() { deref(m_model).setMapLoaded(true); } private slots: void slot_onCharacterAdded(SharedGroupChar character); diff --git a/src/timers/TimerWidget.cpp b/src/timers/TimerWidget.cpp index e39d80d65..8a575da69 100644 --- a/src/timers/TimerWidget.cpp +++ b/src/timers/TimerWidget.cpp @@ -20,10 +20,10 @@ TimerWidget::TimerWidget(CTimers &timers, QWidget *parent) layout->setContentsMargins(0, 0, 0, 0); layout->setSpacing(0); - m_model = new TimerModel(m_timers, this); m_view = new QTableView(this); + m_model = new TimerModel(m_timers, m_view); m_view->setModel(m_model); - auto *delegate = new TimerDelegate(this); + auto *delegate = new TimerDelegate(m_view); for (int i = 0; i < TimerModel::ColCount; ++i) { m_view->setItemDelegateForColumn(i, delegate); } From f0f3b683c925cc7e7c558faf965e1af4bbfd0603 Mon Sep 17 00:00:00 2001 From: Nils Schimmelmann Date: Mon, 20 Apr 2026 10:33:04 -0500 Subject: [PATCH 03/12] prevent close event while non-cancelable async tasks are running Before this change, the game could close before the async saver had a chance to finish. This commit: - Adds a 'cancelDisposition' to AsyncBase to define if a task is interruptible. - Updates MainWindow::closeEvent to ignore the close request if an active async task is marked as non-cancelable. - Ensures settings are written only after the close event is accepted. - Prevents redundant save prompts when a save task is already in progress. --- src/mainwindow/mainwindow-async.cpp | 25 +++++++++++++++++++---- src/mainwindow/mainwindow.cpp | 31 +++++++++++++++++++++++++---- src/mainwindow/mainwindow.h | 1 + 3 files changed, 49 insertions(+), 8 deletions(-) diff --git a/src/mainwindow/mainwindow-async.cpp b/src/mainwindow/mainwindow-async.cpp index bbddc7c3e..777c30eb1 100644 --- a/src/mainwindow/mainwindow-async.cpp +++ b/src/mainwindow/mainwindow-async.cpp @@ -263,10 +263,13 @@ struct NODISCARD MainWindow::AsyncBase { public: const std::shared_ptr progressCounter; + const CancelDispositionEnum cancelDisposition = CancelDispositionEnum::Allow; public: - explicit AsyncBase(std::shared_ptr pc) + explicit AsyncBase(std::shared_ptr pc, + const CancelDispositionEnum cancelDisposition_) : progressCounter{std::move(pc)} + , cancelDisposition{cancelDisposition_} { if (!progressCounter) { throw std::invalid_argument("pc"); @@ -285,12 +288,16 @@ struct NODISCARD MainWindow::AsyncBase NODISCARD PollResultEnum poll() { return poll(std::chrono::milliseconds{0}); } void request_cancel(); NODISCARD bool requested_cancel() const; + NODISCARD bool is_allowed_to_cancel() const; }; MainWindow::AsyncBase::~AsyncBase() = default; void MainWindow::AsyncBase::request_cancel() { + if (!is_allowed_to_cancel()) { + return; + } progressCounter->requestCancel(); virt_request_cancel(); } @@ -300,6 +307,11 @@ bool MainWindow::AsyncBase::requested_cancel() const return progressCounter->requestedCancel(); } +bool MainWindow::AsyncBase::is_allowed_to_cancel() const +{ + return cancelDisposition == CancelDispositionEnum::Allow; +} + MainWindow::AsyncTask::AsyncTask(QObject *parent) : QObject(parent) {} @@ -337,7 +349,7 @@ void MainWindow::AsyncTask::tick() return; } - if (m_task->poll() != PollResultEnum::Finished) { + if (deref(m_task).poll() != PollResultEnum::Finished) { return; } @@ -347,7 +359,12 @@ void MainWindow::AsyncTask::tick() void MainWindow::AsyncTask::request_cancel() { - m_task->request_cancel(); + deref(m_task).request_cancel(); +} + +bool MainWindow::AsyncTask::is_allowed_to_cancel() const +{ + return deref(m_task).is_allowed_to_cancel(); } void MainWindow::AsyncTask::reset() @@ -403,7 +420,7 @@ struct NODISCARD MainWindow::AsyncHelper : public AsyncBase UniqueStorage ps, const QString &dialogText, const CancelDispositionEnum allow_cancel) - : AsyncBase{std::move(pc)} + : AsyncBase{std::move(pc), allow_cancel} , mainWindow{mw} , fileName{name} , pDevice(std::move(pd)) diff --git a/src/mainwindow/mainwindow.cpp b/src/mainwindow/mainwindow.cpp index 5340baaa5..af5d0f3f9 100644 --- a/src/mainwindow/mainwindow.cpp +++ b/src/mainwindow/mainwindow.cpp @@ -1556,8 +1556,16 @@ bool MainWindow::eventFilter(QObject *const obj, QEvent *const event) void MainWindow::closeEvent(QCloseEvent *const event) { - // REVISIT: wait and see if we're actually exiting first? - writeSettings(); + qInfo() << MM_SOURCE_LOCATION().function_name(); + + if (m_asyncTask) { + // first check avoids prompting to save while saving. + if (!m_asyncTask.is_allowed_to_cancel()) { + qInfo() << "Note: Ignoring close request because the current async task cannot be canceled."; + event->ignore(); + return; + } + } if (!maybeSave()) { event->ignore(); @@ -1565,9 +1573,24 @@ void MainWindow::closeEvent(QCloseEvent *const event) } if (m_asyncTask) { - qInfo() << "Attempting to async task for faster shutdown"; - m_progressDlg->reject(); + // second check is in case we just scheduled a save. + if (!m_asyncTask.is_allowed_to_cancel()) { + qInfo() << "Note: Ignoring close request because the scheduled async task cannot be canceled."; + event->ignore(); + return; + } + if (m_asyncTask.isWorking()) { + qInfo() << "Attempting to cancel async task for faster shutdown"; + m_asyncTask.request_cancel(); + } + if (auto dlg = m_progressDlg.get()) { + qInfo() << "Attempting to reject the progress dialog for faster shutdown"; + dlg->reject(); + } } + + writeSettings(); + event->accept(); } diff --git a/src/mainwindow/mainwindow.h b/src/mainwindow/mainwindow.h index 86622774d..ea03c1ecc 100644 --- a/src/mainwindow/mainwindow.h +++ b/src/mainwindow/mainwindow.h @@ -271,6 +271,7 @@ class NODISCARD_QOBJECT MainWindow final : public QMainWindow void begin(std::unique_ptr task); void tick(); void request_cancel(); + NODISCARD bool is_allowed_to_cancel() const; private: void reset(); From 84222a929f82d1a35c545a6efb6c2fa8edc81954 Mon Sep 17 00:00:00 2001 From: nschimme <5505185+nschimme@users.noreply.github.com> Date: Mon, 20 Apr 2026 17:05:36 +0000 Subject: [PATCH 04/12] fix: resolve ERROR_SHARING_VIOLATION (32) on Windows during save - Reorder operations in FileSaver::close() to close the file handle before renaming the temporary file. - Wrap MapDestination::finalize() in a try-catch block in AsyncSaver::finish_saving to prevent uncaught exceptions and improve error reporting. --- src/mainwindow/mainwindow-async.cpp | 12 ++++++++++-- src/mapstorage/filesaver.cpp | 2 +- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/mainwindow/mainwindow-async.cpp b/src/mainwindow/mainwindow-async.cpp index 777c30eb1..020dcafcc 100644 --- a/src/mainwindow/mainwindow-async.cpp +++ b/src/mainwindow/mainwindow-async.cpp @@ -685,9 +685,17 @@ struct NODISCARD MainWindow::AsyncSaver final : public AsyncHelper finish_saving(success); } - void finish_saving(const bool success) + void finish_saving(bool success) { - pMapDestination->finalize(); + try { + pMapDestination->finalize(); + } catch (const std::exception &ex) { + success = false; + const auto msg = QString::asprintf("Finalize exception: %s", ex.what()); + mainWindow.slot_log("AsyncSaver", msg); + qWarning().noquote() << msg; + } + if constexpr (CURRENT_PLATFORM == PlatformEnum::Wasm) { if (success) { assert(pMapDestination->isFileWasm()); diff --git a/src/mapstorage/filesaver.cpp b/src/mapstorage/filesaver.cpp index 6f7d1c2e7..ea82739b5 100644 --- a/src/mapstorage/filesaver.cpp +++ b/src/mapstorage/filesaver.cpp @@ -60,6 +60,6 @@ void FileSaver::close() CAN_THROW file.flush(); // REVISIT: check return value? std::ignore = ::io::fsync(file); - remove_tmp_suffix(m_filename); file.close(); + remove_tmp_suffix(m_filename); } From 674aa1d79b4a3e15eb7eb3ce350d314b32f4a904 Mon Sep 17 00:00:00 2001 From: nschimme <5505185+nschimme@users.noreply.github.com> Date: Mon, 20 Apr 2026 17:27:23 +0000 Subject: [PATCH 05/12] fix: resolve ERROR_SHARING_VIOLATION (32) on Windows during save - Reorder operations in FileSaver::close() to close the file handle before renaming the temporary file. - Wrap MapDestination::finalize() in a try-catch block in AsyncSaver::finish_saving to prevent uncaught exceptions and improve error reporting. From 9d6671cbb5f2c93933dbbd33999391c6160d8762 Mon Sep 17 00:00:00 2001 From: nschimme <5505185+nschimme@users.noreply.github.com> Date: Mon, 20 Apr 2026 17:42:02 +0000 Subject: [PATCH 06/12] fix: resolve ERROR_SHARING_VIOLATION (32) on Windows and improve save error visibility - Reorder operations in FileSaver::close() to close the file handle before renaming the temporary file, preventing sharing violations on Windows. - Wrap MapDestination::finalize() in a try-catch block in AsyncSaver::finish_saving. - Capture finalization exceptions and display them to the user via a warning dialog for better visibility. --- src/mainwindow/mainwindow-async.cpp | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/mainwindow/mainwindow-async.cpp b/src/mainwindow/mainwindow-async.cpp index 020dcafcc..5398f3442 100644 --- a/src/mainwindow/mainwindow-async.cpp +++ b/src/mainwindow/mainwindow-async.cpp @@ -687,13 +687,15 @@ struct NODISCARD MainWindow::AsyncSaver final : public AsyncHelper void finish_saving(bool success) { + QString errorMsg; try { pMapDestination->finalize(); } catch (const std::exception &ex) { success = false; - const auto msg = QString::asprintf("Finalize exception: %s", ex.what()); - mainWindow.slot_log("AsyncSaver", msg); - qWarning().noquote() << msg; + errorMsg = mmqt::toQStringUtf8(ex.what()); + const auto logMsg = QString::asprintf("Finalize exception: %s", ex.what()); + mainWindow.slot_log("AsyncSaver", logMsg); + qWarning().noquote() << logMsg; } if constexpr (CURRENT_PLATFORM == PlatformEnum::Wasm) { @@ -706,9 +708,13 @@ struct NODISCARD MainWindow::AsyncSaver final : public AsyncHelper extraBlockers.reset(); if (!success) { - mainWindow.showAsyncFailure(fileName, - AsyncTypeEnum::Save, - progressCounter->requestedCancel()); + if (errorMsg.isEmpty()) { + mainWindow.showAsyncFailure(fileName, + AsyncTypeEnum::Save, + progressCounter->requestedCancel()); + } else { + mainWindow.showWarning(tr("Failed to finalize file %1:\n%2").arg(fileName, errorMsg)); + } return; } From 9b890c983717b5a6d15a64cc64922a0bf19fad62 Mon Sep 17 00:00:00 2001 From: nschimme <5505185+nschimme@users.noreply.github.com> Date: Mon, 20 Apr 2026 17:47:50 +0000 Subject: [PATCH 07/12] fix: resolve ERROR_SHARING_VIOLATION (32) on Windows and improve save error visibility - Reorder operations in FileSaver::close() to close the file handle before renaming the temporary file, preventing sharing violations on Windows. - Wrap MapDestination::finalize() in a try-catch block in AsyncSaver::finish_saving. - Capture finalization exceptions and display them to the user via a warning dialog for better visibility. - Ensure correct clang-format in src/mainwindow/mainwindow-async.cpp. --- src/mainwindow/mainwindow-async.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mainwindow/mainwindow-async.cpp b/src/mainwindow/mainwindow-async.cpp index 5398f3442..b958bdab8 100644 --- a/src/mainwindow/mainwindow-async.cpp +++ b/src/mainwindow/mainwindow-async.cpp @@ -713,7 +713,7 @@ struct NODISCARD MainWindow::AsyncSaver final : public AsyncHelper AsyncTypeEnum::Save, progressCounter->requestedCancel()); } else { - mainWindow.showWarning(tr("Failed to finalize file %1:\n%2").arg(fileName, errorMsg)); + mainWindow.showWarning(tr("Failed to save %1:\n%2").arg(fileName, errorMsg)); } return; } From f9e9879e18f0312d8c528b8b313fbc211d539730 Mon Sep 17 00:00:00 2001 From: nschimme <5505185+nschimme@users.noreply.github.com> Date: Mon, 20 Apr 2026 17:57:58 +0000 Subject: [PATCH 08/12] fix: resolve ERROR_SHARING_VIOLATION (32) and improve save error visibility - Separated close() and commit() in FileSaver to ensure file handle release before atomic rename on Windows. - Implemented Windows error message retrieval using FormatMessageW and CP_UTF8. - Added friendly translations for common I/O errors (e.g. sharing violation, access denied). - Robustly propagated background exceptions to the UI in MainWindow::AsyncHelper. - Updated MainWindow::showAsyncFailure to display detailed exception messages in popup dialogs. - Moved file finalization (commit) to background thread for better UI responsiveness. --- src/global/io.cpp | 86 +++++++++++++++++++++- src/mainwindow/mainwindow-async.cpp | 107 ++++++++++++++++------------ src/mainwindow/mainwindow.cpp | 13 ++-- src/mainwindow/mainwindow.h | 5 +- src/mapstorage/MapDestination.cpp | 1 + src/mapstorage/filesaver.cpp | 9 +++ src/mapstorage/filesaver.h | 7 ++ 7 files changed, 175 insertions(+), 53 deletions(-) diff --git a/src/global/io.cpp b/src/global/io.cpp index acbd5d64c..16ffc849a 100644 --- a/src/global/io.cpp +++ b/src/global/io.cpp @@ -32,7 +32,37 @@ ErrorNumberMessage::ErrorNumberMessage(const int error_number) noexcept : m_error_number{error_number} { #ifdef Q_OS_WIN - /* nop */ + LPWSTR messageBuffer = nullptr; + const DWORD size = FormatMessageW(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM + | FORMAT_MESSAGE_IGNORE_INSERTS, + nullptr, + static_cast(error_number), + MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), + reinterpret_cast(&messageBuffer), + 0, + nullptr); + if (size > 0) { + const int res = WideCharToMultiByte(CP_UTF8, + 0, + messageBuffer, + static_cast(size), + m_buf, + static_cast(sizeof(m_buf)) - 1, + nullptr, + nullptr); + if (res > 0) { + m_buf[res] = '\0'; + // Trim trailing newlines and spaces + int len = res; + while (len > 0 + && (m_buf[len - 1] == '\r' || m_buf[len - 1] == '\n' || m_buf[len - 1] == ' ' + || m_buf[len - 1] == '\t' || m_buf[len - 1] == '.')) { + m_buf[--len] = '\0'; + } + m_str = m_buf; + } + LocalFree(messageBuffer); + } #elif defined(__GLIBC__) /* GNU/Linux version can return a pointer to a static string */ m_str = ::strerror_r(error_number, m_buf, sizeof(m_buf)); @@ -44,13 +74,63 @@ ErrorNumberMessage::ErrorNumberMessage(const int error_number) noexcept #endif } +namespace { // anonymous +NODISCARD std::string getFriendlyMessage(const int error_number) +{ +#ifdef Q_OS_WIN + switch (error_number) { + case ERROR_ACCESS_DENIED: + return "Access denied. You might not have permission to write to this location."; + case ERROR_SHARING_VIOLATION: + return "The file is being used by another process."; + case ERROR_FILE_NOT_FOUND: + case ERROR_PATH_NOT_FOUND: + return "The system cannot find the file or path specified."; + case ERROR_DISK_FULL: + return "The disk is full."; + case ERROR_WRITE_FAULT: + return "The system could not write to the specified device."; + default: + return ""; + } +#else + switch (error_number) { + case EACCES: + return "Permission denied."; + case ENOENT: + return "No such file or directory."; + case ENOSPC: + return "No space left on device."; + case EBUSY: + return "Resource busy."; + case EROFS: + return "Read-only file system."; + default: + return ""; + } +#endif +} +} // namespace + IOException IOException::withErrorNumber(const int error_number) { + const std::string friendly = getFriendlyMessage(error_number); if (const auto msg = ErrorNumberMessage{error_number}) { - return IOException{msg.getErrorMessage()}; + const std::string sysMsg = msg.getErrorMessage(); + if (friendly.empty()) { + return IOException{sysMsg}; + } + if (friendly == sysMsg) { + return IOException{friendly}; + } + return IOException{friendly + " (" + sysMsg + ")"}; } - return IOException{"unknown error_number: " + std::to_string(error_number)}; + const std::string unknown = "unknown error code " + std::to_string(error_number); + if (!friendly.empty()) { + return IOException{friendly + " (" + unknown + ")"}; + } + return IOException{unknown}; } IOException IOException::withCurrentErrno() diff --git a/src/mainwindow/mainwindow-async.cpp b/src/mainwindow/mainwindow-async.cpp index b958bdab8..bf2db1f73 100644 --- a/src/mainwindow/mainwindow-async.cpp +++ b/src/mainwindow/mainwindow-async.cpp @@ -177,20 +177,28 @@ NODISCARD PollResultEnum wait_for(std::future &future, const std::chrono::mil } template -NODISCARD std::optional extract(std::future> &future, MainWindow &mainWindow) +struct NODISCARD BackgroundResult final +{ + std::optional data; + QString exceptionMsg; +}; + +template +NODISCARD BackgroundResult extract(std::future> &future, MainWindow &mainWindow) { try { - return future.get(); + return {future.get(), QString{}}; } catch (const MapStorageError &ex) { - QMessageBox::critical(&mainWindow, - MainWindow::tr("MapStorage Error"), - mmqt::toQStringUtf8(ex.what())); + const QString msg = mmqt::toQStringUtf8(ex.what()); + QMessageBox::critical(&mainWindow, MainWindow::tr("MapStorage Error"), msg); + return {std::nullopt, msg}; } catch (const std::exception &ex) { - const auto msg = QString::asprintf("Exception: %s", ex.what()); - mainWindow.slot_log("AbstractMapStorage", msg); - qWarning().noquote() << msg; + const QString msg = mmqt::toQStringUtf8(ex.what()); + const auto logMsg = QString::asprintf("Exception: %s", ex.what()); + mainWindow.slot_log("AbstractMapStorage", logMsg); + qWarning().noquote() << logMsg; + return {std::nullopt, msg}; } - return std::nullopt; } } // namespace mwa_detail @@ -540,13 +548,15 @@ struct NODISCARD MainWindow::AsyncLoader final : public MainWindow::AsyncHelper void virt_finish() final { - const Result result = mwa_detail::extract(future, mainWindow); + const auto background = mwa_detail::extract(future, mainWindow); + const Result &result = background.data; // REVISIT: what if you just wanted to load markers? if (!result || result->mapPair.modified.getRoomsCount() == 0) { mainWindow.showAsyncFailure(fileName, AsyncTypeEnum::Load, - progressCounter->requestedCancel()); + progressCounter->requestedCancel(), + background.exceptionMsg); return; } @@ -609,11 +619,13 @@ struct NODISCARD MainWindow::AsyncMerge final : public AsyncHelper void virt_finish() final { - const Result result = mwa_detail::extract(future, mainWindow); + const auto background = mwa_detail::extract(future, mainWindow); + const Result &result = background.data; if (!result) { mainWindow.showAsyncFailure(fileName, AsyncTypeEnum::Merge, - progressCounter->requestedCancel()); + progressCounter->requestedCancel(), + background.exceptionMsg); return; } @@ -669,7 +681,16 @@ struct NODISCARD MainWindow::AsyncSaver final : public AsyncHelper { AbstractMapStorage &storage = deref(pStorage); const MapData &mapData = deref(mainWindow.m_mapData); - return background::save(storage, mapData, mode); + if (!background::save(storage, mapData, mode)) { + return false; + } + + try { + pMapDestination->finalize(); + return true; + } catch (...) { + throw; + } } private: @@ -680,24 +701,13 @@ struct NODISCARD MainWindow::AsyncSaver final : public AsyncHelper void virt_finish() final { - const Result result = mwa_detail::extract(future, mainWindow); - const bool success = result.has_value() && result.value(); - finish_saving(success); + const auto background = mwa_detail::extract(future, mainWindow); + const bool success = background.data.has_value() && background.data.value(); + finish_saving(success, background.exceptionMsg); } - void finish_saving(bool success) + void finish_saving(const bool success, const QString &exceptionMsg) { - QString errorMsg; - try { - pMapDestination->finalize(); - } catch (const std::exception &ex) { - success = false; - errorMsg = mmqt::toQStringUtf8(ex.what()); - const auto logMsg = QString::asprintf("Finalize exception: %s", ex.what()); - mainWindow.slot_log("AsyncSaver", logMsg); - qWarning().noquote() << logMsg; - } - if constexpr (CURRENT_PLATFORM == PlatformEnum::Wasm) { if (success) { assert(pMapDestination->isFileWasm()); @@ -708,13 +718,10 @@ struct NODISCARD MainWindow::AsyncSaver final : public AsyncHelper extraBlockers.reset(); if (!success) { - if (errorMsg.isEmpty()) { - mainWindow.showAsyncFailure(fileName, - AsyncTypeEnum::Save, - progressCounter->requestedCancel()); - } else { - mainWindow.showWarning(tr("Failed to save %1:\n%2").arg(fileName, errorMsg)); - } + mainWindow.showAsyncFailure(fileName, + AsyncTypeEnum::Save, + progressCounter->requestedCancel(), + exceptionMsg); return; } @@ -927,12 +934,17 @@ bool MainWindow::slot_checkMapConsistency() } void virt_finish() override { - const Result result = mwa_detail::extract(future, mainWindow); - const bool success = result.has_value() && result.value(); + const auto background = mwa_detail::extract(future, mainWindow); + const bool success = background.data.has_value() && background.data.value(); if (success) { mainWindow.showWarning("Map is consistent."); } else { - mainWindow.showWarning("ERROR: Failed map consistency check."); + if (background.exceptionMsg.isEmpty()) { + mainWindow.showWarning("ERROR: Failed map consistency check."); + } else { + mainWindow.showWarning( + tr("ERROR: Failed map consistency check:\n%1").arg(background.exceptionMsg)); + } } } }; @@ -1037,15 +1049,20 @@ bool MainWindow::slot_generateBaseMap() } void virt_finish() override { - Result result = mwa_detail::extract(future, mainWindow); - if (!result) { + auto background = mwa_detail::extract(future, mainWindow); + if (!background.data) { const bool wasCanceled = progressCounter->requestedCancel(); - const char *const msg = wasCanceled ? "User canceled generation of the base map" - : "Failed to generate the base map"; - mainWindow.showWarning(tr(msg)); + if (wasCanceled) { + mainWindow.showWarning(tr("User canceled generation of the base map")); + } else if (background.exceptionMsg.isEmpty()) { + mainWindow.showWarning(tr("Failed to generate the base map")); + } else { + mainWindow.showWarning( + tr("Failed to generate the base map:\n%1").arg(background.exceptionMsg)); + } return; } - onSuccess(std::move(result.value())); + onSuccess(std::move(background.data.value())); } void onSuccess(BaseMapData result) { diff --git a/src/mainwindow/mainwindow.cpp b/src/mainwindow/mainwindow.cpp index af5d0f3f9..6daa1ee2c 100644 --- a/src/mainwindow/mainwindow.cpp +++ b/src/mainwindow/mainwindow.cpp @@ -1779,12 +1779,17 @@ void MainWindow::showWarning(const QString &s) void MainWindow::showAsyncFailure(const QString &fileName, const AsyncTypeEnum mode, - const bool wasCanceled) + const bool wasCanceled, + const QString &exceptionMsg) { const char *const modeName = get_type_name(mode); - const char *const msg = wasCanceled ? "User canceled the %1 of file %2" - : "Failed to %1 file %2"; - showWarning(tr(msg).arg(modeName, fileName)); + if (wasCanceled) { + showWarning(tr("User canceled the %1 of file %2").arg(modeName, fileName)); + } else if (exceptionMsg.isEmpty()) { + showWarning(tr("Failed to %1 file %2").arg(modeName, fileName)); + } else { + showWarning(tr("Failed to %1 file %2:\n%3").arg(modeName, fileName, exceptionMsg)); + } } void MainWindow::slot_onFindRoom() diff --git a/src/mainwindow/mainwindow.h b/src/mainwindow/mainwindow.h index ea03c1ecc..bb9e60296 100644 --- a/src/mainwindow/mainwindow.h +++ b/src/mainwindow/mainwindow.h @@ -293,7 +293,10 @@ class NODISCARD_QOBJECT MainWindow final : public QMainWindow void percentageChanged(uint32_t); private: - void showAsyncFailure(const QString &fileName, AsyncTypeEnum mode, bool wasCanceled); + void showAsyncFailure(const QString &fileName, + AsyncTypeEnum mode, + bool wasCanceled, + const QString &exceptionMsg = {}); NODISCARD std::unique_ptr getLoadOrMergeMapStorage( const std::shared_ptr &pc, std::shared_ptr &source); diff --git a/src/mapstorage/MapDestination.cpp b/src/mapstorage/MapDestination.cpp index 8810ee193..44acce40d 100644 --- a/src/mapstorage/MapDestination.cpp +++ b/src/mapstorage/MapDestination.cpp @@ -88,6 +88,7 @@ void MapDestination::finalize() } else if (isFileNative()) { assert(m_fileSaver); m_fileSaver->close(); + m_fileSaver->commit(); } else { assert(isDirectory()); } diff --git a/src/mapstorage/filesaver.cpp b/src/mapstorage/filesaver.cpp index ea82739b5..592718f58 100644 --- a/src/mapstorage/filesaver.cpp +++ b/src/mapstorage/filesaver.cpp @@ -61,5 +61,14 @@ void FileSaver::close() CAN_THROW // REVISIT: check return value? std::ignore = ::io::fsync(file); file.close(); +} + +void FileSaver::commit() CAN_THROW +{ + auto &file = deref(m_file); + if (file.isOpen()) { + throw std::runtime_error("FileSaver::commit() called while file is still open"); + } + remove_tmp_suffix(m_filename); } diff --git a/src/mapstorage/filesaver.h b/src/mapstorage/filesaver.h index e783a03d9..b2184c053 100644 --- a/src/mapstorage/filesaver.h +++ b/src/mapstorage/filesaver.h @@ -44,4 +44,11 @@ class NODISCARD FileSaver final /*! \exception std::runtime_error if the file can't be safely closed. */ void close() CAN_THROW; + + /*! \brief Finalize the save operation by renaming the temporary file. + * + * This must be called after close(). + * \exception std::runtime_error if the rename operation fails. + */ + void commit() CAN_THROW; }; From 9e83bfe30cab536764e48c054f01ab76528a5d83 Mon Sep 17 00:00:00 2001 From: nschimme <5505185+nschimme@users.noreply.github.com> Date: Mon, 20 Apr 2026 18:08:15 +0000 Subject: [PATCH 09/12] fix: resolve ERROR_SHARING_VIOLATION (32) and improve save error visibility - Reimplemented io::ErrorNumberMessage for Windows to retrieve system error strings via FormatMessageW. - Added a friendly error translation system in io::IOException to provide user-friendly messages for common errors (sharing violation, access denied, etc.). - Separated FileSaver::close() (flush/sync/close) from FileSaver::commit() (atomic rename) to ensure file handles are released before renaming. - Refactored AsyncHelper to robustly propagate background exception messages to the UI. - Updated MainWindow::showAsyncFailure to display these detailed messages in popup dialogs. - Moved file finalization (including rename) to the background thread to prevent UI freezing and ensure consistent error reporting. - Added TestGlobal::ioExceptionTest to verify the error translation logic. --- tests/TestGlobal.cpp | 15 +++++++++++++++ tests/TestGlobal.h | 1 + 2 files changed, 16 insertions(+) diff --git a/tests/TestGlobal.cpp b/tests/TestGlobal.cpp index f2e84cc9e..a846f5459 100644 --- a/tests/TestGlobal.cpp +++ b/tests/TestGlobal.cpp @@ -18,6 +18,7 @@ #include "../src/global/TaggedString.h" #include "../src/global/TextUtils.h" #include "../src/global/WeakHandle.h" +#include "../src/global/io.h" #include "../src/global/emojis.h" #include "../src/global/entities.h" #include "../src/global/float_cast.h" @@ -146,6 +147,20 @@ void TestGlobal::caseUtilsTest() test::testCaseUtils(); } +void TestGlobal::ioExceptionTest() +{ +#ifdef Q_OS_WIN + const auto ex = io::IOException::withErrorNumber(32); // ERROR_SHARING_VIOLATION + QVERIFY(QString::fromStdString(ex.what()).contains("The file is being used by another process")); +#else + const auto ex = io::IOException::withErrorNumber(EBUSY); + QVERIFY(QString::fromStdString(ex.what()).contains("Resource busy")); +#endif + const auto exUnknown = io::IOException::withErrorNumber(999999); + const QString msg = QString::fromStdString(exUnknown.what()); + QVERIFY(msg.contains("999999")); +} + void TestGlobal::castTest() { test::test_int_cast(); diff --git a/tests/TestGlobal.h b/tests/TestGlobal.h index 97e96d798..e4e1dfbbe 100644 --- a/tests/TestGlobal.h +++ b/tests/TestGlobal.h @@ -20,6 +20,7 @@ private Q_SLOTS: static void ansiTextUtilsTest(); static void ansiToRgbTest(); static void caseUtilsTest(); + static void ioExceptionTest(); static void castTest(); static void charsetTest(); static void charUtilsTest(); From 98ede01b298544240719fb927bf543e4bbc3d7ac Mon Sep 17 00:00:00 2001 From: nschimme <5505185+nschimme@users.noreply.github.com> Date: Mon, 20 Apr 2026 18:25:11 +0000 Subject: [PATCH 10/12] fix: resolve Windows sharing violation (32) and improve error reporting This change addresses a bug on Windows where map saving would fail with ERROR_SHARING_VIOLATION because the file handle was still open during rename. It also modernizes the error reporting system. Technical changes: - Separated FileSaver::close() (releasing handle) from commit() (renaming). - Implemented Windows error string retrieval via FormatMessageW. - Added a translation layer for common I/O errors (Windows & POSIX) to provide user-friendly descriptions. - Linked ws2_32 to mm_global to support socket/IO functions in tests. - Refactored AsyncHelper/AsyncTask to propagate background exceptions to the UI, displaying them in a popup dialog. - Moved finalization (sync/close/rename) to the background thread to keep the UI responsive. - Fixed MSVC preprocessor warning in mm_source_location.h. - Added unit tests in TestGlobal to verify error translations. --- src/CMakeLists.txt | 1 + src/global/mm_source_location.h | 4 ++-- tests/TestGlobal.cpp | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 2d2dfbf8a..7f9965d02 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -852,6 +852,7 @@ endif() if(WIN32) target_link_libraries(mmapper PRIVATE ws2_32) + target_link_libraries(mm_global PUBLIC ws2_32) endif() if(EMSCRIPTEN) diff --git a/src/global/mm_source_location.h b/src/global/mm_source_location.h index 87fed4181..f12cc3037 100644 --- a/src/global/mm_source_location.h +++ b/src/global/mm_source_location.h @@ -6,8 +6,8 @@ #include -#if __cplusplus >= 202000L \ - && __has_builtin(__builtin_source_location) // && __cpp_lib_source_location >= 201907L +#if __cplusplus >= 202000L && __has_builtin(__builtin_source_location) +// && __cpp_lib_source_location >= 201907L #include namespace mm { using source_location = std::source_location; diff --git a/tests/TestGlobal.cpp b/tests/TestGlobal.cpp index a846f5459..819a9ce47 100644 --- a/tests/TestGlobal.cpp +++ b/tests/TestGlobal.cpp @@ -18,11 +18,11 @@ #include "../src/global/TaggedString.h" #include "../src/global/TextUtils.h" #include "../src/global/WeakHandle.h" -#include "../src/global/io.h" #include "../src/global/emojis.h" #include "../src/global/entities.h" #include "../src/global/float_cast.h" #include "../src/global/int_cast.h" +#include "../src/global/io.h" #include "../src/global/string_view_utils.h" #include "../src/global/unquote.h" #include "../src/global/utils.h" From 76583bd2a4750e9fd91617feece1042ba0a2ebc4 Mon Sep 17 00:00:00 2001 From: nschimme <5505185+nschimme@users.noreply.github.com> Date: Mon, 20 Apr 2026 18:27:32 +0000 Subject: [PATCH 11/12] fix: resolve Windows sharing violation (32) and improve error reporting This change addresses a bug on Windows where map saving would fail with ERROR_SHARING_VIOLATION because the file handle was still open during rename. It also modernizes the error reporting system. Technical changes: - Separated FileSaver::close() (releasing handle) from commit() (renaming). - Implemented Windows error string retrieval via FormatMessageW. - Added a translation layer for common I/O errors (Windows & POSIX) to provide user-friendly descriptions. - Linked ws2_32 to mm_global to support socket/IO functions in tests. - Refactored AsyncHelper/AsyncTask to propagate background exceptions to the UI, displaying them in a popup dialog. - Moved file finalization (sync/close/rename) to the background thread to keep the UI responsive. - Fixed MSVC preprocessor warning in mm_source_location.h. - Added unit tests in TestGlobal to verify error translations and covered POSIX friendly error cases to improve code coverage. --- tests/TestGlobal.cpp | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/tests/TestGlobal.cpp b/tests/TestGlobal.cpp index 819a9ce47..fe1c8cfa5 100644 --- a/tests/TestGlobal.cpp +++ b/tests/TestGlobal.cpp @@ -153,12 +153,23 @@ void TestGlobal::ioExceptionTest() const auto ex = io::IOException::withErrorNumber(32); // ERROR_SHARING_VIOLATION QVERIFY(QString::fromStdString(ex.what()).contains("The file is being used by another process")); #else - const auto ex = io::IOException::withErrorNumber(EBUSY); - QVERIFY(QString::fromStdString(ex.what()).contains("Resource busy")); + for (int err : {EACCES, ENOENT, ENOSPC, EBUSY, EROFS}) { + const auto ex = io::IOException::withErrorNumber(err); + const QString msg = QString::fromStdString(ex.what()); + QVERIFY(!msg.isEmpty()); + QVERIFY(msg != QString("unknown error_number: %1").arg(err)); + } + QVERIFY(QString::fromStdString(io::IOException::withErrorNumber(EACCES).what()) + .contains("Permission denied")); + QVERIFY(QString::fromStdString(io::IOException::withErrorNumber(ENOENT).what()) + .contains("No such file or directory")); #endif const auto exUnknown = io::IOException::withErrorNumber(999999); const QString msg = QString::fromStdString(exUnknown.what()); QVERIFY(msg.contains("999999")); + + const io::IOException exStr("test message"); + QCOMPARE(exStr.what(), std::string("test message")); } void TestGlobal::castTest() From 26b9a16a541b11d5089f688a05918e4aa6faa59d Mon Sep 17 00:00:00 2001 From: nschimme <5505185+nschimme@users.noreply.github.com> Date: Mon, 20 Apr 2026 19:05:41 +0000 Subject: [PATCH 12/12] fix: resolve Windows sharing violation (32) and improve error reporting This change addresses a bug on Windows where map saving would fail with ERROR_SHARING_VIOLATION because the file handle was still open during rename. It also modernizes the error reporting system. Technical changes: - Separated FileSaver::close() (releasing handle) from commit() (renaming). - Implemented Windows error string retrieval via FormatMessageW. - Added a translation layer for common I/O errors (Windows & POSIX) to provide user-friendly descriptions. - Linked ws2_32 to mm_global to support socket/IO functions in tests. - Refactored AsyncHelper/AsyncTask to propagate background exceptions to the UI, displaying them in a popup dialog. - Moved file finalization (sync/close/rename) to the background thread to keep the UI responsive. - Fixed MSVC preprocessor warning in mm_source_location.h. - Added unit tests in TestGlobal to verify error translations and covered POSIX friendly error cases and io::fsyncNoexcept to improve code coverage. --- tests/TestGlobal.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/TestGlobal.cpp b/tests/TestGlobal.cpp index fe1c8cfa5..546647513 100644 --- a/tests/TestGlobal.cpp +++ b/tests/TestGlobal.cpp @@ -170,6 +170,15 @@ void TestGlobal::ioExceptionTest() const io::IOException exStr("test message"); QCOMPARE(exStr.what(), std::string("test message")); + + // coverage for io::fsyncNoexcept and io::ErrorNumberMessage + QFile dummyFile; + QCOMPARE(io::fsyncNoexcept(dummyFile), io::IOResultEnum::EXCEPTION); + + const io::ErrorNumberMessage msgObj(EBUSY); + QVERIFY(msgObj); + QVERIFY(msgObj.getErrorMessage() != nullptr); + QCOMPARE(msgObj.getErrorNumber(), EBUSY); } void TestGlobal::castTest()