From 053ff0b6cfe58ab250ce438e55b6192a8b742291 Mon Sep 17 00:00:00 2001 From: ispyisail Date: Fri, 9 Oct 2026 22:30:26 +1300 Subject: [PATCH] Fix wires moving to another symbol after saving an older project (#1408) Symbols copied in old versions can share one uuid on a folio. Up to 0.100 that was harmless: a wire on a terminal without uuid was saved by terminal number. Since #1118 every terminal has a uuid, so every wire was saved by symbol uuid and terminal uuid, and on reopening findTerminal() took the first symbol carrying it: all the copies' wires landed on it. - Save: a wire end on a symbol whose uuid another symbol of the folio also carries is written by terminal number again. - Load: when a wire's uuid names several symbols, pickEnds() takes the pair whose distance matches the wire's saved path, then the symbols whose labels match the ones saved with the wire, then two terminals no wire joins yet; otherwise the first, as before. This repairs a file a build with #1118 saved once. tst_sharedsymboluuid gives the three lamps of Habitat-Schemas_developpes.qet one uuid: master loses a net after a save (8 of 9) and reopens the file it saved with 8; with this, 9 both times. The 25 examples save the same as before. Co-Authored-By: Claude Opus 5.5 --- sources/diagram.cpp | 148 +++- sources/qetgraphicsitem/conductor.cpp | 13 +- sources/qetgraphicsitem/conductor.h | 4 +- tests/qttest/CMakeLists.txt | 12 + .../fixtures/shared_symbol_uuid_0200.qet | 685 ++++++++++++++++++ tests/qttest/tst_sharedsymboluuid.cpp | 171 +++++ 6 files changed, 1002 insertions(+), 31 deletions(-) create mode 100644 tests/qttest/fixtures/shared_symbol_uuid_0200.qet create mode 100644 tests/qttest/tst_sharedsymboluuid.cpp diff --git a/sources/diagram.cpp b/sources/diagram.cpp index 7bde5c1f9..ab0c504f7 100644 --- a/sources/diagram.cpp +++ b/sources/diagram.cpp @@ -1315,10 +1315,19 @@ QDomDocument Diagram::toXml(bool whole_content, bool is_copy_command) { } if (!list_conductors.isEmpty()) { + //Symbols copied in old versions can share one uuid. A wire + //saved by that uuid would reopen on the first of them (#1408). + QSet seen_uuids, shared_uuids; + for (auto elmt : std::as_const(list_elements)) { + if (seen_uuids.contains(elmt->uuid())) + shared_uuids.insert(elmt->uuid()); + seen_uuids.insert(elmt->uuid()); + } auto dom_conductors = document.createElement(QStringLiteral("conductors")); for (auto cond : list_conductors) { dom_conductors.appendChild(cond->toXml(document, - table_adr_id)); + table_adr_id, + shared_uuids)); } dom_root.appendChild(dom_conductors); } @@ -1464,18 +1473,20 @@ bool Diagram::initFromXml(QDomElement &document, } /** - @brief findTerminal - Find terminal to which the conductor should be connected + @brief findTerminals + Find the terminals to which the conductor could be connected @param conductor_index 1 or 2 depending on which terminal is searched @param f Conductor xml element @param table_adr_id Hash table to all terminal id assignement (legacy) @param added_elements Elements found in the xml file - @return + @return the terminal, or one per symbol when several symbols of the + folio carry the uuid the wire names (old copies could share one, #1408); + empty if none is found */ -Terminal* findTerminal(int conductor_index, - QDomElement& f, - QHash& table_adr_id, - QList& added_elements) +QList findTerminals(int conductor_index, + QDomElement& f, + QHash& table_adr_id, + QList& added_elements) { assert(conductor_index == 1 || conductor_index == 2); @@ -1483,39 +1494,45 @@ Terminal* findTerminal(int conductor_index, QString element_index = QStringLiteral("element") + str_index; QString terminal_index = QStringLiteral("terminal") + str_index; + QList found; if (f.hasAttribute(element_index)) { QUuid element_uuid = QUuid(f.attribute(element_index)); // element1 did not exist in the conductor part of the xml until prior 0.7 // It is used as an indicator that uuid's are used to identify terminals bool element_found = false; + QUuid terminal_uuid = QUuid(f.attribute(terminal_index)); for (auto element: added_elements) { if (element->uuid() != element_uuid) continue; element_found = true; - QUuid terminal_uuid = QUuid(f.attribute(terminal_index)); + Terminal *match = nullptr; for (auto terminal: element->terminals()) { - if (terminal->uuid() != terminal_uuid) - continue; - - return terminal; + if (terminal->uuid() == terminal_uuid) { + match = terminal; + break; + } } //The uuid a project gave a terminal on opening is worked out //from where the terminal is in its symbol: if the symbol's //definition has since been replaced by one whose terminals //carry other uuids, the terminal at that place is still it. for (auto terminal: element->terminals()) { + if (match) + break; if (terminal->derivedUuid() == terminal_uuid) - return terminal; + match = terminal; } - qDebug() << "Diagram::fromXml() : " - << terminal_index - << ":" - << terminal_uuid - << "not found in " - << element_index - << ":" - << element_uuid; - break; + if (match) + found << match; + else + qDebug() << "Diagram::fromXml() : " + << terminal_index + << ":" + << terminal_uuid + << "not found in " + << element_index + << ":" + << element_uuid; } if (!element_found) qDebug() << "Diagram::fromXml() : " @@ -1532,9 +1549,83 @@ Terminal* findTerminal(int conductor_index, << id_p1 << " not found"; } else - return table_adr_id.value(id_p1); + found << table_adr_id.value(id_p1); } - return nullptr; + return found; +} + +/** + @brief pickEnds + Choose the two ends of a wire when its uuids name several symbols of + the folio (#1408). The pair whose distance matches the wire's saved + path wins, then the symbols whose labels match the ones saved with the + wire, then two terminals no wire joins yet; on a tie, the first symbols + in the file, as before. + @param f Conductor xml element + @param ends1 candidates for the first end, from findTerminals() + @param ends2 candidates for the second end + @return the two terminals, or nullptr where a list is empty +*/ +QPair pickEnds(const QDomElement &f, + const QList &ends1, + const QList &ends2) +{ + if ((ends1.size() <= 1 && ends2.size() <= 1) + || ends1.isEmpty() || ends2.isEmpty()) + return {ends1.value(0), ends2.value(0)}; + + //The saved path runs from the first end to the second + QPointF path_length; + bool has_path = false; + for (QDomElement segment = f.firstChildElement(QStringLiteral("segment")); + !segment.isNull(); + segment = segment.nextSiblingElement(QStringLiteral("segment"))) { + bool ok = false; + const qreal length = segment.attribute(QStringLiteral("length")).toDouble(&ok); + if (!ok || !qIsFinite(length)) + continue; + has_path = true; + if (segment.attribute(QStringLiteral("orientation")) == QLatin1String("horizontal")) + path_length.rx() += length; + else + path_length.ry() += length; + } + + auto label_matches = [&f](Terminal *terminal, const QString &index) { + const QString label = f.attribute(QStringLiteral("element") + index + + QStringLiteral("_label")); + return !label.isEmpty() + && terminal->parentElement()->actualLabel() == label; + }; + + QPair best; + int best_score = -1; + for (auto t1 : ends1) { + for (auto t2 : ends2) { + if (t1 == t2) + continue; + int score = 0; + if (has_path) { + const QPointF gap = t2->dockConductor() - t1->dockConductor(); + if (qAbs(gap.x() - path_length.x()) <= 1.0 + && qAbs(gap.y() - path_length.y()) <= 1.0) + score += 8; + } + if (label_matches(t1, QStringLiteral("1"))) + score += 2; + if (label_matches(t2, QStringLiteral("2"))) + score += 2; + if (!t1->isLinkedTo(t2)) + score += 1; + if (score > best_score) { + best_score = score; + best = {t1, t2}; + } + } + } + if (!best.first) + return {ends1.first(), ends2.first()}; + return best; } /** @@ -1879,8 +1970,11 @@ bool Diagram::fromXml(QDomElement &document, //Check if terminal that conductor must be linked is know - Terminal* p1 = findTerminal(1, f, table_adr_id, added_elements); - Terminal* p2 = findTerminal(2, f, table_adr_id, added_elements); + const auto ends = pickEnds(f, + findTerminals(1, f, table_adr_id, added_elements), + findTerminals(2, f, table_adr_id, added_elements)); + Terminal* p1 = ends.first; + Terminal* p2 = ends.second; //Keep a trace of the wire, it will be missing from the next save if ((!p1 || !p2) && consider_informations) diff --git a/sources/qetgraphicsitem/conductor.cpp b/sources/qetgraphicsitem/conductor.cpp index afae70051..66fe92dbb 100644 --- a/sources/qetgraphicsitem/conductor.cpp +++ b/sources/qetgraphicsitem/conductor.cpp @@ -1162,11 +1162,16 @@ bool Conductor::fromXml(QDomElement &dom_element) @param table_adr_id : Hash stockant les correspondances entre les ids des bornes dans le document XML et leur adresse en memoire + @param shared_uuids : uuids carried by more than one symbol of the + folio. An end on such a symbol is written by its terminal id, as for a + terminal without uuid: by uuid it would reopen on the first symbol + carrying it (#1408). @return Un element XML representant le conducteur */ QDomElement Conductor::toXml(QDomDocument &dom_document, QHash &table_adr_id) const + int> &table_adr_id, + const QSet &shared_uuids) const { QDomElement dom_element = dom_document.createElement("conductor"); @@ -1176,7 +1181,8 @@ QDomElement Conductor::toXml(QDomDocument &dom_document, dom_element.setAttribute("y", QString::number(pos().y())); // Terminal is uniquely identified by the uuid of the terminal and the element - if (terminal1->uuid().isNull()) { + if (terminal1->uuid().isNull() + || shared_uuids.contains(terminal1->parentElement()->uuid())) { // legacy method to identify the terminal dom_element.setAttribute("terminal1", table_adr_id.value(terminal1)); // for backward compatibility } else { @@ -1192,7 +1198,8 @@ QDomElement Conductor::toXml(QDomDocument &dom_document, dom_element.setAttribute("terminalname1", terminal1->name()); } - if (terminal2->uuid().isNull()) { + if (terminal2->uuid().isNull() + || shared_uuids.contains(terminal2->parentElement()->uuid())) { // legacy method to identify the terminal dom_element.setAttribute("terminal2", table_adr_id.value(terminal2)); // for backward compatibility } else { diff --git a/sources/qetgraphicsitem/conductor.h b/sources/qetgraphicsitem/conductor.h index 387fc9d20..34edfe436 100644 --- a/sources/qetgraphicsitem/conductor.h +++ b/sources/qetgraphicsitem/conductor.h @@ -21,6 +21,7 @@ #include "../conductorproperties.h" #include +#include #include class ConductorProfile; @@ -108,7 +109,8 @@ class Conductor : public QGraphicsObject QDomElement toXml ( QDomDocument &, QHash &) const; + int> &, + const QSet &shared_uuids = QSet()) const; private: bool pathFromXml(const QDomElement &); diff --git a/tests/qttest/CMakeLists.txt b/tests/qttest/CMakeLists.txt index b33b94130..de3223a1c 100644 --- a/tests/qttest/CMakeLists.txt +++ b/tests/qttest/CMakeLists.txt @@ -920,6 +920,18 @@ target_compile_definitions(tst_derivedwireuuid PRIVATE "QET_TEST_BINARY_PATH=\"$\"" "QET_EXAMPLES_DIR=\"${QET_DIR}/examples\"") +# Symbols sharing one uuid on a folio keep their wires through save and +# reopen (#1408): nets of examples/Habitat-Schemas_developpes.qet. +add_executable( + tst_sharedsymboluuid + tst_sharedsymboluuid.cpp) +add_test(NAME tst_sharedsymboluuid COMMAND tst_sharedsymboluuid) +add_dependencies(tst_sharedsymboluuid qelectrotech) +target_link_libraries(tst_sharedsymboluuid PRIVATE Qt::Test Qt::Xml) +target_compile_definitions(tst_sharedsymboluuid PRIVATE + "QET_TEST_BINARY_PATH=\"$\"" + "QET_EXAMPLES_DIR=\"${QET_DIR}/examples\"") + # Element numbering schemes carry an id and elements follow them by it: # a legacy file gets stable ids on --resave; through --run, rename, edit, # refused removal and undo/redo keep the elements following the scheme. diff --git a/tests/qttest/fixtures/shared_symbol_uuid_0200.qet b/tests/qttest/fixtures/shared_symbol_uuid_0200.qet new file mode 100644 index 000000000..652c2f0a9 --- /dev/null +++ b/tests/qttest/fixtures/shared_symbol_uuid_0200.qet @@ -0,0 +1,685 @@ + + + + 17/04/2021 + 17-04-2021 + 2021-04-17 + Habitat-Schemas_developpes + C:/Users/VD5385/Documents/Github/qelectrotech-source-mirror/qelectrotech-source-mirror/examples/Habitat-Schemas_developpes.qet + 14:45 + + + + + + + + + + + + + + + + + + + + + + + + + + + + L + + + + L + label + + + + + + + + + + + + + label + + + + + + + + + + + + 16A + + + + 16A + label + + + + + + + + + + + + 20A + + + + 20A + label + + + + + + + + + + + + S1 + + + + S1 + label + + + + + + + + + + + + S2 + + + + S2 + label + + + + + + + + + + + + + PC1 + + + + PC1 + label + + + + + + + + + + + + + L1 + + + + L1 + label + + + + + + + + + + + + + L2 + + + + L2 + label + + + + + + + + + + + + + L3 + + + + L3 + label + + + + + + + + + + + + 10A + + + + 10A + label + + + + + + + + + + + + 20A + + + + 20A + label + + + + + + + + + + + N + + + + N + label + + + + + + + + + + + + + label + + + + + + + + + + + PE + + + + PE + label + + + + + + + + + + + + + label + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Импортированные элементы + Imported elements + Elementi importati + Éléments importés + Elementy importowane + Elementos importados + Zavedené prvky + + + + مصادر + Quellen + Источники + Fontes + Sources + Sorgenti + Sources + Źródła zasilania + Fuentes + Zdroje + + + + متعددة الأسلاك + Mehradrig + Многолинейные + Multifilar + Multiline + Multifilari + Multifilaire + Schemat wielokreskowyowy + Multihilo + Několik vedení + + + + + Нейтраль + Protection Electrique + + Author: The QElectroTech team +License: see http://qelectrotech.org/wiki/doc/elements_license + + + + + + + + + + + مصدر وجه + Phase + Фаза + Fonte de fase + Phase source + Sorgente fase + Source phase + Przewód liniowy + Fuente fase + Fázový zdroj + + Author: The QElectroTech team +License: see http://qelectrotech.org/wiki/doc/elements_license + + + + + + + + + + + مصدر محايد + Neutralleiter + Нейтраль + Fonte de neutro + Neutral source + Sorgente neutro + Source neutre + Przewód neutralny + Fuente neutro + Nulový zdroj + + Author: The QElectroTech team +License: see http://qelectrotech.org/wiki/doc/elements_license + + + + + + + + + + + + حمايات + Sicherheit + Защитные уст-ва + Protecções + Protections + Protezioni + Protections + Zabezpieczenia + Protecciones + Ochrany + + + + قواطع + Lastschalter + Выключатели + Disjuntores + Circuit-breakers + Interruttori + Disjoncteurs + Wyłączniki + Disyuntores + Jističe + + + + + قاطع أحدي القطب + Circuit-breaker + Disjoncteur unipolaire + Wyłącznik + Jednopólový jistič + Int. Aut. Magneto-termico 1P + + Author: The QElectroTech team +License: see http://qelectrotech.org/wiki/doc/elements_license + + + + + + + + + + + + + + + + + + + + + + ملامسات + Kontakte + Контакты + Contactos + Contacts + Contatti + Contacts + Zestyki + Terminales + Kontakty + + + + مفاتيح + Schalter + переключателей + Interruptores + Switches + Interruttori + Interrupteurs + Przełącznik + Interruptores + Spínače + + + + + مفتاح + Schalter + Переключатель + Interruptor + Switch + Interruttore + Interrupteur + Łącznik + Interruptor + Spínač + + Author: The QElectroTech team +License: see http://qelectrotech.org/wiki/doc/elements_license + + + + + + + + + + + + + + + + Point Lumineux + + Author: The QElectroTech team +License: see http://qelectrotech.org/wiki/doc/elements_license + + + + + + + + + + + + + + + + + + قاطع + Lastschalter + Выключатель + Disjuntor + Circuit-breaker + Sezionatore 1P + Disjoncteur + Wyłącznik + Disyuntor + Jistič + + Author: The QElectroTech team +License: see http://qelectrotech.org/wiki/doc/elements_license + + + + + + + + + + + + + + + + Нейтраль + Protection Electrique + + Author: The QElectroTech team +License: see http://qelectrotech.org/wiki/doc/elements_license + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/tests/qttest/tst_sharedsymboluuid.cpp b/tests/qttest/tst_sharedsymboluuid.cpp new file mode 100644 index 000000000..85ba9bf95 --- /dev/null +++ b/tests/qttest/tst_sharedsymboluuid.cpp @@ -0,0 +1,171 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +#include + +#include +#include +#include +#include +#include +#include +#include +#include + +// Symbols copied in old versions can share one uuid on a folio. Since every +// terminal got a uuid (#1118), the wires on them were saved by symbol uuid +// and reopened on the first symbol carrying it (#1408). +// +// Runs the real binary on examples/Habitat-Schemas_developpes.qet, whose +// folio 1 has three lamps L1, L2, L3, and compares the nets QElectroTech +// exports with those of the unedited example: +// - the lamps given one uuid, saved twice; +// - fixtures/shared_symbol_uuid_0200.qet: that folio as a build with #1118 +// saved it, its wires naming the lamps by the shared uuid. +namespace { + +const QString example = QStringLiteral(QET_EXAMPLES_DIR "/Habitat-Schemas_developpes.qet"); + +QDomDocument load(const QString &path) +{ + QDomDocument doc; + QFile file(path); + if (file.open(QIODevice::ReadOnly)) + doc.setContent(&file); + return doc; +} + +QDomElement firstDiagram(const QDomDocument &doc) +{ + return doc.documentElement().firstChildElement(QStringLiteral("diagram")); +} + +QList childElements(const QDomElement &diagram, const QString &block, + const QString &tag) +{ + QList out; + for (QDomElement e = diagram.firstChildElement(block).firstChildElement(tag); + !e.isNull(); e = e.nextSiblingElement(tag)) + out << e; + return out; +} + +QList lamps(const QDomDocument &doc) +{ + QList out; + for (const QDomElement &e : childElements(firstDiagram(doc), QStringLiteral("elements"), + QStringLiteral("element"))) + if (e.attribute(QStringLiteral("type")).endsWith(QStringLiteral("lampe_pe.elmt"))) + out << e; + return out; +} + +} // namespace + +class tst_sharedsymboluuid : public QObject +{ + Q_OBJECT + + QTemporaryDir m_dir; + int m_run = 0; + + // Run the binary in a sandbox of its own, so a running QElectroTech + // cannot answer instead. + bool run(const QStringList &args) + { + 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), args); + return proc.waitForFinished(120000) && proc.exitCode() == 0; + } + + QString write(const QDomDocument &doc) + { + const QString path = m_dir.filePath(QStringLiteral("in%1.qet").arg(m_run)); + QFile f(path); + if (f.open(QIODevice::WriteOnly)) + f.write(doc.toByteArray()); + return path; + } + + QString resave(const QString &in) + { + const QString out = m_dir.filePath(QStringLiteral("out%1.qet").arg(m_run)); + return run({QStringLiteral("--resave"), in, out}) ? out : QString(); + } + + // Folio 1's nets, each as its sorted symbol labels, sorted: the order + // the export numbers them in follows the file and does not matter. + QStringList nets(const QString &project) + { + const QString out = m_dir.filePath(QStringLiteral("nets%1.json").arg(m_run)); + if (!run({QStringLiteral("--export-nets"), project, out})) + return {}; + QFile f(out); + if (!f.open(QIODevice::ReadOnly)) + return {}; + QStringList result; + const QJsonArray list = QJsonDocument::fromJson(f.readAll()) + .object().value(QStringLiteral("list")).toArray(); + for (const QJsonValue &net : list) { + QStringList ends; + for (const QJsonValue &t : net.toObject().value(QStringLiteral("terminals")).toArray()) { + const QJsonObject end = t.toObject(); + if (end.value(QStringLiteral("folio")).toInt() == 1) + ends << end.value(QStringLiteral("element")).toString(); + } + ends.sort(); + if (!ends.isEmpty()) + result << ends.join(QLatin1Char(',')); + } + result.sort(); + return result; + } + +private slots: + void initTestCase() + { + QVERIFY(m_dir.isValid()); + QVERIFY(QFile::exists(QStringLiteral(QET_TEST_BINARY_PATH))); + QCOMPARE(lamps(load(example)).size(), 3); + } + + void sharedUuidSurvivesSaves() + { + const QStringList expected = nets(example); + QVERIFY(expected.contains(QStringLiteral("L2,L3,S2"))); + + QDomDocument doc = load(example); + const QList l = lamps(doc); + const QString shared = l.first().attribute(QStringLiteral("uuid")); + for (QDomElement e : l) + e.setAttribute(QStringLiteral("uuid"), shared); + + const QString twice = resave(resave(write(doc))); + QVERIFY(!twice.isEmpty()); + QCOMPARE(nets(twice), expected); + + //No wire names a lamp by the uuid they share + for (const QDomElement &c : childElements(firstDiagram(load(twice)), + QStringLiteral("conductors"), + QStringLiteral("conductor"))) { + QVERIFY(c.attribute(QStringLiteral("element1")) != shared); + QVERIFY(c.attribute(QStringLiteral("element2")) != shared); + } + } + + void savedByUuidReopensRight() + { + const QString fixture = QFINDTESTDATA("fixtures/shared_symbol_uuid_0200.qet"); + QVERIFY(!fixture.isEmpty()); + QCOMPARE(nets(fixture), nets(example)); + } +}; + +QTEST_GUILESS_MAIN(tst_sharedsymboluuid) +#include "tst_sharedsymboluuid.moc"