Add pagx embed command and enhance pagx font with --list flag#3405
Add pagx embed command and enhance pagx font with --list flag#3405
Conversation
… partial-read detection.
…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.
…including node removal.
… generic fallback.
|
|
||
| // Variant 1: with a positional argument | ||
| { | ||
| std::streambuf* oldCerr = std::cerr.rdbuf(); |
There was a problem hiding this comment.
🟡 [m4] Minor: cerr/cout rdbuf 重定向模式重复出现 10+ 次，建议提取 RAII 辅助类
以下模式在测试中大量重复：
std::streambuf* oldCerr = std::cerr.rdbuf();
std::ostringstream capturedErr;
std::cerr.rdbuf(capturedErr.rdbuf());
// ... test ...
std::cerr.rdbuf(oldCerr);建议提取为 RAII 辅助类（如 CaptureStream），减少重复并避免异常/断言失败时未恢复 rdbuf 的风险。
| EXPECT_EQ(ret1, 0); | ||
| auto ret2 = CallRun(pagx::cli::RunEmbed, {"embed", out1, "-o", out2}); | ||
| EXPECT_EQ(ret2, 0); | ||
| EXPECT_EQ(ReadFile(out1), ReadFile(out2)); |
There was a problem hiding this comment.
🟡 [m5] Minor: 幂等性测试依赖字节级比较，存在脆弱性
第二次 embed 时，字体嵌入会先 ClearEmbeddedGlyphRuns 再重新 embed。如果 makeNode 生成的 id 带递增计数器，重新 embed 可能产生不同的 id，导致字节级不相等。当前能通过说明 id 生成是确定性的，但属于隐含假设。
建议：在测试注释中说明此假设，或考虑改为语义级比较（如分别检查 Image 节点的 data 不为空、无外部 filePath 等）。
| EXPECT_EQ(ret1, 0); | ||
| auto ret2 = CallRun(pagx::cli::RunEmbed, {"embed", out1, "-o", out2}); | ||
| EXPECT_EQ(ret2, 0); | ||
| EXPECT_EQ(ReadFile(out1), ReadFile(out2)); |
There was a problem hiding this comment.
🟡 [m5] Minor: 幂等性测试依赖字节级比较，存在脆弱性
第二次 embed 时，字体嵌入会先 ClearEmbeddedGlyphRuns 再重新 embed。如果 makeNode 生成的 id 带递增计数器，重新 embed 可能产生不同的 id，导致字节级不相等。当前能通过说明 id 生成是确定性的，但属于隐含假设。
建议：在测试注释中说明此假设，或考虑改为语义级比较（如分别检查 Image 节点的 data 不为空、无外部 filePath 等）。
| << "\n" | ||
| << "Options:\n" | ||
| << " -o, --output <path> Output file path (default: overwrite input)\n" | ||
| << " --file <path> Register a font file (can be specified multiple\n" |
There was a problem hiding this comment.
🟡 [m6] Minor: --file 在 embed 命令中可能引起歧义
pagx embed --file 指的是注册字体文件，但不了解上下文的用户在 embed 命令中看到 --file 可能误以为是指定输入文件。
建议：考虑重命名为 --font-file 或 --font 使语义更明确。如果保持 --file，至少在帮助文本描述中补充 "(font file)"。
…LI against oversized files. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
| nodes[writeIdx++] = std::move(nodes[readIdx]); | ||
| } | ||
| } | ||
| nodes.resize(writeIdx); |
There was a problem hiding this comment.
🔴 [P1] nodeMap 清理使用已释放指针值比较，属于未定义行为
nodes.resize(writeIdx) 触发被移除节点的 unique_ptr 析构后，toRemove 中保存的裸指针已成为悬空指针。下方 nodeMap 清理循环中 toRemove.count(it->second) 使用释放后的指针值做比较，C++ 标准定义这属于 UB（[basic.stc.dynamic.deallocation]）。虽然主流编译器不会实际崩溃，但属于潜在隐患。
修复建议：将 nodeMap 清理循环（第 424-430 行）移到 nodes.resize(writeIdx) 之前执行，此时指针仍有效。
|
|
||
| FontLocation SystemFonts::FindFont(const std::string&, const std::string&) { | ||
| // Windows FreeType backend already implements MakeFromName via DirectWrite. | ||
| return {}; |
There was a problem hiding this comment.
🔴 [P2] Windows FindFont 返回空，CLI 字体功能在 Windows 上失效
CliUtils.h 中 ResolveSystemTypeface 依赖 FindFont 作为 fallback。在 FreeType backend 下 MakeFromName 返回的字体族可能不匹配导致 fall through 到此处，而 Windows 直接返回空，使 pagx font --name 和 pagx embed 字体解析静默失败。
修复建议：使用 DirectWrite IDWriteFontCollection::FindFamilyName 实现，或在头文件注释中明确标注 Windows 平台限制。
| if (document == nullptr) return false; | ||
| auto paths = document->getExternalFilePaths(); | ||
| std::unordered_set<std::string> loaded; | ||
| for (const auto& path : paths) { |
There was a problem hiding this comment.
🟠 [P3] URL 检测逻辑不够精确
头文件注释说明跳过含 :// 的路径，但此处代码依赖 getExternalFilePaths() 返回的路径。如果该函数已过滤了 URL，则此处无需再做 URL 检测；如果未过滤，path.find("://") 可能误判 Windows 路径（如 C://Users/file.png）。建议明确过滤职责归属——要么收敛到 getExternalFilePaths()，要么此处改为检查已知 scheme 前缀（http://、https://、data:）。
| if (options->fontFile.empty() && options->fontName.empty()) { | ||
| std::cerr << "pagx font info: either --file or --name is required\n"; | ||
|
|
||
| if (options.listMode && (!options.fontFile.empty() || !options.fontName.empty())) { |
There was a problem hiding this comment.
🟠 [P4] --list + --size 与 --list + --file/--name 错误处理不一致
--list + --file/--name 报错退出（返回 1），但 --list + --size 仅输出 warning 并继续执行（返回 0）。建议统一行为：直接静默忽略 --size（--list 模式不需要字号信息），无需输出 warning。
| // Fallback: locate the font file via platform APIs and load by path. | ||
| auto location = pagx::SystemFonts::FindFont(family, style); | ||
| if (!location.path.empty()) { | ||
| return tgfx::Typeface::MakeFromPath(location.path, location.ttcIndex); |
There was a problem hiding this comment.
🟠 [P5] 路径函数不支持 Windows 反斜杠
ResolveFallbackTypeface（第 86 行）仅通过 specifier.find('/') 判断是否为文件路径，GetDirectory（第 136 行）也仅检查正斜杠。Windows 路径如 C:\fonts\Arial.ttf 不会被识别为文件路径。建议同时检查 '/' 和 '\\'。
| } | ||
| } | ||
| // Fallback: locate the font file via platform APIs and load by path. | ||
| auto location = pagx::SystemFonts::FindFont(family, style); |
There was a problem hiding this comment.
🟠 [P6] ResolveFallbackTypeface 中大写扩展名判断为死代码
第 90-92 行的 ext = specifier.substr(dot) 未做大小写转换，但后续同时比较 .ttf 和 .TTF。大小写混合的扩展名（如 .Ttf）两边都不匹配。而同文件 GetFileExtension 已做小写转换，两处逻辑不统一。建议对 ext 转小写后只保留小写比较。
| lastErrorPath_ = path; | ||
| return false; | ||
| } | ||
| document->loadFileData(path, data); |
There was a problem hiding this comment.
🟡 [P7] loadFileData 返回值未检查
document->loadFileData(path, data) 返回 bool 表示是否成功，但此处忽略了返回值。防御性编程建议检查返回值，失败时设置 lastErrorPath 并返回 false。
| @@ -24,6 +24,7 @@ | |||
| #include <vector> | |||
There was a problem hiding this comment.
🟡 [P8] 幂等性测试依赖隐式假设
Embed_Idempotent 测试通过字节级比较验证两次 embed 结果相同，但这依赖于 makeNode 的 ID 生成是确定性的。如果未来 ID 生成逻辑变化（如引入递增计数器），测试会假阳性失败。建议在测试注释中标注此假设，或改为语义级比较（如检查 Image 节点的 data 非空、无外部 filePath）。
| pagx font --list | ||
|
|
||
| # Embed fonts and images into a PAGX file | ||
| pagx embed input.pagx |
There was a problem hiding this comment.
🟡 [P9] embed 示例缺少输入文件参数
当前 pagx embed 示例没有展示输入文件参数，容易让用户困惑命令用法。建议改为 pagx embed input.pagx 以展示完整的最简用法。
| @@ -0,0 +1,204 @@ | |||
| <!-- refreshed: 2025-07-15 --> | |||
There was a problem hiding this comment.
🟡 [P10] .planning/codebase/ 目录不应入仓库
这些是 AI 辅助开发的上下文文件（共 7 个文件 +1663 行），不属于项目源码也不属于正式文档。建议从 PR 中移除：git rm --cached .planning/。
…e of dangling pointers.
…atching effective.
shlzxjp
left a comment
shlzxjp
left a comment
There was a problem hiding this comment.
二轮代码评审反馈
在首轮评审基础上做了二次核验，整理出仍建议处理的问题。以 行级评论 给出可精确定位的条目；少量改动落在本 PR diff 之外的旧代码行，集中列在下方 summary。
阻塞级（建议合入前处理）
-
B1 公共头的 friend 污染（见
include/pagx/PAGXDocument.h行级评论）：FontEmbedder位于src/renderer/（私有实现），却被声明为公共头内的 friend；它访问document->nodeMap只为做节点批量移除，建议把移除逻辑下沉为PAGXDocument的公开方法（如removeNodes(const std::unordered_set<Node*>&)或resetLayout()），彻底去掉这条 friend 声明，避免公共 API 反向依赖私有 renderer 模块。 -
B3
FontEmbedder直接赋值font->id = "fontN"绕过registerNode（diff hunk 未覆盖这几行，无法作行级评论）。位置：src/renderer/FontEmbedder.cpp:488与:496：vectorBuilder.font->id = "font" + std::to_string(fontIndex++); ... builderIt->second.font->id = "font" + std::to_string(fontIndex++);
后果：
- 这些 Font 节点不会被写入
PAGXDocument::nodeMap，findNode("font1")永远查不到自身。 - 若原文档已有用户自建 id 为
font1的节点，会静默重名。 ClearEmbeddedGlyphRuns的nodeMap清理分支对这类节点是空跑。
建议：改用document->registerNode(font, id)或makeNode<Font>(id)路径；同时用保留前缀（如__embed_font_1）避免用户空间冲突。
- 这些 Font 节点不会被写入
主要问题（建议本 PR 修）
-
M9/B5
applyLayout契约 vs re-embed 路径矛盾（位置include/pagx/PAGXDocument.h:132-139，不在本 PR diff 内）：文档标注 “should only be called once per document — repeated calls may produce incorrect results”，但CommandEmbed的流程是ClearEmbeddedGlyphRuns → applyLayout → embed。对 CLI 初次运行无碍，但从外部 SDK 串联（已 apply 过的 doc）再进 embed，就会再次触发 apply 违反契约。请明确二选一：要么放宽文档允许“Clear 之后 reapply”，要么让ClearEmbeddedGlyphRuns也重置layoutApplied=false（需要新增公开方法，配合 B1 的下沉方案一起改）。 -
M14
FontEmbedder散落的0.001f魔数（位置src/renderer/FontEmbedder.cpp:121/:171/:361，不在本 PR diff 内）：if (fontSize < 0.001f) return; if (scaleX < 0.001f) return; if (runFontSize < 0.001f) return;
三处重复且无命名，建议抽为
constexpr float MinFontSize或直接用项目已 include 的pag::FloatNearlyZero。
次要问题
-
N16 main.cpp 不分发
validate/optimize，但 README 在宣传：cli/npm/README.md:22-28的命令表与src/cli/main.cpp不一致。推测是 npm 包装层做的扩展，建议 README 明确标记 “npm wrapper 专有” 或在 CLI 帮助里补齐。 -
以下条目以 行级评论 给出：
- M5
IsUrlPath遗漏file:// - M12
IsUrlPath的data:分支是死代码（PAGXImporter已把 data URI 解码到image->data） - M11
getExternalFilePaths + loadFileDataO(N·M) & 路径未去重 - M13 in-place 写入时
WriteStringToFile会把.tmp路径打印为 “wrote xxx.pagx.tmp”，用户看到两行 wrote - M2（原 B2）
ResolveSystemTypeface在 family 命中即返回，未校验 style - N4 macOS
CFSetCreate返回值未校验就被传入CTFontDescriptorCreateMatchingFontDescriptors - N5 Linux
AllFontFamilies双次 map 查找可合并 - N6 Windows
GetFaceName/GetFamilyName重复代码 - N7
std::rename在 Windows 目标已存在时会失败，建议用std::filesystem::rename - N14（原 B4）Windows
wideFamily长度含末尾 NUL，习惯不整洁 - N15 README
pagx embed选项表缺别名--font-file
- M5
赞同
- 子命令扁平化方向正确，retirement 提示考虑到了迁移。
- Commit 历史里修过的 use-after-free、256MB 限额、backslash 等几条硬伤态度很正。
- 整体注释详尽，测试矩阵覆盖默认 / skip 组合 / 缺失资源 / 互斥 / 幂等 / help 等多维场景。
（二轮审查未对编译器警告和 ASan 做验证；如需再做，可以单独出一份动态验证报告。）
| friend class PAGXImporter; | ||
| friend class PAGXExporter; | ||
| friend class TextLayoutContext; | ||
| friend class FontEmbedder; |
There was a problem hiding this comment.
B1 Blocker — friend 声明让公共头反向依赖私有 renderer 模块
FontEmbedder 位于 src/renderer/（私有实现），却作为 friend 出现在对外的公共头里。它之所以要 friend，仅仅是为了在 ClearEmbeddedGlyphRuns 里访问 document->nodeMap，而那是一次 “按节点集合批量移除 + 同步清理 nodeMap” 的通用动作，并不是 embedder 的固有职责。
建议把这段操作下沉为 PAGXDocument 的公开成员（例如）：
void removeNodes(const std::unordered_set<Node*>& toRemove);
// 或
void resetLayout(); // 同时重置 layoutApplied，参考 M9这样此处 friend 可整行删除，FontEmbedder 只依赖对外 API。
| if (path.find("data:") == 0) { | ||
| return true; | ||
| } | ||
| // Match known URL schemes rather than generic :// to avoid false positives on Windows paths |
There was a problem hiding this comment.
M12 死代码 — data: 分支永不触发
PAGXImporter 在解析 <Image source="data:..."/> 时已经调用 DecodeBase64DataURI 把数据写进 image->data，这种 Image 的 filePath 字段保持为空（参考 src/pagx/PAGXImporter.cpp:1503）。到这里时，image->filePath.find("data:") == 0 无论走 IsUrlPath 还是直接判断都不会命中——它不是真正的防御层，而是被剪掉的旧逻辑残留。
建议：要么删掉 data: 分支，只保留 http:///https://；要么在注释里明确标注 “defensive residue, should not trigger”。
| } | ||
| // Match known URL schemes rather than generic :// to avoid false positives on Windows paths | ||
| // like C://Users/file.png. | ||
| return path.find("http://") == 0 || path.find("https://") == 0; |
There was a problem hiding this comment.
M5 URL 白名单遗漏常见 scheme
当前只认 http:// / https://。如果 Image 的 filePath 是 file:///Users/foo.png（很多 SVG 工具会这样生成），这里不会被 skip，下游 std::ifstream(path) 又会失败，最终返回 “failed to load”——语义是 skip 而不是 fail。
建议加入 file:// 白名单；如果长期打算允许更多 scheme，可以改成 “匹配 scheme://，但排除 Windows 盘符 [A-Za-z]:[/\\]” 的通用判定，并在注释里写明边界。
| if (IsUrlPath(image->filePath)) { | ||
| continue; | ||
| } | ||
| paths.push_back(image->filePath); |
There was a problem hiding this comment.
M11 getExternalFilePaths + loadFileData 组合代价 O(N·M)
getExternalFilePaths 遍历 nodes 收集路径但不去重；ImageEmbedder::embed 再对每个路径回调 loadFileData，后者又对 nodes 做一次线性扫描（if (image->filePath != filePath) continue;）。
- 同一张图被多个 Image 节点引用时会重复触发；
- 设 Image 节点数 M、外部引用数 N，总代价 O(N·M)；
- playground 也直接暴露
getExternalFilePaths到 JS 端，重复会体现在 UI 列表里。
建议：
getExternalFilePaths直接去重（unordered_set然后 vectorize），或在注释里写明 “may contain duplicates”。- 加一个批量接口
bool loadFileDataBatch(const std::unordered_map<std::string, std::shared_ptr<Data>>&)，在 PAGXDocument 内部用一次遍历完成匹配写入，彻底消除 O(N·M)。
| // workflow; both produce equivalent family-member sets. | ||
| const void* mandatoryKeys[] = {kCTFontFamilyNameAttribute}; | ||
| CFSetRef mandatoryAttributes = | ||
| CFSetCreate(kCFAllocatorDefault, mandatoryKeys, 1, &kCFTypeSetCallBacks); |
There was a problem hiding this comment.
N4 CFSetCreate 的返回值未校验就被使用
CFSetRef mandatoryAttributes =
CFSetCreate(kCFAllocatorDefault, mandatoryKeys, 1, &kCFTypeSetCallBacks);
...
CFArrayRef members =
CTFontDescriptorCreateMatchingFontDescriptors(familyDescriptor, mandatoryAttributes);Apple 文档里 CTFontDescriptorCreateMatchingFontDescriptors 的第二参允许 NULL，但语义不同：NULL = 无强制 key；非 NULL = 指定集合的 key 必须匹配。此处若 CFSetCreate 失败返回 nullptr，AllFontFamilies 不再做“按 family name 强制匹配”的限制，返回值语义会飘。
CFSetCreate 失败极少见，但仍建议 early-return 空 vector，避免静默降级。
| return typeface; | ||
| } | ||
| } | ||
| // Fallback: locate the font file via platform APIs and load by path. |
There was a problem hiding this comment.
M2（原 B2 降级）针对上方行 57-58 — family 命中即返回，未校验 style
函数顶部的首个匹配分支（本 PR 未改，但文档已被本 PR 重写成"fallback to SystemFonts::FindFont"的语义）：
auto typeface = tgfx::Typeface::MakeFromName(family, style);
if (typeface != nullptr && FontFamilyMatch(family, typeface->fontFamily())) {
return typeface; // ← 只看 family
}当用户请求 "Arial,Italic" 而系统只有 Regular 时，MakeFromName 通常会回退成 Arial Regular。这里 FontFamilyMatch 命中，直接返回，后续 if (!style.empty()) 的 fallback 路径（包括本 PR 新增的 SystemFonts::FindFont 兜底）永远不执行，等价于 style 被静默忽略，而不是 “尝试 default style”。
此行为在 main 分支就已存在，但本 PR 扩展了 fallback 逻辑并改写了函数头注释，顺手修掉更合理：
if (typeface != nullptr && FontFamilyMatch(family, typeface->fontFamily()) &&
(style.empty() || FontStyleMatch(style, typeface->fontStyle()))) {
return typeface;
}需要配一个 FontStyleMatch（大小写不敏感、允许 "Regular" vs "" 等价）。
| auto xml = PAGXExporter::ToXML(*document); | ||
| if (options.outputFile == options.inputFile) { | ||
| auto tempPath = options.outputFile + ".tmp"; | ||
| if (!WriteStringToFile(xml, tempPath, "pagx embed")) { |
There was a problem hiding this comment.
M13 in-place 写入时 WriteStringToFile 会打印 .tmp 路径的 wrote 消息
当前顺序：
WriteStringToFile(xml, tempPath, "pagx embed")—— 内部会std::cout << command << ": wrote " << filePath << "\n";打出pagx embed: wrote output.pagx.tmp。std::rename(tempPath, outputFile)。- 再
std::cout << "pagx embed: wrote " << options.outputFile << "\n";—— 第二条pagx embed: wrote output.pagx。
用户会看到两条 wrote 消息，其中一条指向不应存在的 .tmp 文件，非常困惑。
建议任选：
- 给
WriteStringToFile增加bool silent = false形参；in-place 路径传true。 - 或者在
CommandEmbed里直接用std::ofstream写 tmp，不经WriteStringToFile。
| std::remove(tempPath.c_str()); | ||
| return 1; | ||
| } | ||
| if (std::rename(tempPath.c_str(), options.outputFile.c_str()) != 0) { |
There was a problem hiding this comment.
N7 std::rename 在 Windows 目标已存在时会失败
POSIX 的 rename 允许覆盖已存在目标；Win32 rename 则会失败（ERROR_ALREADY_EXISTS）。当前的 in-place 策略是 write tmp → rename tmp → output，在 Windows 上当 output 已存在（绝大多数场景）会命中失败分支，用户看到 “failed to replace 'output.pagx'”。
项目在测试里已经使用 C++17 <filesystem>（std::filesystem::copy_file），建议替换为：
std::error_code ec;
std::filesystem::rename(tempPath, options.outputFile, ec);
if (ec) { ... }std::filesystem::rename 在 Windows 上会用 MoveFileEx(..., MOVEFILE_REPLACE_EXISTING)，跨平台都能覆盖。
| << " bounds Query rendered pixel bounds of layers (for crop regions)\n" | ||
| << " font Query font metrics or embed fonts into a PAGX file\n" | ||
| << " font Query font metrics\n" | ||
| << " embed Embed fonts and images into a PAGX file\n" |
There was a problem hiding this comment.
N16 main.cpp 的命令表与 npm README 不一致
README（cli/npm/README.md:22-28）还在宣传 pagx validate 与 pagx optimize 两个命令，但这里的 PrintUsage 和 main() 都没有注册这两个命令（grep -n "validate\|optimize" src/cli/main.cpp 无命中）。推测是 npm 包装层额外实现的。
本 PR 在 README 里新增了 embed、改了 font，正好顺手把这两条一并核对：
- 若 validate/optimize 是 npm 专属，建议在 README 命令表旁加标注 “npm wrapper only”；
- 若二进制也要支持，请补齐
PrintUsage+main分发。
| | Option | Description | | ||
| |--------|-------------| | ||
| | `-o, --output <path>` | Output file path (default: overwrite input) | | ||
| | `--file <path>` | Register a font file (repeatable) | |
There was a problem hiding this comment.
N15 README 未提 --font-file 别名
src/cli/CommandEmbed.cpp 里 embed 接受两种拼写：--font-file 与 --file（if ((arg == "--file" || arg == "--font-file")...）。但 README 只列了 --file。
建议写成 \--font-file, --file `与 CLI help 保持一致；或若倾向精简，就去掉--font-file` 别名。
3 participants
为 pagx CLI 工具新增 embed 子命令和字体查询增强功能：