Merge pull request #1109 from ispyisail/fix/second-save-changes-nothing

Fix a just-saved project changing again on its next save

sources/diagramcontext.cpp

@@ -144,6 +144,27 @@ bool DiagramContext::operator!=(const DiagramContext &dc) const
 	return(!(*this == dc));
 }
 
+namespace {
+/**
+	The value as it is saved, and so as it is read back: stray leading and
+	trailing whitespace around real content trimmed, but not a value that IS
+	whitespace -- unconditionally trimming an all-whitespace string collapses
+	it to "", which is silently indistinguishable from a value that was never
+	set. A title-block custom variable set to a single space -- a workaround
+	for #973, where an unset variable renders as its own literal placeholder
+	-- would otherwise vanish on the very next save.
+	Applied when reading as well as when writing, so that what is in memory
+	after a load is what the next save writes: a label shown from an
+	untrimmed value would otherwise keep its spaces on screen and in its
+	displayed copy until the project was saved and opened again, and a
+	just-saved project would change on its second save.
+*/
+QString storedValue(const QString &raw)
+{
+	return raw.trimmed().isEmpty() ? raw : raw.trimmed();
+}
+} // namespace
+
 /**
 	Export this context properties under the \a e XML element, using tags
 	named \a tag_name (defaults to "property").
@@ -161,15 +182,7 @@ void DiagramContext::toXml(QDomElement &e, const QString &tag_name) const
 		property.removeAttribute("name");
 		property.setAttribute("show", m_content_show[key]);
 		property.setAttribute("name", key);
-		// Trim stray leading/trailing whitespace around real content, but
-		// not a value that IS whitespace: unconditionally trimming an
-		// all-whitespace string collapses it to "", which is silently
-		// indistinguishable from a value that was never set. A title-block
-		// custom variable set to a single space -- a workaround for #973,
-		// where an unset variable renders as its own literal placeholder --
-		// would otherwise vanish on the very next save.
-		const QString stored = raw.trimmed().isEmpty() ? raw : raw.trimmed();
-		QDomText value = e.ownerDocument().createTextNode(stored);
+		QDomText value = e.ownerDocument().createTextNode(storedValue(raw));
 		property.appendChild(value);
 		e.appendChild(property);
 	}
@@ -182,7 +195,7 @@ void DiagramContext::toXml(QDomElement &e, const QString &tag_name) const
 void DiagramContext::fromXml(const QDomElement &e, const QString &tag_name) {
 	foreach (QDomElement property, QET::findInDomElement(e, tag_name)) {
 		if (!property.hasAttribute("name")) continue;
-		addValue(property.attribute("name"), QVariant(property.text()));
+		addValue(property.attribute("name"), QVariant(storedValue(property.text())));
 		m_content_show.insert(property.attribute("name"), property.attribute("show", "1").toInt());
 	}
 }
@@ -198,7 +211,8 @@ void DiagramContext::fromXml(const pugi::xml_node &dom_element, const QString &t
 {
 	for(auto node = dom_element.child(tag_name.toStdString().c_str()) ; node ; node = node.next_sibling(tag_name.toStdString().c_str()))
 	{
-		addValue(node.attribute("name").as_string(), QVariant(node.text().as_string()));
+		addValue(node.attribute("name").as_string(),
+				 QVariant(storedValue(QString::fromUtf8(node.text().as_string()))));
 		m_content_show.insert(node.attribute("name").as_string(), node.attribute("show").empty()? 1 : node.attribute("show").as_int());
 	}
 }

sources/qetgraphicsitem/element.cpp

@@ -1033,7 +1033,11 @@ QDomElement Element::toXml(
 		QDomElement infos =
 				document.createElement(QStringLiteral("elementInformations"));
 		m_data.m_informations.toXml(infos, QStringLiteral("elementInformation"));
-		element.appendChild(infos);
+			//toXml() skips empty values: an element whose information is
+			//all empty would otherwise be written an empty block, which the
+			//next load reads as no information and the next save drops.
+		if (infos.hasChildNodes())
+			element.appendChild(infos);
 	}
 
 		//Save override properties (For now, only used when the element is a terminal)

tests/qttest/CMakeLists.txt

@@ -328,3 +328,30 @@ target_include_directories(tst_conductorselfretrace PRIVATE ${QET_DIR}/sources)
 target_link_libraries(tst_conductorselfretrace PRIVATE Qt::Test)
 target_compile_definitions(tst_conductorselfretrace PRIVATE
   "QET_TEST_BINARY_PATH=\"$<TARGET_FILE:qelectrotech>\"")
+
+# Saving a project that was just saved changes nothing: runs the real
+# binary's --resave twice on every project in examples/, and on one whose
+# title block holds a single-space value (#973).
+add_executable(
+  tst_resaveunchanged
+  tst_resaveunchanged.cpp)
+add_test(NAME tst_resaveunchanged COMMAND tst_resaveunchanged)
+add_dependencies(tst_resaveunchanged qelectrotech)
+target_link_libraries(tst_resaveunchanged PRIVATE Qt::Test)
+target_compile_definitions(tst_resaveunchanged PRIVATE
+  "QET_TEST_BINARY_PATH=\"$<TARGET_FILE:qelectrotech>\""
+  "QET_EXAMPLES_DIR=\"${QET_DIR}/examples\"")
+
+# DiagramContext::fromXml() -- the two readers (QDom for projects, pugixml
+# for element definitions in the collection) give the same values: stray
+# spaces trimmed, accents kept.
+add_executable(
+  tst_diagramcontext
+  tst_diagramcontext.cpp
+  ${QET_DIR}/sources/diagramcontext.cpp
+  ${QET_DIR}/sources/qet.cpp
+  ${QET_DIR}/sources/qeticons.cpp
+  ${QET_DIR}/sources/shortcutmanager.cpp)
+add_test(NAME tst_diagramcontext COMMAND tst_diagramcontext)
+target_include_directories(tst_diagramcontext PRIVATE ${QET_DIR}/sources)
+target_link_libraries(tst_diagramcontext PRIVATE Qt::Test Qt::Widgets Qt::Xml pugixml::pugixml)

tests/qttest/tst_diagramcontext.cpp

@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+#include <QtTest>
+
+#include "diagramcontext.h"
+#include "qetapp.h"
+
+QString QETApp::m_interface_language;
+
+/**
+	DiagramContext::fromXml() has two readers: QDom, for projects, and
+	pugixml, for element definitions in the collection. Both must give the
+	values the next save writes -- stray spaces around real content trimmed,
+	accented characters kept.
+*/
+class tst_diagramcontext : public QObject
+{
+	Q_OBJECT
+
+	static QByteArray xml(const QString &value)
+	{
+		return QStringLiteral(
+				   "<elementInformations>"
+				   "<elementInformation name=\"v\" show=\"1\">%1</elementInformation>"
+				   "</elementInformations>")
+				.arg(value)
+				.toUtf8();
+	}
+
+	static QString fromDom(const QByteArray &data)
+	{
+		QDomDocument doc;
+		if (!doc.setContent(data)) return QStringLiteral("<parse error>");
+		DiagramContext dc;
+		dc.fromXml(doc.documentElement(), QStringLiteral("elementInformation"));
+		return dc.value(QStringLiteral("v")).toString();
+	}
+
+	static QString fromPugi(const QByteArray &data)
+	{
+		pugi::xml_document doc;
+		if (!doc.load_buffer(data.constData(), size_t(data.size())))
+			return QStringLiteral("<parse error>");
+		DiagramContext dc;
+		dc.fromXml(doc.document_element(), QStringLiteral("elementInformation"));
+		return dc.value(QStringLiteral("v")).toString();
+	}
+
+private slots:
+	void bothReadersAgree_data()
+	{
+		QTest::addColumn<QString>("value");
+		QTest::addColumn<QString>("expected");
+
+		QTest::newRow("plain")       << "PRISE" << "PRISE";
+		QTest::newRow("stray spaces") << " PRISE  " << "PRISE";
+		QTest::newRow("accents")     << "Armoire façade été" << "Armoire façade été";
+		QTest::newRow("accents and stray spaces") << "  Moteur à cage " << "Moteur à cage";
+		QTest::newRow("non-Latin")   << "Двигатель 電機" << "Двигатель 電機";
+	}
+
+	void bothReadersAgree()
+	{
+		QFETCH(QString, value);
+		QFETCH(QString, expected);
+		QCOMPARE(fromDom(xml(value)), expected);
+		QCOMPARE(fromPugi(xml(value)), expected);
+	}
+};
+
+QTEST_APPLESS_MAIN(tst_diagramcontext)
+
+#include "tst_diagramcontext.moc"

