Add pagx embed command, restructure pagx font, and support external font files. by YyZz-wy · Pull Request #3471 · Tencent/libpag

YyZz-wy · 2026-05-29T08:19:29Z

本 PR 整合 PAGX CLI 的三阶段重构：

新增 pagx embed 命令：统一字形与图片嵌入流程，支持 --skip-fonts / --skip-images / --fallback 选项；ImageEmbedder 与 FontEmbedder 拆分清晰。
重构 pagx font 命令：扁平化为单一查询命令，移除 embed 子命令（保留兼容重定向），新增 --list 列出系统字体。
PAGX Font 节点新增 file 属性：可引用外部字体文件，pagx embed 自动扫描并注册字体到 FontConfig，无需 --font-file 参数；ClearEmbeddedGlyphRuns 在 Font 含 file 时保留节点结构仅清除 Glyph 子节点，支持幂等 re-embed。

附带 SystemFonts 在 macOS / Windows / Linux / FreeType 后端新增 AllFontFamilies 与 FindFont 实现，修复若干内存泄漏与悬空指针问题，并更新 spec、skills 文档及 CLI 测试。

… partial-read detection.

…ip flags.

…phRun nodes from document. Previously only cleared Text::glyphRuns vector but left Font, Glyph, GlyphRun, PathData, and bitmap Image nodes in document->nodes, causing duplicate nodes on re-embed.

…nd idempotency. 9 CLI_TEST cases cover EMBED-01..05, EMBED-07, EMBED-08, EMBED-10, EMBED-11.

…phRun nodes from document. Previously only cleared Text::glyphRuns vector but left Font, Glyph, GlyphRun, PathData, and bitmap Image nodes in document->nodes, causing duplicate nodes on re-embed.

…Runs to follow coding convention.

…elpers.

…tions policy.

…including node removal.

…tEmbedUsage.

…onts.h.

… generic fallback.

…command layer.

…to pagx font.

…ist.

…the source node.

…tired info subsection.

… resolution.

codecov-commenter · 2026-05-29T09:05:41Z

Codecov Report

❌ Patch coverage is 72.65625% with 210 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.81%. Comparing base (1917e8e) to head (a7c64a0).

Files with missing lines	Patch %	Lines
src/pagx/SystemFonts.cpp	37.37%	50 Missing and 12 partials ⚠️
src/cli/CommandEmbed.cpp	62.61%	29 Missing and 11 partials ⚠️
src/cli/CliUtils.h	14.28%	28 Missing and 2 partials ⚠️
src/pagx/PAGXDocument.cpp	51.92%	19 Missing and 6 partials ⚠️
test/src/PAGXCliTest.cpp	94.47%	3 Missing and 15 partials ⚠️
src/cli/CommandFont.cpp	80.51%	12 Missing and 3 partials ⚠️
src/renderer/ImageEmbedder.cpp	61.29%	5 Missing and 7 partials ⚠️
src/renderer/FontEmbedder.cpp	75.00%	3 Missing and 3 partials ⚠️
src/pagx/PAGXImporter.cpp	77.77%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3471      +/-   ##
==========================================
+ Coverage   79.70%   79.81%   +0.10%     
==========================================
  Files         580      584       +4     
  Lines       60904    61526     +622     
  Branches    18921    19003      +82     
==========================================
+ Hits        48546    49105     +559     
- Misses       8685     8707      +22     
- Partials     3673     3714      +41

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…_embed

… ExporterUtils.

shlzxjp · 2026-06-03T12:55:35Z

