Merge pull request #1167 from ispyisail/revive/654-rotating-backups

Keep three crash-recovery snapshots instead of overwriting one

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

sources/qetapp.cpp

@@ -57,6 +57,7 @@
 #define QUOTE(x) STRINGIFY(x)
 #define STRINGIFY(x) #x
 #include <QFileDialog>
+#include <QFileInfo>
 #include <QFontDatabase>
 #include <QProcessEnvironment>
 #include <QRegularExpression>
@@ -68,6 +69,9 @@
 #else
 #	include <KAutoSaveFile>
 #endif
+#include "ui/backuprestoredialog.h"
+
+#include <algorithm>
 
 #ifdef QET_ALLOW_OVERRIDE_CED_OPTION
 QString QETApp::m_overrided_common_elements_dir = QString();
@@ -2945,48 +2949,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<KAutoSaveFile *> &stale_files)
 {
-	QString text;
-	if(stale_files.size() == 1) {
-		text.append(tr("<b>Le fichier de restauration suivant a été trouvé,<br>"
-					   "Voulez-vous l'ouvrir ?</b><br>"));
-	} else {
-		text.append(tr("<b>Les fichiers de restauration suivant on été trouvé,<br>"
-					   "Voulez-vous les ouvrir ?</b><br>"));
+		//Group the snapshots by the project they recover, newest first.
+	QHash<QString, QList<KAutoSaveFile *>> 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("<br>" + kasf->managedFile().path().remove(0,1));
-#	else
-	text.append("<br>" + 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<KAutoSaveFile *> 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

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);
 	});
 }
 

sources/qetproject.h

@@ -39,6 +39,8 @@
 #include <QSet>
 #include <QFuture>
 
+#include <array>
+
 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<bool> m_backup_future;
-		KAutoSaveFile m_backup_file;
+			/// Crash-recovery snapshots, written in turn by writeBackup()
+		std::array<KAutoSaveFile, BackupGenerations> m_backup_files;
+		int m_next_backup_slot = 0;
 		QUuid m_uuid = QUuid::createUuid();
 		QHash<QString, int> m_derived_uuid_keys;
 		QSet<QUuid> m_saved_item_uuids;	//symbol and wire uuids the file carries, see derivedItemUuid()

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 <http://www.gnu.org/licenses/>.
+*/
+
+#include "backuprestoredialog.h"
+
+#ifdef BUILD_WITHOUT_KF
+#	include "nokde/kautosavefile.h"
+#else
+#	include <KAutoSaveFile>
+#endif
+
+#include <QComboBox>
+#include <QDateTime>
+#include <QDialogButtonBox>
+#include <QFileInfo>
+#include <QGridLayout>
+#include <QLabel>
+#include <QLocale>
+#include <QVBoxLayout>
+
+/**
+	@brief BackupRestoreDialog::BackupRestoreDialog
+	@param groups : managed project path -> its recovery generations
+	(newest-first)
+	@param parent : parent widget
+*/
+BackupRestoreDialog::BackupRestoreDialog(
+	const QHash<QString, QList<KAutoSaveFile *>> &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("<b>Des fichiers de restauration ont été trouvés,<br>"
+		   "voulez-vous les ouvrir ?</b><br>"
+		   "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<KAutoSaveFile *> &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<KAutoSaveFile *> BackupRestoreDialog::selectedFiles() const
+{
+	QList<KAutoSaveFile *> 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<KAutoSaveFile *> BackupRestoreDialog::discardedFiles() const
+{
+	QList<KAutoSaveFile *> 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<KAutoSaveFile *> &generations = it.value();
+		for (int i = 0; i < generations.size(); ++i) {
+			if (i != index) {
+				discarded << generations.at(i);
+			}
+		}
+	}
+	return discarded;
+}

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 <http://www.gnu.org/licenses/>.
+*/
+
+#ifndef BACKUPRESTOREDIALOG_H
+#define BACKUPRESTOREDIALOG_H
+
+#include <QDialog>
+#include <QHash>
+#include <QList>
+#include <QString>
+
+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<QString, QList<KAutoSaveFile *>> &groups,
+			QWidget *parent = nullptr);
+		~BackupRestoreDialog() override;
+
+		/// Valid once accepted: one entry per project, the chosen generation
+		/// (defaults to the most recent one).
+		QList<KAutoSaveFile *> selectedFiles() const;
+		/// The generations the user did not pick; the caller should discard
+		/// (release + delete) these.
+		QList<KAutoSaveFile *> discardedFiles() const;
+
+	private:
+		QHash<QString, QList<KAutoSaveFile *>> m_groups;
+		QHash<QString, QComboBox *> m_combo_for_path;
+};
+
+#endif // BACKUPRESTOREDIALOG_H

tests/catch/src/kautosavefile_test.cpp

@@ -3,14 +3,18 @@
 #include <catch2/catch.hpp>
 
 #include <QCoreApplication>
+#include <QDateTime>
 #include <QFile>
 #include <QFileInfo>
 #include <QTemporaryDir>
 #include <QUrl>
 
+#include <algorithm>
 #include <memory>
+#include <vector>
 
 #ifdef Q_OS_UNIX
+#include <ctime>
 #include <signal.h>
 #include <sys/types.h>
 #include <sys/wait.h>
@@ -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("<project/>\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<std::unique_ptr<KAutoSaveFile>> backups;
+		for (int i = 0; i < generations; ++i) {
+			auto backup = std::make_unique<KAutoSaveFile>(
+				QUrl::fromLocalFile(managed_path));
+			if (!backup->open(QIODevice::WriteOnly
+						  | QIODevice::Truncate
+						  | QIODevice::Text)) {
+				_exit(2);
+			}
+			const QByteArray payload =
+				"<project><generation n=\"" + QByteArray::number(i)
+				+ "\" /></project>\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<KAutoSaveFile> 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
+}