tests/qttest/tst_resaveunchanged.cpp

@@ -0,0 +1,121 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+#include <QtTest>
+
+#include <QDir>
+#include <QFile>
+#include <QProcess>
+#include <QProcessEnvironment>
+#include <QRegularExpression>
+#include <QTemporaryDir>
+
+// Saving a project that was just saved must change nothing. Two things
+// made the second save differ from the first, both cleanup done on save
+// but not on load:
+//  - symbol information whose values were all empty was written as an
+//    empty <elementInformations/> block, which the next load read as no
+//    information and the next save dropped (Projet_vierge.qet);
+//  - information values were trimmed on save but not on load, so a label
+//    with stray spaces kept them in its displayed copy until the project
+//    was opened again (m_000.qet).
+// Runs the real binary's --resave twice on every example, and on a
+// project whose title block holds a value that is a single space (#973).
+class tst_resaveunchanged : public QObject
+{
+	Q_OBJECT
+
+	QTemporaryDir m_dir;
+	int m_run = 0;
+
+	// --resave @p in to a new file, in a sandbox of its own (so a running
+	// QElectroTech cannot answer instead); returns the new file's path.
+	QString resave(const QString &in)
+	{
+		const QString out = m_dir.filePath(QStringLiteral("out%1.qet").arg(m_run));
+		const QString home = m_dir.filePath(QStringLiteral("home%1").arg(m_run++));
+		QDir().mkpath(home);
+		QProcessEnvironment env = QProcessEnvironment::systemEnvironment();
+		env.insert(QStringLiteral("QT_QPA_PLATFORM"), QStringLiteral("offscreen"));
+		env.insert(QStringLiteral("HOME"), home);
+		env.insert(QStringLiteral("XDG_CONFIG_HOME"), home + QStringLiteral("/config"));
+		env.insert(QStringLiteral("XDG_DATA_HOME"), home + QStringLiteral("/data"));
+		QProcess proc;
+		proc.setProcessEnvironment(env);
+		proc.start(QStringLiteral(QET_TEST_BINARY_PATH), {QStringLiteral("--resave"), in, out});
+		if (!proc.waitForFinished(120000) || proc.exitCode() != 0) return {};
+		return out;
+	}
+
+	static QByteArray read(const QString &path)
+	{
+		QFile f(path);
+		return f.open(QIODevice::ReadOnly) ? f.readAll() : QByteArray();
+	}
+
+private slots:
+	void initTestCase()
+	{
+		QVERIFY(m_dir.isValid());
+		QVERIFY(QFile::exists(QStringLiteral(QET_TEST_BINARY_PATH)));
+	}
+
+	// Projet_vierge.qet has empty information values, m_000.qet values
+	// with stray spaces; every other example is here so a new cause shows.
+	void secondSaveChangesNothing_data()
+	{
+		QTest::addColumn<QString>("project");
+		const QDir examples(QStringLiteral(QET_EXAMPLES_DIR));
+		const QStringList projects =
+				examples.entryList({QStringLiteral("*.qet")}, QDir::Files, QDir::Name);
+		QVERIFY(!projects.isEmpty());
+		for (const QString &project : projects)
+			QTest::newRow(project.toUtf8().constData()) << examples.filePath(project);
+	}
+
+	void secondSaveChangesNothing()
+	{
+		QFETCH(QString, project);
+		const QString first = resave(project);
+		QVERIFY2(!first.isEmpty(), "first --resave failed");
+		const QString second = resave(first);
+		QVERIFY2(!second.isEmpty(), "second --resave failed");
+		const QByteArray a = read(first), b = read(second);
+		QVERIFY(!a.isEmpty());
+		QVERIFY2(a == b, "the second save changed the file");
+	}
+
+	// A title-block value that is a single space is kept through two saves
+	// (#973), and a value with accents comes back as it went in.
+	void singleSpaceValueKept()
+	{
+		QByteArray xml = read(QStringLiteral(QET_EXAMPLES_DIR "/Projet_vierge.qet"));
+		QVERIFY(xml.contains("<properties>"));
+		xml.replace("<properties>",
+					"<properties>"
+					"<property show=\"1\" name=\"space\"> </property>"
+					"<property show=\"1\" name=\"accents\">Armoire façade été</property>");
+		const QString in = m_dir.filePath(QStringLiteral("space.qet"));
+		QFile f(in);
+		QVERIFY(f.open(QIODevice::WriteOnly));
+		f.write(xml);
+		f.close();
+
+		const QString first = resave(in);
+		QVERIFY2(!first.isEmpty(), "first --resave failed");
+		const QString second = resave(first);
+		QVERIFY2(!second.isEmpty(), "second --resave failed");
+		const QByteArray a = read(first), b = read(second);
+		QVERIFY2(a == b, "the second save changed the file");
+
+		const QString saved = QString::fromUtf8(b);
+		QVERIFY2(saved.contains(QRegularExpression(
+					 QStringLiteral("<property [^>]*name=\"space\"[^>]*> </property>"))),
+				 "the single-space value was lost");
+		QVERIFY2(saved.contains(QRegularExpression(
+					 QStringLiteral("<property [^>]*name=\"accents\"[^>]*>Armoire façade été</property>"))),
+				 "the accented value changed");
+	}
+};
+
+QTEST_APPLESS_MAIN(tst_resaveunchanged)
+
+#include "tst_resaveunchanged.moc"