Repository navigation
i18n: the CJK fallback sits on the Latin baseline (#1309) - #1395
Merged
Merged
Conversation
FeiLiuEM
added a commit
to FeiLiuEM/effectcraft
that referenced
this pull request
Oct 18, 2026
…s the print ones Windows ships a UI variant of its Japanese and Chinese families (`Meiryo`/`Meiryo UI`, `Yu Gothic`/`Yu Gothic UI`, `MS Gothic`/`MS UI Gothic`, `Microsoft YaHei`/`Microsoft YaHei UI`, `Microsoft JhengHei`/`Microsoft JhengHei UI`) whose kana and kanji are drawn for the screen, and its own link table for `Segoe UI` picks the UI one for everything it draws. The interface should ask for it - but the same lists feed `fonts::fallback_for`, which shapes composed frames, so reordering them would change how existing projects render. The two orders are therefore separate: this leaves `crates/text/src/fonts.rs` exactly as it is and gives the interface its own candidate list in `install_script_fallbacks`, which falls back to the shared choice when the platform has no UI variant, none is installed, or it does not cover the probe. `PMingLiU` is in the interface list for its proportional spacing, not as a sibling of `MingLiU`: it is MingLiU's proportional relative. The rendered order is pinned by a test now: the Japanese list keeps `Yu Gothic` ahead of `Yu Gothic UI`, and no UI name appears in the lists a document renders with. `the_interface_asks_for_the_ platforms_ui_faces` pins the interface list and checks that every other script the install probes has none, so a script that draws correctly in any face keeps the shared order. Review fixes: the locale is mapped once (the call passes the language, the helper maps it), the new test sits after the storytold#394 test it had split from its doc comment, and the fonts.rs test asserts instead of panicking inside a closure. Rebased on current main; storytold#1395 has not landed yet, so there is nothing to rebase after it.
Contributor
Author
|
Just checking in on the order rather than the timing: this and #1322 both change Nothing else is outstanding from my side as far as I can tell: the test reads the sign now, the doc |
Co-Authored-By: DeepSeek V4.1 Flash <noreply@deepseek.com>
Measured on a second machine (Noto Sans CJK JP), the ink-bottom gap after the correction is 1.125 / 1.9375 / 1.71875 pt at 12 / 15 / 18 pt, so the old bound failed at 15 and 18 where the interface is right. Without the correction the kana sits 3-5 pt above the Latin ink, so the sign separates the two and the magnitude only has to allow the face's own kana overshoot. Co-Authored-By: DeepSeek V4.1 Flash <noreply@deepseek.com>
epaint exposes no baseline, but a capital without a descender has its ink bottom on it, so the Latin half of the pair is load-bearing. Suggested by FeiLiuEM in storytold#1309. Co-Authored-By: DeepSeek V4.1 Flash <noreply@deepseek.com>
zitann2020
force-pushed
the
i18n/ja-cjk-baseline
branch
from
October 25, 2026 19:13
54283f5 to
6938cf4
Compare
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Calculate baseline adjustment crates/ui-egui/src/theme.rs |
Native helpers round font metrics at 12 px and derive a size-independent FontTweak from the Inter row font and each fallback face. |
Apply and test fallback adjustment crates/ui-egui/src/theme.rs |
The installer retains the resolved Inter face and assigns a calculated FontTweak to each fallback. A test checks Japanese glyph-bottom drift across four font families and sizes of 12, 15, and 18 pt. |
Priority:
Estimated code review effort: 2 (Simple) | ~12 minutes
Change: Bug fix
Possibly related PRs
- storytold/effectcraft#2257: This PR changes CJK face selection in the same fallback installer.
- storytold/effectcraft#2284: This PR changes fallback resolution and tests metric-compatible font resolution, which relates to the fallback faces used here.
Suggested reviewers: feiliuem
Merge Risk: 
6938c
-
The fallback alignment change has no established merge-blocking issue; it is ready for normal checks.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: aligning the CJK fallback with the Latin baseline. Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 1 files. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Member
|
Merged, thank you @zitann2020! It landed on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The CJK fallback faces sat about 3 pt high at 12 pt, measured against the Latin capital's ink
bottom; they now sit on the same baseline. The tweak's factor is derived at install time from the face
and the row height rather than hardcoded, so a font swap does not need a new number.
Rebased onto main after #1322 landed: that PR gave the interface its own candidate list in
install_script_fallbacks, and this keeps the generated face name it introduced while applying thetweak —
name.clone()rather than the oldname.to_string().Verification:
cargo +1.95 build -p effectcraft-ui-egui -j 1andfmt --checkare clean, andscript_font_testspasses (7 tests).clippywas not run here — the machine has 3.5 GB free and rustcruns out of memory under it — so CI is the authority for that one.
Summary by CodeRabbit