feat: Add text wrapping to Lyra Extended theme#1074
feat: Add text wrapping to Lyra Extended theme#1074iandchasse wants to merge 6 commits intocrosspoint-reader:masterfrom
Conversation
📝 WalkthroughWalkthroughThe changes implement UTF-8 aware word-wrapping for book titles in the Lyra theme component, replacing single-line truncation with multi-line support (up to 3 lines). A dynamic height calculation adjusts the selection box based on the number of wrapped title lines, and the home cover tile height metric is increased from 287 to 300 pixels. Changes
Possibly related PRs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/themes/lyra/Lyra3CoversTheme.cpp (2)
92-146: Consider extracting text wrapping logic into a helper function.This 55-line block handles word parsing, line wrapping, and ellipsis truncation all inline. Extracting it into a separate helper (e.g.,
wrapTitleToLines(renderer, title, maxLineWidth, maxLines)) would improve readability and make it reusable/testable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/themes/lyra/Lyra3CoversTheme.cpp` around lines 92 - 146, The inline 55-line text-wrapping block should be extracted into a reusable helper to improve readability and testability: create a function (e.g., wrapTitleToLines or wrapTextToLines) that accepts Renderer& renderer, const std::string& title, int maxLineWidth, int maxLines, font identifiers (SMALL_FONT_ID, EpdFontFamily::REGULAR) and returns vector<string> titleLines; move all word splitting, truncation logic (including utf8RemoveLastChar usage and ellipsis handling) into that helper, replace the block in Lyra3CoversTheme.cpp with a call to the new helper, and ensure behavior (spaceWidth calculation, per-word truncation, and final line ellipsizing) is preserved.
20-20: Remove unused global variablecoverWidth.This variable is never used in the file. Line 54 declares a local
float coverWidththat shadows it, making this global completely unreachable.🧹 Proposed fix
constexpr int hPaddingInSelection = 8; constexpr int cornerRadius = 6; -int coverWidth = 0; } // namespace🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/themes/lyra/Lyra3CoversTheme.cpp` at line 20, Remove the unused global variable coverWidth declared at the top of Lyra3CoversTheme.cpp; it is shadowed by a local float coverWidth later in the file and never referenced elsewhere, so delete the global declaration to avoid shadowing and unused-symbol warnings.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/themes/lyra/Lyra3CoversTheme.cppsrc/components/themes/lyra/Lyra3CoversTheme.h
⏰ 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 (2)
src/components/themes/lyra/Lyra3CoversTheme.h (1)
28-28: LGTM!The increased tile height appropriately accommodates the new 3-line text wrapping layout introduced in the corresponding implementation file.
src/components/themes/lyra/Lyra3CoversTheme.cpp (1)
148-170: LGTM!The dynamic title box height calculation correctly adapts to the actual number of wrapped lines, and the selection box/text rendering logic properly uses these computed values.
🤖 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/components/themes/lyra/Lyra3CoversTheme.cpp`:
- Around line 92-146: The inline 55-line text-wrapping block should be extracted
into a reusable helper to improve readability and testability: create a function
(e.g., wrapTitleToLines or wrapTextToLines) that accepts Renderer& renderer,
const std::string& title, int maxLineWidth, int maxLines, font identifiers
(SMALL_FONT_ID, EpdFontFamily::REGULAR) and returns vector<string> titleLines;
move all word splitting, truncation logic (including utf8RemoveLastChar usage
and ellipsis handling) into that helper, replace the block in
Lyra3CoversTheme.cpp with a call to the new helper, and ensure behavior
(spaceWidth calculation, per-word truncation, and final line ellipsizing) is
preserved.
- Line 20: Remove the unused global variable coverWidth declared at the top of
Lyra3CoversTheme.cpp; it is shadowed by a local float coverWidth later in the
file and never referenced elsewhere, so delete the global declaration to avoid
shadowing and unused-symbol warnings.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
open-x4-sdksrc/components/themes/lyra/Lyra3CoversTheme.cpp
✅ Files skipped from review due to trivial changes (1)
- open-x4-sdk
🔇 Additional comments (1)
src/components/themes/lyra/Lyra3CoversTheme.cpp (1)
8-11: LGTM — required headers for wrapping logic.The added includes look appropriate for the new word-wrapping behavior.
🤖 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/Lyra3CoversTheme.cpp`:
- Around line 110-119: The truncation logic in the for-loop that builds
titleLines (in Lyra3CoversTheme.cpp) can append "..." twice to the third line;
before appending "..." to titleLines.back() check whether it already ends with
"..." (using a UTF-8 safe check of the last 3 bytes/characters) and only append
if it does not; then run the existing width-adjust loop (which calls
renderer.getTextWidth(SMALL_FONT_ID, ..., EpdFontFamily::REGULAR) and uses
utf8RemoveLastChar) as-is to shrink the line until it fits. Ensure the guard
uses the same string used by utf8RemoveLastChar to avoid double-ellipsis while
preserving UTF-8 safety.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/themes/lyra/Lyra3CoversTheme.cpp (1)
152-152: Consider extracting the magic number5to a named constant.The value
5appears here and again at line 167, representing vertical padding above the title text. Extracting it to a named constant (e.g.,vPaddingInSelectionortitleTextTopPadding) would improve maintainability.♻️ Suggested refactor
Add to the anonymous namespace at the top:
constexpr int vPaddingInSelection = 5;Then update both usages:
- const int dynamicTitleBoxHeight = dynamicBlockHeight + hPaddingInSelection + 5; + const int dynamicTitleBoxHeight = dynamicBlockHeight + hPaddingInSelection + vPaddingInSelection;- int currentY = tileY + Lyra3CoversMetrics::values.homeCoverHeight + hPaddingInSelection + 5; + int currentY = tileY + Lyra3CoversMetrics::values.homeCoverHeight + hPaddingInSelection + vPaddingInSelection;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/themes/lyra/Lyra3CoversTheme.cpp` at line 152, Extract the magic number 5 into a named constexpr and replace both usages so padding is clear and maintainable: add a constexpr int vPaddingInSelection = 5 to the anonymous namespace in Lyra3CoversTheme.cpp, then update the expressions that compute dynamicTitleBoxHeight (which currently uses dynamicBlockHeight + hPaddingInSelection + 5) and the other occurrence at the later line (line 167) to use vPaddingInSelection instead of the literal 5.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/themes/lyra/Lyra3CoversTheme.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-19T12:17:05.421Z
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:05.421Z
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/Lyra3CoversTheme.cpp
⏰ 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 (5)
src/components/themes/lyra/Lyra3CoversTheme.cpp (5)
8-11: LGTM!The new includes are appropriate for the UTF-8 aware word-wrapping functionality.
92-103: LGTM!The word-splitting logic correctly handles edge cases like leading spaces, consecutive spaces, and uses
reserve(8)as a reasonable optimization for typical title lengths.
110-133: The double-ellipsis concern from previous review has been addressed.The guard at lines 112-114 correctly prevents appending "..." when the line already ends with it, using the suggested fix. The UTF-8 truncation logic using
utf8RemoveLastCharis appropriate for multi-byte character handling.
[duplicate_comment, approve_code_changes]
135-147: LGTM!The line-building logic correctly uses
getTextAdvanceXfor cumulative width measurement and properly handles word wrapping with space width calculations.
154-171: LGTM!The selection box rendering correctly adapts its height based on the number of title lines, and the text rendering loop properly positions each line with consistent spacing.
🤖 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/components/themes/lyra/Lyra3CoversTheme.cpp`:
- Line 152: Extract the magic number 5 into a named constexpr and replace both
usages so padding is clear and maintainable: add a constexpr int
vPaddingInSelection = 5 to the anonymous namespace in Lyra3CoversTheme.cpp, then
update the expressions that compute dynamicTitleBoxHeight (which currently uses
dynamicBlockHeight + hPaddingInSelection + 5) and the other occurrence at the
later line (line 167) to use vPaddingInSelection instead of the literal 5.
|
|
||
| for (auto& w : words) { | ||
| if (titleLines.size() >= 3) { | ||
| if (titleLines.back().size() < 3 || titleLines.back().compare(titleLines.back().size() - 3, 3, "...") != 0) { |
There was a problem hiding this comment.
It might be nice to use a UTF-8 ellipsis character here instead of three periods
| int wordW = renderer.getTextWidth(SMALL_FONT_ID, w.c_str(), EpdFontFamily::REGULAR); | ||
| while (wordW > maxLineWidth && !w.empty()) { | ||
| utf8RemoveLastChar(w); | ||
| std::string withE = w + "..."; | ||
| wordW = renderer.getTextWidth(SMALL_FONT_ID, withE.c_str(), EpdFontFamily::REGULAR); | ||
| if (wordW <= maxLineWidth) { |
There was a problem hiding this comment.
We have almost the same logic in GfxRenderer::truncatedText, could this be simplified to use that implementation?
| // Wrap title to up to 3 lines (word-wrap by advance width) | ||
| const std::string& lastBookTitle = recentBooks[i].title; |
There was a problem hiding this comment.
Overall, this algorithm overlaps a lot with what's already in BaseTheme::drawRecentBookCover. I wonder if we could add a new utility for doing this type of multiline layout, like:
/// Word-wrap `text` into at most `maxLines` lines, each no wider than
/// `maxWidth` pixels. Overflowing words and excess lines are UTF-8-safely
/// truncated with an ellipsis (U+2026).
std::vector<std::string> wrapText(
const GfxRenderer& renderer,
int fontId,
const std::string& text,
int maxWidth,
int maxLines,
EpdFontFamily::Style style = EpdFontFamily::REGULAR);
## 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>
2 participants
Summary
Wrap the text of the 3-cover Lyra Theme to follow the work from fix: Fixed book title in home screen #1013 but preceding the work to be done continuing from feat: Add ability to wrap text to UI elements #911
Changes font size for 3 cover theme, works nicely, wraps up to 3 lines of text for each book. Still maintains good spacing.
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 >