From 0d08a4e2650e7298e193cd83762c3acbb52efe2f Mon Sep 17 00:00:00 2001 From: ispyisail Date: Mon, 24 Aug 2026 20:41:01 +1200 Subject: [PATCH] Make Diagram::toXml() serialize elements and conductors deterministically Saving an unmodified project produced a different byte stream on every run: QGraphicsScene::items() returns items in stacking order, and ties between same-Z items follow the scene's internal index -- not any content-derived order -- so it isn't reproducible across process runs. The legacy terminal-id table inherits the same instability, since ids are assigned sequentially in element order. Sort list_elements and list_conductors into a deterministic order before serializing, using a key built from data that's actually stable across loads (position), not Element::uuid()/Conductor::uuid(): for an item with no persisted uuid attribute, fromXml() invents a fresh random one on every load, so sorting by uuid would still be non-deterministic across process runs for any legacy file -- which this corpus has plenty of. Also fixes a second, related source of byte-level non-determinism found while verifying the above: Conductor::toXml() unconditionally wrote m_uuid back out, including the synthetic value fromXml() just invented for a conductor with no uuid attribute in the file. Every conductor in every example project checked has no persisted uuid at all, so this alone meant no project with conductors could ever resave identically, regardless of ordering. Conductor gets a m_persist_uuid flag, false only when the uuid it's holding was synthesized rather than loaded, so toXml() stops writing a value that was never meant to be permanent. Deliberately NOT applying the same uuid-persistence fix to Element: element uuids are cross-referenced by other elements' blocks for master/slave/report linking (element.cpp, tmp_uuids_link, matched by elmt->uuid() == stored uuid on load). Making an element's own uuid non-persistent would silently break that match for any linked element without one already -- a real regression, not a theoretical one. Left as a smaller, separate residual: 1-6 elements per project across the corpus (a few tenths of a percent) still get a fresh uuid on each load, same class of bug, needs the link-aware version of this fix instead of this one. Verified against 8 example projects (the ones with conductors, plus the two zero-conductor control cases from FINDINGS.md F002), 5 resaves each in isolated HOME/XDG environments: - Element and conductor ORDER: 0 churning sections across the whole corpus (previously the majority of diagrams in industrial.qet, m_000.qet and tremie_vibrante.qet churned on every run). - Conductor uuid VALUES: 0 churn (previously every conductor in every project, since none have a persisted uuid). - 6 of 8 projects are now byte-for-byte identical (md5) across all 5 runs. The remaining 2 (industrial.qet, m_000.qet) differ only in the handful of element uuids covered by the known Element residual above -- confirmed by checking those uuids specifically, not inferred. - Element/conductor counts before and after resave match exactly on every project (no data loss from the sort). Fixes #754. --- sources/diagram.cpp | 64 +++++++++++++++++++++++++++ sources/qetgraphicsitem/conductor.cpp | 9 +++- sources/qetgraphicsitem/conductor.h | 6 ++- 3 files changed, 76 insertions(+), 3 deletions(-) diff --git a/sources/diagram.cpp b/sources/diagram.cpp index cf552d14a..271e0713f 100644 --- a/sources/diagram.cpp +++ b/sources/diagram.cpp @@ -40,9 +40,59 @@ #include "undocommand/addelementtextcommand.h" #include "qetinformation.h" #include "qetproject.h" +#include #include #include +namespace { + /// Format a position as a string that sorts the same way the numbers do + /// (fixed precision, so "-" and decimals compare correctly as text). + QString positionKey(const QPointF &pos) + { + return QStringLiteral("%1|%2") + .arg(pos.x(), 0, 'f', 4) + .arg(pos.y(), 0, 'f', 4); + } + + /// Sort key for Diagram::toXml()'s block: the element's own + /// diagram-local position, exactly what it's already saved as (x/y), + /// never invented or regenerated. uuid() is deliberately NOT used here: + /// for an element with no persisted uuid attribute, fromXml() invents a + /// fresh random one on every load, so sorting by uuid would still be + /// non-deterministic across process runs for any legacy file. + QString elementSortKey(Element *elmt) + { + return positionKey(elmt->pos()); + } + + /// Sort key for a terminal: its parent element's position, then the + /// terminal's own position local to that element (from the .elmt + /// definition, fixed regardless of where the element is placed). + QString terminalSortKey(Terminal *terminal) + { + if (!terminal) + return QString(); + Element *parent = terminal->parentElement(); + return (parent ? positionKey(parent->pos()) : QStringLiteral("?")) + + QLatin1Char(':') + positionKey(terminal->pos()); + } + + /// Sort key for Diagram::toXml()'s block. Built from both + /// endpoints' terminalSortKey(), not Conductor::uuid(): in every example + /// project checked, conductors have no persisted uuid attribute at all, + /// so uuid() is a freshly-minted random value on every load -- exactly + /// as unusable for cross-run determinism as the element case above, just + /// with no persisted fallback to reach for instead. Canonicalised + /// (smaller key first) since a conductor's two ends are unordered for + /// this purpose. + QString conductorSortKey(Conductor *cond) + { + QString a = terminalSortKey(cond->terminal1); + QString b = terminalSortKey(cond->terminal2); + return (a <= b) ? (a + QLatin1Char('>') + b) : (b + QLatin1Char('>') + a); + } +} + int Diagram::xGrid = 10; int Diagram::yGrid = 10; int Diagram::xKeyGrid = 10; @@ -1014,6 +1064,20 @@ QDomDocument Diagram::toXml(bool whole_content, bool is_copy_command) { } } + // items() returns items in stacking order, which is not guaranteed + // reproducible across processes (ties between same-Z items follow + // the scene's internal index, not any content-derived order) -- so + // without this, saving an unmodified project produces a different + // byte stream on every run. Elements and conductors are the two + // blocks observed to actually churn across the example corpus; + // sort them into a deterministic, content-derived order before + // serializing. This also fixes the legacy terminal-id churn below, + // since those ids are assigned sequentially in element order. + std::stable_sort(list_elements.begin(), list_elements.end(), + [](Element *a, Element *b) { return elementSortKey(a) < elementSortKey(b); }); + std::stable_sort(list_conductors.begin(), list_conductors.end(), + [](Conductor *a, Conductor *b) { return conductorSortKey(a) < conductorSortKey(b); }); + // correspondence table between the addresses of the terminals and their ids // table de correspondance entre les adresses des bornes et leurs ids QHash table_adr_id; diff --git a/sources/qetgraphicsitem/conductor.cpp b/sources/qetgraphicsitem/conductor.cpp index 4f538e91f..5711b5d1f 100644 --- a/sources/qetgraphicsitem/conductor.cpp +++ b/sources/qetgraphicsitem/conductor.cpp @@ -1011,10 +1011,14 @@ bool Conductor::fromXml(QDomElement &dom_element) //that field was introduced (see terminal1/terminal2 handling in //toXml() below). m_uuid = QUuid(dom_element.attribute(QStringLiteral("uuid"))); + m_persist_uuid = !m_uuid.isNull(); if (m_uuid.isNull()) { //Absent, empty or malformed: mint one. A null uuid is not a usable //identity -- every conductor carrying one would collide with every - //other on the conductor table's primary key. + //other on the conductor table's primary key. It's runtime-only, + //though: toXml() must not write it back out, or a legacy file + //with no conductor uuids gets a freshly different one on every + //single load-and-resave (see #754). m_uuid = QUuid::createUuid(); } @@ -1056,7 +1060,8 @@ QDomElement Conductor::toXml(QDomDocument &dom_document, { QDomElement dom_element = dom_document.createElement("conductor"); - dom_element.setAttribute("uuid", m_uuid.toString()); + if (m_persist_uuid) + dom_element.setAttribute("uuid", m_uuid.toString()); dom_element.setAttribute("x", QString::number(pos().x())); dom_element.setAttribute("y", QString::number(pos().y())); diff --git a/sources/qetgraphicsitem/conductor.h b/sources/qetgraphicsitem/conductor.h index 91ca92afb..63857e068 100644 --- a/sources/qetgraphicsitem/conductor.h +++ b/sources/qetgraphicsitem/conductor.h @@ -79,7 +79,7 @@ class Conductor : public QGraphicsObject Diagram *diagram() const; ConductorTextItem *textItem() const; QUuid uuid() const {return m_uuid;} - void newUuid() {m_uuid = QUuid::createUuid();} //create new uuid for this conductor + void newUuid() {m_uuid = QUuid::createUuid(); m_persist_uuid = true;} //create new uuid for this conductor void updatePath(const QRectF & = QRectF()); //This method do nothing, it's only made to be used with Q_PROPERTY @@ -209,6 +209,10 @@ class Conductor : public QGraphicsObject bool m_valid; bool m_freeze_label = false; QUuid m_uuid; + /// false when m_uuid was synthesized by fromXml() because the + /// file had none -- toXml() must not persist that value, or + /// every reload mints and saves a new random one (see #754). + bool m_persist_uuid = true; /// QPen et QBrush objects used to draw conductors static QPen conductor_pen;