From b3a4e30010b26446ec71eb29aa22b920501cb207 Mon Sep 17 00:00:00 2001 From: ispyisail Date: Mon, 28 Sep 2026 20:57:02 +1300 Subject: [PATCH] Give wires saved without a uuid a lasting one, from what they connect A wire saved without a uuid got a random one on every load, never saved (#754): it had no identity from one session to the next, so a script could only name it as "the wire on terminal N of symbol X", and a comparison of two versions could not tell a moved wire from a new one. When a folio is loaded, such a wire now gets a UUID v5 derived from its two ends -- the symbol and terminal at each, sorted so the direction it was drawn in does not matter -- and it is written on save. Never its place in the file or its folio's index: inserting or moving a folio, or saving the wires in another order, does not change it. Once saved the uuid no longer depends on the ends, so re-connecting the wire keeps it; QETProject::derivedItemUuid() never hands out a uuid the file already carries, so a wire later drawn on the ends it left gets another one. Wires that have a uuid keep it; a paste still renews them. The 24 example projects: 3,189 wires, none with a uuid before, all 3,189 after one save, none lost, no uuid used twice in any project; two saves of the same file are identical, and a second save keeps every wire's uuid. Discussion #1103 has the measurements behind the recipe. tst_derivedwireuuid runs --resave on a fixture naming ends by uuid and on examples/tremie_vibrante.qet (ends by terminal number): every wire gets a distinct uuid, the same on every load, read back after a save, kept per wire when a folio is inserted, the wires are reordered or a wire is drawn the other way, and a newcomer on a re-connected wire's old ends gets another uuid. Without this change 14 of the 18 fail; with the uuid taken from folio index and file order instead, the folio-insert and reorder tests fail. Co-Authored-By: Claude Opus 5.5 --- sources/diagram.cpp | 22 ++ sources/qetgraphicsitem/conductor.h | 1 + tests/qttest/CMakeLists.txt | 14 ++ tests/qttest/tst_derivedwireuuid.cpp | 307 +++++++++++++++++++++++++++ 4 files changed, 344 insertions(+) create mode 100644 tests/qttest/tst_derivedwireuuid.cpp diff --git a/sources/diagram.cpp b/sources/diagram.cpp index ee3b6540b..237d0a5a8 100644 --- a/sources/diagram.cpp +++ b/sources/diagram.cpp @@ -1825,6 +1825,28 @@ bool Diagram::fromXml(QDomElement &document, { addItem(c); c -> fromXml(f); + //A wire saved without a uuid got a random one that was + //never saved (#754), so it had no identity from one + //session to the next. Derive it from what it connects: + //the symbol and terminal at each end, sorted so the + //direction it was drawn in does not matter. Never its + //place in the file or its folio's index, so inserting or + //moving a folio, or saving the wires in another order, + //does not change it. It is saved from now on, so + //re-connecting the wire later keeps it; derivedItemUuid() + //never hands out a uuid the file already carries, so a + //wire later drawn on the ends it left gets another one. + if (consider_informations && m_project + && QUuid(f.attribute(QStringLiteral("uuid"))).isNull()) { + auto end = [](const Terminal *t) { + return t->parentElement()->uuid().toString() + + QLatin1Char('/') + t->stableUuid().toString(); + }; + QStringList ends{end(p1), end(p2)}; + ends.sort(); + c->setUuid(m_project->derivedItemUuid(QStringLiteral("conductor"), + ends.join(QLatin1Char('\n')))); + } added_conductors << c; } else diff --git a/sources/qetgraphicsitem/conductor.h b/sources/qetgraphicsitem/conductor.h index d1ecd6e43..c2f3232e5 100644 --- a/sources/qetgraphicsitem/conductor.h +++ b/sources/qetgraphicsitem/conductor.h @@ -80,6 +80,7 @@ class Conductor : public QGraphicsObject ConductorTextItem *textItem() const; QUuid uuid() const {return m_uuid;} void newUuid() {m_uuid = QUuid::createUuid(); m_persist_uuid = true;} //create new uuid for this conductor + void setUuid(const QUuid &uuid) {m_uuid = uuid; m_persist_uuid = true;} //saved from now on void updatePath(const QRectF & = QRectF()); //This method do nothing, it's only made to be used with Q_PROPERTY diff --git a/tests/qttest/CMakeLists.txt b/tests/qttest/CMakeLists.txt index 52d156547..678fe151c 100644 --- a/tests/qttest/CMakeLists.txt +++ b/tests/qttest/CMakeLists.txt @@ -340,3 +340,17 @@ 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=\"$\"") + +# A wire saved without a uuid gets one derived from its two ends: the same +# on every load, saved, and unchanged by inserting a folio, reordering the +# wires or drawing a wire the other way. Runs the real binary's --resave on +# a fixture and on examples/tremie_vibrante.qet (ends by terminal number). +add_executable( + tst_derivedwireuuid + tst_derivedwireuuid.cpp) +add_test(NAME tst_derivedwireuuid COMMAND tst_derivedwireuuid) +add_dependencies(tst_derivedwireuuid qelectrotech) +target_link_libraries(tst_derivedwireuuid PRIVATE Qt::Test Qt::Xml) +target_compile_definitions(tst_derivedwireuuid PRIVATE + "QET_TEST_BINARY_PATH=\"$\"" + "QET_EXAMPLES_DIR=\"${QET_DIR}/examples\"") diff --git a/tests/qttest/tst_derivedwireuuid.cpp b/tests/qttest/tst_derivedwireuuid.cpp new file mode 100644 index 000000000..7bfc78b04 --- /dev/null +++ b/tests/qttest/tst_derivedwireuuid.cpp @@ -0,0 +1,307 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +// A wire saved without a uuid must get one that is the same on every load, +// is written on save, and survives the edits people make before that save: +// inserting a folio, saving the wires in another order, drawing the wire +// the other way round. Until this was fixed it got a random uuid on every +// load, never saved (#754). +// +// Runs the real binary (--resave) on two fixtures -- one whose wires name +// their ends by symbol and terminal uuid, one older file naming them by +// terminal number -- and reads the wires' 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 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; +} + +QList wires(const QDomElement &diagram) +{ + QList out; + const QDomElement block = diagram.firstChildElement(QStringLiteral("conductors")); + for (QDomElement c = block.firstChildElement(QStringLiteral("conductor")); + !c.isNull(); c = c.nextSiblingElement(QStringLiteral("conductor"))) + out << c; + return out; +} + +// Every wire's uuid in the file, sorted: equal lists mean every wire kept +// its uuid. +QStringList wireUuids(const QDomDocument &doc) +{ + QStringList out; + for (const QDomElement &d : diagrams(doc)) + for (const QDomElement &c : wires(d)) + out << c.attribute(QStringLiteral("uuid")); + out.sort(); + return out; +} + +// Which wire carries which uuid: each wire keyed by its two ends as the +// saved file names them, sorted. An older file names an end by terminal +// number, resolved here to the symbol and the terminal's place on it. +QMap wireMap(const QDomDocument &doc) +{ + QMap out; + for (const QDomElement &d : diagrams(doc)) { + QHash by_number; + const QDomNodeList symbols = d.firstChildElement(QStringLiteral("elements")) + .elementsByTagName(QStringLiteral("element")); + for (int i = 0; i < symbols.size(); ++i) { + const QDomElement e = symbols.at(i).toElement(); + const QDomNodeList ts = e.elementsByTagName(QStringLiteral("terminal")); + for (int j = 0; j < ts.size(); ++j) { + const QDomElement t = ts.at(j).toElement(); + by_number.insert(t.attribute(QStringLiteral("id")), + e.attribute(QStringLiteral("uuid")) + QLatin1Char('/') + + t.attribute(QStringLiteral("x")) + QLatin1Char(',') + + t.attribute(QStringLiteral("y"))); + } + } + for (const QDomElement &c : wires(d)) { + QStringList ends; + for (const QString n : {QStringLiteral("1"), QStringLiteral("2")}) { + const QString element = c.attribute(QStringLiteral("element") + n); + const QString terminal = c.attribute(QStringLiteral("terminal") + n); + ends << (element.isEmpty() ? by_number.value(terminal, QStringLiteral("?")) + : element + QLatin1Char('/') + terminal); + } + ends.sort(); + out.insert(ends.join(QLatin1Char('|')), c.attribute(QStringLiteral("uuid"))); + } + } + return out; +} + +} // namespace + +class tst_derivedwireuuid : 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(120000) || proc.exitCode() != 0) return {}; + return load(out); + } + + // The fixture as an older QElectroTech saved it: wires without uuids. + QDomDocument fixture() + { + QFETCH(QString, path); + QDomDocument doc = load(path); + for (const QDomElement &d : diagrams(doc)) + for (QDomElement c : wires(d)) + c.removeAttribute(QStringLiteral("uuid")); + return doc; + } + + // The saved uuids of the unedited fixture, checked for sanity. + QStringList reference() + { + QFETCH(int, count); + const QStringList ids = wireUuids(resave(fixture())); + if (ids.size() != count) return {}; + for (const QString &u : ids) + if (QUuid(u).isNull()) return {}; + return ids; + } + + void fixtures() + { + QTest::addColumn("path"); + QTest::addColumn("count"); + QTest::newRow("ends by uuid") + << QFINDTESTDATA("fixtures/qet_bug_repro_resaved.qet") << 7; + QTest::newRow("ends by terminal number (older file)") + << QStringLiteral(QET_EXAMPLES_DIR "/tremie_vibrante.qet") << 77; + } + +private slots: + void initTestCase() + { + QVERIFY(m_dir.isValid()); + QVERIFY(QFile::exists(QStringLiteral(QET_TEST_BINARY_PATH))); + } + + void savedAndUnique_data() { fixtures(); } + void savedAndUnique() + { + QFETCH(int, count); + const QStringList ids = reference(); + QCOMPARE(ids.size(), count); // every wire has one + QCOMPARE(QSet(ids.begin(), ids.end()).size(), count); // all different + QCOMPARE(wireMap(resave(fixture())).size(), count); // the test's wire key is unique too + } + + void sameOnEveryLoad_data() { fixtures(); } + void sameOnEveryLoad() + { + const QStringList a = reference(); + QVERIFY(!a.isEmpty()); + QCOMPARE(wireUuids(resave(fixture())), a); + } + + void savedUuidIsReadBack_data() { fixtures(); } + void savedUuidIsReadBack() + { + const QDomDocument once = resave(fixture()); + QVERIFY(!wireUuids(once).isEmpty()); + QCOMPARE(wireUuids(resave(once)), wireUuids(once)); + } + + void insertingAFolioChangesNothing_data() { fixtures(); } + void insertingAFolioChangesNothing() + { + QVERIFY(!reference().isEmpty()); + const QMap before = wireMap(resave(fixture())); + QDomDocument doc = fixture(); + const QDomElement first = diagrams(doc).first(); + QDomElement blank = first.cloneNode(false).toElement(); + blank.setAttribute(QStringLiteral("title"), QStringLiteral("new")); + blank.removeAttribute(QStringLiteral("uuid")); + blank.appendChild(doc.createElement(QStringLiteral("elements"))); + doc.documentElement().insertBefore(blank, first); + QCOMPARE(wireMap(resave(doc)), before); + } + + void wireOrderDoesNotMatter_data() { fixtures(); } + void wireOrderDoesNotMatter() + { + QVERIFY(!reference().isEmpty()); + const QMap before = wireMap(resave(fixture())); + QDomDocument doc = fixture(); + for (const QDomElement &d : diagrams(doc)) { + QDomElement block = d.firstChildElement(QStringLiteral("conductors")); + const QList ws = wires(d); + for (const QDomElement &w : ws) + block.removeChild(w); + for (auto it = ws.crbegin(); it != ws.crend(); ++it) // reversed + block.appendChild(*it); + } + QCOMPARE(wireMap(resave(doc)), before); + } + + void directionDoesNotMatter_data() { fixtures(); } + void directionDoesNotMatter() + { + QVERIFY(!reference().isEmpty()); + const QMap before = wireMap(resave(fixture())); + QDomDocument doc = fixture(); + static const QRegularExpression pair( + QStringLiteral("^(element|terminal|terminalname)([12])(.*)$")); + for (const QDomElement &d : diagrams(doc)) { + for (QDomElement w : wires(d)) { + const QDomNamedNodeMap attrs = w.attributes(); + QHash swapped; + for (int i = 0; i < attrs.size(); ++i) { + const QString name = attrs.item(i).nodeName(); + const auto m = pair.match(name); + if (m.hasMatch()) + swapped.insert(m.captured(1) + (m.captured(2) == QLatin1String("1") + ? QStringLiteral("2") : QStringLiteral("1")) + + m.captured(3), + attrs.item(i).nodeValue()); + } + for (auto it = swapped.cbegin(); it != swapped.cend(); ++it) + w.setAttribute(it.key(), it.value()); + } + } + QCOMPARE(wireMap(resave(doc)), before); + } + + // A wire keeps its uuid once saved, even when re-connected. A wire saved + // without a uuid that later turns up on the ends it left (a hand edit, an + // older version, another tool) would derive the same uuid: it must get + // another one instead. + void newcomerOnAReconnectedWiresEndsGetsAnotherUuid_data() { fixtures(); } + void newcomerOnAReconnectedWiresEndsGetsAnotherUuid() + { + QFETCH(int, count); + QDomDocument doc = resave(fixture()); + QVERIFY(!wireUuids(doc).isEmpty()); + const QList ws = wires(diagrams(doc).first()); + QVERIFY(ws.size() >= 2); + QDomElement x = ws.at(0); + const QDomElement y = ws.at(1); + QDomElement newcomer = x.cloneNode(true).toElement(); // x's old ends + newcomer.removeAttribute(QStringLiteral("uuid")); + // re-connect x's second end to y's second end, keeping x's uuid + static const QRegularExpression end2(QStringLiteral("^(element|terminal|terminalname)2")); + const QDomNamedNodeMap attrs = y.attributes(); + for (int i = 0; i < attrs.size(); ++i) + if (end2.match(attrs.item(i).nodeName()).hasMatch()) + x.setAttribute(attrs.item(i).nodeName(), attrs.item(i).nodeValue()); + x.parentNode().appendChild(newcomer); + + const QStringList after = wireUuids(resave(doc)); + QCOMPARE(after.size(), count + 1); + QCOMPARE(QSet(after.begin(), after.end()).size(), count + 1); + } + + // QElectroTech refuses a second wire between two terminals already joined + // (Terminal::canBeLinkedTo()), so two wires on one folio never share both + // ends. A file that has one anyway must still load to the same uuids. + void doubledWireIsDroppedAndOthersKeepTheirs_data() { fixtures(); } + void doubledWireIsDroppedAndOthersKeepTheirs() + { + QVERIFY(!reference().isEmpty()); + const QMap before = wireMap(resave(fixture())); + QDomDocument doc = fixture(); + QDomElement w = wires(diagrams(doc).first()).first(); + w.parentNode().appendChild(w.cloneNode(true)); + QCOMPARE(wireMap(resave(doc)), before); + } +}; + +QTEST_APPLESS_MAIN(tst_derivedwireuuid) + +#include "tst_derivedwireuuid.moc"