+        }
+        if (node->nodeType() == NodeType::Font) {
+          auto* font = static_cast<Font*>(node.get());
+          ResolveRelativePath(basePath, font->file);


Font.file 在导入时被改写为绝对路径，但 PAGXExporter 在写出 Font 节点时直接 xml.addAttribute("file", font->file) 没有反向相对化（参见 src/pagx/PAGXExporter.cpp:1007-1009）。结果：用户写的 <Font file="./fonts/X.otf"/> 经 pagx embed 后会变成 file="/abs/path/.../X.otf"，把整个 PAGX 复制到别的机器或目录后外部字体引用就失效了。文档（attributes.md）声称嵌入后保留 file 是为了"源文件可追溯性"，但绝对路径反而破坏了这一语义。新增测试 Embed_FontFile_ReembedPreservesNode、Embed_FontFile_AutoRegistersAndEmbeds 都没断言路径形式，所以问题没暴露。建议导出时对原本是相对形式的路径做反向相对化，或在 importer 里保留原始字符串、仅在解析阶段使用临时副本。

shlzxjp · 2026-06-03T12:56:04Z

+   * invalidated after this call. Callers must first collect all affected nodes to remove
+   * before calling.
+   */
+  void removeNodes(const std::unordered_set<Node*>& nodesToRemove);


removeNodes / setNodeId / resetLayoutState 三个新方法本质上是为 FontEmbedder::ClearEmbeddedGlyphRuns 服务的内部协作接口，却暴露在公开头文件 include/pagx/PAGXDocument.h 中。第三方调用者一旦使用 removeNodes，就要承担注释里写的"any external pointers… become dangling"风险，没有任何编译期或运行期保护。建议把这三个方法改成 private + friend class FontEmbedder，或者迁移到内部头文件，避免污染公开 API surface。

shlzxjp · 2026-06-03T12:56:31Z

      continue;
    }
