Skip to content

feat: add latin-cyrillic / greek-letters / musical-symbols presets#1916

Open
jpirnay wants to merge 2 commits into
crosspoint-reader:masterfrom
jpirnay:sdfont-yaml-presets
Open

feat: add latin-cyrillic / greek-letters / musical-symbols presets#1916
jpirnay wants to merge 2 commits into
crosspoint-reader:masterfrom
jpirnay:sdfont-yaml-presets

Conversation

@jpirnay
Copy link
Copy Markdown
Contributor

@jpirnay jpirnay commented May 10, 2026

Add three new INTERVAL_PRESETS entries to fontconvert_sdcard.py and re-point existing sd-fonts.yaml entries at them, plus add a 10pt size to every family.

  • latin-cyrillic — combined Latin (incl. punctuation) + extended
    Cyrillic in one preset, for fonts that ship both
    scripts. Picks up the three Cyrillic extended
    blocks (1C80, 2DE0, A640 ranges) on top of plain
    "cyrillic", since Latin+Cyrillic fonts generally
    also cover those.
  • greek-letters — modern Greek letters only (0x0370-0x03FF), drops
    the polytonic block that "greek" includes. Useful
    for fonts that don't ship polytonic glyphs, where
    requesting "greek" produces empty slots in
    0x1F00-0x1FFF.
  • musical-symbols — card suits + Western musical notation (0x2660-
    0x266F + 0x1D100-0x1D1FF). Small, common in body
    text (e.g. ♪ ♫ ♬), enabled across all readable
    families.

Existing presets are unchanged. Existing family entries in sd-fonts.yaml are updated to use the new presets and to add a 10pt size; the previous {12,14,16,18} stays.

Behaviour deltas vs prior config:

  • Polytonic Greek (0x1F00-0x1FFF) is no longer requested for fonts that switched from "greek" to "greek-letters". For families that ship modern Greek but no polytonic glyphs (most), this trims empty glyph slots; for those that do ship polytonic, callers can still request "greek" explicitly.
  • Cyrillic extended blocks are now requested via "latin-cyrillic" for Latin+Cyrillic families that previously requested "latin-ext,cyrillic".
  • Musical-symbols block adds ~20 glyphs to the BMP block plus the SMP notation block. The on-disk format renders SMP cps fine; kerning / ligature data for SMP cps is filtered out by the existing pipeline.
  • 10pt size adds one extra .cpfont per family on disk.

Summary

  • What is the goal of this PR? (e.g., Implements the new feature for file uploading.)
  • What changes are included?

Additional Context

  • Add any other information that might be helpful for the reviewer (e.g., performance implications, potential risks,
    specific areas to focus on).

AI Usage

While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it
helps set the right context for reviewers.

Did you use AI tools to help write this code? < YES | PARTIALLY | NO >

…presets

Add three new INTERVAL_PRESETS entries to fontconvert_sdcard.py and
re-point existing sd-fonts.yaml entries at them, plus add a 10pt size
to every family.

* latin-cyrillic   — combined Latin (incl. punctuation) + extended
                     Cyrillic in one preset, for fonts that ship both
                     scripts. Picks up the three Cyrillic extended
                     blocks (1C80, 2DE0, A640 ranges) on top of plain
                     "cyrillic", since Latin+Cyrillic fonts generally
                     also cover those.
* greek-letters    — modern Greek letters only (0x0370-0x03FF), drops
                     the polytonic block that "greek" includes. Useful
                     for fonts that don't ship polytonic glyphs, where
                     requesting "greek" produces empty slots in
                     0x1F00-0x1FFF.
* musical-symbols  — card suits + Western musical notation (0x2660-
                     0x266F + 0x1D100-0x1D1FF). Small, common in body
                     text (e.g. ♪ ♫ ♬), enabled across all readable
                     families.

Existing presets are unchanged. Existing family entries in
sd-fonts.yaml are updated to use the new presets and to add a 10pt
size; the previous {12,14,16,18} stays.

Behaviour deltas vs prior config:
- Polytonic Greek (0x1F00-0x1FFF) is no longer requested for fonts
  that switched from "greek" to "greek-letters". For families that
  ship modern Greek but no polytonic glyphs (most), this trims empty
  glyph slots; for those that do ship polytonic, callers can still
  request "greek" explicitly.
- Cyrillic extended blocks are now requested via "latin-cyrillic" for
  Latin+Cyrillic families that previously requested "latin-ext,cyrillic".
- Musical-symbols block adds ~20 glyphs to the BMP block plus the SMP
  notation block. The on-disk format renders SMP cps fine; kerning /
  ligature data for SMP cps is filtered out by the existing pipeline.
- 10pt size adds one extra .cpfont per family on disk.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 10, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

