feat: Add sd fonts (integrate and improve upstream #1327 by adriancaruana)#147
Merged
feat: Add sd fonts (integrate and improve upstream #1327 by adriancaruana)#147
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds end-to-end SD-card font support: discovery/conversion of .cpfont files, SdCardFont loader with on-demand glyph-miss handling and overflow ring buffer, registry/manager/system facades for lifecycle and font-ID resolution, renderer/cache integration, settings/UI plumbing, and CLI/build scripts. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Layout as ParsedText
participant Renderer as GfxRenderer
participant CacheMgr as FontCacheManager
participant SDFont as SdCardFont
participant SD as SD_Storage
participant Miss as GlyphMissHandler
App->>Renderer: ensureSdCardFontReady(fontId, text)
Renderer->>SDFont: prewarm(text, styleMask, metadataOnly=true)
SDFont->>SD: read glyph metadata (seek/read)
SD-->>SDFont: metadata
SDFont-->>Renderer: metrics cached
Layout->>Renderer: request glyph bitmap(codepoint)
Renderer->>CacheMgr: check caches
alt glyph cached
CacheMgr-->>Layout: bitmap
else miss
Renderer->>Miss: glyphMissHandler(ctx, codepoint)
Miss->>SDFont: load metadata+bitmap into overflow buffer
SDFont->>SD: read metadata+bitmap
SD-->>SDFont: glyph data
SDFont-->>Miss: EpdGlyph* (overflow)
Miss->>Renderer: provide glyph (bitmap via getOverflowBitmap)
Renderer-->>Layout: bitmap
end
App->>Renderer: clearSdCardFontAccumulation()
Renderer->>SDFont: clearAccumulation()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
lib/GfxRenderer/GfxRenderer.h (1)
105-121: Prefer one source-exclusive registration API perfontId.
removeFont(),registerSdCardFont(), andunregisterSdCardFont()can leave the samefontIdin both maps or neither. BecauseisSdCardFont()only consultssdCardFonts_, a stale SD entry will keep routing measurement/prewarm down the SD path even after a built-in family is reinserted. Consider collapsing these into a single helper that updates both backings consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/GfxRenderer/GfxRenderer.h` around lines 105 - 121, removeFont, registerSdCardFont, and unregisterSdCardFont can leave a fontId present in both fontMap and sdCardFonts_ leading to stale routing; implement a single exclusive-registration helper (e.g., setFontRegistration or registerFontExclusive) and update removeFont, registerSdCardFont, and unregisterSdCardFont to call it so that any registration always updates both backings atomically: when registering an SdCardFont for fontId, erase fontMap[fontId] and set sdCardFonts_[fontId]=font; when registering a built-in family via fontMap, erase sdCardFonts_.erase(fontId); when removing via removeFont/unregisterSdCardFont clear the counterpart map as well; keep isSdCardFont() as the single source-of-truth (sdCardFonts_) and locate changes around removeFont, registerSdCardFont, unregisterSdCardFont, fontMap, and sdCardFonts_.lib/Epub/Epub/ParsedText.cpp (1)
162-169: Avoid duplicating the whole paragraph just to prewarm.This creates a second contiguous copy of the paragraph in
allText. On large EPUB paragraphs that is a noticeable temporary heap spike on ESP32-class devices. A chunked/per-word accumulation API would preserve the prewarm benefit without the extra full-buffer allocation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/Epub/Epub/ParsedText.cpp` around lines 162 - 169, The current code builds a full concatenated string (allText) to prewarm the SD card font which duplicates large paragraphs; instead, stream the text to the renderer in small pieces to avoid the temporary full-buffer. Replace the allText accumulation and single ensureSdCardFontReady call with a loop that calls renderer.ensureSdCardFontReady(fontId, piece) for each word and for the intervening single-space (and a trailing hyphen if hyphenationEnabled), e.g. call the method per words[i], then call it with " " between words and "-" at the end if needed, removing the allText buffer entirely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/EpdFont/scripts/fontconvert_sdcard.py`:
- Around line 465-484: Move the face.set_char_size call to before validating
intervals so glyphs are rendered with the correct size: call
face.set_char_size(size << 6, size << 6, 150, 150) prior to the loop that
iterates intervals and calls load_glyph(), ensuring scaled rendering and
avoiding Invalid_Size_Handle; update the code around the "Validating intervals
against font..." block (where intervals, validated_intervals, load_glyph,
style_label and total_glyphs are used) to run after setting the character size.
In `@lib/EpdFont/SdCardFont.cpp`:
- Around line 435-458: The current content hash in SdCardFont.cpp only covers
the header and style TOC (hash variable computed after fnv1a(headerBuf,
HEADER_SIZE) and updated in the style TOC loop) but must also include every
bytes of each style's payload (intervals, glyph metrics, kerning, ligatures,
bitmap data) so edits that leave the TOC unchanged still change the hash; fix by
continuing to feed every payload buffer into fnv1a using the running hash (e.g.,
hash = fnv1a(payloadBuf, payloadSize, hash)) whenever you read each section for
a style (after the STYLE_TOC loop, and inside the code that reads intervals,
glyph metrics, kerning, ligatures and bitmap blobs), and for large bitmap reads
update the hash incrementally while streaming chunks, then assign the final
running hash to contentHash_ before closing the file.
- Around line 1215-1236: Check the return value of file.seekSet() before each
subsequent file.read() in the overflow path: validate the seek when positioning
to glyphFileOff (before reading tempGlyph) and again when positioning to
s.bitmapFileOffset + tempGlyph.dataOffset (before reading tempBitmap); on seek
failure, log an error (with codepoint/style context), close the file,
free/delete any allocated tempBitmap, decrement overflowCount_ if
!wasAtCapacity, and return nullptr so no garbage data is cached (referencing
seekSet, file.read, tempGlyph, tempBitmap, overflowCount_, wasAtCapacity).
In `@lib/EpdFont/SdCardFontManager.cpp`:
- Around line 39-50: The selection loop in SdCardFont/SdCardFontManager.cpp
(variables: family.files, targetPtSize, selected, bestDiff, SdCardFontFileInfo)
is non-deterministic on ties because it only updates when diff < bestDiff;
change this by centralizing the logic into a single chooser function (e.g.,
PickClosestFontFile or ChooseClosestPointSize) used here and in
src/SdCardFontSystem.cpp that compares diffs and, on equal diffs, applies a
deterministic tie-break (for example prefer the smaller pointSize). Update the
loop to call that helper and ensure the helper returns a const
SdCardFontFileInfo* selected deterministically.
In `@lib/Epub/Epub/ParsedText.cpp`:
- Around line 159-169: computeLineBreaks() can insert a fallback hyphen even
when hyphenationEnabled is false, but the current prewarm logic only appends '-'
to allText when hyphenationEnabled is true, causing SD-backed fonts to load
glyph data on demand; update the block that builds allText (the local variable
allText assembled from words) so it always includes a '-' character when calling
renderer.ensureSdCardFontReady(fontId, allText.c_str()) (i.e., append '-'
unconditionally or based on whether any word could be split), and keep using
renderer.ensureSdCardFontReady with the resulting allText to guarantee the
hyphen glyph is prewarmed.
In `@lib/GfxRenderer/FontCacheManager.cpp`:
- Around line 17-19: The SD branch currently dereferences entries in
sdCardFonts_ and treats negative returns from prewarm(...) as success; update
all uses of sdCardFonts_ (e.g., the loop that calls font->clearCache()) to
null-check each font pointer before dereferencing, and change the prewarm(...)
handling (the callsite named prewarm) to explicitly detect negative/error return
values and propagate or log an error instead of returning silently; ensure both
the cache-clear loop and any other blocks working with sdCardFonts_ (the similar
blocks around the prewarm call and later uses) validate pointers and handle
prewarm failures by reporting/failing fast rather than assuming success.
In `@lib/GfxRenderer/GfxRenderer.cpp`:
- Around line 43-46: The prewarm call is skipping kerning/ligature tables but
getTextAdvanceX still calls applyLigatures() and getKerning(), causing layout
drift; change the prewarm invocation or implementation so that when used for
layout measurement it still loads GSUB/GPOS (ligature and kerning) metadata —
e.g., add a flag or branch in prewarm (the method called via
it->second->prewarm) to load ligature/kerning tables even if bitmap data is
skipped, or call prewarm with metadataOnly=false for layout paths; ensure
getTextAdvanceX/applyLigatures/getKerning will find the necessary tables after
prewarm.
In `@src/activities/ActivityManager.cpp`:
- Around line 285-287: Move the ensureSdFontLoaded() call so it executes while
holding the same RenderLock used for renderer state transitions: acquire
RenderLock before calling ensureSdFontLoaded() and before calling
replaceActivity(...) in ActivityManager::goToReader (and similarly in
replaceWithReader), or alternatively move ensureSdFontLoaded() into
ReaderActivity::onEnter() and call it there while holding RenderLock; this
prevents concurrent mutation of renderer.fontMap and renderer.sdCardFonts_ by
SdCardFontManager::loadFamily()/unloadAll() during renderTaskLoop().
In `@src/CrossPointSettings.cpp`:
- Around line 267-279: The code currently checks sdFontFamilyName to decide
Bookerly-style line compression, causing layout to follow an SD font even when
the renderer fell back to another font; change the logic to query the effective
font being used (the same value you use elsewhere to decide rendering/fallback)
instead of sdFontFamilyName, then apply the lineSpacing switch
(TIGHT/NORMAL/WIDE) based on that effective font; update the branch that returns
0.95f/1.0f/1.1f to trigger only when the effective font family is an SD/Bookerly
family (not merely when sdFontFamilyName is set) so per-book overrides and
built-in fallbacks yield correct compression.
In `@src/SdCardFontGlobals.h`:
- Around line 14-18: The resolver resolveSdCardFontId currently only takes const
char* familyName and drops the font-size, causing all lookups to ignore per-book
sizes; change its signature to accept the font-size enum (e.g., add a parameter
like FontSizeEnum fontSize) and update its implementation to perform the
(familyName, fontSizeEnum) lookup, then update all callers to pass the correct
font-size enum (including places that handle per-book overrides and any
CrossPointSettings uses) so the correct SD card font variant is returned.
---
Nitpick comments:
In `@lib/Epub/Epub/ParsedText.cpp`:
- Around line 162-169: The current code builds a full concatenated string
(allText) to prewarm the SD card font which duplicates large paragraphs;
instead, stream the text to the renderer in small pieces to avoid the temporary
full-buffer. Replace the allText accumulation and single ensureSdCardFontReady
call with a loop that calls renderer.ensureSdCardFontReady(fontId, piece) for
each word and for the intervening single-space (and a trailing hyphen if
hyphenationEnabled), e.g. call the method per words[i], then call it with " "
between words and "-" at the end if needed, removing the allText buffer
entirely.
In `@lib/GfxRenderer/GfxRenderer.h`:
- Around line 105-121: removeFont, registerSdCardFont, and unregisterSdCardFont
can leave a fontId present in both fontMap and sdCardFonts_ leading to stale
routing; implement a single exclusive-registration helper (e.g.,
setFontRegistration or registerFontExclusive) and update removeFont,
registerSdCardFont, and unregisterSdCardFont to call it so that any registration
always updates both backings atomically: when registering an SdCardFont for
fontId, erase fontMap[fontId] and set sdCardFonts_[fontId]=font; when
registering a built-in family via fontMap, erase sdCardFonts_.erase(fontId);
when removing via removeFont/unregisterSdCardFont clear the counterpart map as
well; keep isSdCardFont() as the single source-of-truth (sdCardFonts_) and
locate changes around removeFont, registerSdCardFont, unregisterSdCardFont,
fontMap, and sdCardFonts_.
🪄 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: b90ca07b-d519-498f-94b9-bd36eef8a32c
📒 Files selected for processing (29)
.gitignorelib/EpdFont/EpdFont.cpplib/EpdFont/EpdFontData.hlib/EpdFont/FontDecompressor.cpplib/EpdFont/SdCardFont.cpplib/EpdFont/SdCardFont.hlib/EpdFont/SdCardFontManager.cpplib/EpdFont/SdCardFontManager.hlib/EpdFont/SdCardFontRegistry.cpplib/EpdFont/SdCardFontRegistry.hlib/EpdFont/scripts/fontconvert_sdcard.pylib/EpdFont/scripts/generate-sd-fonts.shlib/Epub/Epub/ParsedText.cpplib/GfxRenderer/FontCacheManager.cpplib/GfxRenderer/FontCacheManager.hlib/GfxRenderer/GfxRenderer.cpplib/GfxRenderer/GfxRenderer.hsrc/CrossPointSettings.cppsrc/CrossPointSettings.hsrc/JsonSettingsIO.cppsrc/SdCardFontGlobals.hsrc/SdCardFontSystem.cppsrc/SdCardFontSystem.hsrc/SettingsList.hsrc/activities/ActivityManager.cppsrc/activities/reader/EpubReaderActivity.cppsrc/activities/settings/SettingsActivity.cppsrc/main.cppsrc/network/CrossPointWebServer.cpp
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/activities/reader/EpubReaderActivity.cpp (1)
1742-1795:⚠️ Potential issue | 🔴 CriticalSleep-page rendering will not compile; non-static member function called from static context.
drawCurrentPageToBuffer()is a static function, so the bare calls togetEffectiveReaderLineCompression()at lines 1790 and 1795 are ill-formed C++ and will fail to compile.Beyond the compilation error, the sleep-mode path uses a local font-resolution lambda that returns only built-in font IDs and does not check for SD card fonts. The live reader path uses
getEffectiveReaderFontId(), which respectsSETTINGS.sdFontFamilyNameand callsresolveSdCardFontId()when active. This divergence means sleep-mode redraws load/rebuild a different section cache than the live reader whenever a global SD font is set. SincegetEffectiveReaderLineCompression()depends on the effective font ID to compute compression, the issue compounds.Move the SD-aware font ID resolution into a shared static helper and use it in both the sleep and live paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activities/reader/EpubReaderActivity.cpp` around lines 1742 - 1795, drawCurrentPageToBuffer is static and currently calls the non-static getEffectiveReaderLineCompression and uses a local lambda getEffectiveFontId that ignores SD fonts, causing compile errors and mismatched section caches; fix by extracting SD-aware font-resolution into a shared static helper (e.g. a static getEffectiveReaderFontId or resolveEffectiveFontId) that calls resolveSdCardFontId/uses SETTINGS.sdFontFamilyName like the live-path getEffectiveReaderFontId, then in drawCurrentPageToBuffer call that static helper to obtain the effective font id and pass that font id into getEffectiveReaderLineCompression (or make getEffectiveReaderLineCompression accept a font-id parameter) and use the returned id when calling Section::loadSectionFile and Section::createSectionFile so both sleep and live paths use the same SD-aware resolution.
♻️ Duplicate comments (1)
lib/EpdFont/SdCardFont.cpp (1)
1004-1009:⚠️ Potential issue | 🟠 MajorCheck
seekSet()before prewarm reads.Lines 1006 and 1081 ignore seek failures. If a
.cpfontis truncated or an offset is invalid, the followingread()continues from the previous file position and can cache garbage glyph metadata or bitmap bytes into the mini font data.Also applies to: 1079-1084
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/EpdFont/SdCardFont.cpp` around lines 1004 - 1009, The code calls file.seekSet(fileOff) before reads (in the loop that fills s.miniGlyphs using gIdx, lastReadIndex, fileOff and EpdGlyph) but ignores seek failures, so a failed seek can make subsequent file.read() return garbage; modify the logic to check the return value of file.seekSet(fileOff) and handle failure by aborting the prewarm/read (e.g., set an error flag, log/return error and stop filling s.miniGlyphs) before calling file.read(), and apply the same check to the other prewarm site around lines 1079–1084 to avoid caching invalid glyph metadata or bitmaps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/EpdFont/scripts/fontconvert_sdcard.py`:
- Around line 503-540: The code building pixels4g and pixels2b assumes tightly
packed rows by using enumerate(bitmap.buffer) and x = i % bitmap.width; instead
iterate per-row using the bitmap.pitch (use abs(bitmap.pitch) for signed pitch)
to compute the buffer offset as row_offset = row * abs(bitmap.pitch) and access
bytes via row_offset + col so that pixels4g construction (and subsequent
pixels2b downsampling) uses the correct per-row stride instead of bitmap.width;
update the loops that fill pixels4g and read from it (references: pixels4g,
pixels2b, bitmap.buffer, bitmap.width, bitmap.rows, bitmap.pitch) to use row/col
indexing with pitch and preserve the existing packing logic for odd widths.
In `@lib/EpdFont/SdCardFont.h`:
- Around line 13-15: SdCardFont currently relies on a custom destructor
(~SdCardFont) to free many raw pointers but does not forbid copying or moving,
which would cause shallow copies and double-free/use-after-free; make the class
explicitly non-copyable and non-movable by deleting the copy constructor, copy
assignment, move constructor, and move assignment for SdCardFont (referencing
SdCardFont(), ~SdCardFont(), and class SdCardFont) so any attempts to copy or
move will fail at compile time.
In `@lib/EpdFont/SdCardFontManager.cpp`:
- Around line 11-15: The destructor currently only deletes raw font pointers in
SdCardFontManager::~SdCardFontManager (deleting lf.font) but does not unregister
them from GfxRenderer; update teardown so manager_.unloadAll(renderer) is
invoked before fonts are freed. Add a destructor
SdCardFontSystem::~SdCardFontSystem() that obtains the global/owned GfxRenderer
and calls manager_.unloadAll(renderer) (ensuring this runs before GfxRenderer is
destroyed), or alternatively modify SdCardFontManager to hold a GfxRenderer* and
call manager_.unloadAll(renderer) inside SdCardFontManager::~SdCardFontManager
before deleting lf.font. Ensure you reference and call
manager_.unloadAll(renderer) and perform it prior to deleting lf.font to avoid
stale IDs in GfxRenderer.
---
Outside diff comments:
In `@src/activities/reader/EpubReaderActivity.cpp`:
- Around line 1742-1795: drawCurrentPageToBuffer is static and currently calls
the non-static getEffectiveReaderLineCompression and uses a local lambda
getEffectiveFontId that ignores SD fonts, causing compile errors and mismatched
section caches; fix by extracting SD-aware font-resolution into a shared static
helper (e.g. a static getEffectiveReaderFontId or resolveEffectiveFontId) that
calls resolveSdCardFontId/uses SETTINGS.sdFontFamilyName like the live-path
getEffectiveReaderFontId, then in drawCurrentPageToBuffer call that static
helper to obtain the effective font id and pass that font id into
getEffectiveReaderLineCompression (or make getEffectiveReaderLineCompression
accept a font-id parameter) and use the returned id when calling
Section::loadSectionFile and Section::createSectionFile so both sleep and live
paths use the same SD-aware resolution.
---
Duplicate comments:
In `@lib/EpdFont/SdCardFont.cpp`:
- Around line 1004-1009: The code calls file.seekSet(fileOff) before reads (in
the loop that fills s.miniGlyphs using gIdx, lastReadIndex, fileOff and
EpdGlyph) but ignores seek failures, so a failed seek can make subsequent
file.read() return garbage; modify the logic to check the return value of
file.seekSet(fileOff) and handle failure by aborting the prewarm/read (e.g., set
an error flag, log/return error and stop filling s.miniGlyphs) before calling
file.read(), and apply the same check to the other prewarm site around lines
1079–1084 to avoid caching invalid glyph metadata or bitmaps.
🪄 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: c3f9094d-9b64-4ef1-9fdd-0b1e58aca47e
📒 Files selected for processing (15)
lib/EpdFont/SdCardFont.cpplib/EpdFont/SdCardFont.hlib/EpdFont/SdCardFontManager.cpplib/EpdFont/SdCardFontRegistry.cpplib/EpdFont/SdCardFontRegistry.hlib/EpdFont/scripts/fontconvert_sdcard.pylib/GfxRenderer/FontCacheManager.cpplib/GfxRenderer/GfxRenderer.cppsrc/CrossPointSettings.cppsrc/SdCardFontGlobals.hsrc/SdCardFontSystem.cppsrc/activities/ActivityManager.cppsrc/activities/reader/EpubReaderActivity.cppsrc/activities/reader/EpubReaderActivity.hsrc/activities/reader/EpubReaderMenuActivity.cpp
✅ Files skipped from review due to trivial changes (1)
- src/SdCardFontGlobals.h
🚧 Files skipped from review as they are similar to previous changes (4)
- src/activities/ActivityManager.cpp
- src/SdCardFontSystem.cpp
- lib/GfxRenderer/GfxRenderer.cpp
- lib/GfxRenderer/FontCacheManager.cpp
Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/EpdFont/SdCardFont.h (1)
13-15:⚠️ Potential issue | 🔴 CriticalBlock copying/moving for this owning type.
At Line 13–15,
SdCardFonthas a custom destructor and owns multiple raw pointers (directly and viaPerStyle/overflow entries), but copy/move operations are still implicitly available. That can cause shallow-copy double-free/UAF.Suggested fix
SdCardFont() = default; ~SdCardFont(); + SdCardFont(const SdCardFont&) = delete; + SdCardFont& operator=(const SdCardFont&) = delete; + SdCardFont(SdCardFont&&) = delete; + SdCardFont& operator=(SdCardFont&&) = delete;#!/bin/bash set -euo pipefail echo "== Locate SdCardFont header/source ==" fd -a 'SdCardFont\.(h|hpp|cpp)$' echo echo "== Verify special member declarations in class ==" rg -nP -C2 --type=cpp --type=h --type=hpp \ 'class\s+SdCardFont\b|~SdCardFont\s*\(|SdCardFont\s*\(\s*const\s+SdCardFont\s*&\s*\)\s*=\s*delete|operator=\s*\(\s*const\s+SdCardFont\s*&\s*\)\s*=\s*delete|SdCardFont\s*\(\s*SdCardFont\s*&&\s*\)\s*=\s*delete|operator=\s*\(\s*SdCardFont\s*&&\s*\)\s*=\s*delete' \ lib/EpdFont/SdCardFont.h echo echo "== Check for by-value SdCardFont usage in repo ==" rg -nP -C2 --type=cpp --type=h --type=hpp \ '\bSdCardFont\b\s+\w+\s*(=|;|\))|std::(vector|array|deque|optional)\s*<\s*SdCardFont\b'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/EpdFont/SdCardFont.h` around lines 13 - 15, SdCardFont declares a custom destructor and owns raw pointers (directly and via PerStyle/overflow entries), so you must avoid implicit shallow copies; update the class to follow the Rule of Five by explicitly disabling copy operations and either implementing or defaulting move operations: add a deleted copy constructor and copy assignment operator (e.g., SdCardFont(const SdCardFont&) = delete; operator=(const SdCardFont&) = delete) and then implement or default a move constructor and move assignment (SdCardFont(SdCardFont&&) noexcept and operator=(SdCardFont&&) noexcept) that transfer ownership of the internal pointers (PerStyle entries and overflow data) and null out the source pointers to prevent double-free/UAF in the destructor; locate these changes in the SdCardFont declaration and ensure any code constructing/returning SdCardFont by value is updated to use pointers/refs or move semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/Epub/Epub/ParsedText.cpp`:
- Around line 160-166: The current loop always inserts a space between tokens
(using if (i > 0) allText += ' '), which incorrectly separates continuation
tokens; update both the totalSize calculation and the concatenation loop to skip
adding an inter-word space when the current token is a continuation token
(and/or the previous token indicates continuation). Specifically, adjust the
earlier totalSize increment (currently totalSize += words.size() - 1) to only
count spaces for non-continuation boundaries, and replace the if (i > 0) allText
+= ' ' in the loop with a check like: if (i > 0 && !isContinuation(words[i]))
allText += ' '; implement or call an existing
isContinuation/isContinuationPrefix helper to detect continuation tokens so
continuation tokens remain attached and no extra spaces are reserved or
inserted.
---
Duplicate comments:
In `@lib/EpdFont/SdCardFont.h`:
- Around line 13-15: SdCardFont declares a custom destructor and owns raw
pointers (directly and via PerStyle/overflow entries), so you must avoid
implicit shallow copies; update the class to follow the Rule of Five by
explicitly disabling copy operations and either implementing or defaulting move
operations: add a deleted copy constructor and copy assignment operator (e.g.,
SdCardFont(const SdCardFont&) = delete; operator=(const SdCardFont&) = delete)
and then implement or default a move constructor and move assignment
(SdCardFont(SdCardFont&&) noexcept and operator=(SdCardFont&&) noexcept) that
transfer ownership of the internal pointers (PerStyle entries and overflow data)
and null out the source pointers to prevent double-free/UAF in the destructor;
locate these changes in the SdCardFont declaration and ensure any code
constructing/returning SdCardFont by value is updated to use pointers/refs or
move semantics.
🪄 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: 7597a2a6-d20b-4b51-969a-ad1b3505946b
📒 Files selected for processing (3)
lib/EpdFont/SdCardFont.cpplib/EpdFont/SdCardFont.hlib/Epub/Epub/ParsedText.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/EpdFont/SdCardFont.cpp
Co-authored-by: Copilot <copilot@github.com>
jpirnay
changed the title
Co-authored-by: Copilot <copilot@github.com>
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
1 participant
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.
Summary
Additional Context
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 >
Summary by CodeRabbit
New Features
Behavior Changes
Chores