-    if (image->filePath.find("data:") == 0) {
+    if (IsUrlPath(image->filePath)) {


新的 IsUrlPath 仅识别 http://、https://、file://，不再排除 data: URI。代码注释说"data: is handled by PAGXImporter before this point"，对 FromFile 路径成立；但 getExternalFilePaths 是公开 API，第三方完全可以绕过 importer 直接构造 PAGXDocument 并设置 image->filePath = "data:image/png;base64,..."。在新逻辑下：1) getExternalFilePaths 会返回 data:... 字符串；2) ImageEmbedder::embed 调用 ifstream 打开失败；3) pagx embed 报 failed to load image 'data:image/png...'，对调用方非常迷惑。建议在 IsUrlPath 中也匹配 data: 前缀，与旧行为对齐（或改名为 IsNonFileScheme）。

shlzxjp · 2026-06-03T12:56:52Z

+    std::cout << "pagx embed: wrote " << options.outputFile << "\n";
+    return 0;
+  }
+  if (!WriteStringToFile(xml, options.outputFile, "pagx embed")) {


覆盖输入文件路径走的是 tempPath + rename 的原子写入（上面 152-175 行），而显式 -o <other> 这条路径直接走 WriteStringToFile，是直接 ofstream 写入。如果 -o 指向的已存在文件，写到一半失败/崩溃就会得到一个截断的输出文件。两条路径的崩溃安全语义不一致。建议统一用 temp+rename helper，或在 WriteStringToFile 内部统一加上原子写入。

shlzxjp · 2026-06-03T12:57:26Z

+    if ((arg == "-o" || arg == "--output") && i + 1 < argc) {
+      options->outputFile = argv[++i];
+    } else if (arg == "--fallback" && i + 1 < argc) {
+      options->fallbacks.push_back(argv[++i]);


用户传 --fallback 但同时传 --skip-fonts 时，fallbacks 收到了值却完全不会被使用（下面 if (!options.skipFonts) 块被跳过），命令也不报错。脚本中常会无脑加 --fallback，发现某次没生效非常难排查。建议在 skipFonts && !fallbacks.empty() 时报错或打印警告。

shlzxjp · 2026-06-03T12:58:12Z

  int fontIndex = 1;
  if (vectorBuilder.font != nullptr) {
-    vectorBuilder.font->id = "font" + std::to_string(fontIndex++);
+    document->setNodeId(vectorBuilder.font, "__embed_font_" + std::to_string(fontIndex++));


setNodeId 内部调用 registerNode，撞 ID 时只在 document->errors 里追加一条记录，然后直接覆盖 nodeMap[id]（参见 PAGXDocument.cpp:66-76）。如果用户的 PAGX 里恰好存在 id 为 __embed_font_1 的节点（不限于 Font，任何节点类型），它会被静默从 nodeMap 中踢掉，且 pagx embed 仍返回 0。__embed_font_ 前缀降低了概率，但前缀本身没有任何文档化的"保留"声明。建议要么在 spec 中明确声明 __embed_* 为保留前缀，要么在 setNodeId 撞 ID 时让命令行调用方拿到非零退出码。

shlzxjp · 2026-06-03T12:59:59Z

+  if (!style.empty()) {
+    FcPatternAddString(pattern, FC_STYLE, reinterpret_cast<const FcChar8*>(style.c_str()));
+  }
+  FcPatternAddBool(pattern, FC_SCALABLE, FcTrue);


Linux FindFont 强制 FC_SCALABLE=true，与 FallbackTypefaces 行为一致；但当用户的 PAGX 显式声明使用一个仅有位图（embedded bitmap-only）的字体（如某些 emoji 字体）时，FindFont 会找不到。FallbackTypefaces 排除位图是合理的，但用户显式查询时是否要放宽这个限制需要确认产品定位。

…edder friend.

…e FC_SCALABLE filter.

…_embed

…text_box tests.

shlzxjp · 2026-06-10T01:57:47Z

+        if (node->nodeType() == NodeType::Font) {
+          auto* font = static_cast<Font*>(node.get());
+          if (!font->file.empty()) {
+            font->fileOriginal = font->file;


fileOriginal 只在 PAGXImporter::FromFile 这一条加载路径上被设置，而 PAGXImporter::FromXML(content) 这条路径不经过这里——只在 ParseFont 中执行 font->file = GetAttribute(node, "file")，所以从 XML 字符串/字节流加载的文档，Font::fileOriginal 永远为空。

现状下没有功能 bug（FromXML 路径下 font->file 没被改写，导出端 !fileOriginal.empty() ? fileOriginal : font->file 退化到 font->file 仍是原值），但这把 fileOriginal 的语义碎片化到了具体加载路径上，而不是数据结构的不变量。

隐患：未来若新增第三种加载路径（如解压 zip 内嵌 PAGX 后再解析），开发者很容易忘记再补一遍这段备份逻辑。建议把"备份原值"的语义下沉到 ParseFont（解析时立即 font->fileOriginal = font->file），由 FromFile 单独负责解析后的相对→绝对重写。这样 fileOriginal == 原始 XML 中的 file 属性 在数据结构层面就是个稳定的不变量。

shlzxjp · 2026-06-10T01:58:11Z

+  {
+    std::ofstream out(tempPath);
+    if (!out.is_open()) {
+      std::cerr << command << ": failed to write '" << tempPath << "'\n";


错误消息暴露了内部 .tmp 路径细节——tempPath 是 filePath + ".tmp" 这一实现选择，对用户而言完全是黑盒。

用户运行 pagx embed -o /readonly/out.pagx input.pagx 失败时会看到：

pagx embed: failed to write '/readonly/out.pagx.tmp'

用户指定的是 out.pagx，但报错里冒出来一个 out.pagx.tmp，会让人困惑（怀疑是不是自己拼写错了，或文件被别的进程占着）。第 73、79、87、88 行的错误消息都有同样问题。

参考主流 CLI（git、cargo、npm），失败消息都报用户指定的最终路径，而不是内部临时文件名。建议把错误消息中的 tempPath 替换为 filePath，例如：failed to write '<filePath>' (creating temp file failed)。

YyZz-wy requested review from Hparty, domchen, kevingpqi123 and shlzxjp as code owners May 29, 2026 08:19

YyZz-wy force-pushed the feature/codywwang_cli_embed branch from b8fbace to 74428c8 Compare May 29, 2026 08:31

YyZz-wy added 25 commits May 29, 2026 16:34

Ignore GSD planning directory from version control.

fc2706f

Add ImageEmbedder header declaring embed and lastErrorPath API.

f6723e6

Implement ImageEmbedder with ReadFileToData supporting empty-file and…

5b6a818

… partial-read detection.

Add CommandEmbed header declaring RunEmbed entry point.

4c99b91

Add pagx embed command orchestrating font and image embedding with sk…

849b4a6

…ip flags.

Register pagx embed command and update top-level usage.

1435101

Add embed_sample pagx and png fixtures for pagx embed CLI tests.

c864a08

Fix FontEmbedder::ClearEmbeddedGlyphRuns to remove stale Font and Gly…

0b403b1

…phRun nodes from document. Previously only cleared Text::glyphRuns vector but left Font, Glyph, GlyphRun, PathData, and bitmap Image nodes in document->nodes, causing duplicate nodes on re-embed.

Add EMBED CLI tests covering default embed skip flags missing image a…

b02b1a7

…nd idempotency. 9 CLI_TEST cases cover EMBED-01..05, EMBED-07, EMBED-08, EMBED-10, EMBED-11.

Fix FontEmbedder::ClearEmbeddedGlyphRuns to remove stale Font and Gly…

78de811

…phRun nodes from document. Previously only cleared Text::glyphRuns vector but left Font, Glyph, GlyphRun, PathData, and bitmap Image nodes in document->nodes, causing duplicate nodes on re-embed.

Replace lambda with explicit loop in FontEmbedder::ClearEmbeddedGlyph…

9da0451

…Runs to follow coding convention.

Retire pagx font embed subcommand with redirect error and drop dead h…

2961465

…elpers.

Revise CommandFont header doc comment to query-only scope.

7e7d61e

Add FONT-01 CLI test asserting pagx font embed redirect error.

103b09b

Add std::nothrow to ImageEmbedder buffer allocation to honor no-excep…

097a127

…tions policy.

Document full reset contract of FontEmbedder::ClearEmbeddedGlyphRuns …

f7436e6

…including node removal.

Trim CommandEmbed header doc to a summary and defer flag list to Prin…

56e0165

…tEmbedUsage.

Add FontFamilyEntry struct and AllFontFamilies declaration to SystemF…

c5d04d1

…onts.h.

Implement SystemFonts::AllFontFamilies for macOS, Windows, Linux, and…

ebcebb7

… generic fallback.

Flatten pagx font into a single query command and delete the info sub…

f0794c7

…command layer.

Rename FontInfo tests to Font, add info redirect and help surface tests.

dcfdf36

Add --list flag parsing, validation, and system font list formatting …

15b57ea

…to pagx font.

Add FontList text, JSON, and mutual exclusion tests for pagx font --l…

3b31a25

…ist.

Update CommandFont.h comment to reflect removed subcommands.

a6e759b

Eliminate redundant map lookup in SystemFonts Linux AllFontFamilies.

1bad5eb

YyZz-wy added 3 commits May 29, 2026 16:45

Update Font::file doc-comment to match the embed flow that preserves …

ef24936

…the source node.

Rewrite pagx font cli reference to match the flat command and drop re…

d1f6586

…tired info subsection.

Extract ResolveRelativePath helper to deduplicate Image and Font path…

815d1fa

… resolution.

YyZz-wy force-pushed the feature/codywwang_cli_embed branch from 74428c8 to 815d1fa Compare May 29, 2026 08:47

YyZz-wy added 2 commits May 29, 2026 16:50

Add missing StreamCapture RAII helper used by Font file CLI tests.

6e4f513

Run clang-format on PAGXDocument.cpp to satisfy project style.

38eb8f4

YyZz-wy commented Jun 1, 2026

View reviewed changes

Comment thread src/pagx/SystemFonts.cpp Outdated

Comment thread src/pagx/PAGXDocument.cpp

YyZz-wy added 4 commits June 1, 2026 19:16

Fix Windows font path resolution and path comments.

a7c64a0

Merge remote-tracking branch 'origin/main' into feature/codywwang_cli…

3c04ca3

…_embed

Remove duplicated MakeStandaloneParams in Text and reuse the one from…

17e370f

… ExporterUtils.

Add missing Font file import round-trip test for Phase 3 plan 03.

6fa0fd7

shlzxjp reviewed Jun 3, 2026

View reviewed changes

YyZz-wy added 9 commits June 3, 2026 21:33

Make removeNodes, setNodeId and resetLayoutState private with FontEmb…

b90fe4b

…edder friend.

Error when --skip-fonts and --fallback are both set in embed command.

890fb04

Fix IsUrlPath to also match data: URI scheme.

1cd6eac

Preserve Font file original path across import/export round-trip.

54f6708

Unify atomic write in WriteStringToFile, fix font ID collision, remov…

9d07c41

…e FC_SCALABLE filter.

Merge remote-tracking branch 'origin/main' into feature/codywwang_cli…

edce376

…_embed

Merge remote-tracking branch 'origin/main' into feature/codywwang_cli…

d31df93

…_embed

Update screenshot baseline for layout_flex_weights test.

e738016

Update screenshot baseline for spec app_icons game_hud glyph_run and …

1d4e909

…text_box tests.

shlzxjp reviewed Jun 10, 2026

View reviewed changes

Conversation

YyZz-wy commented May 29, 2026

Uh oh!

codecov-commenter commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented May 29, 2026 •

edited

Loading