Conversation
Propagate uint32_t deadline through the full rendering stack: - Epub::generateThumbBmp → readItemContentsToStream → ZipFile::readFileToStream - ZipFile: deadline check in stored (read loop) and deflated (inflate loop) paths - JpegToBmpConverter: deadline check in JPEGDEC bmpDrawCallback (per MCU row) - PngToBmpConverter: deadline check per scanline in decode loop - All functions use deadline=0 default for backward compatibility - Uses millis rollover-safe comparison: static_cast<int32_t>(millis() - deadline) >= 0
…and drawBitmap fallback - GfxRenderer: drawBitmap/drawBitmap1Bit now return bool for error detection - UITheme: add getAllCoverHeights() for thumbnail generation - BaseTheme: add BookCoverParams struct, drawClassicalBookCover method, stale buffer clearing, and drawBitmap failure fallback to placeholder - LyraTheme/Lyra3CoversTheme: gate cover rendering on coverDisabled flag - Replace Bookerly font references with Noto Serif (master licensing change)
Check return value of readItemContentsToStream in generateThumbBmp for both JPG and PNG paths to properly handle timeout/extraction failures.
- GfxRenderer: drawBitmap/drawBitmap1Bit return bool instead of void - JpegToBmpConverter/PngToBmpConverter: add missing deadline param to internal callers - Lyra3CoversTheme: fix .values. access and handle drawBitmap failure
- Add confirmation popup when switching Home Screen Cover to Disabled with Disable (future only) and Remove (delete ePub thumbnails) options - Add drawDialogBackground() to BaseTheme with theme-aware styling - Move Home Screen Cover setting after UI Theme in Display settings - Rename setting label from Home Cover to Home Screen Cover - Document Home Screen Cover in USER_GUIDE.md section 3.6.1
…ettings Add drawPopupSelection virtual method to BaseTheme with per-theme overrides: - Classic: solid black fillRect with white inverted text - Lyra: light gray fillRoundedRect with black text - RoundedRaff: black fillRoundedRect with white inverted text Simplify cover popup: single-line descriptions, 20% wider dialog, centered text in selection highlight, no wrappedText logic needed.
… enum and fix resetAllCoverDisabled calls
…heme rendering - Rebuild EPUB cache when missing (load(true,true)) so generateThumbBmp succeeds - Don't re-disable covers on forced regeneration failure (else if !isBookForced) - Add missing hasCover=false when openFileForRead fails in Lyra3CoversTheme - Set coverRendered=true unconditionally to prevent infinite SD re-reads - Fix tileX to include rect.x in Lyra3CoversTheme, LyraTheme, RoundedRaffTheme - Move coverWidth inside anonymous namespace in RoundedRaffTheme (fix linkage) - Swap coverRendered/coverBufferStored order in BaseTheme - Clear forceRenderCoverPath on Delete All in SettingsActivity - Use translated button labels instead of hardcoded English - Handle XTC files in deleteAllCoverThumbs() - Remove dead handleCoverDisablePopup() declaration - Check readItemContentsToStream return in Epub generateCoverBmp - Clean up file handles and temp files in all error paths - Use nothrow allocation in PngToBmpConverter
An empty marker file caused Storage.exists() to return true, preventing future regeneration attempts for books with unsupported cover formats.
The mappedInput.mapLabels style made hints indistinguishable from surrounding text; bracketed labels improve readability.
Adds a 5th short press power button option 'Action Menu' that opens a navigable popup with Sleep, Refresh Screen, and Screenshot actions. Includes theme-aware popup rendering (Classic/Lyra/RoundedRaff) using drawDialogBackground and drawPopupSelection virtual methods.
feat: apply custom cross logo to boot and sleep screens
… HomeActivity conflicts)
…o and orphan function
refactor: remove RoundedRaff theme
📝 WalkthroughWalkthroughThis PR refactors sleep screen rendering with new modes and filters, adds interactive home cover management, implements deadline-aware image generation, enables bitmap rendering success reporting, removes the RoundedRaff theme, and integrates a power button menu activity. ChangesSleep Screen Modes, Cover Management, and Bitmap Rendering
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/activities/boot_sleep/SleepActivity.cpp (1)
162-228:⚠️ Potential issue | 🟠 Major | ⚡ Quick winChange
renderBitmapSleepScreen()to returnbooland propagate renderer failures to callers.The method currently returns
voidand ignores failures fromrenderer.drawBitmap(), which returnsbool. Meanwhile,renderCustomSleepScreen()andrenderCoverSleepScreen()already returnboolto signal success/failure to their callers.When header parsing succeeds but drawing fails (corrupt/unsupported bitmap),
renderBitmapSleepScreen()has no way to report the failure. The calling methods then returntrueanyway, suppressing the fallback-to-logo logic. Other code in BaseTheme, LyraTheme, and Lyra3CoversTheme correctly checksdrawBitmap()return values.Update
renderBitmapSleepScreen()to returnbool, have it returnfalseif anydrawBitmap()call fails (lines 195, 207, 213), and update both callers to check and propagate that return value.🤖 Prompt for 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. In `@src/activities/boot_sleep/SleepActivity.cpp` around lines 162 - 228, renderBitmapSleepScreen currently returns void and ignores the boolean result of renderer.drawBitmap(), so drawing failures aren't propagated and callers (renderCustomSleepScreen, renderCoverSleepScreen) incorrectly treat failures as success; change renderBitmapSleepScreen to return bool, check the return value of each renderer.drawBitmap(...) call (the initial draw and the two grayscale draws) and return false immediately if any call fails, otherwise return true at the end, then update renderCustomSleepScreen and renderCoverSleepScreen to check and propagate the new bool return value from renderBitmapSleepScreen so drawing failures trigger the fallback logic.
🤖 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/PngToBmpConverter/PngToBmpConverter.cpp`:
- Around line 643-646: The scaling path currently allocates rowAccum and
rowCount with new (std::nothrow) but never checks for null, which causes a crash
on OOM; inside the needsScaling branch (where rowAccum, rowCount and
nextOutY_srcStart are set) add null checks after each allocation, free any
partially allocated buffer (delete[] rowAccum and/or delete[] rowCount), set
pointers to nullptr, and return or propagate a clean error (e.g., return false
or set an error state) from the enclosing function (the function containing the
needsScaling logic in PngToBmpConverter.cpp) so the caller can handle allocation
failure instead of dereferencing a null pointer. Ensure all code paths that
assume these buffers exist only run after successful allocation.
In `@lib/ZipFile/ZipFile.cpp`:
- Around line 444-445: ZipFile::readFileToStream must guard against a zero
chunkSize to avoid non-progress loops; add an early check (before creating
ScopedOpenClose or immediately after) that if chunkSize == 0 the function
returns false (or fails fast), and include an appropriate debug/error log if
available; locate the check in ZipFile::readFileToStream around the
ScopedOpenClose usage and return early when chunkSize is zero.
In `@src/activities/boot_sleep/SleepActivity.cpp`:
- Around line 50-63: renderLogoSleepScreen() currently calls
renderer.invertScreen() when SETTINGS.sleepScreenFilter is not FILTER_NEGATIVE,
which is the inverse of how renderBlankSleepScreen() and
renderBitmapSleepScreen() handle FILTER_NEGATIVE; change the branch in
renderLogoSleepScreen() so that renderer.invertScreen() is called only when
SETTINGS.sleepScreenFilter ==
CrossPointSettings::SLEEP_SCREEN_FILTER::FILTER_NEGATIVE to match the other
sleep-screen paths and ensure correct polarity for the logo fallback.
In `@src/activities/home/HomeActivity.cpp`:
- Around line 97-107: The current logic treats any false from
epub.generateThumbBmp(...) as a permanent failure and persists coverDisabled;
change epub.generateThumbBmp to return a richer result (e.g., a struct or
enum/tuple like {bool success; bool timedOut}) so callers can distinguish
timeout vs real failure, then update this call site (the loadRecentCovers block
using deadline, isBookForced, COVER_RENDER_TIMEOUT_MS, epub.generateThumbBmp,
RECENT_BOOKS.setCoverDisabled, and book.coverDisabled) to only persist/set
coverDisabled when success==false AND timedOut==false (i.e., a non-timeout
failure), leaving timed-out attempts as transient (do not call
RECENT_BOOKS.setCoverDisabled nor set book.coverDisabled on timedOut); keep
existing behavior for successful renders and for forced books.
- Around line 76-77: The loop that regenerates covers is capped to only index 0
when forceRegenerate is false (bookCount is set to 1); change bookCount logic so
it iterates over all recentBooks (or up to the active theme's requested count)
instead of just 1. Specifically, update the calculation of bookCount (currently:
int bookCount = forceRegenerate ? static_cast<int>(recentBooks.size()) : 1;) to
compute the full desired count (e.g., int bookCount =
static_cast<int>(recentBooks.size()); or int bookCount =
std::min(static_cast<int>(recentBooks.size()), themeRequestedCount);) so the
for-loop (for (int bi = 0; bi < bookCount && bi <
static_cast<int>(recentBooks.size()); bi++)) will regenerate covers for books
0..N-1 when not forcing a full regenerate.
In `@src/activities/reader/XtcReaderActivity.cpp`:
- Around line 50-52: Guard setting APP_STATE.pendingCoverGeneration behind a
valid reader instance: check the reader pointer (xtc or the reader instance used
in XtcReaderActivity) is non-null/valid before assigning
APP_STATE.pendingCoverGeneration = true, leaving
APP_STATE.readerActivityLoadCount = 0 and APP_STATE.saveToFile() as needed; only
queue cover generation when the reader is confirmed valid to avoid enqueuing
stale work.
In `@src/components/themes/BaseTheme.cpp`:
- Around line 484-487: The code currently sets coverRendered = true immediately
after drawing and then calls storeCoverBuffer(), which can fail and leave
coverRendered true without a stored buffer; change the flow so coverRendered is
only set when storeCoverBuffer() returns success (e.g., assign coverBufferStored
= storeCoverBuffer() and then set coverRendered = coverBufferStored), ensuring
renderer.drawBitmap/drawRect are followed by a successful buffer snapshot before
marking the cover as rendered.
In `@src/images/CrossLarge.h`:
- Line 1: The file CrossLarge.h contains a UTF-8 BOM before the directive;
remove the BOM so the very first byte is '#' and the line reads "`#pragma` once"
exactly, then save the file as UTF-8 without BOM (e.g., using your editor's
"Save without BOM" or re-encode the file); this ensures the preprocessor sees
the `#pragma` once token correctly in CrossLarge.h.
In `@src/JsonSettingsIO.cpp`:
- Around line 243-282: The migration block is triggering for new-format files
because it checks doc["sleepScreen"] (shared by old and new formats); change the
guard so the legacy conversion runs only when legacy-only keys exist (e.g.,
doc["sleepScreenCoverMode"] or doc["sleepScreenCoverFilter"]) and remove the
doc["sleepScreen"] check; update the if condition around the migration in
JsonSettingsIO.cpp to check only !doc["sleepScreenCoverMode"].isNull() ||
!doc["sleepScreenCoverFilter"].isNull(), leaving the rest of the mapping logic
that assigns s.sleepScreen and s.sleepScreenFilter (and uses
CrossPointSettings::SLEEP_SCREEN_MODE / _FILTER enums) unchanged so
current-format sleepScreen values are not reinterpreted.
---
Outside diff comments:
In `@src/activities/boot_sleep/SleepActivity.cpp`:
- Around line 162-228: renderBitmapSleepScreen currently returns void and
ignores the boolean result of renderer.drawBitmap(), so drawing failures aren't
propagated and callers (renderCustomSleepScreen, renderCoverSleepScreen)
incorrectly treat failures as success; change renderBitmapSleepScreen to return
bool, check the return value of each renderer.drawBitmap(...) call (the initial
draw and the two grayscale draws) and return false immediately if any call
fails, otherwise return true at the end, then update renderCustomSleepScreen and
renderCoverSleepScreen to check and propagate the new bool return value from
renderBitmapSleepScreen so drawing failures trigger the fallback logic.
🪄 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: 8f347b34-a0e8-4241-b7ae-5f16d83e05f3
📒 Files selected for processing (53)
USER_GUIDE.mdlib/Epub/Epub.cpplib/Epub/Epub.hlib/GfxRenderer/GfxRenderer.cpplib/GfxRenderer/GfxRenderer.hlib/I18n/translations/czech.yamllib/I18n/translations/english.yamllib/I18n/translations/french.yamllib/I18n/translations/german.yamllib/I18n/translations/italian.yamllib/I18n/translations/polish.yamllib/I18n/translations/portuguese.yamllib/I18n/translations/russian.yamllib/I18n/translations/spanish.yamllib/I18n/translations/swedish.yamllib/I18n/translations/ukrainian.yamllib/JpegToBmpConverter/JpegToBmpConverter.cpplib/JpegToBmpConverter/JpegToBmpConverter.hlib/PngToBmpConverter/PngToBmpConverter.cpplib/PngToBmpConverter/PngToBmpConverter.hlib/ZipFile/ZipFile.cpplib/ZipFile/ZipFile.hsrc/CrossPointSettings.cppsrc/CrossPointSettings.hsrc/CrossPointState.hsrc/JsonSettingsIO.cppsrc/RecentBooksStore.cppsrc/RecentBooksStore.hsrc/SettingsList.hsrc/activities/boot_sleep/BootActivity.cppsrc/activities/boot_sleep/SleepActivity.cppsrc/activities/boot_sleep/SleepActivity.hsrc/activities/home/HomeActivity.cppsrc/activities/reader/EpubReaderActivity.cppsrc/activities/reader/EpubReaderMenuActivity.cppsrc/activities/reader/EpubReaderMenuActivity.hsrc/activities/reader/ReaderActivity.cppsrc/activities/reader/XtcReaderActivity.cppsrc/activities/settings/SettingsActivity.cppsrc/activities/settings/SettingsActivity.hsrc/activities/util/PowerButtonMenuActivity.cppsrc/activities/util/PowerButtonMenuActivity.hsrc/components/UITheme.cppsrc/components/UITheme.hsrc/components/themes/BaseTheme.cppsrc/components/themes/BaseTheme.hsrc/components/themes/lyra/Lyra3CoversTheme.cppsrc/components/themes/lyra/LyraTheme.cppsrc/components/themes/lyra/LyraTheme.hsrc/components/themes/roundedraff/RoundedRaffTheme.cppsrc/components/themes/roundedraff/RoundedRaffTheme.hsrc/images/CrossLarge.hsrc/main.cpp
💤 Files with no reviewable changes (12)
- lib/I18n/translations/french.yaml
- src/components/themes/roundedraff/RoundedRaffTheme.h
- lib/I18n/translations/swedish.yaml
- lib/I18n/translations/polish.yaml
- lib/I18n/translations/ukrainian.yaml
- lib/I18n/translations/portuguese.yaml
- lib/I18n/translations/russian.yaml
- lib/I18n/translations/czech.yaml
- lib/I18n/translations/spanish.yaml
- src/components/themes/roundedraff/RoundedRaffTheme.cpp
- lib/I18n/translations/italian.yaml
- lib/I18n/translations/german.yaml
Comment on lines
643
to
646
| if (needsScaling) { | ||
| rowAccum = new uint32_t[outWidth](); | ||
| rowCount = new uint16_t[outWidth](); | ||
| rowAccum = new (std::nothrow) uint32_t[outWidth](); | ||
| rowCount = new (std::nothrow) uint16_t[outWidth](); | ||
| nextOutY_srcStart = scaleY_fp; |
There was a problem hiding this comment.
Fail fast when the scaling buffers cannot be allocated.
rowAccum and rowCount are dereferenced unconditionally in the scaling path, so new (std::nothrow) turns an OOM into a null-pointer crash here instead of a clean failure.
Suggested fix
if (needsScaling) {
rowAccum = new (std::nothrow) uint32_t[outWidth]();
rowCount = new (std::nothrow) uint16_t[outWidth]();
+ if (!rowAccum || !rowCount) {
+ LOG_ERR("PNG", "Failed to allocate scaling buffers");
+ delete[] rowAccum;
+ delete[] rowCount;
+ delete atkinsonDitherer;
+ delete fsDitherer;
+ delete atkinson1BitDitherer;
+ free(rowBuffer);
+ free(ctx.currentRow);
+ free(ctx.previousRow);
+ return false;
+ }
nextOutY_srcStart = scaleY_fp;
}🤖 Prompt for 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.
In `@lib/PngToBmpConverter/PngToBmpConverter.cpp` around lines 643 - 646, The
scaling path currently allocates rowAccum and rowCount with new (std::nothrow)
but never checks for null, which causes a crash on OOM; inside the needsScaling
branch (where rowAccum, rowCount and nextOutY_srcStart are set) add null checks
after each allocation, free any partially allocated buffer (delete[] rowAccum
and/or delete[] rowCount), set pointers to nullptr, and return or propagate a
clean error (e.g., return false or set an error state) from the enclosing
function (the function containing the needsScaling logic in
PngToBmpConverter.cpp) so the caller can handle allocation failure instead of
dereferencing a null pointer. Ensure all code paths that assume these buffers
exist only run after successful allocation.
Comment on lines
+444
to
445
| bool ZipFile::readFileToStream(const char* filename, Print& out, const size_t chunkSize, uint32_t deadline) { | ||
| const ScopedOpenClose zip{*this}; |
There was a problem hiding this comment.
Guard zero chunkSize to prevent non-progress loops.
Line 444 accepts chunkSize without validation. If it is 0, the deflated path can spin without progress (or rely only on deadline to exit). Add an early guard.
Suggested fix
bool ZipFile::readFileToStream(const char* filename, Print& out, const size_t chunkSize, uint32_t deadline) {
+ if (chunkSize == 0) {
+ LOG_ERR("ZIP", "Invalid chunk size: 0");
+ return false;
+ }
const ScopedOpenClose zip{*this};
if (!zip) return false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| bool ZipFile::readFileToStream(const char* filename, Print& out, const size_t chunkSize, uint32_t deadline) { | |
| const ScopedOpenClose zip{*this}; | |
| bool ZipFile::readFileToStream(const char* filename, Print& out, const size_t chunkSize, uint32_t deadline) { | |
| if (chunkSize == 0) { | |
| LOG_ERR("ZIP", "Invalid chunk size: 0"); | |
| return false; | |
| } | |
| const ScopedOpenClose zip{*this}; |
🤖 Prompt for 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.
In `@lib/ZipFile/ZipFile.cpp` around lines 444 - 445, ZipFile::readFileToStream
must guard against a zero chunkSize to avoid non-progress loops; add an early
check (before creating ScopedOpenClose or immediately after) that if chunkSize
== 0 the function returns false (or fails fast), and include an appropriate
debug/error log if available; locate the check in ZipFile::readFileToStream
around the ScopedOpenClose usage and return early when chunkSize is zero.
Comment on lines
+50
to
+63
| void SleepActivity::renderLogoSleepScreen() const { | ||
| const auto pageWidth = renderer.getScreenWidth(); | ||
| const auto pageHeight = renderer.getScreenHeight(); | ||
|
|
||
| renderer.clearScreen(); | ||
| renderer.drawImage(CrossLarge, (pageWidth - 128) / 2, (pageHeight - 128) / 2, 128, 128); | ||
| renderer.drawCenteredText(UI_10_FONT_ID, pageHeight / 2 + 70, tr(STR_CROSSPOINT), true, EpdFontFamily::BOLD); | ||
| renderer.drawCenteredText(SMALL_FONT_ID, pageHeight / 2 + 95, tr(STR_SLEEPING), true, EpdFontFamily::BOLD); | ||
|
|
||
| if (SETTINGS.sleepScreenFilter != CrossPointSettings::SLEEP_SCREEN_FILTER::FILTER_NEGATIVE) { | ||
| renderer.invertScreen(); | ||
| } | ||
|
|
||
| renderer.displayBuffer(HalDisplay::HALF_REFRESH); |
There was a problem hiding this comment.
Align the logo fallback's negative filter with the other sleep-screen paths.
FILTER_NEGATIVE is handled as "invert" in renderBlankSleepScreen() and renderBitmapSleepScreen(), but this branch inverts for every other filter. The logo fallback will therefore render with the opposite polarity from the selected sleep-screen filter.
Suggested fix
- if (SETTINGS.sleepScreenFilter != CrossPointSettings::SLEEP_SCREEN_FILTER::FILTER_NEGATIVE) {
+ if (SETTINGS.sleepScreenFilter == CrossPointSettings::SLEEP_SCREEN_FILTER::FILTER_NEGATIVE) {
renderer.invertScreen();
}🤖 Prompt for 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.
In `@src/activities/boot_sleep/SleepActivity.cpp` around lines 50 - 63,
renderLogoSleepScreen() currently calls renderer.invertScreen() when
SETTINGS.sleepScreenFilter is not FILTER_NEGATIVE, which is the inverse of how
renderBlankSleepScreen() and renderBitmapSleepScreen() handle FILTER_NEGATIVE;
change the branch in renderLogoSleepScreen() so that renderer.invertScreen() is
called only when SETTINGS.sleepScreenFilter ==
CrossPointSettings::SLEEP_SCREEN_FILTER::FILTER_NEGATIVE to match the other
sleep-screen paths and ensure correct polarity for the logo fallback.
Comment on lines
+76
to
+77
| int bookCount = forceRegenerate ? static_cast<int>(recentBooks.size()) : 1; | ||
| for (int bi = 0; bi < bookCount && bi < static_cast<int>(recentBooks.size()); bi++) { |
There was a problem hiding this comment.
Don't cap normal cover generation to the first recent book.
This activity already loads as many recent books as the active theme asks for, but this branch only regenerates a missing thumbnail for index 0. In multi-cover home themes, books 2..N will keep showing stale/missing art until a forced full regenerate happens.
Suggested fix
- int bookCount = forceRegenerate ? static_cast<int>(recentBooks.size()) : 1;
+ int bookCount = static_cast<int>(recentBooks.size());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| int bookCount = forceRegenerate ? static_cast<int>(recentBooks.size()) : 1; | |
| for (int bi = 0; bi < bookCount && bi < static_cast<int>(recentBooks.size()); bi++) { | |
| int bookCount = static_cast<int>(recentBooks.size()); | |
| for (int bi = 0; bi < bookCount && bi < static_cast<int>(recentBooks.size()); bi++) { |
🤖 Prompt for 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.
In `@src/activities/home/HomeActivity.cpp` around lines 76 - 77, The loop that
regenerates covers is capped to only index 0 when forceRegenerate is false
(bookCount is set to 1); change bookCount logic so it iterates over all
recentBooks (or up to the active theme's requested count) instead of just 1.
Specifically, update the calculation of bookCount (currently: int bookCount =
forceRegenerate ? static_cast<int>(recentBooks.size()) : 1;) to compute the full
desired count (e.g., int bookCount = static_cast<int>(recentBooks.size()); or
int bookCount = std::min(static_cast<int>(recentBooks.size()),
themeRequestedCount);) so the for-loop (for (int bi = 0; bi < bookCount && bi <
static_cast<int>(recentBooks.size()); bi++)) will regenerate covers for books
0..N-1 when not forcing a full regenerate.
Comment on lines
+97
to
107
| const uint32_t deadline = (useTimeout && !isBookForced) ? (millis() + COVER_RENDER_TIMEOUT_MS) : 0; | ||
| LOG_DBG("HOME", "loadRecentCovers: generating cover (deadline=%u ms)", deadline); | ||
| bool success = epub.generateThumbBmp(coverHeight, deadline); | ||
| LOG_DBG("HOME", "loadRecentCovers: generateThumbBmp result=%d", success); | ||
| if (success) { | ||
| RECENT_BOOKS.setCoverDisabled(book.path, false); | ||
| book.coverDisabled = false; | ||
| } else if (!isBookForced) { | ||
| RECENT_BOOKS.setCoverDisabled(book.path, true); | ||
| book.coverDisabled = true; | ||
| } |
There was a problem hiding this comment.
Timeout mode currently turns a transient miss into a permanent per-book disable.
A deadline expiry makes generateThumbBmp() return false, and this branch then persists coverDisabled = true. After one slow render, that book will stop being retried automatically, which makes the “Timeout (10s)” mode behave like “try once, then disable”.
🤖 Prompt for 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.
In `@src/activities/home/HomeActivity.cpp` around lines 97 - 107, The current
logic treats any false from epub.generateThumbBmp(...) as a permanent failure
and persists coverDisabled; change epub.generateThumbBmp to return a richer
result (e.g., a struct or enum/tuple like {bool success; bool timedOut}) so
callers can distinguish timeout vs real failure, then update this call site (the
loadRecentCovers block using deadline, isBookForced, COVER_RENDER_TIMEOUT_MS,
epub.generateThumbBmp, RECENT_BOOKS.setCoverDisabled, and book.coverDisabled) to
only persist/set coverDisabled when success==false AND timedOut==false (i.e., a
non-timeout failure), leaving timed-out attempts as transient (do not call
RECENT_BOOKS.setCoverDisabled nor set book.coverDisabled on timedOut); keep
existing behavior for successful renders and for forced books.
Comment on lines
50
to
52
| APP_STATE.readerActivityLoadCount = 0; | ||
| APP_STATE.pendingCoverGeneration = true; | ||
| APP_STATE.saveToFile(); |
There was a problem hiding this comment.
Guard pending cover generation behind a valid reader instance.
Line 51 sets cover generation unconditionally. If xtc is null, this can queue cover work for stale state.
🔧 Proposed fix
void XtcReaderActivity::onExit() {
Activity::onExit();
APP_STATE.readerActivityLoadCount = 0;
- APP_STATE.pendingCoverGeneration = true;
+ if (xtc) {
+ APP_STATE.pendingCoverGeneration = true;
+ }
APP_STATE.saveToFile();
xtc.reset();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| APP_STATE.readerActivityLoadCount = 0; | |
| APP_STATE.pendingCoverGeneration = true; | |
| APP_STATE.saveToFile(); | |
| APP_STATE.readerActivityLoadCount = 0; | |
| if (xtc) { | |
| APP_STATE.pendingCoverGeneration = true; | |
| } | |
| APP_STATE.saveToFile(); |
🤖 Prompt for 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.
In `@src/activities/reader/XtcReaderActivity.cpp` around lines 50 - 52, Guard
setting APP_STATE.pendingCoverGeneration behind a valid reader instance: check
the reader pointer (xtc or the reader instance used in XtcReaderActivity) is
non-null/valid before assigning APP_STATE.pendingCoverGeneration = true, leaving
APP_STATE.readerActivityLoadCount = 0 and APP_STATE.saveToFile() as needed; only
queue cover generation when the reader is confirmed valid to avoid enqueuing
stale work.
Comment on lines
+484
to
+487
| if (renderer.drawBitmap(bitmap, bookX, bookY, bookWidth, bookHeight)) { | ||
| renderer.drawRect(bookX, bookY, bookWidth, bookHeight); | ||
| coverBufferStored = storeCoverBuffer(); | ||
| coverRendered = true; |
There was a problem hiding this comment.
Only mark the cover as rendered after the buffer snapshot succeeds.
If storeCoverBuffer() fails here, later renders skip drawBitmap() because coverRendered is already true, but they also cannot restore anything because no buffer was stored. The next full redraw leaves the cover blank.
Suggested fix
- coverBufferStored = storeCoverBuffer();
- coverRendered = true;
+ coverBufferStored = storeCoverBuffer();
+ coverRendered = coverBufferStored;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (renderer.drawBitmap(bitmap, bookX, bookY, bookWidth, bookHeight)) { | |
| renderer.drawRect(bookX, bookY, bookWidth, bookHeight); | |
| coverBufferStored = storeCoverBuffer(); | |
| coverRendered = true; | |
| if (renderer.drawBitmap(bitmap, bookX, bookY, bookWidth, bookHeight)) { | |
| renderer.drawRect(bookX, bookY, bookWidth, bookHeight); | |
| coverBufferStored = storeCoverBuffer(); | |
| coverRendered = coverBufferStored; |
🤖 Prompt for 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.
In `@src/components/themes/BaseTheme.cpp` around lines 484 - 487, The code
currently sets coverRendered = true immediately after drawing and then calls
storeCoverBuffer(), which can fail and leave coverRendered true without a stored
buffer; change the flow so coverRendered is only set when storeCoverBuffer()
returns success (e.g., assign coverBufferStored = storeCoverBuffer() and then
set coverRendered = coverBufferStored), ensuring renderer.drawBitmap/drawRect
are followed by a successful buffer snapshot before marking the cover as
rendered.
| @@ -0,0 +1,112 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
Remove the UTF-8 BOM from this header.
Line 1 includes a BOM before #pragma once; that can cause preprocessing issues on stricter compilers.
🔧 Proposed fix
-`#pragma` once
+#pragma once📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #pragma once | |
| `#pragma` once |
🤖 Prompt for 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.
In `@src/images/CrossLarge.h` at line 1, The file CrossLarge.h contains a UTF-8
BOM before the directive; remove the BOM so the very first byte is '#' and the
line reads "`#pragma` once" exactly, then save the file as UTF-8 without BOM
(e.g., using your editor's "Save without BOM" or re-encode the file); this
ensures the preprocessor sees the `#pragma` once token correctly in CrossLarge.h.
Comment on lines
+243
to
+282
| // Legacy migration: if old keys exist, convert to unified sleep screen format. | ||
| if (!doc["sleepScreenCoverMode"].isNull() || !doc["sleepScreen"].isNull()) { | ||
| uint8_t oldSleepScreen = doc["sleepScreen"] | (uint8_t)0; | ||
| uint8_t oldCoverMode = doc["sleepScreenCoverMode"] | (uint8_t)0; | ||
| switch (oldSleepScreen) { | ||
| case 0: // DARK | ||
| s.sleepScreen = CrossPointSettings::SLEEP_SCREEN_MODE::LOGO; | ||
| break; | ||
| case 1: // LIGHT | ||
| s.sleepScreen = CrossPointSettings::SLEEP_SCREEN_MODE::LOGO; | ||
| s.sleepScreenFilter = CrossPointSettings::SLEEP_SCREEN_FILTER::FILTER_NEGATIVE; | ||
| break; | ||
| case 2: // CUSTOM | ||
| s.sleepScreen = CrossPointSettings::SLEEP_SCREEN_MODE::CUSTOM; | ||
| break; | ||
| case 3: // COVER | ||
| s.sleepScreen = (oldCoverMode == 1) ? CrossPointSettings::SLEEP_SCREEN_MODE::COVER_CROP | ||
| : CrossPointSettings::SLEEP_SCREEN_MODE::COVER_FIT; | ||
| break; | ||
| case 4: // BLANK | ||
| s.sleepScreen = CrossPointSettings::SLEEP_SCREEN_MODE::BLANK; | ||
| break; | ||
| case 5: // COVER_CUSTOM | ||
| s.sleepScreen = CrossPointSettings::SLEEP_SCREEN_MODE::COVER_CUSTOM; | ||
| break; | ||
| default: | ||
| s.sleepScreen = CrossPointSettings::SLEEP_SCREEN_MODE::LOGO; | ||
| break; | ||
| } | ||
| if (!doc["sleepScreenCoverFilter"].isNull()) { | ||
| uint8_t oldFilter = doc["sleepScreenCoverFilter"] | (uint8_t)0; | ||
| if ((s.sleepScreen == CrossPointSettings::SLEEP_SCREEN_MODE::COVER_FIT || | ||
| s.sleepScreen == CrossPointSettings::SLEEP_SCREEN_MODE::COVER_CROP || | ||
| s.sleepScreen == CrossPointSettings::SLEEP_SCREEN_MODE::COVER_CUSTOM) && | ||
| oldFilter < CrossPointSettings::SLEEP_SCREEN_FILTER_COUNT) { | ||
| s.sleepScreenFilter = oldFilter; | ||
| } | ||
| } | ||
| if (needsResave) *needsResave = true; | ||
| } |
There was a problem hiding this comment.
This migration rewrites already-migrated sleepScreen values.
New-format settings still use the sleepScreen key, so this block also runs on current files and interprets the new enum as the legacy one. That will silently change persisted settings on load/resave.
🤖 Prompt for 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.
In `@src/JsonSettingsIO.cpp` around lines 243 - 282, The migration block is
triggering for new-format files because it checks doc["sleepScreen"] (shared by
old and new formats); change the guard so the legacy conversion runs only when
legacy-only keys exist (e.g., doc["sleepScreenCoverMode"] or
doc["sleepScreenCoverFilter"]) and remove the doc["sleepScreen"] check; update
the if condition around the migration in JsonSettingsIO.cpp to check only
!doc["sleepScreenCoverMode"].isNull() ||
!doc["sleepScreenCoverFilter"].isNull(), leaving the rest of the mapping logic
that assigns s.sleepScreen and s.sleepScreenFilter (and uses
CrossPointSettings::SLEEP_SCREEN_MODE / _FILTER enums) unchanged so
current-format sleepScreen values are not reinterpreted.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/CrossPointSettings.h (1)
110-118:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
SLEEP_3_MINappended at end breaks UI label mapping.This is the root cause of the sleep timeout mismatch flagged in
SettingsList.h. The newSLEEP_3_MIN = 5is added at the end of the enum, but the UI labels inSettingsList.hplace "3 min" second in the list. MoveSLEEP_3_MINto index 1 for correct mapping.🤖 Prompt for 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. In `@src/CrossPointSettings.h` around lines 110 - 118, The SLEEP_3_MIN enumerator was appended at the end of the SLEEP_TIMEOUT enum which breaks the mapping to UI labels in SettingsList.h; relocate SLEEP_3_MIN to index 1 in the SLEEP_TIMEOUT enum (i.e., place it right after SLEEP_1_MIN) so the order becomes SLEEP_1_MIN, SLEEP_3_MIN, SLEEP_5_MIN, SLEEP_10_MIN, SLEEP_15_MIN, SLEEP_30_MIN and ensure SLEEP_TIMEOUT_COUNT remains the last value; update any explicit numeric values in the enum to reflect the new order if present so the integer mapping matches the labels in SettingsList.h.
🧹 Nitpick comments (6)
src/SettingsList.h (1)
72-72: 💤 Low valueUTF-8 encoding corruption in comment.
The text
â€"should be an em-dash (—). This is a minor display issue in the source.Fix encoding
- // SD font name not found in registry â€" fall through to built-in + // SD font name not found in registry — fall through to built-in🤖 Prompt for 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. In `@src/SettingsList.h` at line 72, Replace the corrupted UTF-8 sequence in the comment inside SettingsList.h: change the text "SD font name not found in registry — fall through to built-in" to use a proper em-dash ("—") so it reads "SD font name not found in registry — fall through to built-in", and ensure the file is saved in UTF-8 encoding (without a BOM) so the em-dash renders correctly.lib/I18n/translations/spanish.yaml (1)
64-65: 💤 Low valueMissing translations for new strings added in this PR.
The Spanish translation file retains old keys (
STR_SLEEP_SCREEN,STR_SLEEP_COVER_MODE) but is missing the new strings added toenglish.yaml:
- Sleep screen mode/filter strings (
STR_SLEEP_SCREEN_MODE,STR_SLEEP_SCREEN_FILTER,STR_SLEEP_MODE_*,STR_SLEEP_FILTER_*)STR_ACTION_MENU- Home cover settings (
STR_HOME_COVER_*)- Cover disable dialog (
STR_COVER_DISABLE_*)The app will fall back to English for these untranslated strings. Consider adding placeholder translations or tracking this as a follow-up task.
🤖 Prompt for 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. In `@lib/I18n/translations/spanish.yaml` around lines 64 - 65, The Spanish translation file is missing the newly added keys (e.g. STR_SLEEP_SCREEN_MODE, STR_SLEEP_SCREEN_FILTER, STR_SLEEP_MODE_*, STR_SLEEP_FILTER_*, STR_ACTION_MENU, STR_HOME_COVER_*, STR_COVER_DISABLE_*) so update the Spanish translations by adding these keys with appropriate Spanish text or safe placeholder translations (e.g. English text or “TODO: traducir”) to avoid fallback to English; ensure each new key exactly matches the key names used in english.yaml and follow existing YAML formatting/quoting for values, or add a note/ticket to track completing proper translations later.src/activities/network/WifiSelectionActivity.cpp (1)
551-567: 💤 Low valueUnused
metricsparameter in several render methods.The
metricsparameter is passed but not used inrenderConnecting,renderConnected,renderConnectionFailed, andrenderForgetPrompt. This is likely intentional for API consistency withrenderNetworkListwhich does use it.Also applies to: 569-587, 631-642, 644-684
🤖 Prompt for 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. In `@src/activities/network/WifiSelectionActivity.cpp` around lines 551 - 567, These render methods (renderConnecting, renderConnected, renderConnectionFailed, renderForgetPrompt) accept a ThemeMetrics* parameter that is not used and triggers unused-parameter warnings; fix this by explicitly marking the parameter as unused in each function body (e.g., add (void)metrics; or use [[maybe_unused]] on the parameter) so the API remains consistent with renderNetworkList while removing the warning. Ensure you update all four functions (renderConnecting, renderConnected, renderConnectionFailed, renderForgetPrompt) in the file.src/activities/reader/EpubReaderActivity.cpp (1)
40-44: 💤 Low valuePointer to vector element - ensure vector stability.
findRecentBookreturns a raw pointer to an element withinRECENT_BOOKS.getBooks(). This is safe as long as the vector isn't modified (which would invalidate the pointer). The current usage at lines 246-252 is safe since the pointer is consumed immediately before any vector-modifying operations.🤖 Prompt for 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. In `@src/activities/reader/EpubReaderActivity.cpp` around lines 40 - 44, findRecentBook currently returns a raw pointer to an element in RECENT_BOOKS.getBooks(), which can dangle if the vector is modified; change the function to return a safe, non-dangling result such as std::optional<RecentBook> (or the element index) instead of const RecentBook* so callers (e.g., code using findRecentBook at lines ~246-252) receive a stable copy or an index and avoid relying on the vector's iterator/pointer stability; update callers to handle the optional/index accordingly and remove any assumptions about pointer lifetime tied to RECENT_BOOKS.getBooks().src/components/UITheme.cpp (2)
61-92: 💤 Low valueUnused
hasSideButtonHintsparameter.The
hasSideButtonHintsparameter is accepted but never used in the function body. All branches only checkhasFrontButtonHints. If side button hints no longer affect the safe area, consider deprecating or removing this parameter in a future cleanup.🤖 Prompt for 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. In `@src/components/UITheme.cpp` around lines 61 - 92, The hasSideButtonHints parameter of UITheme::getScreenSafeArea is unused; remove it from the function signature and all callers (and any forward declarations) so the function becomes getScreenSafeArea(const GfxRenderer& renderer) and callers pass only the renderer, or alternatively if side hints should affect layout, update the switch branches to check hasSideButtonHints where appropriate; adjust references to currentMetrics and GfxRenderer::Orientation cases accordingly to maintain the same safeArea behavior.
102-102: 💤 Low valueHardcoded cover heights.
getAllCoverHeights()returns a hardcoded list{226, 300, 400}. This works but couples the theme system to specific pixel values. Consider deriving these from theme metrics or documenting why these specific heights are used.🤖 Prompt for 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. In `@src/components/UITheme.cpp` at line 102, The method UITheme::getAllCoverHeights currently returns hardcoded pixel values {226, 300, 400}; update it to derive these heights from theme configuration or central metrics rather than literal numbers — e.g., read from a ThemeMetrics/ThemeConfig instance or use existing constants (like coverHeightSmall/coverHeightMedium/coverHeightLarge) so the values remain consistent and configurable across the app; change the implementation of UITheme::getAllCoverHeights to fetch those metric values (or document why fixed values must be used if no metrics exist) and ensure any callers still receive the same vector<int> shape.
🤖 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 `@src/activities/reader/EpubReaderPercentSelectionActivity.cpp`:
- Around line 77-79: The fixed barWidth (360) can overflow in narrow safe areas
because barX centers using screen.width; update the layout to clamp barWidth
against the safe-area width before computing barX. In the
EpubReaderPercentSelectionActivity layout code, compute an effectiveWidth =
min(barWidth, safeAreaWidth or (screen.width - horizontalPadding*2)) and then
use that effectiveWidth when calculating barX and any positioning (instead of
the constant barWidth) so the slider never renders outside the visible safe
area.
In `@src/SettingsList.h`:
- Around line 178-179: The sleep timeout labels in SettingsList
(StrId::STR_MIN_1, STR_MIN_3, STR_MIN_5, STR_MIN_10, STR_MIN_15, STR_MIN_30) do
not map to the existing CrossPointSettings::SLEEP_TIMEOUT enum order (which
currently maps 3 and 5 out of order), causing wrong values to be written when
using SettingInfo::Enum; fix by making the enum ordering match the UI labels
(reorder CrossPointSettings::SLEEP_TIMEOUT entries so their ordinal values
correspond to {1,3,5,10,15,30}) or alternatively reorder the labels in
SettingsList to match the current enum ordering—update whichever file
(CrossPointSettings.h for enum, or SettingsList.h for labels) and ensure
SettingInfo::Enum usage remains index-based and consistent.
---
Duplicate comments:
In `@src/CrossPointSettings.h`:
- Around line 110-118: The SLEEP_3_MIN enumerator was appended at the end of the
SLEEP_TIMEOUT enum which breaks the mapping to UI labels in SettingsList.h;
relocate SLEEP_3_MIN to index 1 in the SLEEP_TIMEOUT enum (i.e., place it right
after SLEEP_1_MIN) so the order becomes SLEEP_1_MIN, SLEEP_3_MIN, SLEEP_5_MIN,
SLEEP_10_MIN, SLEEP_15_MIN, SLEEP_30_MIN and ensure SLEEP_TIMEOUT_COUNT remains
the last value; update any explicit numeric values in the enum to reflect the
new order if present so the integer mapping matches the labels in
SettingsList.h.
---
Nitpick comments:
In `@lib/I18n/translations/spanish.yaml`:
- Around line 64-65: The Spanish translation file is missing the newly added
keys (e.g. STR_SLEEP_SCREEN_MODE, STR_SLEEP_SCREEN_FILTER, STR_SLEEP_MODE_*,
STR_SLEEP_FILTER_*, STR_ACTION_MENU, STR_HOME_COVER_*, STR_COVER_DISABLE_*) so
update the Spanish translations by adding these keys with appropriate Spanish
text or safe placeholder translations (e.g. English text or “TODO: traducir”) to
avoid fallback to English; ensure each new key exactly matches the key names
used in english.yaml and follow existing YAML formatting/quoting for values, or
add a note/ticket to track completing proper translations later.
In `@src/activities/network/WifiSelectionActivity.cpp`:
- Around line 551-567: These render methods (renderConnecting, renderConnected,
renderConnectionFailed, renderForgetPrompt) accept a ThemeMetrics* parameter
that is not used and triggers unused-parameter warnings; fix this by explicitly
marking the parameter as unused in each function body (e.g., add (void)metrics;
or use [[maybe_unused]] on the parameter) so the API remains consistent with
renderNetworkList while removing the warning. Ensure you update all four
functions (renderConnecting, renderConnected, renderConnectionFailed,
renderForgetPrompt) in the file.
In `@src/activities/reader/EpubReaderActivity.cpp`:
- Around line 40-44: findRecentBook currently returns a raw pointer to an
element in RECENT_BOOKS.getBooks(), which can dangle if the vector is modified;
change the function to return a safe, non-dangling result such as
std::optional<RecentBook> (or the element index) instead of const RecentBook* so
callers (e.g., code using findRecentBook at lines ~246-252) receive a stable
copy or an index and avoid relying on the vector's iterator/pointer stability;
update callers to handle the optional/index accordingly and remove any
assumptions about pointer lifetime tied to RECENT_BOOKS.getBooks().
In `@src/components/UITheme.cpp`:
- Around line 61-92: The hasSideButtonHints parameter of
UITheme::getScreenSafeArea is unused; remove it from the function signature and
all callers (and any forward declarations) so the function becomes
getScreenSafeArea(const GfxRenderer& renderer) and callers pass only the
renderer, or alternatively if side hints should affect layout, update the switch
branches to check hasSideButtonHints where appropriate; adjust references to
currentMetrics and GfxRenderer::Orientation cases accordingly to maintain the
same safeArea behavior.
- Line 102: The method UITheme::getAllCoverHeights currently returns hardcoded
pixel values {226, 300, 400}; update it to derive these heights from theme
configuration or central metrics rather than literal numbers — e.g., read from a
ThemeMetrics/ThemeConfig instance or use existing constants (like
coverHeightSmall/coverHeightMedium/coverHeightLarge) so the values remain
consistent and configurable across the app; change the implementation of
UITheme::getAllCoverHeights to fetch those metric values (or document why fixed
values must be used if no metrics exist) and ensure any callers still receive
the same vector<int> shape.
In `@src/SettingsList.h`:
- Line 72: Replace the corrupted UTF-8 sequence in the comment inside
SettingsList.h: change the text "SD font name not found in registry — fall
through to built-in" to use a proper em-dash ("—") so it reads "SD font name not
found in registry — fall through to built-in", and ensure the file is saved in
UTF-8 encoding (without a BOM) so the em-dash renders correctly.
🪄 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: d4a18ead-51ec-43ef-8161-eac85ced00cc
📒 Files selected for processing (42)
lib/I18n/translations/belarusian.yamllib/I18n/translations/catalan.yamllib/I18n/translations/czech.yamllib/I18n/translations/danish.yamllib/I18n/translations/dutch.yamllib/I18n/translations/english.yamllib/I18n/translations/finnish.yamllib/I18n/translations/french.yamllib/I18n/translations/german.yamllib/I18n/translations/hungarian.yamllib/I18n/translations/italian.yamllib/I18n/translations/kazakh.yamllib/I18n/translations/lithuanian.yamllib/I18n/translations/polish.yamllib/I18n/translations/portuguese.yamllib/I18n/translations/romanian.yamllib/I18n/translations/russian.yamllib/I18n/translations/slovenian.yamllib/I18n/translations/spanish.yamllib/I18n/translations/swedish.yamllib/I18n/translations/turkish.yamllib/I18n/translations/ukrainian.yamlsrc/CrossPointSettings.cppsrc/CrossPointSettings.hsrc/RecentBooksStore.cppsrc/RecentBooksStore.hsrc/SettingsList.hsrc/activities/network/WifiSelectionActivity.cppsrc/activities/network/WifiSelectionActivity.hsrc/activities/reader/EpubReaderActivity.cppsrc/activities/reader/EpubReaderActivity.hsrc/activities/reader/EpubReaderChapterSelectionActivity.cppsrc/activities/reader/EpubReaderMenuActivity.cppsrc/activities/reader/EpubReaderPercentSelectionActivity.cppsrc/activities/reader/KOReaderSyncActivity.cppsrc/components/UITheme.cppsrc/components/UITheme.hsrc/components/themes/BaseTheme.cppsrc/components/themes/BaseTheme.hsrc/components/themes/lyra/Lyra3CoversTheme.cppsrc/components/themes/lyra/Lyra3CoversTheme.hsrc/components/themes/lyra/LyraTheme.cpp
💤 Files with no reviewable changes (2)
- src/components/themes/lyra/Lyra3CoversTheme.h
- src/components/themes/lyra/Lyra3CoversTheme.cpp
✅ Files skipped from review due to trivial changes (4)
- lib/I18n/translations/turkish.yaml
- lib/I18n/translations/kazakh.yaml
- lib/I18n/translations/dutch.yaml
- lib/I18n/translations/german.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/UITheme.h
- src/components/themes/BaseTheme.h
- src/CrossPointSettings.cpp
- src/components/themes/BaseTheme.cpp
Comment on lines
+77
to
79
| const int barX = screen.x + (screen.width - barWidth) / 2; | ||
| const int barY = contentTop + metrics.verticalSpacing * 2; | ||
|
|
There was a problem hiding this comment.
Clamp the slider width to the safe-area width.
barWidth is fixed at 360 while centering uses screen.width; in narrow safe areas the slider can render partially off-screen.
Suggested fix
+#include <algorithm>
...
- constexpr int barWidth = 360;
+ const int barWidth = std::max(1, std::min(360, screen.width - 8));
constexpr int barHeight = 16;
const int barX = screen.x + (screen.width - barWidth) / 2;🤖 Prompt for 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.
In `@src/activities/reader/EpubReaderPercentSelectionActivity.cpp` around lines 77
- 79, The fixed barWidth (360) can overflow in narrow safe areas because barX
centers using screen.width; update the layout to clamp barWidth against the
safe-area width before computing barX. In the EpubReaderPercentSelectionActivity
layout code, compute an effectiveWidth = min(barWidth, safeAreaWidth or
(screen.width - horizontalPadding*2)) and then use that effectiveWidth when
calculating barX and any positioning (instead of the constant barWidth) so the
slider never renders outside the visible safe area.
Comment on lines
+178
to
+179
| {StrId::STR_MIN_1, StrId::STR_MIN_3, StrId::STR_MIN_5, StrId::STR_MIN_10, StrId::STR_MIN_15, | ||
| StrId::STR_MIN_30}, |
There was a problem hiding this comment.
Critical: Sleep timeout UI labels don't match enum order.
The UI displays options as {1, 3, 5, 10, 15, 30} minutes, but CrossPointSettings::SLEEP_TIMEOUT enum values are {1=0, 5=1, 10=2, 15=3, 30=4, 3=5}. Since SettingInfo::Enum stores the selection index directly, selecting "3 min" (index 1) will actually set SLEEP_5_MIN.
Either reorder the enum in CrossPointSettings.h to match the desired UI order, or reorder the labels here to match the existing enum.
Recommended fix: Reorder enum in CrossPointSettings.h
enum SLEEP_TIMEOUT {
SLEEP_1_MIN = 0,
- SLEEP_5_MIN = 1,
- SLEEP_10_MIN = 2,
- SLEEP_15_MIN = 3,
- SLEEP_30_MIN = 4,
- SLEEP_3_MIN = 5,
+ SLEEP_3_MIN = 1,
+ SLEEP_5_MIN = 2,
+ SLEEP_10_MIN = 3,
+ SLEEP_15_MIN = 4,
+ SLEEP_30_MIN = 5,
SLEEP_TIMEOUT_COUNT
};🤖 Prompt for 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.
In `@src/SettingsList.h` around lines 178 - 179, The sleep timeout labels in
SettingsList (StrId::STR_MIN_1, STR_MIN_3, STR_MIN_5, STR_MIN_10, STR_MIN_15,
STR_MIN_30) do not map to the existing CrossPointSettings::SLEEP_TIMEOUT enum
order (which currently maps 3 and 5 out of order), causing wrong values to be
written when using SettingInfo::Enum; fix by making the enum ordering match the
UI labels (reorder CrossPointSettings::SLEEP_TIMEOUT entries so their ordinal
values correspond to {1,3,5,10,15,30}) or alternatively reorder the labels in
SettingsList to match the current enum ordering—update whichever file
(CrossPointSettings.h for enum, or SettingsList.h for labels) and ensure
SettingInfo::Enum usage remains index-based and consistent.
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
4 participants
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
Bug Fixes
Documentation
Localization