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 cyclic part could only ever be rendered at its natural width, which is
fine for one of @scorpio810's two real layouts and wrong for the other:
April 5000/2000, 32-point cards %IX0.0 .. %IX0.31, then %IX1.0
Schneider M340, 64-point cards I1.00 .. I1.63, then I2.00
The first wants no padding, the second wants two digits. Since the two
conflict, the width cannot be derived from the modulus or from the part
type -- it has to be the user's to set.
Add a format field holding a run of zeros, the same convention a
spreadsheet uses for integer padding: "00" renders 7 as 07, "000" as 007.
The field's length is the minimum number of digits. It applies to every
numeric part type, not only cyclic ones, so "Chiffre 01" can be widened
past two digits without inventing another type for it.
An empty mask means the part type's own natural width, so it reproduces
exactly what every existing context does today -- Chiffre 1 stays 7,
Chiffre 01 stays 07, Chiffre 001 stays 007. That is what makes this safe
for existing projects: absent is the default, and absent changes nothing.
Stored as a sixth field on the context part and as an XML attribute
written only when set, following how modulus was added: readers guard on
size() and treat a short item as "no format". All seven places that
rebuild a part while incrementing it now carry the format through --
missing one would have silently dropped the padding on the second element
numbered.
The editor field is restricted to zeros by a validator, and is enabled
only for types that render as a number.
Measured:
April, mask empty %IX0.29 %IX0.30 %IX0.31 %IX1.0 %IX1.1
M340, mask "00" I1.00 I1.01 ... I1.62 I1.63 I2.00 I2.01
no mask unit 7,8,9 ten 07,08,09 hundred 007,008,009
ten with mask "0000" 0007 0008 0009
Reported by @scorpio810 on #632 with a screenshot: a "Chiffre 1" followed by
a "Cyclique (modulo) 8" numbers elements 0..7 and then jumps straight to 9,
never showing 8, and never producing the 0-7 / 10-17 / 20-27 pattern the
feature exists for.
The cause is that the wrap-and-carry feature shipped without its rendering
half. Commit 68c2603 added the arithmetic and the editor UI across seven
files, none of them assignvariables.*, so there is no %seqw_ variable, no
wrap list in sequentialNumbers, no branch in setSequential(), and no branch
in numerotationContextToFormula(). A cyclic part therefore contributes
nothing to the generated formula and cannot be referenced from one -- it is
invisible.
Invisible but not inert: it still advances and still carries. So the digit
in front of it receives +1 from the carry on top of its own increment, and
the only digit the label does show jumps by two. That is the missing 8.
Add the missing half:
- sequentialNumbers gains a wrap list, handled in the copy constructor,
assignment, comparison, clear(), toXml() and fromXml();
- setSequential() collects wrap parts when the label uses %seqw_;
- assignSequence() substitutes %seqw_N and counts wrap in its bound, so a
context whose only counter is cyclic still resolves;
- numerotationContextToFormula() emits %seqw_N, so adding a Cyclique part
in the editor now puts a token in the formula instead of nothing.
Old projects are unaffected: <wrap> is simply absent from files written
before this, which fromXml() reads as an empty list, and such files have no
cyclic parts to reference in the first place. An older QET reading a newer
file ignores the unknown child.
Measured on the exact configuration from the report, unit + wrap(mod 8):
formula generated %sequ_1%seqw_1 (was %sequ_1 -- wrap contributed none)
carry digit increment 1 00 11 22 33 44 55 66 77 90 101 112 ...
carry digit increment 0 00 01 02 03 04 05 06 07 10 11 ... 17 20 21
The second line is the requested pattern. The first shows what is left once
the rendering is fixed but the carry digit still increments itself as well
as receiving the carry -- worth a UI decision, noted on the PR.
Adds a real base-26 incrementing part type to the autonumbering engine,
alongside the 14 existing NumStrategy leaves. Unlike StringNum (a fixed,
non-incrementing text segment), AlphaNum::next()/previous() carry/borrow
entirely within the part's own value -- the composition loop in
NumerotationContextCommands doesn't need to change, since (unlike #578's
wrap-and-carry) nothing here needs to signal an adjacent part.
- incrementAlpha()/decrementAlpha() implement the spreadsheet-column-name
algorithm: increment carries right-to-left on 'z'/'Z' overflow,
prepending a new leading letter if the whole value overflows (z -> aa,
az -> ba). decrement is the exact inverse, including the symmetric
shrink case (aa -> z) once every position has borrowed. A single letter
already at "a"/"A" has no representable predecessor and is clamped
rather than turned into "z" -- caught via manual testing, since the
initial implementation mutated the string in the borrow loop before
checking whether to clamp, silently discarding the original value.
- Registered in NumerotationContext::validRegExpNum() but deliberately
not in validRegExpNumber(), so addValue() doesn't force alphabetic
values through int conversion.
- New "Cyclique"-adjacent "Alphabétique" entry in numparteditorw's type
dropdown, with its own letters-only QRegularExpressionValidator; the
increase spinbox is disabled since the step is always exactly one
letter, not a configurable amount.
Also wires the new part type through to actual element/conductor labels,
which turned out to be required for the feature to do anything visible
beyond folio numbering (which applies a NumerotationContext's
represented string directly). Element and conductor numbering instead
go through a separate formula-substitution layer
(autonum::sequentialNumbers + %sequ_/%seqt_/%seqh_-style placeholders in
AssignVariables::assignSequence()) that numerotationContextToFormula()
auto-populates. Without a matching placeholder, an "alpha" part would
silently vanish from the generated formula and never reach the label,
even though the underlying counter was advancing correctly:
- sequentialNumbers gained an `alpha` QStringList member (copy ctor,
operator=, operator==, toXml/fromXml, clear()).
- numerotationContextToFormula() emits a new %seqa_N placeholder for
alpha parts, the same way %sequ_N is emitted for unit parts.
- setSequential()/setSequentialToList() populate seqStruct.alpha,
passing the raw string through as-is rather than the .toInt()-based
formatting used for the numeric part types.
- AssignVariables::assignSequence() substitutes %seqa_N from
seqStruct.alpha, mirroring the existing %sequ_N/%seqt_N/%seqh_N
substitutions.
No "alphafolio" variant was added, matching the discussion's scope (only
unit/ten/hundred have folio-anchored variants).
Verified against production code via the numbering config dialog's own
Suivant/Précédent buttons: from "a", 25 clicks reached "z"; one more
produced "aa"; 25 more reached "az"; one more produced "ba" (carry).
Reversed: "ba"->"az"->(25 clicks)->"aa"->"z" (shrink)->(25 clicks)->"a".
One more "previous" at "a" correctly stayed at "a" after the clamp fix.
Also confirmed the Formule field auto-updates to "%seqa_1" the instant
the type is switched to "Alphabétique", confirming the formula-generation
wiring works live in the UI, not just at the engine level.
When using composite text in report elements, the name of the variable
was displayed when inserting the reportElement into the drawing (e.g.
%{function}). This is corrected here.
Add missing variables to assignvariables.cpp
ispyisail