From 0721b42e2103705dcb2d1983fa14dad942204074 Mon Sep 17 00:00:00 2001 From: ispyisail Date: Mon, 28 Sep 2026 20:13:31 +1300 Subject: [PATCH 1/3] Give symbols saved without a uuid the same one on every load A symbol saved without a uuid got a random one from Element::fromXml() on every load, and the next save wrote it out: two loads of the same file gave the same symbol two identities, and anything pointing at it by uuid (a script, a comparison of two versions, a wire's identity) could not follow it from one session to the next. When a folio is loaded, such a symbol now gets a UUID v5 derived from what it is and where it sits: its type, its position on the folio and its orientation. Never the folio's index, so inserting or moving a folio does not change it. Identical symbols stacked on one spot, or a copied folio, are told apart by a counter kept per project (QETProject::derivedUuid()), in load order among those symbols alone. A paste still renews uuids. Symbols that have a uuid in the file keep it. All 24 example projects already have one for every symbol, so they are unchanged; with the symbols' uuids stripped, each saves byte-for-byte the same twice (master: different every time). tst_derivedsymboluuid runs --resave on a fixture with its uuids stripped: same uuids on every load, same after a folio is inserted in front, saved uuids kept, stacked copies differ. The first two fail without this change. Co-Authored-By: Claude Opus 5.5 --- sources/diagram.cpp | 17 +++ sources/qetgraphicsitem/element.h | 1 + sources/qetproject.cpp | 21 +++ sources/qetproject.h | 2 + tests/qttest/CMakeLists.txt | 12 ++ tests/qttest/tst_derivedsymboluuid.cpp | 184 +++++++++++++++++++++++++ 6 files changed, 237 insertions(+) create mode 100644 tests/qttest/tst_derivedsymboluuid.cpp diff --git a/sources/diagram.cpp b/sources/diagram.cpp index 4831b6484..bc0cd7c9e 100644 --- a/sources/diagram.cpp +++ b/sources/diagram.cpp @@ -1676,6 +1676,23 @@ bool Diagram::fromXml(QDomElement &document, delete nvel_elmt; qDebug() << QStringLiteral("Diagram::fromXml() : Le chargement des parametres d'un element a echoue"); } else { + //A symbol saved without a uuid got a random one from + //Element::fromXml(): a different identity on every load, + //written out on the next save. Derive it instead from what + //the symbol is and where it sits on its folio -- never from + //the folio's index, so inserting or moving a folio does not + //change it. Only for a folio being loaded: a paste renews + //uuids anyway. + if (consider_informations && m_project + && QUuid(element_xml.attribute(QStringLiteral("uuid"))).isNull()) { + nvel_elmt->setUuid(m_project->derivedUuid( + QStringLiteral("element"), + QStringList{type_id, + element_xml.attribute(QStringLiteral("x")), + element_xml.attribute(QStringLiteral("y")), + element_xml.attribute(QStringLiteral("orientation"))} + .join(QLatin1Char('\n')))); + } ItemGroups::setGroup(nvel_elmt, ItemGroups::read(element_xml)); added_elements << nvel_elmt; } diff --git a/sources/qetgraphicsitem/element.h b/sources/qetgraphicsitem/element.h index 9bb02202d..33d8b752e 100644 --- a/sources/qetgraphicsitem/element.h +++ b/sources/qetgraphicsitem/element.h @@ -245,6 +245,7 @@ class Element : public QetGraphicsItem QString linkTypeToString() const; void newUuid() {m_uuid = QUuid::createUuid();} //create new uuid for this element + void setUuid(const QUuid &uuid) {m_uuid = uuid;} protected: void drawAxes(QPainter *, const QStyleOptionGraphicsItem *); diff --git a/sources/qetproject.cpp b/sources/qetproject.cpp index df45c2c08..e66714577 100644 --- a/sources/qetproject.cpp +++ b/sources/qetproject.cpp @@ -275,6 +275,27 @@ QUuid QETProject::uuid() const return m_uuid; } +/** + @brief QETProject::derivedUuid + A uuid for an item of this project that was saved without one, the same + on every load of the same file. + @p key describes the item by what it is, never by its place in the file + or its folio's index: inserting or moving a folio must not change it. + Items with the same @p kind and @p key anywhere in the project (a copied + folio, two identical symbols stacked on one spot) are told apart by a + counter, in load order among those items alone. + @return a UUID v5, which cannot collide with the v4 uuids given to new + items +*/ +QUuid QETProject::derivedUuid(const QString &kind, const QString &key) +{ + static const QUuid derived_ns(QStringLiteral("{7d1e9c3a-5b2f-4e8a-9c61-2f4b8d0e6a17}")); + const QString full = kind + QLatin1Char('\n') + key; + const int n = m_derived_uuid_keys[full]++; + return QUuid::createUuidV5(derived_ns, + n ? full + QLatin1Char('\n') + QString::number(n) : full); +} + /** @brief QETProject::init */ diff --git a/sources/qetproject.h b/sources/qetproject.h index e05f3b216..991b9f1a9 100644 --- a/sources/qetproject.h +++ b/sources/qetproject.h @@ -107,6 +107,7 @@ class QETProject : public QObject ProjectPropertiesHandler& projectPropertiesHandler(); projectDataBase *dataBase(); QUuid uuid() const; + QUuid derivedUuid(const QString &kind, const QString &key); ProjectState state() const; QList diagrams() const; int folioIndex(const Diagram *) const; @@ -365,6 +366,7 @@ class QETProject : public QObject QFuture m_backup_future; KAutoSaveFile m_backup_file; QUuid m_uuid = QUuid::createUuid(); + QHash m_derived_uuid_keys; projectDataBase m_data_base; QVector m_terminal_strip_vector; diff --git a/tests/qttest/CMakeLists.txt b/tests/qttest/CMakeLists.txt index 94283cc09..52d156547 100644 --- a/tests/qttest/CMakeLists.txt +++ b/tests/qttest/CMakeLists.txt @@ -328,3 +328,15 @@ 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=\"$\"") + +# A symbol saved without a uuid gets the same one on every load, and +# inserting a folio does not change it. Runs the real binary's --resave on +# fixtures/qet_bug_repro_resaved.qet with the symbols' uuids stripped. +add_executable( + tst_derivedsymboluuid + tst_derivedsymboluuid.cpp) +add_test(NAME tst_derivedsymboluuid COMMAND tst_derivedsymboluuid) +add_dependencies(tst_derivedsymboluuid qelectrotech) +target_link_libraries(tst_derivedsymboluuid PRIVATE Qt::Test Qt::Xml) +target_compile_definitions(tst_derivedsymboluuid PRIVATE + "QET_TEST_BINARY_PATH=\"$\"") diff --git a/tests/qttest/tst_derivedsymboluuid.cpp b/tests/qttest/tst_derivedsymboluuid.cpp new file mode 100644 index 000000000..d3fa5861a --- /dev/null +++ b/tests/qttest/tst_derivedsymboluuid.cpp @@ -0,0 +1,184 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +#include + +#include +#include +#include +#include +#include +#include +#include +#include + +// A symbol saved without a uuid must get the same one on every load of the +// same file, and inserting a folio in front of it must not change it. Until +// this was fixed it got a random uuid each time (Element::fromXml), which +// the next save wrote out. +// +// Runs the real binary (--resave) on the fixture with its symbols' uuids +// stripped, and reads the uuids back from the saved file. +namespace { + +QDomDocument load(const QString &path) +{ + QDomDocument doc; + QFile file(path); + if (file.open(QIODevice::ReadOnly)) + doc.setContent(&file); + return doc; +} + +QList symbols(const QDomElement &diagram) +{ + QList out; + const QDomNodeList nodes = diagram.firstChildElement(QStringLiteral("elements")) + .elementsByTagName(QStringLiteral("element")); + for (int i = 0; i < nodes.size(); ++i) { + const QDomElement e = nodes.at(i).toElement(); + if (e.parentNode().parentNode() == diagram) + out << e; + } + return out; +} + +QList diagrams(const QDomDocument &doc) +{ + QList out; + for (QDomElement d = doc.documentElement().firstChildElement(QStringLiteral("diagram")); + !d.isNull(); d = d.nextSiblingElement(QStringLiteral("diagram"))) + out << d; + return out; +} + +// Symbols without uuids, and no wires (they refer to symbols by uuid). +void stripUuids(QDomDocument &doc) +{ + for (QDomElement d : diagrams(doc)) { + for (QDomElement e : symbols(d)) + e.removeAttribute(QStringLiteral("uuid")); + d.removeChild(d.firstChildElement(QStringLiteral("conductors"))); + } +} + +// type|x|y|orientation -> uuid, for every symbol in the file +QMultiHash symbolUuids(const QDomDocument &doc) +{ + QMultiHash out; + for (const QDomElement &d : diagrams(doc)) + for (const QDomElement &e : symbols(d)) + out.insert(QStringList{e.attribute(QStringLiteral("type")), + e.attribute(QStringLiteral("x")), + e.attribute(QStringLiteral("y")), + e.attribute(QStringLiteral("orientation"))} + .join(QLatin1Char('|')), + e.attribute(QStringLiteral("uuid"))); + return out; +} + +} // namespace + +class tst_derivedsymboluuid : public QObject +{ + Q_OBJECT + + QTemporaryDir m_dir; + int m_run = 0; + + // Save @p doc, run --resave on it in a sandbox of its own (so a running + // QElectroTech cannot answer instead), and return what was saved. + QDomDocument resave(const QDomDocument &doc) + { + const QString in = m_dir.filePath(QStringLiteral("in%1.qet").arg(m_run)); + 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); + QFile f(in); + if (!f.open(QIODevice::WriteOnly)) return {}; + f.write(doc.toByteArray()); + f.close(); + + 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(60000) || proc.exitCode() != 0) return {}; + return load(out); + } + + QDomDocument fixture() + { + QDomDocument doc = load(QFINDTESTDATA("fixtures/qet_bug_repro_resaved.qet")); + stripUuids(doc); + return doc; + } + +private slots: + void initTestCase() + { + QVERIFY(m_dir.isValid()); + QVERIFY(QFile::exists(QStringLiteral(QET_TEST_BINARY_PATH))); + QVERIFY(!fixture().isNull()); + } + + void sameUuidsOnEveryLoad() + { + const QMultiHash a = symbolUuids(resave(fixture())); + const QMultiHash b = symbolUuids(resave(fixture())); + QCOMPARE(a.size(), 9); // the fixture's placed symbols + QCOMPARE(a, b); + for (const QString &u : a) + QVERIFY2(!QUuid(u).isNull(), qPrintable(u)); + QCOMPARE(QSet(a.begin(), a.end()).size(), a.size()); + } + + void insertingAFolioChangesNothing() + { + QDomDocument moved = fixture(); + QDomElement first = diagrams(moved).first(); + QDomElement blank = first.cloneNode(false).toElement(); + blank.setAttribute(QStringLiteral("title"), QStringLiteral("new")); + blank.appendChild(moved.createElement(QStringLiteral("elements"))); + moved.documentElement().insertBefore(blank, first); + + const QMultiHash after = symbolUuids(resave(moved)); + QCOMPARE(after.size(), 9); + QCOMPARE(after, symbolUuids(resave(fixture()))); + } + + void savedUuidsAreKept() + { + QDomDocument doc = load(QFINDTESTDATA("fixtures/qet_bug_repro_resaved.qet")); + QCOMPARE(symbolUuids(resave(doc)), symbolUuids(doc)); + } + + void stackedIdenticalSymbolsDiffer() + { + QDomDocument doc = fixture(); + QDomElement one = symbols(diagrams(doc).first()).first(); + QDomElement copy = one.cloneNode(true).toElement(); + //A copy's terminals carry their own file ids; a symbol whose + //terminal ids are already taken is not loaded at all. + const QDomNodeList terminals = copy.elementsByTagName(QStringLiteral("terminal")); + for (int i = 0; i < terminals.size(); ++i) + terminals.at(i).toElement().setAttribute(QStringLiteral("id"), 90000 + i); + one.parentNode().appendChild(copy); + + const QStringList both = symbolUuids(resave(doc)).values( + QStringList{one.attribute(QStringLiteral("type")), + one.attribute(QStringLiteral("x")), + one.attribute(QStringLiteral("y")), + one.attribute(QStringLiteral("orientation"))} + .join(QLatin1Char('|'))); + QCOMPARE(both.size(), 2); + QVERIFY(both.at(0) != both.at(1)); + } +}; + +QTEST_APPLESS_MAIN(tst_derivedsymboluuid) + +#include "tst_derivedsymboluuid.moc" From e12410e439b42c914a0ce14fdb1a87596dbf72c1 Mon Sep 17 00:00:00 2001 From: ispyisail Date: Mon, 28 Sep 2026 20:46:01 +1300 Subject: [PATCH 2/3] Never derive a uuid the file already carries A symbol keeps its derived uuid once saved, even when it is moved. A symbol saved without a uuid that later turns up on the spot it left -- a hand edit, an older version, another tool writing the file -- derived the same uuid, and the project had two symbols with one identity. readDiagramsXml() now collects every symbol and wire uuid the file carries, on any folio, before a folio loads; derivedItemUuid() moves to the next counter value while a candidate is among them. The result still depends on the file alone. Renamed from derivedUuid(), which QETProject already has for the project's own uuid. tst_derivedsymboluuid: newcomerOnAMovedSymbolsSpotGetsAnotherUuid fails with the check switched off. Co-Authored-By: Claude Opus 5.5 --- sources/diagram.cpp | 2 +- sources/qetproject.cpp | 37 ++++++++++++++++++++++---- sources/qetproject.h | 4 ++- tests/qttest/tst_derivedsymboluuid.cpp | 22 +++++++++++++++ 4 files changed, 58 insertions(+), 7 deletions(-) diff --git a/sources/diagram.cpp b/sources/diagram.cpp index bc0cd7c9e..ee3b6540b 100644 --- a/sources/diagram.cpp +++ b/sources/diagram.cpp @@ -1685,7 +1685,7 @@ bool Diagram::fromXml(QDomElement &document, //uuids anyway. if (consider_informations && m_project && QUuid(element_xml.attribute(QStringLiteral("uuid"))).isNull()) { - nvel_elmt->setUuid(m_project->derivedUuid( + nvel_elmt->setUuid(m_project->derivedItemUuid( QStringLiteral("element"), QStringList{type_id, element_xml.attribute(QStringLiteral("x")), diff --git a/sources/qetproject.cpp b/sources/qetproject.cpp index e66714577..b89acc626 100644 --- a/sources/qetproject.cpp +++ b/sources/qetproject.cpp @@ -276,7 +276,7 @@ QUuid QETProject::uuid() const } /** - @brief QETProject::derivedUuid + @brief QETProject::derivedItemUuid A uuid for an item of this project that was saved without one, the same on every load of the same file. @p key describes the item by what it is, never by its place in the file @@ -284,16 +284,31 @@ QUuid QETProject::uuid() const Items with the same @p kind and @p key anywhere in the project (a copied folio, two identical symbols stacked on one spot) are told apart by a counter, in load order among those items alone. + + A derived uuid is never one the file already carries: an item saved with + a derived uuid and then moved or re-connected keeps it, so a newcomer + later taking its old place or ends would otherwise derive the same one. + The file's saved uuids are collected before any folio loads + (readDiagramsXml()), so the result still depends on the file alone. + + uuids are unique within one project; copies of a project share them, as + they share every saved uuid. Anything bringing items in from another + project must renew them, as paste does. @return a UUID v5, which cannot collide with the v4 uuids given to new items */ -QUuid QETProject::derivedUuid(const QString &kind, const QString &key) +QUuid QETProject::derivedItemUuid(const QString &kind, const QString &key) { static const QUuid derived_ns(QStringLiteral("{7d1e9c3a-5b2f-4e8a-9c61-2f4b8d0e6a17}")); const QString full = kind + QLatin1Char('\n') + key; - const int n = m_derived_uuid_keys[full]++; - return QUuid::createUuidV5(derived_ns, - n ? full + QLatin1Char('\n') + QString::number(n) : full); + int &n = m_derived_uuid_keys[full]; + QUuid uuid; + do { + uuid = QUuid::createUuidV5(derived_ns, + n ? full + QLatin1Char('\n') + QString::number(n) : full); + ++n; + } while (m_saved_item_uuids.contains(uuid)); + return uuid; } /** @@ -1909,6 +1924,18 @@ void QETProject::readDiagramsXml(QDomDocument &xml_project) //Search the diagrams in the project QDomNodeList diagram_nodes = xml_project.elementsByTagName(QStringLiteral("diagram")); + //Every symbol and wire uuid the file already carries, on any folio, + //before a folio derives one for an item saved without: see + //derivedItemUuid(). + for (const QString &tag : {QStringLiteral("element"), QStringLiteral("conductor")}) { + const QDomNodeList nodes = xml_project.elementsByTagName(tag); + for (int i = 0; i < nodes.size(); ++i) { + const QUuid saved(nodes.at(i).toElement().attribute(QStringLiteral("uuid"))); + if (!saved.isNull()) + m_saved_item_uuids.insert(saved); + } + } + if(dlgWaiting) dlgWaiting->setProgressBarRange(0, diagram_nodes.length()*3); diff --git a/sources/qetproject.h b/sources/qetproject.h index 991b9f1a9..35830912c 100644 --- a/sources/qetproject.h +++ b/sources/qetproject.h @@ -36,6 +36,7 @@ #endif #include +#include #include class Diagram; @@ -107,7 +108,7 @@ class QETProject : public QObject ProjectPropertiesHandler& projectPropertiesHandler(); projectDataBase *dataBase(); QUuid uuid() const; - QUuid derivedUuid(const QString &kind, const QString &key); + QUuid derivedItemUuid(const QString &kind, const QString &key); ProjectState state() const; QList diagrams() const; int folioIndex(const Diagram *) const; @@ -367,6 +368,7 @@ class QETProject : public QObject KAutoSaveFile m_backup_file; QUuid m_uuid = QUuid::createUuid(); QHash m_derived_uuid_keys; + QSet m_saved_item_uuids; //symbol and wire uuids the file carries, see derivedItemUuid() projectDataBase m_data_base; QVector m_terminal_strip_vector; diff --git a/tests/qttest/tst_derivedsymboluuid.cpp b/tests/qttest/tst_derivedsymboluuid.cpp index d3fa5861a..e5a0e67f8 100644 --- a/tests/qttest/tst_derivedsymboluuid.cpp +++ b/tests/qttest/tst_derivedsymboluuid.cpp @@ -156,6 +156,28 @@ private slots: QCOMPARE(symbolUuids(resave(doc)), symbolUuids(doc)); } + // A symbol keeps its derived uuid once saved, even when moved. A symbol + // saved without a uuid that later turns up on the spot it left (a hand + // edit, an older version, another tool) would derive the same uuid: it + // must get another one instead. + void newcomerOnAMovedSymbolsSpotGetsAnotherUuid() + { + QDomDocument doc = resave(fixture()); + QDomElement moved = symbols(diagrams(doc).first()).first(); + QVERIFY(!QUuid(moved.attribute(QStringLiteral("uuid"))).isNull()); + QDomElement newcomer = moved.cloneNode(true).toElement(); + newcomer.removeAttribute(QStringLiteral("uuid")); + const QDomNodeList terminals = newcomer.elementsByTagName(QStringLiteral("terminal")); + for (int i = 0; i < terminals.size(); ++i) + terminals.at(i).toElement().setAttribute(QStringLiteral("id"), 90000 + i); + moved.setAttribute(QStringLiteral("x"), moved.attribute(QStringLiteral("x")).toInt() + 500); + moved.parentNode().appendChild(newcomer); + + const QMultiHash after = symbolUuids(resave(doc)); + QCOMPARE(after.size(), 10); + QCOMPARE(QSet(after.begin(), after.end()).size(), 10); + } + void stackedIdenticalSymbolsDiffer() { QDomDocument doc = fixture(); From cdcff93190422388c58bfab284378721f19ac8dc Mon Sep 17 00:00:00 2001 From: Laurent Trinques Date: Mon, 28 Sep 2026 16:42:05 +0200 Subject: [PATCH 3/3] Update CMakeLists.txt --- tests/qttest/CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/qttest/CMakeLists.txt b/tests/qttest/CMakeLists.txt index b1613d522..ca296b6f8 100644 --- a/tests/qttest/CMakeLists.txt +++ b/tests/qttest/CMakeLists.txt @@ -343,6 +343,7 @@ target_compile_definitions(tst_derivedsymboluuid PRIVATE # 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)