fix: render missing Unicode reader glyphs#104
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds procedural rendering of synthetic glyphs to fill missing Unicode blocks and Greek characters. The implementation defines classification and metric helpers, integrates measurement into text sizing, and wires rendering into the main text pipeline for normal and rotated text. ChangesSynthetic Glyph Rendering for Unicode Block & Greek Fallback Characters
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/EpdFontFamily.cpp`:
- Around line 46-47: The code currently applies
syntheticGlyph::aliasCodepoint(cp) based only on the input codepoint; instead,
first perform the real glyph lookup (the same lookup used elsewhere in this
file, e.g., getGlyphIndex/lookupGlyph or the existing "real glyph" path) and
only if that lookup returns a miss (glyph index == missing/0/-1) then call
syntheticGlyph::aliasCodepoint(cp) and retry the lookup; update all
corresponding renderer paths to follow the exact same miss-then-fallback flow
(the blocks around syntheticGlyph::aliasCodepoint and their counterparts in
lines ~49-98) so fonts that actually contain those glyphs use the real metrics
and future table additions take effect.
In `@lib/GfxRenderer/GfxRenderer.cpp`:
- Around line 162-184: The rotated-90-CW Greek glyph Y coordinate is computed
incorrectly; in drawSyntheticGreekGlyphRotated90CW update the per-pixel Y to
match the bitmap renderer's mapping (cursorY - metrics.left - glyphX).
Concretely, stop using baseY = cursorY - metrics.left - metrics.width + 1 and
baseY - gx; instead compute the Y as cursorY - metrics.left - gx (or set baseY =
cursorY - metrics.left and call renderer.drawPixel(baseX + gy, baseY - gx,
pixelState)), keeping the X calculation (baseX + gy) unchanged so the rotated
glyph covers the same Y span as renderCharImpl<TextRotation::Rotated90CW>.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c12151c-8967-4dc9-aefd-bea4db9cf7af
📒 Files selected for processing (4)
CHANGELOG.mdlib/EpdFont/EpdFontData.hlib/EpdFont/EpdFontFamily.cpplib/GfxRenderer/GfxRenderer.cpp
22ab03f to
23096f9
Compare
|
Addressed the CodeRabbit findings in the latest force-push (
Extra validation after the fix:
|
|
@uxjulia hi there! I spotted a bug in the implementation I submitted, but wasn't able to open a new PR to fix it - you can find it in https://github.com/chriskd/CrossInk/tree/fix/synthetic-unicode-glyphs The bug was that synthetic glyph drawing could write outside the display bounds. This can happen when a synthetic glyph is partially off-screen during layout/rendering, especially in books with lots of block characters such as U+2588 FULL BLOCK. The renderer logs out-of-bounds pixel writes heavily, and on device this manifested as instability/crashes while opening or rendering affected EPUBs. The way this manifested, and how I caught it, was when trying to run KOReader sync on a page where the glyphs were rendered. Doing that would lead to a crash. Sorry for the hassle, and thank you for the great work on this project! |
|
Hey thanks for letting me know! I'll take a look. Sorry, I had to lock down the PRs, I felt like the project was starting to slip away from me a bit with people submitting things left and right that didn't fit with my vision and I felt like I needed to reign things in before it got out of hand. I appreciated your PR though as it addressed an interesting gap without bloating the firmware. |
|
Totally understand! People don’t appreciate the work that goes into maintaining FOSS projects, especially when they catch on and get popular in niche communities. And then I come along with a buggy vibe coded patch 😅 thank you again for the hard work! |
Summary
Why
The primary motivator was There Is No Antimemetics Division, which includes black-bar redactions and symbolic/category text that exposed missing glyph coverage in the tiny firmware fonts. On device, those missing glyphs either collapsed into confusing whitespace or displayed replacement diamonds. This was especially confusing for black-bar redactions because the redacted span looked like a normal gap in the sentence.
I scanned the visible text in that EPUB against the tiny firmware font headers and found the unsupported visible text codepoints were U+02BB, U+0393, U+03B5, U+03C9, U+2588, and U+25A0. This patch handles that set generically rather than adding a book-specific workaround.
Validation
Disclosure
This patch was written with Codex assistance; in plain terms, it was vibecoded and then locally formatted, built, and checked before opening this PR.
Summary by CodeRabbit
New Features
Bug Fixes