From 75450d7102f8d45696327e290391f4e92ffcbba5 Mon Sep 17 00:00:00 2001 From: ispyisail Date: Mon, 28 Sep 2026 21:59:02 +1300 Subject: [PATCH 1/2] Make saving a just-saved project change nothing Saving a project that had just been saved changed it again in 18 of the 24 example projects, so a project kept in version control showed changes nobody made. Both causes were cleanup done on save but not on load: - Symbol information whose values were all empty was written as an empty block (DiagramContext::toXml() skips empty values, Element::toXml() wrote the block anyway). The next load read it as no information and the next save dropped it. The block is now written only when something went into it. - Information values were trimmed on save but not on load, so a label with stray spaces (" PRISE") kept them in memory and in its displayed copy until the project was opened again. The same rule, kept in one place, now applies when reading: stray whitespace around real content trimmed, a value that is only whitespace kept (#973). All 24 examples now save identically a second time (master: 6), and each one's first save is byte-for-byte what master wrote only on its second. A title-block property set to a single space keeps it through two saves. tst_resaveunchanged runs --resave twice on Projet_vierge.qet and m_000.qet; both fail without this change. Co-Authored-By: Claude Opus 5.5 --- sources/diagramcontext.cpp | 36 +++++++++---- sources/qetgraphicsitem/element.cpp | 6 ++- tests/qttest/CMakeLists.txt | 13 +++++ tests/qttest/tst_resaveunchanged.cpp | 81 ++++++++++++++++++++++++++++ 4 files changed, 124 insertions(+), 12 deletions(-) create mode 100644 tests/qttest/tst_resaveunchanged.cpp diff --git a/sources/diagramcontext.cpp b/sources/diagramcontext.cpp index 5137a7bf5..94b8696a2 100644 --- a/sources/diagramcontext.cpp +++ b/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()); } } diff --git a/sources/qetgraphicsitem/element.cpp b/sources/qetgraphicsitem/element.cpp index fbbc3df5d..eb9dbfbe2 100644 --- a/sources/qetgraphicsitem/element.cpp +++ b/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) diff --git a/tests/qttest/CMakeLists.txt b/tests/qttest/CMakeLists.txt index 94283cc09..525ccb8cd 100644 --- a/tests/qttest/CMakeLists.txt +++ b/tests/qttest/CMakeLists.txt @@ -328,3 +328,16 @@ 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=\"$\"") + +# Saving a project that was just saved changes nothing: runs the real +# binary's --resave twice on examples/Projet_vierge.qet (empty information +# values) and examples/m_000.qet (information values with stray spaces). +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=\"$\"" + "QET_EXAMPLES_DIR=\"${QET_DIR}/examples\"") diff --git a/tests/qttest/tst_resaveunchanged.cpp b/tests/qttest/tst_resaveunchanged.cpp new file mode 100644 index 000000000..aff0fe360 --- /dev/null +++ b/tests/qttest/tst_resaveunchanged.cpp @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +#include + +#include +#include +#include +#include +#include + +// 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 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 each example. +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))); + } + + void secondSaveChangesNothing_data() + { + QTest::addColumn("project"); + QTest::newRow("empty information values") << QStringLiteral("Projet_vierge.qet"); + QTest::newRow("information values with stray spaces") << QStringLiteral("m_000.qet"); + } + + void secondSaveChangesNothing() + { + QFETCH(QString, project); + const QString first = resave(QStringLiteral(QET_EXAMPLES_DIR "/") + 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"); + } +}; + +QTEST_APPLESS_MAIN(tst_resaveunchanged) + +#include "tst_resaveunchanged.moc" From e25ae3cbfbcfc0dafc2e034cfd6503dd66459e66 Mon Sep 17 00:00:00 2001 From: ispyisail Date: Mon, 28 Sep 2026 23:29:51 +1300 Subject: [PATCH 2/2] Test the second save on every example, a single-space value and accents Review of #1109 (scorpio810): - tst_resaveunchanged now resaves every .qet in examples/ twice instead of two of them (24 rows, ~40 s). Dropping the load-time trim now also fails affuteuse_250h.qet, which the two-file version missed. - New case singleSpaceValueKept: a title-block property set to one space survives two saves (#973), and an accented property comes back unchanged. Fails if qetproject.cpp stops parsing with PreserveSpacingOnlyNodes. - New tst_diagramcontext: the QDom reader (projects) and the pugixml reader (element definitions in the collection) return the same value for plain, stray-spaced, accented and non-Latin text. Fails if the pugixml path decodes as Latin-1 or either reader stops trimming. The pugixml reader still reads a single-space value as "": pugixml drops whitespace-only text unless parse_ws_pcdata is set, as it did before this PR. That reader only sees element definitions, never a project, so #973's title-block values do not go through it. Co-Authored-By: Claude Opus 5.5 --- tests/qttest/CMakeLists.txt | 18 ++++++- tests/qttest/tst_diagramcontext.cpp | 72 ++++++++++++++++++++++++++++ tests/qttest/tst_resaveunchanged.cpp | 48 +++++++++++++++++-- 3 files changed, 132 insertions(+), 6 deletions(-) create mode 100644 tests/qttest/tst_diagramcontext.cpp diff --git a/tests/qttest/CMakeLists.txt b/tests/qttest/CMakeLists.txt index 525ccb8cd..a32d385f4 100644 --- a/tests/qttest/CMakeLists.txt +++ b/tests/qttest/CMakeLists.txt @@ -330,8 +330,8 @@ target_compile_definitions(tst_conductorselfretrace PRIVATE "QET_TEST_BINARY_PATH=\"$\"") # Saving a project that was just saved changes nothing: runs the real -# binary's --resave twice on examples/Projet_vierge.qet (empty information -# values) and examples/m_000.qet (information values with stray spaces). +# 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) @@ -341,3 +341,17 @@ target_link_libraries(tst_resaveunchanged PRIVATE Qt::Test) target_compile_definitions(tst_resaveunchanged PRIVATE "QET_TEST_BINARY_PATH=\"$\"" "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) diff --git a/tests/qttest/tst_diagramcontext.cpp b/tests/qttest/tst_diagramcontext.cpp new file mode 100644 index 000000000..f0e004b0d --- /dev/null +++ b/tests/qttest/tst_diagramcontext.cpp @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +#include + +#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( + "" + "%1" + "") + .arg(value) + .toUtf8(); + } + + static QString fromDom(const QByteArray &data) + { + QDomDocument doc; + if (!doc.setContent(data)) return QStringLiteral(""); + 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(""); + DiagramContext dc; + dc.fromXml(doc.document_element(), QStringLiteral("elementInformation")); + return dc.value(QStringLiteral("v")).toString(); + } + +private slots: + void bothReadersAgree_data() + { + QTest::addColumn("value"); + QTest::addColumn("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" diff --git a/tests/qttest/tst_resaveunchanged.cpp b/tests/qttest/tst_resaveunchanged.cpp index aff0fe360..55fed0942 100644 --- a/tests/qttest/tst_resaveunchanged.cpp +++ b/tests/qttest/tst_resaveunchanged.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include // Saving a project that was just saved must change nothing. Two things @@ -16,7 +17,8 @@ // - 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 each example. +// 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 @@ -56,17 +58,23 @@ private slots: 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("project"); - QTest::newRow("empty information values") << QStringLiteral("Projet_vierge.qet"); - QTest::newRow("information values with stray spaces") << QStringLiteral("m_000.qet"); + 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(QStringLiteral(QET_EXAMPLES_DIR "/") + project); + const QString first = resave(project); QVERIFY2(!first.isEmpty(), "first --resave failed"); const QString second = resave(first); QVERIFY2(!second.isEmpty(), "second --resave failed"); @@ -74,6 +82,38 @@ private slots: 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("")); + xml.replace("", + "" + " " + "Armoire façade été"); + 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("]*name=\"space\"[^>]*> "))), + "the single-space value was lost"); + QVERIFY2(saved.contains(QRegularExpression( + QStringLiteral("]*name=\"accents\"[^>]*>Armoire façade été"))), + "the accented value changed"); + } }; QTEST_APPLESS_MAIN(tst_resaveunchanged)