From 1dd56df04849310a701dbd783a6583f713b8215a Mon Sep 17 00:00:00 2001 From: ispyisail Date: Wed, 30 Sep 2026 10:45:25 +1300 Subject: [PATCH] Keep three crash-recovery snapshots instead of overwriting one Discussion #598, reviving PR #654. The crash-recovery backup written every 20 minutes went to a single file. If the project was already in a bad state when a backup ran, that bad state replaced the only recovery copy. QETProject now writes the backups in turn to three KAutoSaveFile slots (BackupGenerations), so one bad write only replaces the oldest snapshot. Scope is crash recovery only; the opt-in autosave is unchanged. After a crash, the recovery prompt groups the snapshots by project and offers one row per project with a list to pick the snapshot to reopen, newest selected by default. The snapshots not picked are deleted. Ported onto current master: writeBackup() keeps the "skip if nothing changed" check (bugtracker #273) and offerBackupFiles() keeps its place after the stale-file filter and before the crash report (#901). Tested with the backup interval shortened to 4 s (test build only): after three changes, master holds one recovery file, overwritten each time; this branch holds three, with 4, 5 and 6 folios. After killing QET, the prompt lists the project; picking the oldest snapshot reopens 4 folios, the default reopens 6. Twice each. ctest 34/34 with and without KDE Frameworks; the carried-over KAutoSaveFile test passes in the nokde build. Co-Authored-By: Claude Opus 5.5 --- cmake/qet_compilation_vars.cmake | 2 + sources/qetapp.cpp | 55 +++++----- sources/qetproject.cpp | 27 +++-- sources/qetproject.h | 10 +- sources/ui/backuprestoredialog.cpp | 142 +++++++++++++++++++++++++ sources/ui/backuprestoredialog.h | 60 +++++++++++ tests/catch/src/kautosavefile_test.cpp | 105 ++++++++++++++++++ 7 files changed, 365 insertions(+), 36 deletions(-) create mode 100644 sources/ui/backuprestoredialog.cpp create mode 100644 sources/ui/backuprestoredialog.h diff --git a/cmake/qet_compilation_vars.cmake b/cmake/qet_compilation_vars.cmake index d57817384..eab3ba0a7 100644 --- a/cmake/qet_compilation_vars.cmake +++ b/cmake/qet_compilation_vars.cmake @@ -752,6 +752,8 @@ set(QET_SRC_FILES ${QET_DIR}/sources/ui/diagrampropertieseditordockwidget.h ${QET_DIR}/sources/ui/backupdialog.cpp ${QET_DIR}/sources/ui/backupdialog.h + ${QET_DIR}/sources/ui/backuprestoredialog.cpp + ${QET_DIR}/sources/ui/backuprestoredialog.h ${QET_DIR}/sources/ui/dialogwaiting.cpp ${QET_DIR}/sources/ui/dialogwaiting.h ${QET_DIR}/sources/ui/duplicateoffsetdialog.cpp diff --git a/sources/qetapp.cpp b/sources/qetapp.cpp index f7c086756..568cca5de 100644 --- a/sources/qetapp.cpp +++ b/sources/qetapp.cpp @@ -55,6 +55,7 @@ #include #define QUOTE(x) STRINGIFY(x) #define STRINGIFY(x) #x +#include #include #include #include @@ -65,6 +66,9 @@ #else # include #endif +#include "ui/backuprestoredialog.h" + +#include #ifdef QET_ALLOW_OVERRIDE_CED_OPTION QString QETApp::m_overrided_common_elements_dir = QString(); @@ -2839,48 +2843,47 @@ void QETApp::checkBackupFiles() /** @brief QETApp::offerBackupFiles Ask whether to reopen the recovery files left by a previous run, and - open or discard them accordingly. + open or discard them accordingly. A project can leave several recovery + files, one per snapshot (@see QETProject::writeBackup): they are grouped + by project, and the user picks which one to reopen, the newest by default. @param stale_files : the recovery files to offer */ void QETApp::offerBackupFiles(const QList &stale_files) { - QString text; - if(stale_files.size() == 1) { - text.append(tr("Le fichier de restauration suivant a été trouvé,
" - "Voulez-vous l'ouvrir ?
")); - } else { - text.append(tr("Les fichiers de restauration suivant on été trouvé,
" - "Voulez-vous les ouvrir ?
")); + //Group the snapshots by the project they recover, newest first. + QHash> groups; + for (KAutoSaveFile *kasf : stale_files) { + groups[kasf->managedFile().path()].append(kasf); } - for(const KAutoSaveFile *kasf : stale_files) - { -# ifdef Q_OS_WIN - //Remove the first character '/' before the name of the drive - text.append("
" + kasf->managedFile().path().remove(0,1)); -# else - text.append("
" + kasf->managedFile().path()); -# endif + for (auto &snapshots : groups) { + std::sort(snapshots.begin(), snapshots.end(), + [](KAutoSaveFile *a, KAutoSaveFile *b) { + return QFileInfo(*a).lastModified() + > QFileInfo(*b).lastModified(); + }); } - //Open backup file - if (QET::QetMessageBox::question(nullptr, - tr("Fichier de restauration"), - text, - QMessageBox::Ok - |QMessageBox::Cancel - ) - == QMessageBox::Ok) + BackupRestoreDialog dialog(groups, nullptr); + if (dialog.exec() == QDialog::Accepted) { + //The snapshots not picked are no longer needed. + for (KAutoSaveFile *discarded : dialog.discardedFiles()) + { + discarded->open(QIODevice::ReadWrite); + delete discarded; + } + + const QList to_open = dialog.selectedFiles(); //If there are open editors, find those that are visible if (diagramEditors().count()) { diagramEditors().first()->setVisible(true); - diagramEditors().first()->openBackupFiles(stale_files); + diagramEditors().first()->openBackupFiles(to_open); } else { QETDiagramEditor *editor = new QETDiagramEditor(); - editor->openBackupFiles(stale_files); + editor->openBackupFiles(to_open); } } else //Clear backup file diff --git a/sources/qetproject.cpp b/sources/qetproject.cpp index a44e127ef..d126b45f9 100644 --- a/sources/qetproject.cpp +++ b/sources/qetproject.cpp @@ -205,7 +205,7 @@ QETProject::QETProject(KAutoSaveFile *backup, QObject *parent) : QETProject::~QETProject() { //Wait for any in-flight async crash-recovery backup to finish: the worker - //writes through &m_backup_file, a member that would otherwise be destroyed + //writes through m_backup_files, a member that would otherwise be destroyed //under it (issue #492). m_backup_future.waitForFinished(); @@ -562,12 +562,16 @@ void QETProject::setFilePath(const QString &filepath) if (filepath == m_file_path) { return; } - //Don't close/re-point the backup file while a backup is still writing it. + //Don't close/re-point the backup files while a backup is still writing one. m_backup_future.waitForFinished(); - if (m_backup_file.isOpen()) { - m_backup_file.close(); + const QUrl managed_file = QUrl::fromLocalFile(filepath); + for (auto &backup_file : m_backup_files) { + if (backup_file.isOpen()) { + backup_file.close(); + } + backup_file.setManagedFile(managed_file); } - m_backup_file.setManagedFile(QUrl::fromLocalFile(filepath)); + m_next_backup_slot = 0; m_file_path = filepath; QFileInfo fi(m_file_path); @@ -2337,14 +2341,17 @@ void QETProject::detachDiagram(Diagram *diagram) /** @brief QETProject::writeBackup - Write a backup file of this project, in the case that QET crash + Write a backup file of this project, in the case that QET crash. + The snapshots are written in turn to m_backup_files, so a write made + while the project is already in a bad state only replaces the oldest + snapshot, and the earlier ones are still there to recover from. */ void QETProject::writeBackup() { if (!m_backup_enabled) return; //Don't launch a new backup while the previous one is still writing: - //both would write through &m_backup_file on different threads. + //both could write through the same m_backup_files slot on different threads. if (m_backup_future.isRunning()) return; //toXml() walks the whole project on the GUI thread, which freezes @@ -2357,8 +2364,10 @@ void QETProject::writeBackup() //Qt5-style QtConcurrent::run(function, reference-args) call did not //survive the Qt6 API change, a lambda behaves identically on both. QDomDocument xml_project(toXml()); - m_backup_future = QtConcurrent::run([this, xml_project]() mutable { - return QET::writeToFile(xml_project, &m_backup_file, nullptr); + KAutoSaveFile *target = &m_backup_files[m_next_backup_slot]; + m_next_backup_slot = (m_next_backup_slot + 1) % BackupGenerations; + m_backup_future = QtConcurrent::run([target, xml_project]() mutable { + return QET::writeToFile(xml_project, target, nullptr); }); } diff --git a/sources/qetproject.h b/sources/qetproject.h index b30b07075..9e260bcc4 100644 --- a/sources/qetproject.h +++ b/sources/qetproject.h @@ -39,6 +39,8 @@ #include #include +#include + class Diagram; class ElementsLocation; class QETResult; @@ -128,6 +130,10 @@ class QETProject : public QObject /// process can destroy the project before the write finishes (crash). static void setBackupEnabled(bool enabled); + /// Number of crash-recovery snapshots kept per project, written in + /// turn by writeBackup(), so one bad write cannot replace the only copy + static constexpr int BackupGenerations = 3; + ///DEFAULT PROPERTIES BorderProperties defaultBorderProperties() const; void setDefaultBorderProperties(const BorderProperties &); @@ -366,7 +372,9 @@ class QETProject : public QObject QTimer m_save_backup_timer, m_autosave_timer; QFuture m_backup_future; - KAutoSaveFile m_backup_file; + /// Crash-recovery snapshots, written in turn by writeBackup() + std::array m_backup_files; + int m_next_backup_slot = 0; QUuid m_uuid = QUuid::createUuid(); QHash m_derived_uuid_keys; QSet m_saved_item_uuids; //symbol and wire uuids the file carries, see derivedItemUuid() diff --git a/sources/ui/backuprestoredialog.cpp b/sources/ui/backuprestoredialog.cpp new file mode 100644 index 000000000..72e46712b --- /dev/null +++ b/sources/ui/backuprestoredialog.cpp @@ -0,0 +1,142 @@ +/* + Copyright 2006-2026 The QElectroTech Team + This file is part of QElectroTech. + + QElectroTech is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation, either version 2 of the License, or + (at your option) any later version. + + QElectroTech 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 QElectroTech. If not, see . +*/ + +#include "backuprestoredialog.h" + +#ifdef BUILD_WITHOUT_KF +# include "nokde/kautosavefile.h" +#else +# include +#endif + +#include +#include +#include +#include +#include +#include +#include +#include + +/** + @brief BackupRestoreDialog::BackupRestoreDialog + @param groups : managed project path -> its recovery generations + (newest-first) + @param parent : parent widget +*/ +BackupRestoreDialog::BackupRestoreDialog( + const QHash> &groups, + QWidget *parent) : + QDialog(parent), + m_groups(groups) +{ + setWindowTitle(tr("Fichiers de restauration", "window title")); + + auto main_layout = new QVBoxLayout(this); + + auto intro = new QLabel( + tr("Des fichiers de restauration ont été trouvés,
" + "voulez-vous les ouvrir ?
" + "Pour un projet ayant plusieurs versions de restauration, " + "la plus récente est sélectionnée par défaut.", + "dialog message")); + intro->setWordWrap(true); + main_layout->addWidget(intro); + + auto grid = new QGridLayout(); + int row = 0; + for (auto it = m_groups.constBegin(); it != m_groups.constEnd(); ++it) + { + const QString &path = it.key(); + const QList &generations = it.value(); + +# ifdef Q_OS_WIN + QString display_path = path; + display_path.remove(0, 1); +# else + const QString &display_path = path; +# endif + grid->addWidget(new QLabel(display_path), row, 0); + + auto combo = new QComboBox(this); + for (int i = 0; i < generations.size(); ++i) + { + const QDateTime modified = + QFileInfo(*generations.at(i)).lastModified(); + QString label = QLocale::system().toString( + modified, QLocale::ShortFormat); + if (i == 0) { + label = tr("%1 (la plus récente)", "recovery generation label") + .arg(label); + } + combo->addItem(label); + } + combo->setEnabled(generations.size() > 1); + grid->addWidget(combo, row, 1); + + m_combo_for_path.insert(path, combo); + ++row; + } + main_layout->addLayout(grid); + + auto buttons = new QDialogButtonBox( + QDialogButtonBox::Ok | QDialogButtonBox::Cancel, this); + connect(buttons, &QDialogButtonBox::accepted, this, &QDialog::accept); + connect(buttons, &QDialogButtonBox::rejected, this, &QDialog::reject); + main_layout->addWidget(buttons); +} + +/** + @brief BackupRestoreDialog::~BackupRestoreDialog +*/ +BackupRestoreDialog::~BackupRestoreDialog() = default; + +/** + @return one recovery generation per project, matching the combo box + selection (the newest generation, unless the user picked another). +*/ +QList BackupRestoreDialog::selectedFiles() const +{ + QList selected; + for (auto it = m_groups.constBegin(); it != m_groups.constEnd(); ++it) + { + const int index = m_combo_for_path.value(it.key())->currentIndex(); + selected << it.value().at(index); + } + return selected; +} + +/** + @return every recovery generation the user did not pick; the caller + should release and delete these. +*/ +QList BackupRestoreDialog::discardedFiles() const +{ + QList discarded; + for (auto it = m_groups.constBegin(); it != m_groups.constEnd(); ++it) + { + const int index = m_combo_for_path.value(it.key())->currentIndex(); + const QList &generations = it.value(); + for (int i = 0; i < generations.size(); ++i) { + if (i != index) { + discarded << generations.at(i); + } + } + } + return discarded; +} diff --git a/sources/ui/backuprestoredialog.h b/sources/ui/backuprestoredialog.h new file mode 100644 index 000000000..30eed692d --- /dev/null +++ b/sources/ui/backuprestoredialog.h @@ -0,0 +1,60 @@ +/* + Copyright 2006-2026 The QElectroTech Team + This file is part of QElectroTech. + + QElectroTech is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation, either version 2 of the License, or + (at your option) any later version. + + QElectroTech 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 QElectroTech. If not, see . +*/ + +#ifndef BACKUPRESTOREDIALOG_H +#define BACKUPRESTOREDIALOG_H + +#include +#include +#include +#include + +class KAutoSaveFile; +class QComboBox; + +/** + @brief Lets the user pick, per crashed project, which recovery + generation to reopen when the crash-recovery rotation + (@see QETProject::writeBackup) left more than one snapshot behind. +*/ +class BackupRestoreDialog : public QDialog +{ + Q_OBJECT + + public: + /// @param groups : managed project file path -> its stale recovery + /// generations, each list already sorted newest-first. Ownership of + /// the KAutoSaveFile objects stays with the caller. + explicit BackupRestoreDialog( + const QHash> &groups, + QWidget *parent = nullptr); + ~BackupRestoreDialog() override; + + /// Valid once accepted: one entry per project, the chosen generation + /// (defaults to the most recent one). + QList selectedFiles() const; + /// The generations the user did not pick; the caller should discard + /// (release + delete) these. + QList discardedFiles() const; + + private: + QHash> m_groups; + QHash m_combo_for_path; +}; + +#endif // BACKUPRESTOREDIALOG_H diff --git a/tests/catch/src/kautosavefile_test.cpp b/tests/catch/src/kautosavefile_test.cpp index 64258c117..6e1e1c829 100644 --- a/tests/catch/src/kautosavefile_test.cpp +++ b/tests/catch/src/kautosavefile_test.cpp @@ -3,14 +3,18 @@ #include #include +#include #include #include #include #include +#include #include +#include #ifdef Q_OS_UNIX +#include #include #include #include @@ -106,3 +110,104 @@ TEST_CASE("Qt-only KAutoSaveFile recovers stale files", "[nokde][autosave]") CHECK_FALSE(QFile::exists(lock_file_name)); #endif } + +TEST_CASE("Multiple KAutoSaveFile generations for one managed file are all " + "found stale after a crash", "[nokde][autosave]") +{ +#ifndef Q_OS_UNIX + SUCCEED("crash-style stale lock test is Unix-only"); +#else + //Simulates QETProject's rotating crash-recovery generations + //(BackupGenerations snapshots written round-robin): several + //KAutoSaveFile instances sharing one managed file, alive at once. + QTemporaryDir data_home; + REQUIRE(data_home.isValid()); + + qputenv("XDG_DATA_HOME", QFile::encodeName(data_home.path())); + QCoreApplication::setOrganizationName(QStringLiteral("QElectroTech")); + QCoreApplication::setApplicationName( + QStringLiteral("KAutoSaveFileGenerationsTest")); + + const auto managed_path = data_home.filePath(QStringLiteral("project.qet")); + QFile managed_file(managed_path); + REQUIRE(managed_file.open(QIODevice::WriteOnly | QIODevice::Text)); + REQUIRE(managed_file.write("\n") > 0); + managed_file.close(); + + constexpr int generations = 3; + + int ready_pipe[2] = {-1, -1}; + REQUIRE(pipe(ready_pipe) == 0); + + const auto child_pid = fork(); + REQUIRE(child_pid >= 0); + + if (child_pid == 0) { + close(ready_pipe[0]); + + std::vector> backups; + for (int i = 0; i < generations; ++i) { + auto backup = std::make_unique( + QUrl::fromLocalFile(managed_path)); + if (!backup->open(QIODevice::WriteOnly + | QIODevice::Truncate + | QIODevice::Text)) { + _exit(2); + } + const QByteArray payload = + "\n"; + if (backup->write(payload) != payload.size()) { + _exit(3); + } + if (!backup->flush()) { + _exit(4); + } + //Give each generation a distinct, increasing mtime. + struct timespec pause{0, 20 * 1000 * 1000}; + nanosleep(&pause, nullptr); + backups.push_back(std::move(backup)); + } + + const char ready = '1'; + if (write(ready_pipe[1], &ready, 1) != 1) { + _exit(5); + } + close(ready_pipe[1]); + + for (;;) { + pause(); + } + } + + close(ready_pipe[1]); + char ready = 0; + REQUIRE(read(ready_pipe[0], &ready, 1) == 1); + close(ready_pipe[0]); + REQUIRE(ready == '1'); + + REQUIRE(kill(child_pid, SIGKILL) == 0); + int status = 0; + REQUIRE(waitpid(child_pid, &status, 0) == child_pid); + REQUIRE(WIFSIGNALED(status)); + REQUIRE(WTERMSIG(status) == SIGKILL); + + auto stale_files = KAutoSaveFile::allStaleFiles(); + REQUIRE(stale_files.size() == generations); + + std::sort(stale_files.begin(), stale_files.end(), + [](KAutoSaveFile *a, KAutoSaveFile *b) { + return QFileInfo(*a).lastModified() < QFileInfo(*b).lastModified(); + }); + + for (int i = 0; i < generations; ++i) { + std::unique_ptr stale_file(stale_files.at(i)); + CHECK(stale_file->managedFile().path() + == QFileInfo(managed_path).absoluteFilePath()); + REQUIRE(stale_file->open(QIODevice::ReadOnly | QIODevice::Text)); + const QByteArray content = stale_file->readAll(); + CHECK(content.contains( + "generation n=\"" + QByteArray::number(i) + "\"")); + } +#endif +}