fix: Fixed book title in home screen#1013
fix: Fixed book title in home screen#1013daveallie merged 6 commits intocrosspoint-reader:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds multi-line, word-wrapped recent-book title rendering, limits metadata title capture to the first Changes
Sequence Diagram(s)sequenceDiagram
participant Loader as RecentBooksStore::loadFromFile
participant Disk as Disk/File
participant Parser as getDataFromBook
participant Store as RecentBooksStore::saveToFile
Loader->>Disk: open recent-books file
Disk-->>Loader: return file header (version)
alt file version < current
Loader->>Disk: read legacy entries
loop for each entry
Loader->>Parser: getDataFromBook(path)
Parser-->>Loader: metadata/enriched entry
alt enriched non-empty
Loader->>Loader: replace legacy entry with enriched data
else
Loader->>Loader: keep legacy entry
end
end
Loader->>Disk: close file
Loader->>Store: saveToFile(updated list)
Store-->>Disk: write new file (v4)
else version >= current
loop for each entry
Loader->>Disk: read entry
alt title empty
Loader->>Parser: getDataFromBook(path)
Parser-->>Loader: metadata (maybe)
Loader->>Loader: fill missing fields if available
end
end
alt metadataRefreshed
Loader->>Store: saveToFile(updated list)
Store-->>Disk: write refreshed file
end
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested Reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 2
🧹 Nitpick comments (1)
src/components/themes/lyra/LyraTheme.cpp (1)
502-510: Consider extracting max lines as a named constant.The magic number
3appears multiple times for the maximum line count. A named constant would improve readability and make future adjustments easier.Suggested refactor
Add to namespace at top of file:
constexpr int maxTitleLines = 3;Then replace occurrences:
- if (titleLines.size() >= 3) { + if (titleLines.size() >= maxTitleLines) {- if (!currentLine.empty() && titleLines.size() < 3) titleLines.push_back(currentLine); + if (!currentLine.empty() && titleLines.size() < maxTitleLines) titleLines.push_back(currentLine);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/themes/lyra/LyraTheme.cpp` around lines 502 - 510, Extract the hard-coded max line count (3) into a named constant (e.g. add constexpr int maxTitleLines = 3; in the file's namespace) and replace all literal `3` usages that govern title wrapping/ellipsis logic — specifically in the block handling titleLines.size() checks, titleLines.back().resize(... - 3), and any loop that relies on that count — ensuring you still call renderer.getTextWidth(...) and utf8RemoveLastChar(...) as before but use maxTitleLines for readability and future adjustments.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/Epub/Epub/parsers/ContentOpfParser.cppsrc/components/themes/BaseTheme.cppsrc/components/themes/lyra/LyraTheme.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-19T12:17:00.962Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 988
File: lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp:649-661
Timestamp: 2026-02-19T12:17:00.962Z
Learning: In ChapterHtmlSlimParser.cpp, when computing footnote word indices in endElement() for footnote links, the wordIndex must be cumulative across the 750-word mid-paragraph flush boundary. The correct calculation is: `wordIndex = wordsExtractedInBlock + currentTextBlock->size()`, not just `currentTextBlock->size()`. This ensures footnotes attach to the page containing their actual anchor word, even after layoutAndExtractLines(false) has extracted and removed earlier words from the block.
Applied to files:
src/components/themes/BaseTheme.cpp
🧬 Code graph analysis (1)
src/components/themes/BaseTheme.cpp (1)
src/main.cpp (1)
renderer(39-39)
⏰ 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 comments (4)
lib/Epub/Epub/parsers/ContentOpfParser.cpp (1)
104-110: LGTM!The guard correctly ensures only the first
dc:titleis captured. The state machine remains consistent: subsequentdc:titleelements stay inIN_METADATA, socharacterDataignores them andendElement(line 367) is a no-op sincestate != IN_BOOK_TITLE.Minor note: EPUB 3 allows multiple
dc:titleelements with refinements (title-typelike "main", "subtitle"). Assuming the first is always the primary title is a reasonable convention and aligns with typical publisher behavior.src/components/themes/lyra/LyraTheme.cpp (2)
6-10: LGTM!The added includes for
Utf8.hand<vector>are necessary for the new multi-line title wrapping implementation.
537-550: LGTM!The vertical alignment logic correctly calculates the total block height (title lines + author) and centers it within the tile. The per-line rendering with explicit heights provides proper spacing.
src/components/themes/BaseTheme.cpp (1)
519-531: Metric consistency between word trimming and line wrapping should be verifiedAt line 506-513, word trimming uses
getTextWidth(bounding box), but line 519-523 usesgetTextAdvanceX(glyph advance sum) for wrapping. These metrics can differ—if advance width exceeds visual width, a trimmed word could still overflowmaxLineWidth. Consider usinggetTextAdvanceXconsistently for both trimming and wrapping, or document why this mixed approach is intentional. Note: LyraTheme.cpp uses the same pattern, so any change should be coordinated across themes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/themes/lyra/LyraTheme.cpp`:
- Around line 504-509: The truncation loop that modifies titleLines.back() can
underflow when calling resize(titleLines.back().size() - 3); update the loop in
LyraTheme.cpp to guard the subtract-by-3 by checking the current string size
(use titleLines.back().size() > 3) before calling resize or otherwise set the
string to empty if <= 3, then call utf8RemoveLastChar and append "..." as
before; reference the loop using titleLines.back(), renderer.getTextWidth(...,
UI_12_FONT_ID, ...), and utf8RemoveLastChar to locate and fix the code path.
- Around line 513-521: The truncation loop in LyraTheme.cpp that shrinks a word
(using utf8RemoveLastChar) can produce an empty w when even "..." won't fit;
after the while loop that updates wordW and w, add a guard that if w.empty()
then either set w to "..." only if that fits (check renderer.getTextWidth with
"..." and maxLineWidth) or skip appending this word to currentLine entirely
(avoid pushing an empty string into currentLine); ensure the check uses the same
variables (wordW, maxLineWidth, w, renderer) and update currentLine only when w
is non-empty or when "..." fits.
---
Nitpick comments:
In `@src/components/themes/lyra/LyraTheme.cpp`:
- Around line 502-510: Extract the hard-coded max line count (3) into a named
constant (e.g. add constexpr int maxTitleLines = 3; in the file's namespace) and
replace all literal `3` usages that govern title wrapping/ellipsis logic —
specifically in the block handling titleLines.size() checks,
titleLines.back().resize(... - 3), and any loop that relies on that count —
ensuring you still call renderer.getTextWidth(...) and utf8RemoveLastChar(...)
as before but use maxTitleLines for readability and future adjustments.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/themes/BaseTheme.cpp (1)
497-525:⚠️ Potential issue | 🟡 MinorUse consistent text metrics for truncation and line fitting.
Line wrapping (lines 521–525) uses
getTextAdvanceX, but the truncation loop (lines 498, 507, 513) usesgetTextWidth. These metrics can diverge: a word trimmed to fitgetTextWidthmay still exceedmaxLineWidthwhen measured bygetTextAdvanceX, causing overflow (especially whencurrentLineis empty and the overflow check is skipped). UsegetTextAdvanceXconsistently for both truncation and line-fit checks.Suggested fix (consistent metric)
while (!lines.back().empty() && lines.back().size() > 3 && - renderer.getTextWidth(UI_12_FONT_ID, lines.back().c_str()) > maxLineWidth) { + renderer.getTextAdvanceX(UI_12_FONT_ID, lines.back().c_str(), EpdFontFamily::REGULAR) > + maxLineWidth) { // Remove "..." first, then remove one UTF-8 char, then add "..." back lines.back().resize(lines.back().size() - 3); // Remove "..." utf8RemoveLastChar(lines.back()); lines.back().append("..."); } break; } - int wordWidth = renderer.getTextWidth(UI_12_FONT_ID, i.c_str()); + int wordWidth = renderer.getTextAdvanceX(UI_12_FONT_ID, i.c_str(), EpdFontFamily::REGULAR); while (wordWidth > maxLineWidth && !i.empty()) { // Word itself is too long, trim it (UTF-8 safe) utf8RemoveLastChar(i); // Check if we have room for ellipsis std::string withEllipsis = i + "..."; - wordWidth = renderer.getTextWidth(UI_12_FONT_ID, withEllipsis.c_str()); + wordWidth = renderer.getTextAdvanceX(UI_12_FONT_ID, withEllipsis.c_str(), EpdFontFamily::REGULAR); if (wordWidth <= maxLineWidth) { i = withEllipsis; break; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/themes/BaseTheme.cpp` around lines 497 - 525, The truncation loop uses renderer.getTextWidth while later fitting uses renderer.getTextAdvanceX, causing inconsistent measurements; update the truncation and ellipsis-check logic to use renderer.getTextAdvanceX (same font id/UI_12_FONT_ID and EpdFontFamily where applicable) instead of getTextWidth so trimmed words and the withEllipsis check use the same metric as the newLineWidth calculation; adjust calls around utf8RemoveLastChar, the withEllipsis construction, and any comparisons to maxLineWidth (and ensure the initial empty-currentLine overflow check still applies) so lines, i, and currentLine are measured consistently with getTextAdvanceX.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/themes/BaseTheme.cppsrc/components/themes/lyra/LyraTheme.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-19T12:17:00.962Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 988
File: lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp:649-661
Timestamp: 2026-02-19T12:17:00.962Z
Learning: In ChapterHtmlSlimParser.cpp, when computing footnote word indices in endElement() for footnote links, the wordIndex must be cumulative across the 750-word mid-paragraph flush boundary. The correct calculation is: `wordIndex = wordsExtractedInBlock + currentTextBlock->size()`, not just `currentTextBlock->size()`. This ensures footnotes attach to the page containing their actual anchor word, even after layoutAndExtractLines(false) has extracted and removed earlier words from the block.
Applied to files:
src/components/themes/lyra/LyraTheme.cppsrc/components/themes/BaseTheme.cpp
🧬 Code graph analysis (1)
src/components/themes/BaseTheme.cpp (1)
src/main.cpp (1)
renderer(39-39)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (3)
src/components/themes/lyra/LyraTheme.cpp (3)
6-10: LGTM!The new includes are correctly added for the multi-line title wrapping feature:
<Utf8.h>providesutf8RemoveLastChar()for proper UTF-8 character removal, and<vector>is needed forstd::vector<std::string>to store title lines.
485-536: Word-wrapping algorithm looks solid.The logic correctly handles: word splitting, line building with up to 3 lines, ellipsis truncation for overflow, and oversized single-word truncation. The past review concerns about underflow in the ellipsis loop (line 504 guard) and empty truncated words (line 522 skip) are addressed.
Minor observation: initial word-width check at line 512 uses
getTextWidth, while line-building (lines 523-525) usesgetTextAdvanceX. These typically return similar values, but could theoretically differ. If any edge-case truncation issues appear, this would be worth investigating.
538-551: LGTM!The vertical centering and rendering logic is well-structured. The block height calculation correctly accounts for title lines and optional author with appropriate spacing. The rendering loop properly iterates through title lines and conditionally renders the author with a half-line gap.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/components/themes/BaseTheme.cpp`:
- Around line 497-525: The truncation loop uses renderer.getTextWidth while
later fitting uses renderer.getTextAdvanceX, causing inconsistent measurements;
update the truncation and ellipsis-check logic to use renderer.getTextAdvanceX
(same font id/UI_12_FONT_ID and EpdFontFamily where applicable) instead of
getTextWidth so trimmed words and the withEllipsis check use the same metric as
the newLineWidth calculation; adjust calls around utf8RemoveLastChar, the
withEllipsis construction, and any comparisons to maxLineWidth (and ensure the
initial empty-currentLine overflow check still applies) so lines, i, and
currentLine are measured consistently with getTextAdvanceX.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/RecentBooksStore.cpp (1)
138-160: Migration logic is correct; consider checkingsaveToFile()result.The migration path correctly reads version 3 format, re-parses book metadata via
getDataFromBook(), and falls back to cached data when the book file cannot be loaded. One minor observation:Line 158:
saveToFile()return value is ignored. If the save fails, migration will appear successful but won't persist—causing a re-migration on every subsequent load. This is self-correcting but inefficient on constrained devices.💡 Optional: Log a warning if save fails
} inputFile.close(); - saveToFile(); + if (!saveToFile()) { + LOG_WRN("RBS", "Migration completed but failed to persist; will re-migrate on next load"); + } LOG_DBG("RBS", "Migrated recent books from version %u, re-parsed %u entries", version, count); return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/RecentBooksStore.cpp` around lines 138 - 160, The migration currently calls saveToFile() and ignores its result; update the migration branch inside RecentBooksStore (where you read count, fill recentBooks using getDataFromBook(), then call saveToFile()) to capture the boolean return, and if saveToFile() fails emit a warning (e.g., LOG_WRN) and return false so the caller knows migration did not persist; keep the existing fallback push_back behavior for RecentBook entries but ensure you check the saveToFile() return and handle failure instead of always returning true.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/RecentBooksStore.cpp
🔇 Additional comments (1)
src/RecentBooksStore.cpp (1)
14-14: LGTM!Version bump to 4 correctly triggers the migration path for users with version 3 files, enabling the re-parsing of book metadata with the updated title logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/RecentBooksStore.cpp`:
- Around line 138-160: The migration currently calls saveToFile() and ignores
its result; update the migration branch inside RecentBooksStore (where you read
count, fill recentBooks using getDataFromBook(), then call saveToFile()) to
capture the boolean return, and if saveToFile() fails emit a warning (e.g.,
LOG_WRN) and return false so the caller knows migration did not persist; keep
the existing fallback push_back behavior for RecentBook entries but ensure you
check the saveToFile() return and handle failure instead of always returning
true.
daveallie
left a comment
daveallie
left a comment
There was a problem hiding this comment.
Overall looks good, mostly just a little confused as to why the changes in src/RecentBooksStore.cpp are necessary.
| const int spaceWidth = renderer.getSpaceWidth(UI_12_FONT_ID, EpdFontFamily::BOLD); | ||
| std::vector<std::string> titleLines; | ||
| std::string currentLine; | ||
| for (auto& w : words) { |
There was a problem hiding this comment.
It would be great if the text wrapping function could be centralized elsewhere, like in https://github.com/crosspoint-reader/crosspoint-reader/pull/911/changes
That way we could also clean up the classic theme home screen implementation.
There was a problem hiding this comment.
Would it require us to refactor the entire Lyra theme?
| const int spaceWidth = renderer.getSpaceWidth(UI_12_FONT_ID, EpdFontFamily::BOLD); | ||
| std::vector<std::string> titleLines; | ||
| std::string currentLine; | ||
| for (auto& w : words) { |
## Summary * **What is the goal of this PR?** (e.g., Implements the new feature for file uploading.) * The goal is to fix the title of books in the Home Screen. Before  After:  * **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, Cursor
## Summary * **What is the goal of this PR?** (e.g., Implements the new feature for file uploading.) * **What changes are included?** Conrgegate the changes of #1074 , #1013 , and extended upon #911 by @lkristensen New function implemented in GfxRenderer.cpp ```C++ std::vector<std::string> GfxRenderer::wrappedText(const int fontId, const char* text, const int maxWidth, const int maxLines, const EpdFontFamily::Style style) const ``` Applied logic to all uses in Lyra, Lyra Extended, and base theme (continue reading card as pointed out by @znelson ## Additional Context    --- ### 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 >**_ --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…oint-reader#1141) ## Summary * **What is the goal of this PR?** (e.g., Implements the new feature for file uploading.) * **What changes are included?** Conrgegate the changes of crosspoint-reader#1074 , crosspoint-reader#1013 , and extended upon crosspoint-reader#911 by @lkristensen New function implemented in GfxRenderer.cpp ```C++ std::vector<std::string> GfxRenderer::wrappedText(const int fontId, const char* text, const int maxWidth, const int maxLines, const EpdFontFamily::Style style) const ``` Applied logic to all uses in Lyra, Lyra Extended, and base theme (continue reading card as pointed out by @znelson ## Additional Context    --- ### 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 >**_ --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
3 participants
Summary
Before
After:
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, Cursor