Fourth and last instance of the ordering defect, and the inner half of the
one fixed in the previous commit. ProjectDBModel::toXml() builds each
section's role list from m_header_data.value(key).keys(), and m_header_data
is a QHash<int, QHash<int, QVariant>> -- so both levels are randomised per
process. Sorting the sections left the roles inside each section still
arriving shuffled, which showed up as <data> children with the same
section="0" swapping places between two saves.
With this, save idempotence across the shipped examples goes from 6 of 23 to
22 of 24.
The two that remain fail for unrelated reasons, not for ordering:
schema_indus.qet stores no uuid attribute on its elements at all, so
fromXml() invents a fresh one on every load; and affuteuse_250h.qet loses a
title block logo's storage attribute and renames a title block template on
save. Both are separate defects.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Third instance of the ordering defect the two previous commits fixed, and
the one that was still making five of the shipped examples save
irreproducibly after those: QETXML::modelHeaderDataToXml() iterates
data_hash.keys() directly, and data_hash is a QHash<int, QList<int>> whose
key order is randomised per process. The <data> children of <header_data>
therefore came out in a different order on every save, which is what a
diff of two saves of industrial.qet showed -- the same EditRole, FontRole
and TextAlignmentRole entries, shuffled.
Sorting the section list fixes it. The roles within a section are a QList
and were already written in a stable order, so they are left alone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Same defect as the <xref> ordering fixed in the previous commit, in the same
function and left behind by it: conductorAutoNum(), folioAutoNum() and
elementAutoNum() are QHash, whose key order is randomised per process, and
all three were iterated directly. A project holding more than one scheme in
any of the three categories therefore wrote those children in a different
order on every save, so opening and saving without an edit produced a file
that differed from the original, and differed again next time.
Three of the shipped examples are affected: Projet_vierge.qet has 8 conductor
schemes, industrial.qet has 4 element and 2 folio schemes, and
tableau_domestique.qet has 2 element schemes.
Sorting the key list is the same remedy already applied to the xrefs, and
changes nothing else: the same children are written, with the same contents.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Diagram::toXml() sorts elements by position alone. That is not a total
order: two elements can sit at the same x/y. lmdg.qet has a pair of
text elements both at 780,350, their sort keys are identical, and
std::stable_sort then falls back to the order QGraphicsScene handed us,
which varies between runs. The two swapped places on every save.
Appending the uuid gives a total order. This keeps the reasoning in the
existing comment intact rather than contradicting it: that comment warns
against sorting *by* uuid, because an element with no persisted uuid
attribute is given a fresh random one by fromXml() on every load. As a
tiebreaker the uuid is only consulted when two positions are equal, so
elements carrying a persisted uuid -- the colliding pair in lmdg.qet
included -- become deterministic, and a collision between two legacy
elements is no better ordered than before, but no worse.
Measured with tests/determinism, on top of the xref ordering fix:
before both fixes I1 0/23
xref ordering only I1 5/23
with this as well I1 6/23 (lmdg.qet newly reproducible)
No regressions against the baseline, I3 stays 23/23. Also checked
lmdg.qet directly three times rather than once, since the failure is
nondeterministic by nature and a single passing run proves nothing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
QETProject::toXml() iterated defaultXRefProperties().keys() straight into
the document. That is a QHash, and Qt randomises hash iteration order per
process, so every save wrote the <xref> children in a different sequence.
Saving an unchanged project therefore produced a different file each
time. The content was identical -- same size, same elements -- but the
order moved, so version control showed spurious changes on every save and
comparing two saved files showed differences that were not there.
Sorting the keys before writing makes a save reproducible. This is the
same class of problem, and the same fix, as the sort already applied to
Diagram::toXml()'s <elements> and <conductors> blocks.
Measured with tests/determinism (resave twice, compare):
before: I1 idempotent save 0/23
after: I1 idempotent save 5/23
with ArduinoLCD, ShellyParts, convertisseur, schema_indus and
schema_unifilaire_voltaique2 newly reproducible, and no regressions
against the baseline.
Not the only remaining source of save instability -- the other 18
projects still fail I1 for other reasons. This fixes the hash-ordering
source only.
Note this is not a Qt6 regression. The Qt5 build happened to produce a
favourable hash order for four projects and Qt6 does not, but both were
writing an unspecified order; only the dice changed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Making the Informations tab visible for Slave elements is only half the
change: ElementScene::toXml() writes the <elementInformations> block for
Simple, Master, Terminal and Thumbnail, and Slave was not in that list. It
is the only place in the tree that writes that block, so the editor would
have shown an editable tab for a slave, accepted whatever the user typed
into it, and dropped it silently on save.
Visible in the shipped collection, which matches the condition exactly:
0 of 75 slave elements carry an <elementInformations> block, against 41 of
70 terminal elements.
Adding Slave is safe in both directions. ElementData::fromXml() reads
<elementInformations> unconditionally, with no check on the base type, so
existing slave elements are unaffected and newly written ones load back
correctly. It also makes populateTree()'s PLC-slave branch reachable for
the first time -- the five PLC info rows it adds are stored in
m_informations, so until now they could not have been saved either.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
exportBomCsv() calls query.value(i).toString(). QSqlQuery::value() returns
a QVariant, but the translation unit only ever sees the forward declaration
that arrives through qobject.h, so the call does not compile:
sources/bomexport.cpp:79:50: error: invalid use of incomplete type
'class QVariant'
79 | values.append(query.value(i).toString());
Reproduced on a clean checkout of master with Qt 5.15.18. Qt6 pulls the
full definition in by another path, so the Windows CI workflow does not
see it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N64mk33R9GdbU1PkYcc9SP
When a placed nomenclature or summary table cannot display every row its
model holds, checkInsufficientRowsCount() informs the user with a modal
message box. It used QMessageBox directly rather than QET::QetMessageBox,
so it ignored the non-interactive mode that main.cpp sets for the
command-line verbs, and every headless verb (--info, --resave, --export-*)
blocked forever on a dialog nobody could answer.
This is the same defect fixed in e3d11a499 for the other modals reachable
from the command line; this call site was missed.
Found with a gdb backtrace on a hung --info: the process was parked in
QDialog::exec() under this function.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L6MRq2Ach1ogvnGcbuqNLr
ElementPictureFactory caches the QPicture it builds for an element
definition, keyed by that definition's uuid. Definitions saved before uuids
were written do not have one, and every one of them presented the same null
uuid. getPictures() spotted that and took an uncached path, so the drawing
was rebuilt from the XML for every instance the project placed.
Counted on the shipped examples:
examples/m_000.qet 831 builds for 97 definitions
examples/affuteuse_250h.qet 256 builds for 106 definitions
examples/industrial.qet 65 builds, 553 cache hits (has uuids)
13 of the 23 example projects carry definitions without a uuid, so this is
not a rare shape.
Derive a key from the location when the definition has no uuid of its own.
ElementsLocation::toString() qualifies an embedded path with the id of the
project owning it, and QETApp hands out project ids from an ever-increasing
counter and never reuses them, so the derived key cannot collide with an
element of another project.
Measured with callgrind, which counts instructions and so does not depend
on what else the machine is doing, opening examples/affuteuse_250h.qet:
4,801,381,735 -> 4,285,411,908 instructions (-10.7 %)
ElementPictureFactory::build 995 M -> 478 M
ElementPictureFactory::getPictures 1289 M -> 774 M
The halving of build() matches the counters independently: 106 definitions
against 256 instances is 41 %, and the cost falls to 48 %.
This also retires a latent aliasing bug rather than a measured one:
build() inserted into m_primitives_H under the same null uuid for every
definition lacking one, and getPrimitives() read back through that shared
key. Its only caller is the image export dialog, which the command line
does not reach, so no wrong output could be demonstrated here -- but the
entries could only ever have belonged to whichever element was built last.
--info stays byte identical on all 23 example projects, and the SVG export
of affuteuse_250h.qet -- a project whose definitions all lack uuids -- is
byte identical too.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L6MRq2Ach1ogvnGcbuqNLr
setPainterStyle() built its QRegularExpression as a local, so the pattern
was compiled from scratch on every call -- and it is called for every
graphics primitive of every element instance a project places. A callgrind
profile of opening examples/affuteuse_250h.qet put 28 % of all instructions
inside libpcre2, and 12 % of the whole run inside this one function.
Making it static const compiles the pattern once for the life of the
process. Nothing else changes: same pattern, same matching, same named
captures.
Measured with callgrind, which counts instructions and so does not depend
on what else the machine is doing, opening examples/affuteuse_250h.qet:
4,801,381,735 -> 4,297,629,948 instructions (-10.5 %)
setPainterStyle 582 M (12.13 %) -> 79 M (1.83 %)
--info stays byte identical on all 23 example projects, and so does every
SVG this produces for industrial.qet -- which is the output that would
change if the styles were parsed any differently.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N64mk33R9GdbU1PkYcc9SP
Destroying a project cost more than loading it: on a 1000 folio project
--info reported its work done in 160 s but the process ran for 657 s, and
the difference was ~QETProject().
Timing each destructor puts 94 % of that teardown in
~QetGraphicsTableItem(), with the cost per table doubling as the project
grows (48 ms at 100 folios, 122 ms at 250). The database's own per-element
deletes are 1 % of it and linear; element and conductor teardown is linear.
A table destructor repairs the chain it belonged to, which relinks the
neighbouring tables, which assigns a model -- and one branch of
setPreviousTable() builds a fresh ProjectDBModel, whose copy constructor
calls setQuery(), which rebuilds the whole database. Destroying a 250 folio
project did that 12 times, for a project that is being thrown away.
So block the rebuild for the lifetime of the destructor, next to the
blockSignals(true) already there for the same reason. Nothing can observe
the result: the database is destroyed moments later as a member of the
project. Teardown drops about fivefold at every size measured -- 1.30 s to
0.26 s at 100 folios, 7.75 s to 1.73 s at 250, 19.92 s to 4.02 s at 400 --
and the number of full rebuilds in a run stops growing with project size.
Teardown is still superlinear, now dominated by
QetGraphicsTableItem::setUpColumnAndRowMinimumSize() measuring every cell of
the nomenclature each time a chain is relinked. That is left alone here.
--info stays byte identical on all 23 example projects, as do --export-bom,
--export-wires, --export-cables, --export-nets and --export-wiring.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L6MRq2Ach1ogvnGcbuqNLr
ProjectDBModel::setQuery() calls projectDataBase::updateDB(), which drops
and repopulates every table in the database. The rebuild does not depend on
the query, so each table model that queries the database while a project is
being read triggers another complete repopulate of the same content.
Opening a 100 folio project ran updateDB() 26 times, 9.9 s of a 15.7 s load.
Two changes, because the first alone is not enough:
setUpdateBlocked() lets a bulk operation suppress the rebuild and do it
once when it is done. readProjectXml() already wrapped the load in
blockSignals(true) "to avoid hundreds of unnecessary emitted signal", but
that suppresses only the signal, not the work it announces; this extends
the same intent to the work. Both early returns in readProjectXml() sit
before the block, so no path leaves the database permanently blocked.
Further rebuilds are triggered after readProjectXml() returns, where the
load phase timers cannot see them -- with only the block in place
updateDB() still ran 5 times on examples/industrial.qet. So the database
now also tracks whether anything has changed since the last rebuild, and
skips repopulating when nothing has. dataBaseUpdated() is still emitted in
that case: callers and models rely on it to refresh, and what they read
back is the same either way. Every method of the class that writes rows
marks the flag; from outside, the database is reachable only through
newQuery(), and all five call sites read.
Repeating the rebuild was wasteful rather than wrong -- each
populate*Table() begins with a DELETE -- so this changes no output.
Verified byte identical --info on all 23 example projects, and identical
--export-bom, --export-wires, --export-cables, --export-nets and
--export-wiring on industrial.qet. Its load drops from 5.51 s to 5.31 s
(median of 6).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L6MRq2Ach1ogvnGcbuqNLr
The remaining two defects from bugtracker #671's original analysis,
which #686 knowingly didn't cover (see that PR's review thread and the
comment on the now-closed #672).
## #671 item 5: the XML matching ignored nesting
prefixFromLabelFile() was a flat token scan: it matched any <category
name="..."> whose name equalled the next path segment, with no check
that the match was actually a *child* of the previous match. It gave
correct results on the shipped 10_electric/qet_labels.xml only because
that file's document order happens to line up with its hierarchy --
any file with a same-named category at the wrong nesting depth would
silently return the wrong prefix.
Reproduced with a synthetic file where a top-level sibling category
happens to share a name with what should be an unmatched grandchild:
the old (already re-verified-fixed-for-whitespace) lookup returns a
prefix from a completely unrelated branch of the document; this
rewrite correctly reports "not found".
Fixed by replacing the QXmlStreamReader token walk with a QDomDocument
walk that only ever considers a matched node's direct <category>
children (firstChildElement()/nextSiblingElement(), scoped to that
node), which cannot cross into a same-named sibling subtree. This also
makes the whitespace-dependence fixed in #686 moot for the same
reason: DOM parsing doesn't distinguish pretty-printed from minified
input to begin with.
The inheritance rule ("if a directory has no prefix, use its parent's,
and so on") and the empty-<prefix/>-overrides-inheritance behaviour
#686 added both carry over unchanged: a category's own <prefix> child,
even an empty one, always overrides whatever a shallower ancestor
already provided; a category with no <prefix> child at all leaves the
inherited value untouched.
## #671 item 2: common-collection trees other than 10_electric
The lookup only ever consulted commonElementsDir()/10_electric --
literally: `if (current_location.fileName() == "10_electric")`. The
common collection ships four other top-level trees (20_logic,
30_hydraulic, 50_pneumatic, 60_energy); none of them could carry a
qet_labels.xml at all, because nothing ever looked for one.
Generalised to commonElementsDir()/<tree>/qet_labels.xml for whichever
top-level tree the element's path actually walks up to, tried first,
then custom, then company -- each of the latter two tried against both
a from-root layout (matching a custom/company file organised as a
mirror of the common collection, tree name included) and a
tree-relative one (matching a file scoped to just one tree), so
existing custom files keep working either way. This is the same
multi-candidate structure #686 already established for custom-then-
company; it now also covers which common-collection tree to check.
## Testing
Same constraint as #686: no working full build in this sandbox
(missing generated headers/deps), so the exact functions as committed
were extracted into a standalone Qt6 harness and run against the real
shipped 10_electric/qet_labels.xml (pretty-printed and minified),
a synthetic empty-prefix-override file, and the nesting-trap file
above -- 9/9, including the three cases #686 already fixed (direct
prefix, inherited prefix, not-found) staying correct, confirming this
rewrite doesn't regress that work.
Not exercised here (needs a real running QETApp / ElementsLocation,
which the standalone harness can't stand up): the elementPrefixForLocation()
candidate-list wiring itself -- collection_root computation, the
from-root/tree-relative dual lookup, and the common-then-custom-then-
company ordering. That code is mechanical and was reviewed carefully
by hand, but it has not been run.
Requested by @scorpio810 in review: an empty <prefix/> in the custom
collection should cancel a company-collection prefix, not fall through
to it. QXmlStreamReader::readElementText() returns a null QString for an
empty element, and the caller's isNull() check treats that the same as
"not found" -- distinguish the two so an explicit override actually
overrides. Verified in a standalone harness against a synthetic
override file, pretty-printed and minified.
Two more while in the same function, both from the original bugtracker
#671 analysis that this PR only partially addressed:
- QString path[10] with an unbounded index becomes a QStringList. The
deepest category in the shipped collection already needs 9 of the 10
slots; a custom collection can nest deeper, and overflow was writing
QString objects past the end of a stack array (#671 item 3).
- The common-collection lookup still concatenated
commonElementsDir() + "10_electric/qet_labels.xml" directly.
commonElementsDir() returns the configured path verbatim with no
guaranteed trailing separator, so relocating the collection to a path
without one silently mangles this into one word and the file is never
found -- the single most-reported cause of "prefixes don't work"
(#671 item 1, forum #2178/#2651). QDir::filePath() joins correctly
either way; applied to all three lookups (common, custom, company).
Also fixes a defect not in that original analysis: the token-matching
loop in prefixFromLabelFile() advanced twice per matched element --
once explicitly after a match, once more unconditionally at the bottom
of the loop -- which only produced the right result because a
pretty-printed file inserts a whitespace Characters token between
adjacent elements for the second advance to land on. A minified
qet_labels.xml has no such token, so the second advance skips clean
over the very element being searched for and the lookup silently finds
nothing -- reproduced against the real shipped 10_electric/qet_labels.xml
(returns "" instead of "K" for a plain coil, on every case tested, not
just the inheritance one). A single `continue` after a handled match
removes the double advance.
Testing: extracted the exact functions as committed into a standalone
Qt6 harness (outside the full QET build, which needs a dependency
fetch this sandbox doesn't have) and ran them against the real shipped
qet_labels.xml, pretty-printed and minified, covering a direct prefix,
inherited-from-ancestor prefix, not-found, and the explicit-empty-
override case -- 8/8, matching between formats, no regressions in the
pretty-printed results. The QDir::filePath() fix was verified
separately against both a trailing-slash and no-trailing-slash base
path. Not yet built inside the actual application (pugixml and other
generated headers aren't available standalone); the algorithm itself,
which is where all four defects lived, is what was under test.
A .qm compiled from a 0%-translated .ts (fi, no, rs, sk, sl, sr) loads
successfully but contains no messages, so setLanguage() treated the
language as loaded and never fell back to qet_en: users got the French
source strings instead of English. Treat an empty translator as not
loaded.
Also log the QET and Qt .qm files actually loaded in the startup
diagnostics (MachineInfo), to make translation reports easier to triage.
windeployqt runs with --no-translations, so standard buttons (OK/Cancel)
and dialogs stayed in English. Copy each qtbase_XX.qm from the MSYS2 Qt
translations into files/lang/qt_XX.qm, where QETApp::setLanguage() looks,
with aliases for QET languages Qt only ships with a region (pt, zh).
ispyisail