From e7cbb8f50e9472f7aca0bc55761230f03c15cbd6 Mon Sep 17 00:00:00 2001 From: Levi Jetzer Date: Fri, 7 Aug 2026 17:20:45 +0200 Subject: [PATCH 1/3] Reading prefixes from company collection Added a reading for company collection prefixes which are overwritten by user collection prefixes --- sources/autoNum/assignvariables.cpp | 137 +++++++++++----------------- 1 file changed, 54 insertions(+), 83 deletions(-) diff --git a/sources/autoNum/assignvariables.cpp b/sources/autoNum/assignvariables.cpp index 8e2d465ec..b39e1d361 100644 --- a/sources/autoNum/assignvariables.cpp +++ b/sources/autoNum/assignvariables.cpp @@ -703,6 +703,46 @@ namespace autonum return formula; } + /** + @brief prefixFromLabelFile + Look up a prefix for @a path in the qet_labels.xml at @a filepath. + @return the prefix, or a null QString if the file cannot be read + or holds no matching entry. + */ + static QString prefixFromLabelFile(const QString &filepath, const QString path[10], int i, int dirLevel) + { + QFile file(filepath); + if (!file.open(QFile::ReadOnly | QFile::Text)) + return QString(); + + QXmlStreamReader rxml; + rxml.setDevice(&file); + rxml.readNext(); + + while (!rxml.atEnd()) { + if (rxml.attributes().value("name").toString() == path[i]) { + rxml.readNext(); + i = i - 1; + + if (i == 0) { + for (int j = i ; j <= dirLevel ; ++j) { + + if (rxml.name().toString() == "prefix") { + return rxml.readElementText(); + } else { + while (rxml.readNextStartElement() && rxml.name().toString() != "prefix") { + rxml.skipCurrentElement(); + rxml.readNext(); + } + } + } + } + } + rxml.readNext(); + } + return QString(); + } + /** @brief elementPrefixForLocation @param location @@ -716,7 +756,6 @@ namespace autonum if (!location.isProject()) return QString(); - QXmlStreamReader rxml; QString path[10]; int i = -1; ElementsLocation current_location = location; @@ -739,91 +778,23 @@ namespace autonum dirLevel = 0; } - // Create Custom labels if qet_labels.xml exits in customElementsDir - if (current_location.fileName() != "10_electric"){ - QString custom_labels = "qet_labels.xml"; - QString customfilepath = QETApp::customElementsDir().append(custom_labels); - - QFile file(customfilepath); - file.isReadable(); - if (!file.open(QFile::ReadOnly | QFile::Text)) - return QString(); - rxml.setDevice(&file); - rxml.readNext(); - - while(!rxml.atEnd()) - { - if (rxml.attributes().value("name").toString() == path[i]) - { - rxml.readNext(); - i=i-1; - //reached element directory - if (i==0) - { - for (int j=i; j<= dirLevel; j = j +1) - { - //if there is a prefix available apply prefix - if(rxml.name().toString()=="prefix") - { - return rxml.readElementText(); - } - //if there isn't a prefix available, find parent prefix in parent folder - else - { - while (rxml.readNextStartElement() && rxml.name().toString()!="prefix") - { - rxml.skipCurrentElement(); - rxml.readNext(); - } - } - } - } - } - rxml.readNext(); - } + if (current_location.fileName() == "10_electric") { + return prefixFromLabelFile( + QETApp::commonElementsDir() + "10_electric/qet_labels.xml", + path, i, dirLevel); } - else - { - QString qet_labels = "10_electric/qet_labels.xml"; - QString filepath = QETApp::commonElementsDir().append(qet_labels); - QFile file(filepath); - file.isReadable(); - if (!file.open(QFile::ReadOnly | QFile::Text)) - return QString(); - - rxml.setDevice(&file); - rxml.readNext(); - while(!rxml.atEnd()) - { - if (rxml.attributes().value("name").toString() == path[i]) - { - rxml.readNext(); - i=i-1; - //reached element directory - if (i==0) - { - for (int j=i; j<= dirLevel; j = j +1) - { - //if there is a prefix available apply prefix - if(rxml.name().toString()=="prefix") - { - return rxml.readElementText(); - } - //if there isn't a prefix available, find parent prefix in parent folder - else - { - while (rxml.readNextStartElement() && rxml.name().toString()!="prefix") - { - rxml.skipCurrentElement(); - rxml.readNext(); - } - } - } - } + //Look in the custom collection first, then in the company + //collection, so a user override wins over the shared one. + const QStringList candidates = { + QETApp::customElementsDir() + "qet_labels.xml", + QETApp::companyElementsDir() + "qet_labels.xml" + }; + for (const QString &candidate : candidates) { + const QString prefix = prefixFromLabelFile(candidate, path, i, dirLevel); + if (!prefix.isNull()) { + return prefix; } - rxml.readNext(); - } } return QString(); } From 0b7197118a1ceaf1059b0c1b9e90f0aed382d864 Mon Sep 17 00:00:00 2001 From: ispyisail Date: Sat, 12 Sep 2026 06:22:26 +1200 Subject: [PATCH 2/3] Fix three follow-on defects in the prefix lookup this PR just refactored Requested by @scorpio810 in review: an empty 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. --- sources/autoNum/assignvariables.cpp | 88 +++++++++++++++++++++-------- 1 file changed, 63 insertions(+), 25 deletions(-) diff --git a/sources/autoNum/assignvariables.cpp b/sources/autoNum/assignvariables.cpp index b39e1d361..1a13a0047 100644 --- a/sources/autoNum/assignvariables.cpp +++ b/sources/autoNum/assignvariables.cpp @@ -24,6 +24,7 @@ #include "../qetgraphicsitem/element.h" #include "../qetxml.h" #include "../qetproject.h" +#include #include #include #include @@ -705,11 +706,12 @@ namespace autonum /** @brief prefixFromLabelFile - Look up a prefix for @a path in the qet_labels.xml at @a filepath. + Look up a prefix for @a path (path[i] outermost, path[1] the + deepest directory) in the qet_labels.xml at @a filepath. @return the prefix, or a null QString if the file cannot be read or holds no matching entry. */ - static QString prefixFromLabelFile(const QString &filepath, const QString path[10], int i, int dirLevel) + static QString prefixFromLabelFile(const QString &filepath, const QStringList &path, int i, int dirLevel) { QFile file(filepath); if (!file.open(QFile::ReadOnly | QFile::Text)) @@ -728,7 +730,16 @@ namespace autonum for (int j = i ; j <= dirLevel ; ++j) { if (rxml.name().toString() == "prefix") { - return rxml.readElementText(); + //An empty is a deliberate override + //(cancel a company prefix from the custom + //collection) and must stop the search here. + //readElementText() returns a null QString for + //an empty element, not an empty one, so a + //null-vs-empty check is needed to tell "found, + //empty" apart from "not found" -- the caller + //treats isNull() as "keep looking". + const QString text = rxml.readElementText(); + return text.isNull() ? QString("") : text; } else { while (rxml.readNextStartElement() && rxml.name().toString() != "prefix") { rxml.skipCurrentElement(); @@ -737,6 +748,19 @@ namespace autonum } } } + //The readNext() above already advanced past the token + //that matched path[i] (or, when i reached 0, past + //whatever the search for left current on). + //Falling through to the unconditional readNext() below + //as well would advance a *second* time per match, which + //only happens to land back on the right token because a + //pretty-printed file inserts a whitespace-only + //Characters token between adjacent elements for it to + //consume. A minified qet_labels.xml has no such token, + //so that second advance skips clean over the very + //element the next loop iteration needs to see, and the + //lookup silently finds nothing. Skip it here instead. + continue; } rxml.readNext(); } @@ -756,42 +780,56 @@ namespace autonum if (!location.isProject()) return QString(); - QString path[10]; - int i = -1; + //Directory names from the element up to (not including) the + //collection root, outermost last -- path[dirLevel] is the + //top-level category, path[1] the element's immediate parent + //directory, path[0] the element's own file name (never matched + //against a category: the search below stops descending once it + //has matched path[1], the deepest real directory). An + //unbounded QStringList rather than a fixed-size array, because + //a custom collection can nest deeper than the shipped one -- + //see bugtracker #671 item 3. + QStringList path; ElementsLocation current_location = location; - int dirLevel = -1; - - //Add location name to path array - while((current_location.parent() != current_location) && (current_location.parent().fileName() != "import")) + while ((current_location.parent() != current_location) + && (current_location.parent().fileName() != "import")) { - i++; - path[i]=current_location.fileName(); + path << current_location.fileName(); current_location = current_location.parent(); - dirLevel++; } - //User Element without folder treatment - if (i == -1) - { - i = 0; - path[i]=current_location.fileName(); + //User element without folder treatment + if (path.isEmpty()) { + path << current_location.fileName(); current_location = current_location.parent(); - dirLevel = 0; } + const int dirLevel = path.size() - 1; if (current_location.fileName() == "10_electric") { + //commonElementsDir() -- unlike customElementsDir(), which + //normalises this itself -- returns whatever path the user + //configured verbatim, with no guaranteed trailing + //separator. Concatenating a suffix onto it directly used + //to silently mangle the path (and so the prefix lookup) + //for any install relocated to a directory without a + //trailing slash; QDir::filePath() joins them correctly + //either way. return prefixFromLabelFile( - QETApp::commonElementsDir() + "10_electric/qet_labels.xml", - path, i, dirLevel); + QDir(QETApp::commonElementsDir()).filePath( + QStringLiteral("10_electric/qet_labels.xml")), + path, dirLevel, dirLevel); } //Look in the custom collection first, then in the company //collection, so a user override wins over the shared one. - const QStringList candidates = { - QETApp::customElementsDir() + "qet_labels.xml", - QETApp::companyElementsDir() + "qet_labels.xml" + const QStringList candidate_dirs = { + QETApp::customElementsDir(), + QETApp::companyElementsDir() }; - for (const QString &candidate : candidates) { - const QString prefix = prefixFromLabelFile(candidate, path, i, dirLevel); + for (const QString &dir : candidate_dirs) { + const QString candidate = + QDir(dir).filePath(QStringLiteral("qet_labels.xml")); + const QString prefix = + prefixFromLabelFile(candidate, path, dirLevel, dirLevel); if (!prefix.isNull()) { return prefix; } From 7c0e867226313b1dd7ee257662b5c95b85f7a25d Mon Sep 17 00:00:00 2001 From: ispyisail Date: Sat, 12 Sep 2026 06:32:07 +1200 Subject: [PATCH 3/3] Rewrite element-prefix lookup for nesting and multi-tree common collections 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 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 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--overrides-inheritance behaviour #686 added both carry over unchanged: a category's own child, even an empty one, always overrides whatever a shallower ancestor already provided; a category with no 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()//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. --- sources/autoNum/assignvariables.cpp | 178 ++++++++++++++++------------ 1 file changed, 105 insertions(+), 73 deletions(-) diff --git a/sources/autoNum/assignvariables.cpp b/sources/autoNum/assignvariables.cpp index 1a13a0047..c8059bbf2 100644 --- a/sources/autoNum/assignvariables.cpp +++ b/sources/autoNum/assignvariables.cpp @@ -25,6 +25,7 @@ #include "../qetxml.h" #include "../qetproject.h" #include +#include #include #include #include @@ -706,65 +707,70 @@ namespace autonum /** @brief prefixFromLabelFile - Look up a prefix for @a path (path[i] outermost, path[1] the - deepest directory) in the qet_labels.xml at @a filepath. - @return the prefix, or a null QString if the file cannot be read - or holds no matching entry. + Look up a prefix for @a path (path[dirLevel] outermost, path[1] the + deepest directory; path[0], the element's own file name, is never + matched) in the qet_labels.xml at @a filepath. + + Descends through nested \ elements matching + path[dirLevel], path[dirLevel-1], ..., path[1] in turn, considering + only *direct* children at each step -- unlike a flat token scan, + this cannot be fooled by a same-named category living elsewhere in + the document at the wrong nesting depth (bugtracker #671 item 5). + + At each matched level, that category's own \ child -- even + an empty one -- overrides whatever a shallower ancestor already + provided, so an explicit empty \ cancels inheritance + rather than silently falling back to it (the behaviour requested in + PR #686 review). A category with no \ child at all leaves + the inherited value untouched, which is how a directory with no + prefix of its own comes to inherit its parent's, as the file's own + header comment documents. + + @return the prefix that applies, or a null QString if the file + cannot be read, is not well-formed, or does not describe this + path at all (as opposed to describing it with no prefix + anywhere along it, which is a non-null empty string). */ - static QString prefixFromLabelFile(const QString &filepath, const QStringList &path, int i, int dirLevel) + static QString prefixFromLabelFile(const QString &filepath, const QStringList &path, int dirLevel) { QFile file(filepath); if (!file.open(QFile::ReadOnly | QFile::Text)) return QString(); - QXmlStreamReader rxml; - rxml.setDevice(&file); - rxml.readNext(); + QDomDocument document; + if (!document.setContent(&file)) + return QString(); - while (!rxml.atEnd()) { - if (rxml.attributes().value("name").toString() == path[i]) { - rxml.readNext(); - i = i - 1; + QDomElement node = document.documentElement(); + if (node.isNull()) + return QString(); - if (i == 0) { - for (int j = i ; j <= dirLevel ; ++j) { - - if (rxml.name().toString() == "prefix") { - //An empty is a deliberate override - //(cancel a company prefix from the custom - //collection) and must stop the search here. - //readElementText() returns a null QString for - //an empty element, not an empty one, so a - //null-vs-empty check is needed to tell "found, - //empty" apart from "not found" -- the caller - //treats isNull() as "keep looking". - const QString text = rxml.readElementText(); - return text.isNull() ? QString("") : text; - } else { - while (rxml.readNextStartElement() && rxml.name().toString() != "prefix") { - rxml.skipCurrentElement(); - rxml.readNext(); - } - } - } - } - //The readNext() above already advanced past the token - //that matched path[i] (or, when i reached 0, past - //whatever the search for left current on). - //Falling through to the unconditional readNext() below - //as well would advance a *second* time per match, which - //only happens to land back on the right token because a - //pretty-printed file inserts a whitespace-only - //Characters token between adjacent elements for it to - //consume. A minified qet_labels.xml has no such token, - //so that second advance skips clean over the very - //element the next loop iteration needs to see, and the - //lookup silently finds nothing. Skip it here instead. - continue; + QString prefix; + for (int i = dirLevel ; i >= 1 ; --i) { + QDomElement child = node.firstChildElement(QStringLiteral("category")); + while (!child.isNull() + && child.attribute(QStringLiteral("name")) != path[i]) { + child = child.nextSiblingElement(QStringLiteral("category")); + } + if (child.isNull()) + return QString(); + node = child; + + const QDomElement own = node.firstChildElement(QStringLiteral("prefix")); + if (!own.isNull()) { + //readElementText()'s null-vs-empty distinction that PR + //#686 needed for the old QXmlStreamReader-based lookup + //has a QDomElement equivalent: text() on an empty + //element can itself come back null depending on how the + //XML was written, so the same explicit fallback applies + //-- an empty QString here means "found, deliberately + //blank", not "not found". + prefix = own.text(); + if (prefix.isNull()) + prefix = QString(""); } - rxml.readNext(); } - return QString(); + return prefix; } /** @@ -784,11 +790,11 @@ namespace autonum //collection root, outermost last -- path[dirLevel] is the //top-level category, path[1] the element's immediate parent //directory, path[0] the element's own file name (never matched - //against a category: the search below stops descending once it - //has matched path[1], the deepest real directory). An - //unbounded QStringList rather than a fixed-size array, because - //a custom collection can nest deeper than the shipped one -- - //see bugtracker #671 item 3. + //against a category: the search stops descending once it has + //matched path[1], the deepest real directory). An unbounded + //QStringList rather than a fixed-size array, because a custom + //collection can nest deeper than the shipped one -- see + //bugtracker #671 item 3. QStringList path; ElementsLocation current_location = location; while ((current_location.parent() != current_location) @@ -803,24 +809,48 @@ namespace autonum current_location = current_location.parent(); } const int dirLevel = path.size() - 1; + //Name of the top-level tree the element's path was found + //under, e.g. "10_electric" -- or, for a custom/company + //collection not organised that way, whatever its top-level + //folder happens to be called. + const QString collection_root = current_location.fileName(); - if (current_location.fileName() == "10_electric") { - //commonElementsDir() -- unlike customElementsDir(), which - //normalises this itself -- returns whatever path the user - //configured verbatim, with no guaranteed trailing - //separator. Concatenating a suffix onto it directly used - //to silently mangle the path (and so the prefix lookup) - //for any install relocated to a directory without a - //trailing slash; QDir::filePath() joins them correctly - //either way. - return prefixFromLabelFile( - QDir(QETApp::commonElementsDir()).filePath( - QStringLiteral("10_electric/qet_labels.xml")), - path, dirLevel, dirLevel); + //Every top-level common-collection tree (10_electric, + //20_logic, 30_hydraulic, ...) may carry its own + //qet_labels.xml, with categories relative to that tree, the + //same way 10_electric/qet_labels.xml already does -- not just + //10_electric, which is all the hardcoded check this replaces + //used to allow (bugtracker #671 item 2). commonElementsDir() + //-- unlike customElementsDir(), which normalises this itself + //-- returns whatever path the user configured verbatim, with + //no guaranteed trailing separator; concatenating a suffix onto + //it directly used to silently mangle the path (and so the + //prefix lookup) for any install relocated to a directory + //without a trailing slash (#671 item 1). QDir::filePath() + //joins correctly either way. + { + const QString common_file = QDir(QETApp::commonElementsDir()) + .filePath(collection_root + QStringLiteral("/qet_labels.xml")); + const QString prefix = prefixFromLabelFile(common_file, path, dirLevel); + if (!prefix.isNull()) { + return prefix; + } } - //Look in the custom collection first, then in the company - //collection, so a user override wins over the shared one. + /* Which collection an element actually came from is not + * recoverable post-import (addElement() strips the protocol), + * so custom and company labels files are tried against two + * possible layouts: with the collection's top-level tree name + * folded into the path (a custom/company file organised as a + * mirror of the common collection, tree name included) and + * without it (a file scoped to just this one tree, matching + * how the common collection's own files are written). Custom + * is tried before company, so a user override wins over a + * shared one. + */ + QStringList path_from_root = path; + path_from_root << collection_root; + const QStringList candidate_dirs = { QETApp::customElementsDir(), QETApp::companyElementsDir() @@ -828,10 +858,12 @@ namespace autonum for (const QString &dir : candidate_dirs) { const QString candidate = QDir(dir).filePath(QStringLiteral("qet_labels.xml")); - const QString prefix = - prefixFromLabelFile(candidate, path, dirLevel, dirLevel); - if (!prefix.isNull()) { - return prefix; + for (const QStringList &segments : {path_from_root, path}) { + const QString prefix = prefixFromLabelFile( + candidate, segments, segments.size() - 1); + if (!prefix.isNull()) { + return prefix; + } } } return QString();