From b6249ceccfb012869d1f390e2e077f83bdb600f8 Mon Sep 17 00:00:00 2001 From: Hashem Aldhaheri <158606+aenawi@users.noreply.github.com> Date: Sat, 5 Sep 2026 10:22:38 +0400 Subject: [PATCH 1/2] =?UTF-8?q?feat(core,cli):=20font-coverage=20and=20sha?= =?UTF-8?q?ping-broken=20rules=20=E2=80=94=20M4=204.3?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 4.1 and 4.2 built two modules that refuse to draw a conclusion. This draws them, and there are exactly two, because they are two defects with opposite advice. font-coverage says the font has no glyph for the Arabic: it renders as empty boxes, and no shaping table would have saved it. shaping-broken says the font has every letter and no shaping tables: it renders as a row of disconnected letters, and more coverage would change nothing. One rule reporting both would be a rule nobody could act on. These are the first rules that ask about the machine rather than the document, and that is why they are off by default. Every other check gives the same answer on any computer; which fonts a computer has is not a property of the deck, so an audit that resolved them unasked would report differently on a laptop and on a CI runner looking at one file, and neither run could be reproduced. mirsam audit --fonts asks; --font-dir pins the directories, which is what makes the answer reproducible at all; and the report says which of the two audits it is — fonts NOT RUN for a human, "fonts": {"checked": false} for an agent. That is standing rule 4 spent where it was always going to be needed. The golden corpus never passes the flag, so the six committed reports gained one key each and not a single finding. The claim runs one way only. A font that is here and cannot draw the text will not draw it anywhere, because a cmap and a GSUB travel with the font — that is what makes a finding worth making. A font that is here and draws it perfectly proves nothing about the reader's machine, and silence from these rules is not a promise that the document renders. AGENTS.md says so where an agent reads it. Three ways to say nothing, all deliberate. A paragraph naming no complex-script font is complex-font-missing's; guessing which font the application substitutes is the invention this design refuses. A family this machine does not have is the tool losing the ability to say what the reader will see — a fact about the computer, not a defect in the deck, and reporting it would fire on every runner with no fonts installed. A file the shaper cannot parse is a fact about the file. ADR 0008 left the shaping threshold to this item and it is four required joins, counted over letters the font actually has. A final form exists only because the letter before it joined forwards, so every final has a companion initial or medial, and those are the dual-joining letters a font like Arial does shape. Two joins are therefore one such letter, which is exactly the design choice the ADR forbids concluding from and is the whole of a two-letter word. Four are two of them, independently silent, and no font that shapes Arabic looks like that. Unmapped letters are excluded so a Latin-only font cannot raise shaping-broken beside its font-coverage finding: the tool would be arguing with itself about one paragraph, with one repair and two explanations. Severity is what 4.2 built missing_occurrences for. A font answering for none of the Arabic was not the font this text needed — the paragraph goes blank, and that is an error. A font missing some of it is a sound pairing meeting an unusual letter, which Mishafi and Persian peh is, and is a warning. Neither is fixable: the repair for both is a different typeface, an authoring decision the text cannot supply, and unlike complex-font-missing these would be overwriting a font the author chose rather than filling a slot they left empty. latin.ttf joins the fixtures: printable ASCII, no Arabic, no GSUB, written by the same struct-and-nothing-else generator as the other four. Helvetica under a complex-script slot reduced to its principle, so the acceptance is a committed test rather than a manual check. Validated beyond the fixtures on macOS against a corpus deck retyped to each face: Helvetica and Comic Sans MS report every Arabic character of every paragraph; Times New Roman, Geeza Pro, Mishafi, Al Nile, Baghdad and Arial report nothing — ADR 0008's reh, four joins of five, still not a defect. Bookkeeping: M1 and M3 had every sub-item done and a header nobody flipped. Tatweel-as-padding has been in ROADMAP.md's M4 since it was written with no plan item carrying it, so it is now 4.4 and M4 stays open until it lands. --- AGENTS.md | 58 ++- CHANGELOG.md | 36 ++ Cargo.lock | 1 + README.md | 22 + crates/mirsam-cli/Cargo.toml | 1 + crates/mirsam-cli/src/main.rs | 77 +++- crates/mirsam-cli/src/render.rs | 42 +- crates/mirsam-cli/tests/cli.rs | 115 +++++- crates/mirsam-core/src/lib.rs | 7 + crates/mirsam-core/src/ports.rs | 6 +- crates/mirsam-core/src/rules/font.rs | 375 ++++++++++++++++++ crates/mirsam-core/src/rules/mod.rs | 32 ++ crates/mirsam-core/tests/font_rules.rs | 304 ++++++++++++++ crates/mirsam-core/tests/fonts/latin.ttf | Bin 0 -> 1636 bytes crates/mirsam-fonts/tests/resolution.rs | 16 +- crates/mirsam-ooxml/tests/fonts.rs | 132 ++++++ docs/ARCHITECTURE.md | 6 + docs/PLAN.md | 134 ++++++- scripts/make-shaping-fixture.py | 91 ++++- tests/fixtures/broken-arabic.expected.json | 6 + tests/fixtures/clean.expected.json | 6 + .../quarterly-report-correct.expected.json | 6 + tests/fixtures/quarterly-report.expected.json | 6 + .../quarterly-review-correct.expected.json | 3 + tests/fixtures/quarterly-review.expected.json | 3 + tests/fixtures/torture.expected.json | 6 + 26 files changed, 1443 insertions(+), 48 deletions(-) create mode 100644 crates/mirsam-core/src/rules/font.rs create mode 100644 crates/mirsam-core/tests/font_rules.rs create mode 100644 crates/mirsam-core/tests/fonts/latin.ttf create mode 100644 crates/mirsam-ooxml/tests/fonts.rs diff --git a/AGENTS.md b/AGENTS.md index 4b87134..e917921 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -22,6 +22,7 @@ shared code. See [`CREDITS.md`](CREDITS.md). ```bash mirsam audit deck.pptx --format json # full diagnostic model mirsam audit deck.pptx --strict # warnings block too +mirsam audit deck.pptx --fonts # also check the fonts installed here mirsam repair deck.pptx fixed.pptx --format json # repaired copy, both audits mirsam explain "" # reproduce a defect with no document mirsam rules --format json # every check and its id @@ -92,6 +93,40 @@ agrees with the text, reported as an absent one where it contradicts it, with `evidence.inherited_from` naming the style. The repair still writes to the table, never to the style. +### The two font checks, which are off by default + +`font-coverage` and `shaping-broken` are the only rules that ask a question +about **the machine running mirsam** rather than about the document, so they +run only when `--fonts` asks for them. Every report says which of the two +audits it is — `fonts NOT RUN` in the human output, `"fonts": {"checked": +false}` in JSON — and you must read that before concluding anything from their +silence. A check that did not run is `NOT RUN`, never a pass. + +`--font-dir ` (repeatable) searches those directories instead of the +platform's, which is what makes the result reproducible: a fixed directory +gives a fixed answer on any machine. + +**The claim runs one way only.** A font that is *here* and cannot draw the +text will not draw it anywhere — a `cmap` and a `GSUB` travel with the font — +which is what makes a finding worth reporting. A font that is here and draws +the text perfectly proves **nothing** about the reader's machine. Do not turn +silence from these two rules into "the Arabic will render". + +They are two defects, not one, and the advice differs: + +- `font-coverage` — the font has no glyph for some of the Arabic; it renders + as empty boxes. `evidence.offenders` is the exact list, by Unicode name. + An error when the font answers for *none* of the text, a warning when it + misses part of it. +- `shaping-broken` — the font has every letter and no shaping tables; the + Arabic renders as disconnected letters. Reported only on the aggregate — + no joins produced where at least four were required and the font had the + glyphs — because one letter drawn standalone is not a defect (invariant 7). + +Neither is `fixable`. The repair for both is a different typeface, and which +one is an authoring decision; unlike `complex-font-missing`, which fills an +empty slot, these would be overwriting a font the author chose. + ### Repairing `repair ` never modifies `` and refuses ` == ` under @@ -127,9 +162,13 @@ Read [`docs/ARCHITECTURE.md`](docs/ARCHITECTURE.md) first, then [`docs/PLAN.md`](docs/PLAN.md) for the ordered work items. **Adding a rule** — implement `Rule` in `crates/mirsam-core/src/rules/`, -register it in `Engine::with_options`. Nothing else changes. If its repair +register it in `Engine::build`. Nothing else changes. If its repair needs a choice the text cannot make, the choice is a `RepairOptions` field, -not a CLI flag with logic behind it. +not a CLI flag with logic behind it. A rule that needs to know something about +the *machine* rather than the document takes a port instead, the way +`rules/font.rs` takes a `FontSource` — `RepairOptions` is for authoring +preferences, and putting a source there would make an audit's result depend on +which computer ran it without saying so anywhere. **Adding a format** — new crate implementing `DocumentReader`; add `DocumentWriter` only if the format can be faithfully edited in place. @@ -137,8 +176,11 @@ not a CLI flag with logic behind it. **Resolving a font** — `mirsam-core` never opens one. `ports::FontSource` takes a family name and answers with bytes, `mirsam-fonts` implements it over the platform's font directories, and `shape` and `coverage` work on what comes -back. A source answering `None` means this machine has no such font, which is -a reportable state and not an error. Note which way the claim runs: a font +back; `Engine::with_fonts` is what arms the two rules over it, and +`Engine::with_options` leaves them registered and unrun. A source answering +`None` means this machine has no such font, which those rules treat as +silence: a fact about the computer, not a defect in the deck. Note which way +the claim runs: a font that is *here* and cannot draw the text will not draw it anywhere, because a `cmap` travels with the font — but a font that is here proves nothing about the reader's machine, and no report may suggest otherwise. @@ -169,7 +211,7 @@ A report for a format the tool reads but cannot write holds the refusal `repair` gave and the exit code it used, in place of a repair report — so the day a writer lands, it shows up as a diff on a real document. -**The shaping fixtures are fonts, and they are generated too.** The four +**The shaping fixtures are fonts, and they are generated too.** The five under `crates/mirsam-core/tests/fonts/` are written byte by byte by `scripts/make-shaping-fixture.py` — no `fontTools`, for the reason the document generators avoid `python-pptx` — and regenerate with `make fonts`. @@ -182,7 +224,11 @@ fixture: it claims the family `Mirsam Joining` a second time, sorts before `joining.ttf` and shapes nothing, so a font source answering a family name with whichever file it met first fails a test rather than a user's deck. That is macOS's Arial in miniature — eleven files state that family, and only -`Arial.ttf` has any Arabic. +`Arial.ttf` has any Arabic. The fifth, `latin.ttf`, is the other defect +entirely: printable ASCII, no Arabic at all, no `GSUB`, which is Helvetica +under a complex-script slot. It is what `font-coverage` is proved against, and +shaping through it must stay silent — a letter a font has no glyph for says +nothing about whether the font can join. **The Unicode name table is generated as well.** `charname.rs` comes from `scripts/make-char-names.py`, which reads the UCD Python itself ships — no diff --git a/CHANGELOG.md b/CHANGELOG.md index 34e1658..3c2dfd4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,42 @@ Working towards byte-preserving `repair` for PPTX. See ### Added +- **`font-coverage` and `shaping-broken`**, the two rules that turn 4.1's and + 4.2's facts into findings. They are two defects, not one, and the advice + differs: a font with no glyph for the Arabic renders empty boxes, and a font + with every letter and no shaping tables renders a row of disconnected + letters. `font-coverage` names the exact characters that will not render — + `U+067E ARABIC LETTER PEH` — as an error when the font answers for none of + the text and a warning when it misses part of it. Neither is `fixable`: the + repair for both is a different typeface, and unlike `complex-font-missing`, + which fills an *empty* slot, these would be overwriting a font the author + chose. +- **`mirsam audit --fonts`**, and it is opt-in on purpose. These are the only + rules that ask about the machine rather than the document, and an audit that + resolved fonts without being asked would report differently on a laptop and + on a CI runner looking at one file. Every report now says which of the two + audits it is — `fonts NOT RUN` for a human, `"fonts": {"checked": false}` for + an agent — so their silence is never mistaken for a pass. `--font-dir ` + searches given directories instead of the platform's, which is what makes the + result reproducible. The claim runs one way only: a font that is *here* and + cannot draw the text will not draw it anywhere, and a font that is here and + draws it proves nothing about the reader's machine. +- The shaping threshold ADR 0008 left open is **four required joins**, counted + over letters the font actually has. A final form exists only because the + letter before it joined forwards, so every final has a companion initial or + medial — the dual-joining letters a font like Arial does shape. Two joins are + therefore one such letter, which is exactly the design choice the ADR forbids + concluding from, and is the whole of a two-letter word. Unmapped letters are + excluded so that a Latin-only font cannot produce a `shaping-broken` finding + beside its `font-coverage` one: the tool would be arguing with itself about + one paragraph. +- **`latin.ttf`**, a fifth generated fixture font: printable ASCII, no Arabic, + no `GSUB`. Helvetica under a complex-script slot, reduced to its principle, + so PLAN §4.3's acceptance is a committed test rather than a manual check. + Validated beyond the fixtures against macOS — `Helvetica` and `Comic Sans MS` + report every Arabic character of every paragraph, while `Times New Roman`, + `Geeza Pro`, `Mishafi`, `Al Nile`, `Baghdad` and `Arial` report nothing at + all. - **Font coverage.** `mirsam-core::coverage` asks the question underneath shaping — does the font have the letter at all? A font with no shaping tables renders Arabic as disconnected letters; a font with no Arabic renders diff --git a/Cargo.lock b/Cargo.lock index 2e366e5..03f60c6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -220,6 +220,7 @@ dependencies = [ "anyhow", "clap", "mirsam-core", + "mirsam-fonts", "mirsam-ooxml", "serde_json", "zip", diff --git a/README.md b/README.md index aa3443d..b2ae801 100644 --- a/README.md +++ b/README.md @@ -65,6 +65,7 @@ prebuilt binaries for Linux, macOS and Windows, are milestone M7 — see mirsam audit deck.pptx # inspect; exit 1 if blocking mirsam audit deck.pptx --strict # warnings block too mirsam audit deck.pptx --format json # for agents and CI +mirsam audit deck.pptx --fonts # also check the fonts installed here mirsam repair deck.pptx fixed.pptx # write a repaired copy, then audit it mirsam explain "" # reproduce a defect without a document mirsam rules # every check, with its id @@ -118,6 +119,27 @@ written from `ar-SA`; `--force` replaces an existing output. Repairing a repaired file is a no-op that reproduces it byte for byte. +### Checking the fonts + +`--fonts` adds two checks the rest of the tool cannot make from the file +alone. Each paragraph's complex-script typeface is resolved against the fonts +installed on this machine, and the Arabic is put through a real OpenType +shaper: `font-coverage` reports the exact characters the font has no glyph for +— they render as empty boxes — and `shaping-broken` reports a font that has +every letter and no shaping tables, which renders Arabic as a row of +disconnected letters. `--font-dir ` searches given directories instead of +the platform's, which is how you make the answer reproducible. + +It is opt-in because it reads the machine rather than the document, and an +audit that resolved fonts unasked would report differently on two computers +looking at one file. Every report says which of the two audits it is — +`fonts NOT RUN`, or `"fonts": {"checked": false}` — so their silence is never +mistaken for a pass. + +Read the result one way only. A font that is *here* and cannot draw the text +will not draw it anywhere; a font that is here and draws it perfectly proves +nothing about anyone else's machine. + ### With an AI agent `mirsam` is designed to be called by agents. `--format json` emits the full diff --git a/crates/mirsam-cli/Cargo.toml b/crates/mirsam-cli/Cargo.toml index a898226..0a4255a 100644 --- a/crates/mirsam-cli/Cargo.toml +++ b/crates/mirsam-cli/Cargo.toml @@ -15,6 +15,7 @@ path = "src/main.rs" [dependencies] mirsam-core.workspace = true mirsam-ooxml.workspace = true +mirsam-fonts.workspace = true clap.workspace = true serde_json.workspace = true anyhow.workspace = true diff --git a/crates/mirsam-cli/src/main.rs b/crates/mirsam-cli/src/main.rs index c5c3b93..03b9bc3 100644 --- a/crates/mirsam-cli/src/main.rs +++ b/crates/mirsam-cli/src/main.rs @@ -8,14 +8,16 @@ mod render; use anyhow::{Context, Result, bail}; -use clap::{Parser, Subcommand, ValueEnum}; +use clap::{Args, Parser, Subcommand, ValueEnum}; use mirsam_core::error::Error as CoreError; use mirsam_core::rules::{DEFAULT_LOCALE, is_arabic_tag}; -use mirsam_core::{DocumentReader, DocumentWriter, Engine, Repair, RepairOptions}; +use mirsam_core::{DocumentReader, DocumentWriter, Engine, FontSource, Repair, RepairOptions}; +use mirsam_fonts::SystemFonts; use mirsam_ooxml::{DocxDocument, PptxDocument}; use std::fmt; use std::path::{Path, PathBuf}; use std::process::ExitCode; +use std::sync::Arc; /// Exit codes, stable across releases so CI can branch on them. mod exit { @@ -51,6 +53,8 @@ enum Command { /// Treat warnings as blocking. #[arg(long)] strict: bool, + #[command(flatten)] + fonts: FontFlags, #[arg(long, value_enum, default_value_t = Format::Text)] format: Format, }, @@ -89,6 +93,8 @@ enum Command { /// Treat warnings remaining after the repair as blocking. #[arg(long)] strict: bool, + #[command(flatten)] + fonts: FontFlags, #[arg(long, value_enum, default_value_t = Format::Text)] format: Format, }, @@ -113,6 +119,65 @@ enum Format { Json, } +/// Whether to shape the document's Arabic through the fonts this machine has. +/// +/// Off by default, and that is a position rather than caution. Every other +/// check in this tool reads the document and would give the same answer on any +/// computer; `font-coverage` and `shaping-broken` read the *machine*, and an +/// audit whose result depended on which fonts happened to be installed on the +/// runner would be one nobody could reproduce. So the caller asks, and the +/// report says which of the two audits it is — a check that did not run is +/// `NOT RUN`, never an implied pass. +/// +/// Note which way the claim runs when they are on. A font that is *here* and +/// cannot draw the text will not draw it anywhere, because a `cmap` and a +/// `GSUB` travel with the font. A font that is here and draws it perfectly +/// proves nothing about the reader's machine, and silence is not a promise +/// that the document will render. +#[derive(Args, Clone, Debug)] +struct FontFlags { + /// Resolve each paragraph's complex-script typeface against the fonts + /// installed here, and report Arabic the resolved font cannot draw or + /// cannot join. Machine-dependent: see --font-dir. + #[arg(long = "fonts")] + enabled: bool, + /// Search these directories instead of the platform's font directories. + /// Implies --fonts, and is what makes the check reproducible: a fixed + /// directory gives a fixed answer on any machine. + #[arg(long = "font-dir", value_name = "DIR")] + dirs: Vec, +} + +impl FontFlags { + fn requested(&self) -> bool { + self.enabled || !self.dirs.is_empty() + } + + /// The source to arm the font rules with, or `None` to leave them unrun. + /// + /// Built once per invocation and shared: `SystemFonts` indexes lazily and + /// keeps what it found, so a document of forty paragraphs walks the font + /// directories once rather than forty times. + fn source(&self) -> Option> { + if !self.requested() { + return None; + } + Some(Arc::new(if self.dirs.is_empty() { + SystemFonts::new() + } else { + SystemFonts::in_dirs(self.dirs.clone()) + })) + } +} + +/// The engine, with the font rules armed only if the caller asked for them. +fn engine(options: &RepairOptions, fonts: &FontFlags) -> Engine { + match fonts.source() { + Some(source) => Engine::with_fonts(options, source), + None => Engine::with_options(options), + } +} + /// `--lang` must be a tag the `language-missing` rule accepts, or the repair /// would write a value the re-audit immediately reports again. fn arabic_tag(value: &str) -> std::result::Result { @@ -265,6 +330,7 @@ fn run() -> Result { Command::Audit { file, strict, + fonts, format, } => { let mut document = open(&file)?; @@ -272,13 +338,14 @@ fn run() -> Result { .scan() .with_context(|| format!("reading {}", file.display()))?; - let report = Engine::with_default_rules().audit(&units); + let report = engine(&RepairOptions::default(), &fonts).audit(&units); let blocking = report.is_blocking(strict); render::report( &file, document.format(), &report, strict, + fonts.requested(), format == Format::Json, ); @@ -294,6 +361,7 @@ fn run() -> Result { align, force, strict, + fonts, format, } => { // Refuse before reading anything. The writer refuses the first case @@ -316,7 +384,7 @@ fn run() -> Result { convert_bullets, align, }; - let engine = Engine::with_options(&options); + let engine = engine(&options, &fonts); let mut document = open_for_repair(&input)?; let units = document @@ -369,6 +437,7 @@ fn run() -> Result { before: &before, after: &after, strict, + fonts_checked: fonts.requested(), }, format == Format::Json, ); diff --git a/crates/mirsam-cli/src/render.rs b/crates/mirsam-cli/src/render.rs index 877fb79..7503eb0 100644 --- a/crates/mirsam-cli/src/render.rs +++ b/crates/mirsam-cli/src/render.rs @@ -81,6 +81,32 @@ fn escape(text: &str) -> String { .collect() } +/// Whether the two font checks ran, in the report itself. +/// +/// Standing rule 4: a check that did not run is `NOT RUN`, never an implied +/// pass. `font-coverage` and `shaping-broken` are the only rules that read the +/// machine rather than the document, so they are opt-in — and a report that +/// did not say which of the two audits it is would let their silence be +/// mistaken for a clean result. +fn fonts(checked: bool) -> serde_json::Value { + json!({ "checked": checked }) +} + +/// The same, for a human. +fn fonts_line(checked: bool) { + println!( + "fonts {}", + if checked { + "checked against this machine's — a font that is here and cannot draw \ + the text will not draw it anywhere; one that can proves nothing about \ + the reader's machine" + } else { + "NOT RUN — pass --fonts to resolve each typeface here and report Arabic \ + it cannot draw or cannot join" + } + ); +} + /// The counts an agent branches on, in the same shape for every report. fn summary(report: &Report) -> serde_json::Value { json!({ @@ -132,13 +158,21 @@ fn verdict(report: &Report, strict: bool) { ); } -pub fn report(path: &Path, format: &str, report: &Report, strict: bool, as_json: bool) { +pub fn report( + path: &Path, + format: &str, + report: &Report, + strict: bool, + fonts_checked: bool, + as_json: bool, +) { if as_json { let payload = json!({ "file": path.display().to_string(), "format": format, "strict": strict, "blocking": report.is_blocking(strict), + "fonts": fonts(fonts_checked), "summary": summary(report), "diagnostics": report.diagnostics, }); @@ -154,6 +188,7 @@ pub fn report(path: &Path, format: &str, report: &Report, strict: bool, as_json: "units {} | arabic {} | mixed {}", report.units_scanned, report.arabic_units, report.mixed_units ); + fonts_line(fonts_checked); println!(); findings(report); verdict(report, strict); @@ -176,6 +211,9 @@ pub struct Repaired<'a> { /// The audit of the output, re-read from disk. pub after: &'a Report, pub strict: bool, + /// Whether the font checks ran over either audit. Reported for the same + /// reason `audit` reports it: their silence is not a pass. + pub fonts_checked: bool, } /// Where a unit is, in the words `audit` uses; its id if it is not known. @@ -206,6 +244,7 @@ pub fn repair(r: &Repaired<'_>, as_json: bool) { "format": r.format, "strict": r.strict, "blocking": r.after.is_blocking(r.strict), + "fonts": fonts(r.fonts_checked), "options": r.options, "repairs": { "applied": r.applied, @@ -245,6 +284,7 @@ pub fn repair(r: &Repaired<'_>, as_json: bool) { yes_no(r.options.convert_bullets), yes_no(r.options.align), ); + fonts_line(r.fonts_checked); println!(); println!("applied {} repair(s)", r.applied.len()); diff --git a/crates/mirsam-cli/tests/cli.rs b/crates/mirsam-cli/tests/cli.rs index 210dc16..587939f 100644 --- a/crates/mirsam-cli/tests/cli.rs +++ b/crates/mirsam-cli/tests/cli.rs @@ -248,10 +248,14 @@ fn rules_json_lists_every_rule_with_an_id() { assert_eq!(out.code, exit::OK); let value: serde_json::Value = serde_json::from_str(&out.stdout).expect("valid JSON"); let rules = value.as_array().expect("an array of rules"); - assert_eq!(rules.len(), 10, "expected the ten documented rules"); + assert_eq!(rules.len(), 12, "expected the twelve documented rules"); for rule in rules { assert!(rule.get("id").is_some(), "a rule with no id: {rule}"); } + // The two font rules are listed whether or not this invocation could have + // run them: `mirsam rules` describes the rule set, not the last audit. + let ids: Vec<&str> = rules.iter().map(|r| r["id"].as_str().unwrap()).collect(); + assert!(ids.contains(&"font-coverage") && ids.contains(&"shaping-broken")); } #[test] @@ -792,3 +796,112 @@ fn an_unknown_extension_names_every_format_that_can_be_read() { out.stderr ); } + +// -------------------------------------------------------------- font checks + +/// The shaping fixtures, a directory of four fonts that is the same on every +/// machine — which is what makes a font check reproducible at all. +fn font_dir() -> String { + Path::new(env!("CARGO_MANIFEST_DIR")) + .join("../mirsam-core/tests/fonts") + .canonicalize() + .expect("the fixture fonts are committed") + .to_string_lossy() + .into_owned() +} + +#[test] +fn the_font_checks_are_reported_as_not_run_unless_asked_for() { + // Standing rule 4. `font-coverage` and `shaping-broken` are the only rules + // that read the machine rather than the document, so they are opt-in — and + // a report that did not say so would let their silence be mistaken for a + // pass. Both output formats have to say it: an agent reading JSON must + // never have to scrape the human text for it. + let out = run(&["audit", &fixture("torture.pptx")]); + assert!( + out.stdout.contains("fonts NOT RUN"), + "stdout:\n{}", + out.stdout + ); + + let out = run(&["audit", &fixture("torture.pptx"), "--format", "json"]); + assert_eq!( + parse_json(&out)["fonts"]["checked"], + serde_json::json!(false) + ); + + // And neither rule can have reported anything. + let ids = rule_ids(&parse_json(&out)["diagnostics"]); + assert!( + !ids.iter() + .any(|id| id == "font-coverage" || id == "shaping-broken"), + "{ids:?}" + ); +} + +#[test] +fn asking_for_them_says_so_in_both_formats() { + // A fixed directory rather than the platform's, so this asserts what the + // binary did and not what fonts the developer happens to have installed. + let dir = font_dir(); + let out = run(&[ + "audit", + &fixture("torture.pptx"), + "--font-dir", + &dir, + "--format", + "json", + ]); + assert_eq!( + parse_json(&out)["fonts"]["checked"], + serde_json::json!(true) + ); + + let out = run(&["audit", &fixture("torture.pptx"), "--font-dir", &dir]); + assert!( + out.stdout.contains("fonts checked"), + "stdout:\n{}", + out.stdout + ); +} + +#[test] +fn a_typeface_this_machine_does_not_have_changes_nothing() { + // The deck names Calibri and Dubai; the fixture directory has neither, so + // the source answers `None` for every one of them. That is the tool losing + // the ability to say what the reader will see — a fact about the machine, + // not a defect in the deck — and it must not move the exit code. + let plain = run(&["audit", &fixture("torture.pptx"), "--format", "json"]); + let checked = run(&[ + "audit", + &fixture("torture.pptx"), + "--font-dir", + &font_dir(), + "--format", + "json", + ]); + + assert_eq!(plain.code, checked.code); + assert_eq!( + rule_ids(&parse_json(&plain)["diagnostics"]), + rule_ids(&parse_json(&checked)["diagnostics"]), + ); +} + +#[test] +fn repair_reports_the_font_checks_too() { + // The repair report holds two audits, and the same caveat applies to both. + let scratch = Scratch::new("fonts-repair"); + let output = scratch.path("repaired.pptx"); + let out = run(&[ + "repair", + &fixture("broken-arabic.pptx"), + &output, + "--format", + "json", + ]); + assert_eq!( + parse_json(&out)["fonts"]["checked"], + serde_json::json!(false) + ); +} diff --git a/crates/mirsam-core/src/lib.rs b/crates/mirsam-core/src/lib.rs index 7851195..d18974a 100644 --- a/crates/mirsam-core/src/lib.rs +++ b/crates/mirsam-core/src/lib.rs @@ -25,6 +25,13 @@ //! finding the file is a [`ports::FontSource`]'s business, and this crate //! still opens nothing. //! +//! `font-coverage` and `shaping-broken` are what those two become as findings, +//! and they are the only checks here that ask about the *machine* rather than +//! the document. So [`Engine::with_options`] leaves them registered and unrun, +//! [`Engine::with_fonts`] arms them against a source, and a caller that does +//! not arm them must say so in its report: silence from a check that never ran +//! is not a pass. +//! //! ``` //! use mirsam_core::{Engine, TextUnit, Direction, Resolved, Properties}; //! diff --git a/crates/mirsam-core/src/ports.rs b/crates/mirsam-core/src/ports.rs index 9c2d530..d4485be 100644 --- a/crates/mirsam-core/src/ports.rs +++ b/crates/mirsam-core/src/ports.rs @@ -101,7 +101,11 @@ impl FontFile { /// a real and reportable state — text set in a font nobody has renders in /// whatever the application substitutes, and the tool can no longer say what /// the reader will see. It is not an error, and must not be reported as one. -pub trait FontSource { +/// +/// `Send + Sync` because a [`crate::Rule`] holds one: the engine is a set of +/// rules that must stay shareable across threads, and a source that could not +/// be shared would decide that for it. +pub trait FontSource: Send + Sync { /// The file that answers to the typeface `family` names. /// /// Matching is the implementation's business, and is expected to be at diff --git a/crates/mirsam-core/src/rules/font.rs b/crates/mirsam-core/src/rules/font.rs new file mode 100644 index 0000000..5e65964 --- /dev/null +++ b/crates/mirsam-core/src/rules/font.rs @@ -0,0 +1,375 @@ +//! The two rules that judge the font a paragraph's Arabic will be drawn with. +//! +//! [`crate::shape`] and [`crate::coverage`] report facts and refuse to draw a +//! conclusion from them; PLAN §4.3 is where the conclusions are drawn. There +//! are exactly two, and they are different defects with different advice: +//! +//! - **`font-coverage`** — the font has no glyph for some of the Arabic. It +//! renders as empty boxes, and the only repair is a different font. +//! - **`shaping-broken`** — the font has every letter and no shaping tables. +//! It renders as a row of disconnected letters, and the only repair is, +//! again, a different font, but for a completely different reason. +//! +//! ## These are the only rules that ask about the machine +//! +//! Every other rule in this crate reads a document and reasons about it. +//! These two need a font file, so they need a [`FontSource`] — and a source +//! is a fact about the computer the tool is running on, not about the +//! document. Three consequences follow, and all three are deliberate. +//! +//! *No source, no findings.* A rule built without one checks nothing and says +//! nothing. That is not a silent failure as long as the caller says so, which +//! is standing rule 4: a check that did not run is `NOT RUN`, never an +//! implied pass. `mirsam` reports the font checks as unrun unless `--fonts` +//! asked for them, and the reason they are opt-in at all is this one — an +//! audit whose result depended on which fonts happened to be installed on the +//! machine that ran it would be an audit nobody could reproduce. +//! +//! *A font this machine does not have is silence, not a finding.* The source +//! answering `None` means the tool can no longer say what the reader will +//! see, which is a fact about the machine and not a defect in the deck. +//! Reporting it would fire on every CI runner with no fonts installed. +//! +//! *The claim runs one way only.* A font that is *here* and cannot draw the +//! text will not draw it anywhere, because a `cmap` and a `GSUB` travel with +//! the font — that is why a finding is worth making at all. A font that is +//! here and draws the text perfectly proves nothing about the reader's +//! machine, and neither rule says otherwise: silence here is not a claim that +//! the document will render. +//! +//! ## Neither one is fixable +//! +//! The repair for both is "use a different typeface", and which typeface is +//! an authoring decision the text cannot supply — the same reason +//! [`super::typography::ComplexFontMissing`] proposes nothing until `--font` +//! chooses one. It is worse here: that rule fills an *empty* slot, while +//! these two would be overwriting a font the author put there. So they +//! report, and the author chooses. + +use std::sync::Arc; + +use super::Rule; +use crate::charname; +use crate::coverage::{Coverage, coverage}; +use crate::diagnostic::{Diagnostic, Evidence, RuleId, Severity}; +use crate::ports::{FontFile, FontSource}; +use crate::script; +use crate::shape::{Outcome, Shaping, shape}; +use crate::text::TextUnit; + +/// The font a unit's Arabic will actually be drawn with, on this machine. +/// +/// `None` — and so silence from both rules — at every step that cannot be +/// taken: no source was supplied, the text has no Arabic in it, no +/// complex-script font is named, this machine has no such family, or the +/// file that answered is not one the shaper can read. Only the second is +/// about the document, and `complex-font-missing` already reports it. +fn resolve(fonts: Option<&Arc>, unit: &TextUnit) -> Option { + let fonts = fonts?; + if !script::has_arabic(&unit.text) { + return None; + } + let family = unit.props.complex_font.effective()?; + // An error from the source is the machine failing to answer, not the + // document failing a check. Nothing can be concluded either way. + fonts.load(family).ok().flatten() +} + +/// Where the typeface came from, when the unit did not name it itself. +/// +/// A master writes `+mn-cs` and the theme holds the typeface, so a reviewer +/// told only that "Calibri has no Arabic" has nowhere to go. Invariant 6. +fn inherited_from(unit: &TextUnit) -> Option { + unit.props + .complex_font + .origin() + .map(std::string::ToString::to_string) +} + +/// How the finding names the font that answered: the family the *file* states +/// and the path it was read from. +/// +/// Both, because neither alone is checkable. A machine with no `Calibri` may +/// answer with something else entirely, so the requested name would describe +/// a font nobody has; and the family alone leaves a reviewer hunting for +/// which of eleven files it was. +fn answered_by(file: &FontFile) -> String { + format!("{} ({})", file.family, file.path) +} + +/// The Arabic a font resolved for a paragraph has no glyph for. +/// +/// The empty-boxes defect. A deck that points its complex-script slot at a +/// Latin font — Helvetica, Comic Sans, Calibri before its Arabic was added — +/// stores perfectly correct Unicode and renders nothing at all. +pub struct FontCoverage { + /// Where a typeface name is resolved to bytes. `None` — the default — + /// checks nothing; see the module documentation for why that is a + /// position rather than an omission. + pub fonts: Option>, +} + +impl Rule for FontCoverage { + fn id(&self) -> RuleId { + RuleId("font-coverage") + } + + fn description(&self) -> &'static str { + "The font resolved for a paragraph's Arabic has no glyph for some of it" + } + + fn check(&self, unit: &TextUnit) -> Vec { + let Some(file) = resolve(self.fonts.as_ref(), unit) else { + return Vec::new(); + }; + let Some(font) = file.font() else { + return Vec::new(); + }; + let report = coverage(&font, &unit.text); + if report.is_complete() { + return Vec::new(); + } + + vec![ + Diagnostic::new( + self.id(), + severity(&report), + &unit.id, + &unit.location, + message(&file, &report), + ) + .with_evidence(Evidence { + logical: Some(unit.text.clone()), + // The exact characters that will not render, by name, which is + // the whole of what this rule promises. + offenders: report.missing.iter().map(ToString::to_string).collect(), + inherited_from: inherited_from(unit), + ..Default::default() + }), + ] + } +} + +/// How much of the text goes blank decides how loud the finding is. +/// +/// A font that answers for *none* of the Arabic was not the font this text +/// needed, and that is an error: the paragraph renders as empty boxes and +/// there is nothing to argue about. A font missing some of it may be an +/// otherwise correct pairing meeting one unusual letter — Mishafi has no +/// Persian peh — which is still wrong for the characters it hits and is still +/// worth an author's attention, but is not the same claim. +fn severity(report: &Coverage) -> Severity { + if report.covers_nothing() { + Severity::Error + } else { + Severity::Warning + } +} + +fn message(file: &FontFile, report: &Coverage) -> String { + let font = answered_by(file); + if report.covers_nothing() { + format!( + "{font} has no glyph for any of the {} Arabic characters here; \ + the text will render as empty boxes", + report.checked + ) + } else { + format!( + "{font} has no glyph for {} of the {} Arabic characters here; \ + those will render as empty boxes", + report.missing_occurrences(), + report.checked + ) + } +} + +/// Arabic drawn by a font with no shaping tables. +/// +/// The defect M4 exists for, and the first in this tool that no amount of +/// reading XML could find: the text is correct Unicode, correctly directed, +/// correctly aligned, every letter present in the font, and it renders as a +/// row of disconnected letters. +pub struct ShapingBroken { + /// Where a typeface name is resolved to bytes. `None` checks nothing. + pub fonts: Option>, +} + +/// How many letters must have been *required* to join, and been present in +/// the font, before "none of them did" means anything. +/// +/// ADR 0008 leaves the number to this rule and states what it has to survive: +/// one letter drawn standalone is not a defect, because a design may share +/// one glyph between a letter's standalone and final forms and several do — +/// macOS's Arial among them. So the evidence is the aggregate, and the +/// threshold is what stops a scrap of text from being called one. +/// +/// Four, and the arithmetic is why. A letter is required to take a final form +/// only because the letter before it joined forwards, so every final has a +/// companion initial or medial, and initials and medials are dual-joining +/// letters — the ones a font like Arial does shape. Two observable joins are +/// therefore *one* such letter, which is exactly the design choice the ADR +/// forbids concluding from. Four are two of them, independently, both silent. +/// No font that shapes Arabic at all looks like that. +/// +/// A two-letter word offers two, and so is passed over rather than reported +/// on, which is the case ADR 0008 names. +const MIN_OBSERVABLE_JOINS: usize = 4; + +/// Letters that were required to join *and* that the font actually has. +/// +/// An unmapped letter is not evidence about shaping in either direction: a +/// font cannot join a glyph it does not have, and concluding "no shaping +/// tables" from a font that simply has no Arabic would put a second finding, +/// with the same repair and a different explanation, on text `font-coverage` +/// has already reported. Excluding them is what keeps the two rules from +/// arguing with each other about one paragraph. +fn observable_joins(shaping: &Shaping) -> usize { + shaping + .letters + .iter() + .filter(|l| l.required.is_joined() && l.outcome != Outcome::Unmapped) + .count() +} + +impl Rule for ShapingBroken { + fn id(&self) -> RuleId { + RuleId("shaping-broken") + } + + fn description(&self) -> &'static str { + "The font resolved for a paragraph's Arabic produces no joining forms" + } + + fn check(&self, unit: &TextUnit) -> Vec { + let Some(file) = resolve(self.fonts.as_ref(), unit) else { + return Vec::new(); + }; + let Some(font) = file.font() else { + return Vec::new(); + }; + let shaping = shape(&font, &unit.text); + + let observable = observable_joins(&shaping); + if observable < MIN_OBSERVABLE_JOINS || shaping.joins_produced() > 0 { + return Vec::new(); + } + + // The letters the reader will see standing alone, named and + // deduplicated in first-appearance order. Every one of them is in + // the font, so this is a list of shapes that exist and were not used. + let mut offenders: Vec = Vec::new(); + for letter in shaping.drawn_standalone() { + let Some(name) = charname::name(letter.ch) else { + continue; + }; + let entry = format!("U+{:04X} {name}", letter.ch as u32); + if !offenders.contains(&entry) { + offenders.push(entry); + } + } + + vec![ + Diagnostic::new( + self.id(), + Severity::Error, + &unit.id, + &unit.location, + format!( + "{} produced no joining form for any of the {observable} letters that \ + require one; the Arabic will render as disconnected letters", + answered_by(&file) + ), + ) + .with_evidence(Evidence { + logical: Some(unit.text.clone()), + offenders, + inherited_from: inherited_from(unit), + ..Default::default() + }), + ] + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::joining::JoiningForm; + use crate::shape::ShapedLetter; + + /// A shaping result with `joined` letters required to join, `mapped` of + /// them present in the font. The letter and its outcome do not matter + /// here; the counting does. + fn shaping(joined: usize, mapped: usize) -> Shaping { + let letters = (0..joined) + .map(|i| ShapedLetter { + offset: i, + ch: 'ب', + required: JoiningForm::Medial, + outcome: if i < mapped { + Outcome::Standalone + } else { + Outcome::Unmapped + }, + glyphs: Vec::new(), + }) + .collect(); + Shaping { letters } + } + + #[test] + fn a_letter_the_font_has_no_glyph_for_is_not_shaping_evidence() { + // Six letters required a join and the font has none of them: this is + // `font-coverage`'s paragraph, and `shaping-broken` must not also + // claim it. + assert_eq!(observable_joins(&shaping(6, 0)), 0); + assert_eq!(observable_joins(&shaping(6, 6)), 6); + assert_eq!(observable_joins(&shaping(6, 2)), 2); + } + + #[test] + fn the_threshold_is_above_a_two_letter_word() { + // ADR 0008: a container holding one two-letter word gives the + // aggregate nothing to work with. Two joins is one dual-joining + // letter, which is the design choice the ADR forbids concluding from. + const { assert!(MIN_OBSERVABLE_JOINS > 2) }; + } + + #[test] + fn a_coverage_severity_turns_on_how_much_of_the_text_goes_blank() { + let nothing = Coverage { + checked: 5, + missing: vec![crate::coverage::MissingChar { + ch: 'ب', + codepoint: 0x0628, + name: charname::name('ب').unwrap(), + first_offset: 0, + occurrences: 5, + }], + }; + assert_eq!(severity(¬hing), Severity::Error); + + let some = Coverage { + checked: 5, + missing: vec![crate::coverage::MissingChar { + ch: 'ب', + codepoint: 0x0628, + name: charname::name('ب').unwrap(), + first_offset: 0, + occurrences: 1, + }], + }; + assert_eq!(severity(&some), Severity::Warning); + } + + #[test] + fn without_a_source_neither_rule_resolves_anything() { + let unit = TextUnit::new("u1", "مرحبا").with_props(crate::text::Properties { + complex_font: crate::text::Resolved::Explicit("Mirsam Joining".into()), + ..Default::default() + }); + assert!(resolve(None, &unit).is_none()); + assert!(FontCoverage { fonts: None }.check(&unit).is_empty()); + assert!(ShapingBroken { fonts: None }.check(&unit).is_empty()); + } +} diff --git a/crates/mirsam-core/src/rules/mod.rs b/crates/mirsam-core/src/rules/mod.rs index 1f9d16f..e00d667 100644 --- a/crates/mirsam-core/src/rules/mod.rs +++ b/crates/mirsam-core/src/rules/mod.rs @@ -6,13 +6,17 @@ //! independently of one another. mod direction; +mod font; mod typography; mod unicode; pub use typography::{DEFAULT_LOCALE, is_arabic_tag}; +use std::sync::Arc; + use crate::diagnostic::{Diagnostic, Report, RuleId}; use crate::fix::{Fix, Repair}; +use crate::ports::FontSource; use crate::script; use crate::text::{TextUnit, UnitKind}; @@ -91,7 +95,31 @@ impl Engine { /// What is *reported* does not depend on the options: the same defects /// come back whatever the caller intends to do about them. Only the /// proposed fixes vary, and with them `Diagnostic::fixable`. + /// + /// The two font rules are registered and check nothing, because no font + /// source was supplied. See [`with_fonts`](Self::with_fonts): they are + /// listed either way so that `mirsam rules` describes the whole rule set + /// rather than the part this call happened to arm. pub fn with_options(options: &RepairOptions) -> Self { + Self::build(options, None) + } + + /// The same rule set with the font checks armed against `fonts`. + /// + /// `font-coverage` and `shaping-broken` are the only rules that ask a + /// question about the machine rather than about the document, which is + /// why the source is a separate argument and not a [`RepairOptions`] + /// field: it is not an authoring preference, and an audit that resolved + /// fonts without being asked would report differently on two machines + /// looking at one document. + /// + /// A caller that does not arm them **must say so in its report** + /// (standing rule 4). Silence from a check that never ran is not a pass. + pub fn with_fonts(options: &RepairOptions, fonts: Arc) -> Self { + Self::build(options, Some(fonts)) + } + + fn build(options: &RepairOptions, fonts: Option>) -> Self { Self { rules: vec![ Box::new(unicode::BidiControls), @@ -112,6 +140,10 @@ impl Engine { Box::new(typography::LiteralBullet { convert: options.convert_bullets, }), + Box::new(font::FontCoverage { + fonts: fonts.clone(), + }), + Box::new(font::ShapingBroken { fonts }), ], } } diff --git a/crates/mirsam-core/tests/font_rules.rs b/crates/mirsam-core/tests/font_rules.rs new file mode 100644 index 0000000..2d77b2b --- /dev/null +++ b/crates/mirsam-core/tests/font_rules.rs @@ -0,0 +1,304 @@ +//! The two font rules (PLAN §4.3), through the engine that arms them. +//! +//! These run against the hand-built fonts in `tests/fonts/`, which differ in +//! one thing each, so a test that separates them is proving the *rule* and not +//! the shaper. The stub source below is the whole of the machine: a family +//! name in, the bytes of a committed fixture out, and no directory on the +//! developer's computer consulted for anything. +//! +//! What each font is here to prove: +//! +//! | font | coverage | shaping | what it stands for | +//! |---|---|---|---| +//! | `latin.ttf` | none | — | Helvetica under Arabic: empty boxes | +//! | `nonjoining.ttf` | complete | none | a font with no `GSUB`: disconnected letters | +//! | `partial.ttf` | complete | most | macOS's Arial: correct, and must stay silent | +//! | `joining.ttf` | complete | all | a font doing its job | + +use std::sync::Arc; + +use mirsam_core::diagnostic::Severity; +use mirsam_core::ports::{FontFile, FontSource}; +use mirsam_core::{Diagnostic, Engine, Origin, Properties, RepairOptions, Resolved, TextUnit}; + +/// A machine with exactly the four fixture fonts installed, and nothing else. +/// +/// `mirsam-fonts` is the real adapter and has its own suite; what is under +/// test here is what the domain concludes once bytes are in hand, so the +/// source is as small as the port allows. +struct Fixtures; + +impl Fixtures { + fn file(family: &str) -> Option<&'static str> { + Some(match family { + "Mirsam Joining" => "joining.ttf", + "Mirsam Nonjoining" => "nonjoining.ttf", + "Mirsam Partial" => "partial.ttf", + "Mirsam Latin" => "latin.ttf", + _ => return None, + }) + } +} + +impl FontSource for Fixtures { + fn load(&self, family: &str) -> mirsam_core::Result> { + let Some(name) = Self::file(family) else { + return Ok(None); + }; + let path = format!(concat!(env!("CARGO_MANIFEST_DIR"), "/tests/fonts/{}"), name); + Ok(Some(FontFile { + data: std::fs::read(&path).expect("the fixture fonts are committed"), + index: 0, + path, + family: family.to_string(), + })) + } +} + +/// One paragraph of Arabic whose complex-script slot names `font`. +fn paragraph(text: &str, font: &str) -> TextUnit { + TextUnit::new("slide1.xml#p1", text).with_props(Properties { + complex_font: Resolved::Explicit(font.to_string()), + ..Default::default() + }) +} + +/// What the engine reports for one unit, with the font checks armed. +fn audit(unit: TextUnit) -> Vec { + Engine::with_fonts(&RepairOptions::default(), Arc::new(Fixtures)) + .audit(&[unit]) + .diagnostics +} + +/// Only the findings of one rule; the unit also trips `language-missing` and +/// the direction rules, which are not what any of this is about. +fn of(diagnostics: &[Diagnostic], rule: &str) -> Vec { + diagnostics + .iter() + .filter(|d| d.rule.0 == rule) + .cloned() + .collect() +} + +// ------------------------------------------------------------- font-coverage + +#[test] +fn a_latin_font_under_arabic_is_reported_with_the_exact_characters() { + // PLAN §4.3's acceptance. `latin.ttf` maps printable ASCII and no Arabic + // whatsoever, which is what Helvetica is when a deck points the + // complex-script slot at it: every letter renders as an empty box. + let findings = audit(paragraph("مرحبا", "Mirsam Latin")); + let coverage = of(&findings, "font-coverage"); + assert_eq!(coverage.len(), 1, "{findings:#?}"); + + let finding = &coverage[0]; + assert_eq!(finding.severity, Severity::Error); + assert_eq!( + finding.evidence.offenders, + [ + "U+0645 ARABIC LETTER MEEM", + "U+0631 ARABIC LETTER REH", + "U+062D ARABIC LETTER HAH", + "U+0628 ARABIC LETTER BEH", + "U+0627 ARABIC LETTER ALEF", + ], + "every character that will not render, named, in first-appearance order" + ); + // The font that answered is named, and so is the file it was read from: + // a reviewer can open it. + assert!( + finding.message.contains("Mirsam Latin") && finding.message.contains("latin.ttf"), + "{}", + finding.message + ); + + // And the *other* rule stays out of it. A font with no Arabic has no + // shaping to be broken, and a second finding with the same repair would + // be the tool arguing with itself about one paragraph. + assert!(of(&findings, "shaping-broken").is_empty(), "{findings:#?}"); +} + +#[test] +fn one_letter_a_font_is_missing_is_a_warning_not_an_error() { + // The fixtures map U+0621..U+064A, so Persian peh is genuinely absent — + // Mishafi's situation exactly. The pairing is not wrong; one character of + // it will not render, and the severity says which claim is being made. + let findings = audit(paragraph("مرحبا پ", "Mirsam Joining")); + let coverage = of(&findings, "font-coverage"); + assert_eq!(coverage.len(), 1, "{findings:#?}"); + assert_eq!(coverage[0].severity, Severity::Warning); + assert_eq!(coverage[0].evidence.offenders, ["U+067E ARABIC LETTER PEH"]); +} + +#[test] +fn a_font_that_draws_the_text_is_not_reported() { + for family in ["Mirsam Joining", "Mirsam Nonjoining", "Mirsam Partial"] { + let findings = audit(paragraph("مرحبا", family)); + assert!( + of(&findings, "font-coverage").is_empty(), + "{family} covers this text: {findings:#?}" + ); + } +} + +// ------------------------------------------------------------ shaping-broken + +#[test] +fn a_font_with_no_shaping_tables_is_reported() { + // `nonjoining.ttf` has every letter and no `GSUB` at all: the text is + // correct Unicode, correctly directed, every glyph present, and it + // renders as a row of disconnected letters. No attribute in any document + // format can express this, and no amount of reading XML would find it. + let findings = audit(paragraph("مرحبا", "Mirsam Nonjoining")); + let shaping = of(&findings, "shaping-broken"); + assert_eq!(shaping.len(), 1, "{findings:#?}"); + + let finding = &shaping[0]; + assert_eq!(finding.severity, Severity::Error); + assert_eq!( + finding.evidence.offenders, + [ + "U+0645 ARABIC LETTER MEEM", + "U+0631 ARABIC LETTER REH", + "U+062D ARABIC LETTER HAH", + "U+0628 ARABIC LETTER BEH", + "U+0627 ARABIC LETTER ALEF", + ], + "every letter the reader will see standing alone" + ); + assert!( + finding.message.contains("nonjoining.ttf"), + "{}", + finding.message + ); + assert!(of(&findings, "font-coverage").is_empty(), "{findings:#?}"); +} + +#[test] +fn a_font_that_shapes_some_letters_and_not_others_is_never_reported() { + // ADR 0008, and the reason `partial.ttf` is committed. It shapes + // everything except the final forms of the right-joining letters, which + // is macOS's Arial reduced to its principle: `مرحبا` comes back with the + // reh standing alone and renders perfectly. A rule that regressed to a + // per-letter verdict fires here rather than on a user's deck. + for text in ["مرحبا", "مرحبا يا عالم", "بم", "الأداء بنسبة"] { + let findings = audit(paragraph(text, "Mirsam Partial")); + assert!( + of(&findings, "shaping-broken").is_empty(), + "{text:?} through partial.ttf: {findings:#?}" + ); + } +} + +#[test] +fn a_font_that_shapes_everything_is_never_reported() { + let findings = audit(paragraph("مرحبا يا عالم", "Mirsam Joining")); + assert!(of(&findings, "shaping-broken").is_empty(), "{findings:#?}"); +} + +#[test] +fn a_scrap_of_text_is_passed_over_rather_than_reported_on() { + // ADR 0008's stated cost: the aggregate needs enough text to mean + // anything. `بم` requires two joins — one dual-joining letter — and a + // design sharing a glyph between two forms is exactly what the ADR + // forbids concluding a defect from. Through a font with no shaping + // tables at all, and still silent. + for text in ["بم", "سا"] { + let findings = audit(paragraph(text, "Mirsam Nonjoining")); + assert!( + of(&findings, "shaping-broken").is_empty(), + "{text:?} is not enough to conclude from: {findings:#?}" + ); + } + // Two such letters, independently silent, is. The same font, four + // observable joins, and now it is reported. + for text in ["بمبم", "بم مب"] { + let findings = audit(paragraph(text, "Mirsam Nonjoining")); + assert_eq!( + of(&findings, "shaping-broken").len(), + 1, + "{text:?}: {findings:#?}" + ); + } +} + +#[test] +fn text_with_no_join_to_make_proves_nothing_about_a_font() { + // Every letter here is right-joining and followed by another, so no + // letter is required to take a contextual form. The best font on the + // machine and the worst would come back identical. + let findings = audit(paragraph("ا د ر و", "Mirsam Nonjoining")); + assert!(of(&findings, "shaping-broken").is_empty(), "{findings:#?}"); +} + +// -------------------------------------------------------------- the boundary + +#[test] +fn a_font_this_machine_does_not_have_is_silence_not_a_finding() { + // The tool can no longer say what the reader will see, which is a fact + // about this computer and not a defect in the deck. Reporting it would + // fire on every runner with no fonts installed. + let findings = audit(paragraph("مرحبا", "Traditional Arabic")); + assert!(of(&findings, "font-coverage").is_empty(), "{findings:#?}"); + assert!(of(&findings, "shaping-broken").is_empty(), "{findings:#?}"); +} + +#[test] +fn a_paragraph_naming_no_complex_font_is_left_to_the_rule_that_owns_it() { + // `complex-font-missing` reports an empty slot. These two would be + // guessing which font the application is going to substitute. + let findings = audit(TextUnit::new("slide1.xml#p1", "مرحبا")); + assert!(of(&findings, "font-coverage").is_empty(), "{findings:#?}"); + assert!(of(&findings, "shaping-broken").is_empty(), "{findings:#?}"); +} + +#[test] +fn text_with_no_arabic_is_not_a_question_for_the_complex_script_slot() { + let unit = TextUnit::new("slide1.xml#p1", "Revenue rose 25% in Q4").with_props(Properties { + complex_font: Resolved::Explicit("Mirsam Latin".into()), + ..Default::default() + }); + let findings = audit(unit); + assert!(of(&findings, "font-coverage").is_empty(), "{findings:#?}"); +} + +#[test] +fn without_a_font_source_the_checks_do_not_run() { + // And a caller in this position has to say so: standing rule 4. Silence + // from a check that never ran is not a pass, which is why `mirsam` reports + // the font checks as unrun unless `--fonts` asked for them. + let report = Engine::with_default_rules().audit(&[paragraph("مرحبا", "Mirsam Latin")]); + assert!( + !report + .diagnostics + .iter() + .any(|d| matches!(d.rule.0, "font-coverage" | "shaping-broken")), + "{:#?}", + report.diagnostics + ); + // Registered all the same, so `mirsam rules` describes the whole set. + let ids: Vec<&str> = Engine::with_default_rules() + .rules() + .map(|r| r.0.0) + .collect(); + assert!(ids.contains(&"font-coverage") && ids.contains(&"shaping-broken")); +} + +#[test] +fn an_inherited_typeface_is_cited_where_it_was_stated() { + // A master writes `+mn-cs` and the theme holds the typeface, so a + // reviewer told only that the font has no Arabic has nowhere to go. + // Invariant 6, and the reason `evidence.inherited_from` exists. + let unit = TextUnit::new("slide1.xml#p1", "مرحبا").with_props(Properties { + complex_font: Resolved::Inherited( + "Mirsam Latin".into(), + Origin::new("ppt/theme/theme1.xml", "fontScheme/minorFont/cs@typeface"), + ), + ..Default::default() + }); + let findings = audit(unit); + assert_eq!( + of(&findings, "font-coverage")[0].evidence.inherited_from, + Some("ppt/theme/theme1.xml fontScheme/minorFont/cs@typeface".to_string()) + ); +} diff --git a/crates/mirsam-core/tests/fonts/latin.ttf b/crates/mirsam-core/tests/fonts/latin.ttf new file mode 100644 index 0000000000000000000000000000000000000000..04f984ec8c496c61a6189a36cf6ef9dda96ffa22 GIT binary patch literal 1636 zcmeHHJxc>Y5Pi3I2~mP}8mz7mtALmy2nI=EXM&J`1Pk?uXA;OIM{+_`h^bSErG;QC z(MD`6`~f!k5f*|!KpUI&?VTS7qLqb3=GdM0-t5fnF3UgzFpeH{m}|xPRC#8D0)VV$_Unu60WreP8%0)moasLt2p2Z!p36-Jo9+Z#6pu&yvQ@krBXH!b~;ByjM zkb&G$b75;T^8#IeQu5q-=jPBsSzk%?h9pOK` z0o4%(zt%I5R^7@NWA{jKWq<`H$O3E-&AsC&rm&1WYG|Pi3l2===shY86kwx*J=CeR Ueq0sK{>oK<_D!CtUj5zP0N~M%OaK4? literal 0 HcmV?d00001 diff --git a/crates/mirsam-fonts/tests/resolution.rs b/crates/mirsam-fonts/tests/resolution.rs index e451b5f..3984bc7 100644 --- a/crates/mirsam-fonts/tests/resolution.rs +++ b/crates/mirsam-fonts/tests/resolution.rs @@ -1,10 +1,9 @@ //! Resolving a typeface name to the bytes that answer for it. //! -//! The directory searched is the shaping fixtures'. Three files, three -//! families, and one difference each — which makes it a machine for asking -//! whether the right bytes came back rather than merely some bytes: the font -//! that shapes and the font that does not are indistinguishable except by -//! shaping through them. +//! The directory searched is the shaping fixtures'. Four families, one +//! difference each — which makes it a machine for asking whether the right +//! bytes came back rather than merely some bytes: the font that shapes and the +//! font that does not are indistinguishable except by shaping through them. //! //! Nothing here indexes the machine it runs on. A suite that did would assert //! whatever the developer happened to have installed. @@ -27,7 +26,12 @@ fn fixtures() -> SystemFonts { fn every_font_in_the_directory_is_indexed_by_the_name_it_gives_itself() { assert_eq!( fixtures().families().collect::>(), - ["mirsam joining", "mirsam nonjoining", "mirsam partial"] + [ + "mirsam joining", + "mirsam latin", + "mirsam nonjoining", + "mirsam partial" + ] ); } diff --git a/crates/mirsam-ooxml/tests/fonts.rs b/crates/mirsam-ooxml/tests/fonts.rs new file mode 100644 index 0000000..29efd05 --- /dev/null +++ b/crates/mirsam-ooxml/tests/fonts.rs @@ -0,0 +1,132 @@ +//! PLAN §4.3's acceptance, over a real deck. +//! +//! `mirsam-core`'s own suite proves what the two font rules conclude from a +//! `TextUnit` a test wrote by hand. This proves the other half — that a +//! typeface a *deck* names, resolved through the adapter's inheritance chain, +//! reaches those rules and comes back as a finding naming the exact characters +//! that will not render. +//! +//! The machine is a stub. `broken-arabic.pptx` names `Calibri` and `Dubai`, +//! and whether either of them draws Arabic depends entirely on who is running +//! the test — a suite that asked the developer's font directories would assert +//! whatever happened to be installed. So the source below answers every family +//! with one committed fixture font, which is the same statement as "on this +//! reader's machine, the deck's font has no Arabic". + +use std::sync::Arc; + +use mirsam_core::diagnostic::Severity; +use mirsam_core::ports::{FontFile, FontSource}; +use mirsam_core::{Diagnostic, DocumentReader, Engine, RepairOptions, Result}; +use mirsam_ooxml::PptxDocument; +use std::path::{Path, PathBuf}; + +fn fixtures() -> PathBuf { + Path::new(env!("CARGO_MANIFEST_DIR")) + .join("../../tests/fixtures") + .canonicalize() + .expect("tests/fixtures exists") +} + +/// The shaping fixtures, which live with the crate that shapes. +fn font(name: &str) -> Vec { + let path = Path::new(env!("CARGO_MANIFEST_DIR")) + .join("../mirsam-core/tests/fonts") + .join(name); + std::fs::read(&path).expect("the fixture fonts are committed") +} + +/// A machine on which every typeface a document could name is `file`. +struct Everything(&'static str); + +impl FontSource for Everything { + fn load(&self, family: &str) -> Result> { + Ok(Some(FontFile { + data: font(self.0), + index: 0, + path: format!("/{}", self.0), + family: family.to_string(), + })) + } +} + +fn audit(deck: &str, machine: Everything) -> Vec { + let mut document = PptxDocument::open(fixtures().join(deck)).expect("a corpus deck"); + let units = document.scan().expect("the deck lowers"); + Engine::with_fonts(&RepairOptions::default(), Arc::new(machine)) + .audit(&units) + .diagnostics +} + +fn of(diagnostics: &[Diagnostic], rule: &str) -> Vec { + diagnostics + .iter() + .filter(|d| d.rule.0 == rule) + .cloned() + .collect() +} + +#[test] +fn a_deck_whose_font_has_no_arabic_is_reported_with_the_exact_characters() { + // The acceptance, in one sentence: this deck's complex-script slot + // resolves to a font that maps printable ASCII and nothing else, and the + // report says which letters go blank rather than that "the font is wrong". + let findings = audit("broken-arabic.pptx", Everything("latin.ttf")); + let coverage = of(&findings, "font-coverage"); + assert!( + !coverage.is_empty(), + "every Arabic paragraph in this deck should be reported: {findings:#?}" + ); + + for finding in &coverage { + assert_eq!(finding.severity, Severity::Error, "{finding:#?}"); + assert!( + !finding.evidence.offenders.is_empty(), + "a coverage finding with no characters names nothing a reviewer can check" + ); + for offender in &finding.evidence.offenders { + assert!( + offender.starts_with("U+") && offender.contains("ARABIC"), + "{offender:?} is not a named Arabic character" + ); + } + // The unit the finding is on is a real place in the package, so a + // reviewer can open the part and find the paragraph. + assert!(finding.location.part.starts_with("ppt/"), "{finding:#?}"); + } + + // A font with no Arabic has no shaping to be broken. One paragraph, one + // finding, one repair. + assert!(of(&findings, "shaping-broken").is_empty(), "{findings:#?}"); +} + +#[test] +fn a_deck_whose_font_cannot_join_is_reported_as_a_shaping_defect() { + // The same deck, the same correct Unicode, a font that has every letter — + // and no `GSUB`. Nothing in the XML changed and nothing in the XML could + // have said so. + let findings = audit("broken-arabic.pptx", Everything("nonjoining.ttf")); + let shaping = of(&findings, "shaping-broken"); + assert!(!shaping.is_empty(), "{findings:#?}"); + + for finding in &shaping { + assert_eq!(finding.severity, Severity::Error, "{finding:#?}"); + assert!(!finding.evidence.offenders.is_empty(), "{finding:#?}"); + } + assert!(of(&findings, "font-coverage").is_empty(), "{findings:#?}"); +} + +#[test] +fn a_deck_whose_font_does_its_job_is_reported_for_neither() { + // `joining.ttf` covers U+0621..U+064A and shapes all of it. Anything this + // deck's Arabic still trips is a defect in the deck, not in the font — and + // a rule that fired here would be a false positive on every correct + // pairing there is. + for deck in ["broken-arabic.pptx", "clean.pptx"] { + let findings = audit(deck, Everything("joining.ttf")); + assert!( + of(&findings, "shaping-broken").is_empty(), + "{deck}: {findings:#?}" + ); + } +} diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 55025de..ce8b14e 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -165,6 +165,12 @@ crates/ fix.rs format-agnostic repairs ports.rs DocumentReader / DocumentWriter / FontSource rules/ the rule set + font.rs the two rules that judge the font a paragraph resolves + to — the only ones that ask about the machine rather + than the document, which is why the FontSource is an + argument to Engine::with_fonts and not a RepairOptions + field, and why a caller that does not supply one has to + report the checks as NOT RUN mirsam-ooxml/ adapter — PPTX and DOCX; XLSX will share the two modules below that name no element: the package and the scaffold package.rs ZIP access and the byte-preserving rewrite (raw entry copy) diff --git a/docs/PLAN.md b/docs/PLAN.md index ea1a40a..fa290dd 100644 --- a/docs/PLAN.md +++ b/docs/PLAN.md @@ -24,7 +24,7 @@ Status legend: `[x]` done · `[ ]` ready · `[~]` blocked on the item above. --- -## M1 — Repair `[ ]` +## M1 — Repair `[x]` The order matters: the round-trip guarantee must exist *before* the first mutation, or there is nothing to prove the mutation didn't damage anything. @@ -569,7 +569,7 @@ report change, and `make validate-fixtures` still passes on all five decks. --- -## M3 — Word `[ ]` +## M3 — Word `[x]` ### 3.1 Extract the shared package layer `[x]` ZIP access, part enumeration and the byte-preserving rewrite already lived in @@ -905,7 +905,7 @@ committed reports changed in one line each — the key naming the file is --- -## M4 — Shaping `[ ]` +## M4 — Shaping `[~]` ### 4.1 `rustybuzz` shaping `[x]` Shape each Arabic run; assert joining forms are produced. @@ -1038,10 +1038,130 @@ perfectly and is missing only Persian peh and Urdu tteh, and `Arial` resolves to `Arial.ttf` with complete coverage and four joins of five — ADR 0008's reh, still not a defect. -### 4.3 `shaping-broken` and `font-coverage` rules `[~]` - -*Acceptance:* a deck using a Latin-only font for Arabic is reported with the -exact characters that will not render. +### 4.3 `shaping-broken` and `font-coverage` rules `[x]` + +4.1 and 4.2 built two modules that refuse to draw a conclusion. This is where +the conclusions are drawn, and there are exactly two, because they are two +defects with two pieces of advice. **`font-coverage`** says the font has no +glyph for the Arabic: it renders as empty boxes, and no shaping table would +have saved it. **`shaping-broken`** says the font has every letter and no +shaping tables: it renders as a row of disconnected letters, and installing a +font with more coverage would change nothing. Telling an author to pick a +different typeface is the answer to both and for opposite reasons, which is +why one rule reporting both would be a rule that cannot be acted on. + +**These are the first rules in this tool that ask a question about the +machine, and that is why they are off by default.** Every other check reads a +document and gives the same answer on any computer. These two need a font +file, and which fonts a computer has is not a property of the deck: an audit +that resolved them without being asked would report differently on a +developer's laptop and on a CI runner looking at one document, and neither run +could be reproduced from the file alone. So `mirsam audit --fonts` asks for +them, `--font-dir` pins the directories searched — which is what makes the +check reproducible at all — and **the report says which of the two audits it +is**, in both output formats: `fonts NOT RUN` for a human, `"fonts": +{"checked": false}` for an agent. That is standing rule 4 spent where it was +always going to be needed. The golden corpus never passes the flag, so the +six committed reports gained one key each and not a single finding, which is +the claim that nothing else moved. + +*Which way the claim runs, and it only runs one way.* A font that is **here** +and cannot draw the text will not draw it anywhere, because a `cmap` and a +`GSUB` travel with the font — that is what makes a finding worth making. A +font that is here and draws the text perfectly proves nothing whatsoever +about the reader's machine, and silence from these rules is not a promise +that the document renders. `AGENTS.md` says so where an agent will read it. + +*Three ways to say nothing, all of them deliberate.* A paragraph naming no +complex-script font is `complex-font-missing`'s, not theirs — guessing which +font the application will substitute is exactly the invention this design +refuses. A family this machine does not have is the tool losing the ability to +say what the reader will see, which is a fact about the computer and not a +defect in the deck; reporting it would fire on every runner with no fonts +installed. And a file the shaper cannot parse is a fact about the file, +reported by whoever supplied it. + +**Where the shaping threshold sits, which ADR 0008 left to this item.** The +signal is `joins_produced == 0` against enough required joins to mean +something, and the number is **four**, counted over letters the font actually +has. The arithmetic is the argument: a letter is required to take a *final* +form only because the letter before it joined forwards, so every final has a +companion initial or medial, and initials and medials are the dual-joining +letters — the ones a font like Arial does shape. Two joins are therefore +*one* such letter, which is precisely the design choice ADR 0008 forbids +concluding a defect from, and which a two-letter word is the whole of. Four +are two of them, independently, both silent, and no font that shapes Arabic +at all looks like that. Unmapped letters are excluded from the count for a +different reason: a font cannot join a glyph it does not have, so counting +them would let a Latin-only font produce a `shaping-broken` finding next to +its `font-coverage` one — the tool arguing with itself about one paragraph, +with the same repair and two explanations. + +*Severity is what 4.2 built `missing_occurrences` for.* A font answering for +**none** of the Arabic was not the font this text needed: the paragraph goes +blank, there is nothing to argue about, and it is an error. A font missing +*some* of it is an otherwise sound pairing meeting an unusual letter — +Mishafi has no Persian peh — which is still wrong for the characters it hits, +still worth an author's attention, and is not the same claim. Warning. + +**Neither is fixable, and that is not an omission.** The repair for both is a +different typeface, and which one is an authoring decision the text cannot +supply — the reason `complex-font-missing` proposes nothing until `--font` +chooses one. It is stronger here: that rule fills an *empty* slot, while +these two would be overwriting a font the author deliberately put there. + +*The fifth fixture font.* `latin.ttf` maps printable ASCII and no Arabic +whatsoever, written by the same `struct`-and-nothing-else generator as the +other four. It is Helvetica under Arabic reduced to its principle, and it is +what makes the acceptance a committed test rather than a manual check. The +four fonts now disagree about exactly two things — coverage and shaping — and +each rule is separated by the one it is named for. + +*A cost, stated rather than optimised away.* Each rule resolves the typeface +per unit, so a paragraph's font file is read twice and a deck's font file +once per paragraph per rule. On a 126-unit deck resolving to macOS's 2.3 MB +`Helvetica.ttc` that is 42 ms — the operating system's page cache absorbs it — +against 5 ms for the same audit with the checks off. Memoising the bytes is +one `HashMap` in an adapter when a deck arrives that needs it, and +`ARCHITECTURE.md`'s KISS note is the reason it is not there already. + +*Acceptance:* met. `cargo test` audits `broken-arabic.pptx` through the real +PPTX adapter on a machine whose every typeface is `latin.ttf`, and every +Arabic paragraph comes back a `font-coverage` error listing the exact +characters by name; the same deck through `nonjoining.ttf` comes back +`shaping-broken` and not `font-coverage`; through `joining.ttf`, neither. +Validated beyond the fixtures against macOS, on a copy of that deck retyped +to each face: `Helvetica` and `Comic Sans MS` report every Arabic character of +every paragraph, and `Times New Roman`, `Geeza Pro`, `Mishafi`, `Al Nile` and +`Baghdad` report nothing at all. `Arial` reports nothing either — ADR 0008's +reh, four joins of five, still not a defect. + +### 4.4 `tatweel-padding` rule `[ ]` + +U+0640 ARABIC TATWEEL used as visual padding rather than as justification. +`ROADMAP.md` has asked for this since M4 was written and no item carried it, +which is what this entry fixes. + +*The hard part is that tatweel is legitimate*, which makes this ADR 0004's +first failure mode waiting to happen for the third time. It is the kashida: +the letter-stretching Arabic typography has justified text with for a +thousand years, and a font's `GSUB` may insert it. What is a defect is a +*typed* one standing in for a layout the author could not get — a run of them +padding a heading to a width, or one wedged into a word to fake alignment — +and the tool cannot read intent. So the threshold is the whole design +question, the way 4.3's was: state it, argue it, and commit a fixture that +fails a rule which regresses to "any tatweel is a defect". + +*Note that this is a defect in the text*, not in the properties around it, so +its repair edits characters — the second one ever to do so, after +`literal-bullet`, and it inherits that rule's caution: opt-in, because +deleting a character the author typed is not the same as changing an +attribute they left blank. Whether stripping is even safe is part of the +question: a tatweel between two letters that already join is padding, and one +carrying a harakat is not. + +*Acceptance:* a deck padding a heading with typed tatweel is reported with the +offsets, and a deck whose Arabic is justified by its font's kashida is not. --- diff --git a/scripts/make-shaping-fixture.py b/scripts/make-shaping-fixture.py index 845ff57..6b1892c 100644 --- a/scripts/make-shaping-fixture.py +++ b/scripts/make-shaping-fixture.py @@ -34,7 +34,15 @@ Each names itself distinctly — `Mirsam Joining`, `Mirsam Nonjoining`, `Mirsam Partial` — so the directory doubles as a machine for `mirsam-fonts` to index: three files, three families, and a resolver that must hand back the -right bytes for each. A fourth file exists only for that: +right bytes for each. Two more files exist for reasons of their own: + + latin.ttf + Printable ASCII and no Arabic at all, with no GSUB. The *other* + defect, and the one PLAN §4.3's `font-coverage` rule exists for: a + deck that points its complex-script slot at Helvetica renders every + Arabic letter as an empty box, and no shaping table would have saved + it. Shaping through it must stay silent — a letter the font has no + glyph for says nothing about whether the font can join. bold.ttf `Mirsam Joining` again, styled `Bold`, with no GSUB, and named so that @@ -181,14 +189,30 @@ def glyf() -> bytes: return b"\0\0\0\0" -def cmap() -> bytes: - # Three segments, sorted by end code: the space, the letters, and the - # 0xFFFF terminator every format 4 subtable is required to end with. - segments = [ - (0x0020, 0x0020, (GID_SPACE - 0x0020) & 0xFFFF), - (FIRST_CP, LAST_CP, (GID_ISOL - FIRST_CP) & 0xFFFF), - (0xFFFF, 0xFFFF, 1), - ] +def cmap(arabic: bool = True) -> bytes: + """The character-to-glyph map. + + With `arabic` the letters U+0621..U+064A are mapped, which is what makes + the shaping fixtures answerable for Arabic at all. Without it the font + maps printable ASCII and nothing else — `latin.ttf`, the font that has no + Arabic to draw and no shaping table that could have saved it. + """ + if not arabic: + # Printable ASCII onto the glyphs the Arabic letters would have used. + # They have no outlines either; what matters is that a glyph exists + # for a `Q` and does not for a beh. + segments = [ + (0x0020, 0x007E, (GID_SPACE - 0x0020) & 0xFFFF), + (0xFFFF, 0xFFFF, 1), + ] + else: + # Three segments, sorted by end code: the space, the letters, and the + # 0xFFFF terminator every format 4 subtable is required to end with. + segments = [ + (0x0020, 0x0020, (GID_SPACE - 0x0020) & 0xFFFF), + (FIRST_CP, LAST_CP, (GID_ISOL - FIRST_CP) & 0xFFFF), + (0xFFFF, 0xFFFF, 1), + ] seg_count = len(segments) search_range = 2 * (2 ** (seg_count.bit_length() - 1)) @@ -236,7 +260,7 @@ def post() -> bytes: return struct.pack(">IihhIIIII", 0x00030000, 0, -100, 50, 0, 0, 0, 0, 0) -def os2() -> bytes: +def os2(arabic: bool = True) -> bytes: return ( struct.pack( ">HhHHH", @@ -251,9 +275,15 @@ def os2() -> bytes: + struct.pack(">hh", 50, 250) # strikeout + struct.pack(">h", 0) # sFamilyClass + b"\0" * 10 # panose - + struct.pack(">IIII", 1 << 13, 0, 0, 0) # unicode ranges: Arabic + # Unicode ranges: bit 13 is Arabic, bit 0 Basic Latin. Nothing in the + # shaping path reads this, and it is stated correctly anyway — a + # fixture that lied about what it covers would be a fixture to + # distrust. + + struct.pack(">IIII", 1 << 13 if arabic else 1 << 0, 0, 0, 0) + b"MRSM" # achVendID - + struct.pack(">HHH", 0x0040, 0x0020, LAST_CP) # regular, first, last + + struct.pack( + ">HHH", 0x0040, 0x0020, LAST_CP if arabic else 0x007E + ) # regular, first, last + struct.pack(">hhh", 800, -200, 0) # typo ascender/descender/gap + struct.pack(">HH", 800, 200) # win ascent/descent + struct.pack(">II", 0, 0) # code page ranges @@ -367,10 +397,12 @@ def assemble(tables: dict[str, bytes]) -> bytes: return font[:at] + struct.pack(">I", adjustment) + font[at + 4 :] -def build(shaping: bytes | None, family: str, subfamily: str) -> bytes: +def build( + shaping: bytes | None, family: str, subfamily: str, arabic: bool = True +) -> bytes: tables = { - "OS/2": os2(), - "cmap": cmap(), + "OS/2": os2(arabic), + "cmap": cmap(arabic), "glyf": glyf(), "head": head(), "hhea": hhea(), @@ -391,7 +423,8 @@ def build(shaping: bytes | None, family: str, subfamily: str) -> bytes: # ones a real font can leave alone for the reason `partial.ttf` documents. PARTIAL_FINA_FROM = 0x0633 -# filename -> (family the file names itself, style within it, its GSUB) +# filename -> (family the file names itself, style within it, its GSUB, +# whether it maps Arabic at all) # # `bold.ttf` is not a shaping fixture. It is a second file claiming # `Mirsam Joining`, sorting before `joining.ttf`, and carrying no GSUB — so a @@ -400,19 +433,33 @@ def build(shaping: bytes | None, family: str, subfamily: str) -> bytes: # than on a user's deck. This is Arial's situation on macOS reduced to its # principle: eleven files state that family, and only one of them is the font # a document naming `Arial` is asking for. +# +# `latin.ttf` is not a shaping fixture either. It is the *other* defect: a +# font that maps printable ASCII and no Arabic whatsoever, which is what +# Helvetica or Comic Sans is when a deck points the complex-script slot at it. +# Every Arabic character comes back missing, the text renders as a row of +# empty boxes, and no shaping table would have saved it. It is the font +# `font-coverage` exists for, and shaping through it must stay silent — +# a letter the font has no glyph for proves nothing about its `GSUB`. FONTS = { - "bold.ttf": ("Mirsam Joining", "Bold", None), - "joining.ttf": ("Mirsam Joining", "Regular", gsub()), - "nonjoining.ttf": ("Mirsam Nonjoining", "Regular", None), - "partial.ttf": ("Mirsam Partial", "Regular", gsub(fina_from=PARTIAL_FINA_FROM)), + "bold.ttf": ("Mirsam Joining", "Bold", None, True), + "joining.ttf": ("Mirsam Joining", "Regular", gsub(), True), + "latin.ttf": ("Mirsam Latin", "Regular", None, False), + "nonjoining.ttf": ("Mirsam Nonjoining", "Regular", None, True), + "partial.ttf": ( + "Mirsam Partial", + "Regular", + gsub(fina_from=PARTIAL_FINA_FROM), + True, + ), } def main() -> None: os.makedirs(OUT_DIR, exist_ok=True) - for filename, (family, subfamily, shaping) in FONTS.items(): + for filename, (family, subfamily, shaping, arabic) in FONTS.items(): path = os.path.join(OUT_DIR, filename) - data = build(shaping, family, subfamily) + data = build(shaping, family, subfamily, arabic) with open(path, "wb") as handle: handle.write(data) print( diff --git a/tests/fixtures/broken-arabic.expected.json b/tests/fixtures/broken-arabic.expected.json index bb83c54..a479d6d 100644 --- a/tests/fixtures/broken-arabic.expected.json +++ b/tests/fixtures/broken-arabic.expected.json @@ -101,6 +101,9 @@ } ], "file": "broken-arabic.pptx", + "fonts": { + "checked": false + }, "format": "pptx", "strict": false, "summary": { @@ -257,6 +260,9 @@ }, "blocking": false, "file": "broken-arabic.pptx", + "fonts": { + "checked": false + }, "format": "pptx", "options": { "align": true, diff --git a/tests/fixtures/clean.expected.json b/tests/fixtures/clean.expected.json index 8f8d21a..e81b1bb 100644 --- a/tests/fixtures/clean.expected.json +++ b/tests/fixtures/clean.expected.json @@ -3,6 +3,9 @@ "blocking": false, "diagnostics": [], "file": "clean.pptx", + "fonts": { + "checked": false + }, "format": "pptx", "strict": false, "summary": { @@ -47,6 +50,9 @@ }, "blocking": false, "file": "clean.pptx", + "fonts": { + "checked": false + }, "format": "pptx", "options": { "align": true, diff --git a/tests/fixtures/quarterly-report-correct.expected.json b/tests/fixtures/quarterly-report-correct.expected.json index 7b1bbab..f95ae0e 100644 --- a/tests/fixtures/quarterly-report-correct.expected.json +++ b/tests/fixtures/quarterly-report-correct.expected.json @@ -3,6 +3,9 @@ "blocking": false, "diagnostics": [], "file": "quarterly-report-correct.pptx", + "fonts": { + "checked": false + }, "format": "pptx", "strict": false, "summary": { @@ -47,6 +50,9 @@ }, "blocking": false, "file": "quarterly-report-correct.pptx", + "fonts": { + "checked": false + }, "format": "pptx", "options": { "align": true, diff --git a/tests/fixtures/quarterly-report.expected.json b/tests/fixtures/quarterly-report.expected.json index 7d06481..2332402 100644 --- a/tests/fixtures/quarterly-report.expected.json +++ b/tests/fixtures/quarterly-report.expected.json @@ -682,6 +682,9 @@ } ], "file": "quarterly-report.pptx", + "fonts": { + "checked": false + }, "format": "pptx", "strict": false, "summary": { @@ -1474,6 +1477,9 @@ }, "blocking": false, "file": "quarterly-report.pptx", + "fonts": { + "checked": false + }, "format": "pptx", "options": { "align": true, diff --git a/tests/fixtures/quarterly-review-correct.expected.json b/tests/fixtures/quarterly-review-correct.expected.json index 6fa5e88..9b1c8a1 100644 --- a/tests/fixtures/quarterly-review-correct.expected.json +++ b/tests/fixtures/quarterly-review-correct.expected.json @@ -3,6 +3,9 @@ "blocking": false, "diagnostics": [], "file": "quarterly-review-correct.docx", + "fonts": { + "checked": false + }, "format": "docx", "strict": false, "summary": { diff --git a/tests/fixtures/quarterly-review.expected.json b/tests/fixtures/quarterly-review.expected.json index 4d4cbd5..0dd71ab 100644 --- a/tests/fixtures/quarterly-review.expected.json +++ b/tests/fixtures/quarterly-review.expected.json @@ -227,6 +227,9 @@ } ], "file": "quarterly-review.docx", + "fonts": { + "checked": false + }, "format": "docx", "strict": false, "summary": { diff --git a/tests/fixtures/torture.expected.json b/tests/fixtures/torture.expected.json index cfd5235..b11ef26 100644 --- a/tests/fixtures/torture.expected.json +++ b/tests/fixtures/torture.expected.json @@ -177,6 +177,9 @@ } ], "file": "torture.pptx", + "fonts": { + "checked": false + }, "format": "pptx", "strict": false, "summary": { @@ -422,6 +425,9 @@ }, "blocking": false, "file": "torture.pptx", + "fonts": { + "checked": false + }, "format": "pptx", "options": { "align": true, From 011df2022e579fd26b988729da8bb4d936e6b01e Mon Sep 17 00:00:00 2001 From: Hashem Aldhaheri <158606+aenawi@users.noreply.github.com> Date: Sat, 5 Sep 2026 10:25:21 +0400 Subject: [PATCH 2/2] fix(fonts): as_chunks for the fixed-width table directory CI's clippy is ahead of the toolchain this was written on and reads chunks_exact over a constant size as a missed as_chunks. The array type carries the record length, so the field reads below are checked once at the split rather than on every one. --- crates/mirsam-fonts/src/sfnt.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/mirsam-fonts/src/sfnt.rs b/crates/mirsam-fonts/src/sfnt.rs index b441d59..871a534 100644 --- a/crates/mirsam-fonts/src/sfnt.rs +++ b/crates/mirsam-fonts/src/sfnt.rs @@ -107,7 +107,11 @@ fn name_table(file: &mut File, offset: u64) -> io::Result>> { let mut directory = vec![0u8; 16 * tables as usize]; file.read_exact(&mut directory)?; - let Some(record) = directory.chunks_exact(16).find(|r| &r[..4] == b"name") else { + // `as_chunks` rather than `chunks_exact`: the record length is a constant, + // and the array type carries it so the indexing below is checked once here + // instead of on every field read. + let (records, _) = directory.as_chunks::<16>(); + let Some(record) = records.iter().find(|r| &r[..4] == b"name") else { return Ok(None); }; let table_offset = u32::from_be_bytes([record[8], record[9], record[10], record[11]]);