Adds three new Unicode interval presets, updates extract_kerning_fonttools and extract_ligatures_fonttools to ignore SMP codepoints, and applies the new presets plus an additional size 10 to multiple SD-card font family configurations.

Changes

Unicode Interval Presets and Font Configuration

Layer / File(s) Summary
Preset Definitions
lib/EpdFont/scripts/fontconvert_sdcard.py
Added three new presets to INTERVAL_PRESETS: greek-letters, latin-cyrillic, and musical-symbols with inline block comments.
Kerning Extraction BMP filter
lib/EpdFont/scripts/fontconvert_sdcard.py
extract_kerning_fonttools updated to ignore kerning pairs whose left or right codepoint is SMP (>= 0x10000).
Ligature Extraction BMP filter
lib/EpdFont/scripts/fontconvert_sdcard.py
extract_ligatures_fonttools updated to drop ligature rules whose output is SMP or whose input sequence contains any SMP codepoints.
Font Configuration Updates (serif, sans, mono, accessibility)
lib/EpdFont/scripts/sd-fonts.yaml
Multiple families updated to use builtin-prefixed interval presets (including new presets) and all affected families had sizes expanded to include 10.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • ngxson
  • znelson
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main changes: adding three new Unicode interval presets (latin-cyrillic, greek-letters, musical-symbols) to the font configuration system.
Description check ✅ Passed The description is directly related to the changeset, providing detailed context about the three new presets, their Unicode ranges, use cases, and behavior implications compared to prior configuration.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@lib/EpdFont/scripts/fontconvert_sdcard.py`:
- Around line 61-63: The kerning and ligature packing fails for SMP codepoints
because extract_kerning_fonttools and extract_ligatures_fonttools currently
include full resolved intervals (including >= 0x10000) into kern_left_classes,
kern_right_classes and ligature pair sequences; before any packing operations
(the struct.pack calls that use "<HB" and the 32/64-bit compose for ligatures)
filter out or drop codepoints >= 0x10000 from those collections so only BMP
codepoints are packed; update extract_kerning_fonttools to filter
kern_left_classes and kern_right_classes (remove or skip cp where cp >= 0x10000)
and update extract_ligatures_fonttools to skip ligature sequences containing any
cp >= 0x10000 (or remove those entries) prior to assembling the packed values.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9096e16e-3aa0-4d67-ab1c-1b1b998ce928

📥 Commits

Reviewing files that changed from the base of the PR and between 91de6ac and 81b03e2.

📒 Files selected for processing (2)
  • lib/EpdFont/scripts/fontconvert_sdcard.py
  • lib/EpdFont/scripts/sd-fonts.yaml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: cppcheck
  • GitHub Check: build
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-23T19:55:03.054Z
Learnt from: martinbrook
Repo: crosspoint-reader/crosspoint-reader PR: 1136
File: scripts/patch_jpegdec.py:20-23
Timestamp: 2026-02-23T19:55:03.054Z
Learning: In the crosspoint-reader project, enable Python linters (e.g., Ruff or Flake8) in CI for all Python scripts. Run linting as part of the CI workflow and fix reported issues to maintain code quality. Do not skip linting for any Python files.

Applied to files:

  • lib/EpdFont/scripts/fontconvert_sdcard.py
🔇 Additional comments (3)
lib/EpdFont/scripts/fontconvert_sdcard.py (2)

46-50: LGTM - well-designed preset for modern Greek.

The greek-letters preset is a useful addition for fonts that include modern Greek but lack polytonic glyphs. The comment clearly explains the rationale, and the range (U+0370–U+03FF) is correct for the Greek and Coptic block.


52-60: LGTM - comprehensive Latin+Cyrillic preset.

The latin-cyrillic preset provides good coverage for fonts supporting both scripts. Including the extended Cyrillic blocks (U+1C80, U+2DE0, U+A640) alongside the core ranges is appropriate for fonts with comprehensive Cyrillic support.

lib/EpdFont/scripts/sd-fonts.yaml (1)

36-211: LGTM - consistent and well-organized font configuration updates.

The font family updates are well-executed:

  • All intervals values are consistent with the font descriptions (e.g., fonts described as "Latin, Greek, Cyrillic" use builtin,latin-cyrillic,greek-letters,musical-symbols, while Latin-only fonts use builtin,latin-ext)
  • The new preset references (latin-cyrillic, greek-letters, musical-symbols) match the presets added in fontconvert_sdcard.py
  • Size 10 is consistently added to all 16 families
  • NotoSansExtended correctly includes additional scripts (Georgian, Armenian, Ethiopic) while also adopting the new presets

The behavior change (Greek fonts switching from greek to greek-letters will no longer include polytonic Greek U+1F00–U+1FFF) is documented in the PR objectives and intentional.

Comment thread lib/EpdFont/scripts/fontconvert_sdcard.py
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@lib/EpdFont/scripts/fontconvert_sdcard.py`:
- Around line 57-60: The "latin-cyrillic" preset removed the Latin ligature
block (0xFB00–0xFB06) present in the original "latin-ext" set, which can
suppress standard ligature glyphs; update the "latin-cyrillic" tuple list (the
entry named "latin-cyrillic" in fontconvert_sdcard.py) to include the ligature
range (0xFB00, 0xFB06) alongside the existing ranges so ligature codepoints are
requested when using this preset.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 036d3f90-d841-45cd-a49a-5403754127f6

