Skip to content

fix(dxf): stop MTEXT paragraph properties leaking into engraved text - #332

Open
NgoQuocViet2001 wants to merge 4 commits into
earthtojake:developfrom
NgoQuocViet2001:fix-mtext-paragraph-properties-develop
Open

fix(dxf): stop MTEXT paragraph properties leaking into engraved text#332
NgoQuocViet2001 wants to merge 4 commits into
earthtojake:developfrom
NgoQuocViet2001:fix-mtext-paragraph-properties-develop

Conversation

@NgoQuocViet2001

Copy link
Copy Markdown
Contributor

Re-files #327 against develop, per @earthtojake — that one targeted main and edited the generated copy under skills/cad-viewer/.

This one patches packages/cadjs/src/lib/dxf/parseDxf.js and adds the cases to the existing parseDxf.test.js.

The bug

The paragraph-break replace carries the i flag:

text = text.replace(/\\P/gi, "\n").replace(/\\~/g, " ");

\P is a paragraph break. Lowercase \p is paragraph properties — a different command that carries a payload up to a semicolon (\pxqc;, \pxi-2,l2,t2;). Because p is also absent from the inline-property class on the next line, the replace consumes only the two characters and leaves the rest in the text:

\pxqc;PART A        ->  "xqc;PART A"        (want "PART A")
\pxi-2,l2,t2;Item   ->  "xi-2,l2,t2;Item"   (want "Item")

Any MTEXT carrying a paragraph alignment or indent engraves the command payload. Centred titles (\pxqc;) and indented notes are the everyday cases.

The fix

Make the break case-sensitive and add p to the property-run class:

text = text.replace(/\\P/g, "\n").replace(/\\~/g, " ");
text = text.replace(/\\[fFhHcCtTqQwWaAp][^;]*;/g, "");

Order already works in favour of this: the \P replace runs first, so no uppercase \P survives to be eaten by the property run.

Tests

Three cases appended to parseDxf.test.js, covering the properties command, the break, and the other property runs plus plain text.

after   ℹ tests 15   ℹ pass 15   ℹ fail 0
before  ℹ pass 14    ℹ fail 1    ✖ MTEXT paragraph properties are removed whole

Only the broken case fails on develop — the paragraph-break and other-property tests pass either way, which is what makes the case-sensitivity change safe.

The arc-sampling fix is re-filed as #331.

The paragraph-break replace carried the i flag:

    text = text.replace(/\P/gi, "\n")

so lowercase \p matched too. But \p is paragraph PROPERTIES, a different
command that carries a payload up to a semicolon, and p was missing from
the inline-property class on the next line. The replace consumed only
the two characters and left the rest in the text:

    \pxqc;PART A       ->  "xqc;PART A"   (want "PART A")
    \pxi-2,l2,t2;Item  ->  "xi-2,l2,t2;Item"

Any MTEXT with a paragraph alignment or indent -- centred titles and
indented notes are the common ones -- engraved the command payload.

Make the break case-sensitive and add p to the property class. Order
already favours this: the \P replace runs first, so no uppercase \P
survives to be eaten by the property run.
@earthtojake

Copy link
Copy Markdown
Owner

Looks like one failing test case @NgoQuocViet2001 , can you fix that up or remove if the test case is no longer relevant?

@earthtojake

Copy link
Copy Markdown
Owner

The code is right and the base is right now, thanks for the quick turnaround. One
step left: CI fails bundle.sh --check because parseDxf.js is baked into the cad
skill's esbuild bundle, and the source change makes the committed bundle stale. CI
names the fix:

scripts/bundle/bundle-skill.sh cad

Run that, commit the regenerated skills/cad/scripts/packages/cadjs/bin, and push.
Green after that and this merges. (#331 skips this only because
buildDrawingLines.js is not part of that bundle.)

bundle.sh --check fails without this: parseDxf.js is baked into the cad
skill's esbuild bundle, so the source change leaves the committed bundle
stale.

Regenerated with scripts/bundle/bundle-skill.sh cad. The only delta is in
the minified stripMtextFormatting: /\P/gi -> /\P/g, and p added to the
format-command character class.
@NgoQuocViet2001

Copy link
Copy Markdown
Contributor Author

Done — pushed bf83636f with the regenerated bundle.

Ran scripts/bundle/bundle-skill.sh cad; the only delta is in the minified stripMtextFormatting inside skills/cad/scripts/packages/cadjs/bin/dxf-artifact.mjs:

  • e=e.replace(/\\P/gi,"\n")e=e.replace(/\\P/g,"\n")
  • /\\[fFhHcCtTqQwWaA][^;]*;/g/\\[fFhHcCtTqQwWaAp][^;]*;/g

which is exactly the source change, so nothing else in the bundle moved. Pushed as a fast-forward on top of faf30f3e so the base and the reviewed commit are untouched.

One note in case it's useful to you: I first built the bundle against current develop (cce04de6, 0.4.28) and then against the PR head's own base, and dxf-artifact.mjs came out byte-identical (sha256 9d7ffe2e…), so the check isn't sensitive to the base drift here.

parseDxf.js is baked into the dxf skill bundle as well as the cad one, so
bundle.sh --check stayed red after regenerating cad alone. Same two-line
delta in the minified stripMtextFormatting.

scripts/bundle/bundle.sh --check now reports "All bundle outputs are up
to date".
@NgoQuocViet2001

Copy link
Copy Markdown
Contributor Author

Correction to my last comment — regenerating cad alone was not enough, and CI was still red. Fixed in 7301e256.

parseDxf.js is baked into two skill bundles, not one. After bundle-skill.sh cad, bundle.sh --check gets past the cad skill and then fails on the next one:

skills/dxf/scripts/packages/cadjs/bin is stale.
Run scripts/bundle/bundle-skill.sh dxf and commit skills/dxf/scripts/packages/cadjs.

I only saw this once I reproduced the CI job properly. My first pass ran bundle-skill.sh cad and the standalone cad check, which passes; bundle.sh --check walks every skill and needs the repo's own deps installed (npm ci in packages/cadjs, packages/implicitjs, viewer, docs) before it reaches the later ones. With that set up on Node 22 to match the workflow, the dxf failure showed up.

So the branch now carries two bundle commits, cad and dxf, with the same two-line delta in the minified stripMtextFormatting.

Verified locally at the pushed commit, clean tree:

$ scripts/bundle/bundle.sh --check
...
All bundle outputs are up to date.
$ echo $?
0

Both bundle commits sit on top of faf30f3e as fast-forwards, so the reviewed source commit is untouched.

@NgoQuocViet2001

Copy link
Copy Markdown
Contributor Author

Branch was showing behind — merged current develop (cce04de6, 0.4.28) in as 4f0020b6, so you don't need to hit Update branch.

Worth re-checking rather than assuming, because develop moved packages/cadjs/package.json and its lockfile since this branch forked, and those pins feed the bundle. Re-ran the check on the merged tree with the repo's own deps installed:

$ scripts/bundle/bundle.sh --check
...
All bundle outputs are up to date.
$ echo $?
0

Both committed bundles are still correct against the new pins, so nothing needed regenerating a second time. PR diff is unchanged at 4 files — parseDxf.js, parseDxf.test.js, and the two dxf-artifact.mjs bundles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants