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' <links_uuids>
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.
This commit is contained in:
ispyisail
2026-08-24 20:41:01 +12:00
parent 26d7c03a76
commit 0d08a4e265
3 changed files with 76 additions and 3 deletions
+64
View File
@@ -40,9 +40,59 @@
#include "undocommand/addelementtextcommand.h"
#include "qetinformation.h"
#include "qetproject.h"
#include <algorithm>
#include <cassert>
#include <math.h>
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 <elements> 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 <conductors> 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<Terminal *, int> table_adr_id;
+7 -2
View File
@@ -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()));
+5 -1
View File
@@ -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;