📥 Commits

Reviewing files that changed from the base of the PR and between 81b03e2 and d0efd11.

📒 Files selected for processing (1)
  • lib/EpdFont/scripts/fontconvert_sdcard.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build
  • GitHub Check: cppcheck
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-23T19:55:03.054Z
Learnt from: martinbrook
Repo: crosspoint-reader/crosspoint-reader PR: 1136
File: scripts/patch_jpegdec.py:20-23
Timestamp: 2026-02-23T19:55:03.054Z
Learning: In the crosspoint-reader project, enable Python linters (e.g., Ruff or Flake8) in CI for all Python scripts. Run linting as part of the CI workflow and fix reported issues to maintain code quality. Do not skip linting for any Python files.

Applied to files:

  • lib/EpdFont/scripts/fontconvert_sdcard.py
🔇 Additional comments (2)
lib/EpdFont/scripts/fontconvert_sdcard.py (2)

320-333: SMP guard in kerning extraction looks correct

Good change: skipping >= 0x10000 here matches the BMP-only <HB packing path and prevents runtime packing failures.


473-485: Ligature SMP filtering is a solid safeguard

This filtering is consistent with BMP-only pair encoding and prevents malformed packed ligature entries.

Comment on lines +57 to +60
"latin-cyrillic": [(0x0020, 0x007E), (0x0080, 0x00FF), (0x0100, 0x024F),
(0x1E00, 0x1EFF), (0x2000, 0x206F),
(0x0400, 0x04FF), (0x0500, 0x052F),
(0x1C80, 0x1C8F), (0x2DE0, 0x2DFF), (0xA640, 0xA69F)],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Potential ligature regression in latin-cyrillic preset

latin-cyrillic appears to replace latin-ext,cyrillic, but it drops (0xFB00, 0xFB06) from latin-ext. That can suppress standard Latin ligatures for families migrated to this preset because ligature outputs are kept only if their codepoints are requested.

Suggested minimal fix
     "latin-cyrillic": [(0x0020, 0x007E), (0x0080, 0x00FF), (0x0100, 0x024F),
                        (0x1E00, 0x1EFF), (0x2000, 0x206F),
+                       (0xFB00, 0xFB06),
                        (0x0400, 0x04FF), (0x0500, 0x052F),
                        (0x1C80, 0x1C8F), (0x2DE0, 0x2DFF), (0xA640, 0xA69F)],
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"latin-cyrillic": [(0x0020, 0x007E), (0x0080, 0x00FF), (0x0100, 0x024F),
(0x1E00, 0x1EFF), (0x2000, 0x206F),
(0x0400, 0x04FF), (0x0500, 0x052F),
(0x1C80, 0x1C8F), (0x2DE0, 0x2DFF), (0xA640, 0xA69F)],
"latin-cyrillic": [(0x0020, 0x007E), (0x0080, 0x00FF), (0x0100, 0x024F),
(0x1E00, 0x1EFF), (0x2000, 0x206F),
(0xFB00, 0xFB06),
(0x0400, 0x04FF), (0x0500, 0x052F),
(0x1C80, 0x1C8F), (0x2DE0, 0x2DFF), (0xA640, 0xA69F)],
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/EpdFont/scripts/fontconvert_sdcard.py` around lines 57 - 60, The
"latin-cyrillic" preset removed the Latin ligature block (0xFB00–0xFB06) present
in the original "latin-ext" set, which can suppress standard ligature glyphs;
update the "latin-cyrillic" tuple list (the entry named "latin-cyrillic" in
fontconvert_sdcard.py) to include the ligature range (0xFB00, 0xFB06) alongside
the existing ranges so ligature codepoints are requested when using this preset.

@mcrosson
Copy link
Copy Markdown
Contributor

We need to be careful about adding a whole bunch of code points/glyphs. There is a ram cost to this and I'd like to be very mindful of this fact before we add more.

I'd like to defer this awhile as there are plenty of other things that have surfaced with fonts that need our attention.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants