refactor: redesign on-screen keyboard#1644
Merged
znelson merged 25 commits intocrosspoint-reader:masterfrom Apr 18, 2026
Merged
refactor: redesign on-screen keyboard#1644znelson merged 25 commits intocrosspoint-reader:masterfrom
znelson merged 25 commits intocrosspoint-reader:masterfrom
Conversation
…, long-press, visual improvements
Contributor
📝 WalkthroughWalkthroughReplaces legacy keyboard strings with data-driven KeyDef grids and InputType; converts shift to 2-state; adds cursor-mode editing (cursorPos, insert/delete, long-press behaviors), URL-snippet mode, bottom-row SpecialKeyType handling, and updates theme APIs/metrics and keyboard rendering for secondary labels and bottom-row keys. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant KB as KeyboardEntryActivity
participant Theme as ThemeRenderer
participant Gfx as GfxRenderer
User->>KB: navigate / select / long-press / confirm
KB->>KB: update selectedRow/selectedCol, check isBottomRow
alt content key pressed
KB->>KB: resolve KeyDef.primary/secondary (shift/sym)
KB->>KB: insertChar / insertString (update cursorPos)
else bottom-row special
KB->>KB: mapColContentBottom -> SpecialKeyType
KB->>KB: handle Shift/Mode/Space/Del/Ok (toggle, url snippet, clear, delete)
end
KB->>Theme: render keyboard (labels, secondaryLabel, keyType, selection, cursorMode, text cursor)
Theme->>Gfx: draw primitives (text, lines, fills, glyphs)
Gfx-->>Theme: drawn
Theme-->>KB: render complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/components/themes/BaseTheme.cpp (2)
826-826: Dead code:primaryOffsetis always 0.Same issue as in
LyraTheme.cpp- both branches of the ternary evaluate to0.🧹 Suggested cleanup
- const bool hasSecondary = secondaryLabel != nullptr && secondaryLabel[0] != '\0'; - const int primaryOffset = hasSecondary ? 0 : 0; const int itemWidth = renderer.getTextWidth(UI_12_FONT_ID, label); const int textX = rect.x + (rect.width - itemWidth) / 2; - const int textY = rect.y + (rect.height - renderer.getLineHeight(UI_12_FONT_ID)) / 2 + primaryOffset; + const int textY = rect.y + (rect.height - renderer.getLineHeight(UI_12_FONT_ID)) / 2;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/themes/BaseTheme.cpp` at line 826, The variable primaryOffset is dead because const int primaryOffset = hasSecondary ? 0 : 0 always yields 0; remove the redundant primaryOffset declaration from BaseTheme.cpp (and mirror the same cleanup in LyraTheme.cpp if present) or replace the ternary with the correct non-zero value if there was an intended offset based on hasSecondary; update any use-sites that relied on primaryOffset to either use the literal 0 directly or the newly-corrected expression in functions/methods referencing primaryOffset.
831-833: RedundantfillRectcall when selected.Lines 797-798 already fill the key rectangle when
isSelectedis true. This secondfillRectat lines 831-833 is redundant and causes unnecessary overdraw.🔧 Proposed fix
- if (isSelected) { - renderer.fillRect(rect.x, rect.y, rect.width, rect.height, true); - } renderer.drawText(UI_12_FONT_ID, textX, textY, label, !isSelected);🤖 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 831 - 833, There is a redundant renderer.fillRect(rect.x, rect.y, rect.width, rect.height, true) call guarded by isSelected that causes double drawing; remove this duplicated call (the one at the shown snippet) or change its condition so the rectangle is only filled once when isSelected is true — locate the duplicate fillRect usage tied to isSelected in BaseTheme.cpp and delete the redundant block referencing renderer.fillRect and rect to eliminate the overdraw.src/components/themes/lyra/LyraTheme.cpp (1)
636-636: Dead code:primaryOffsetis always 0.Both branches of the ternary evaluate to
0, making this computation unnecessary.🧹 Suggested cleanup
- const bool hasSecondary = secondaryLabel != nullptr && secondaryLabel[0] != '\0'; - const int primaryOffset = hasSecondary ? 0 : 0; const int textWidth = renderer.getTextWidth(UI_12_FONT_ID, label); const int textX = rect.x + (rect.width - textWidth) / 2; - const int textY = rect.y + (rect.height - renderer.getLineHeight(UI_12_FONT_ID)) / 2 + primaryOffset; + const int textY = rect.y + (rect.height - renderer.getLineHeight(UI_12_FONT_ID)) / 2; renderer.drawText(UI_12_FONT_ID, textX, textY, label, !invert); + const bool hasSecondary = secondaryLabel != nullptr && secondaryLabel[0] != '\0'; if (hasSecondary) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/themes/lyra/LyraTheme.cpp` at line 636, The variable primaryOffset is dead because const int primaryOffset = hasSecondary ? 0 : 0 always yields 0; remove primaryOffset and either replace its uses with the literal 0 or implement the intended offset logic depending on hasSecondary (inspect surrounding code that references primaryOffset in LyraTheme.cpp to decide whether the branch should produce a non-zero value). Update references to primaryOffset accordingly and delete the redundant declaration to avoid confusion.src/activities/util/KeyboardEntryActivity.cpp (1)
254-254: Consider explicit cast for clarity.
isBottomRow(selectedRow)returnsbool, butbottomSelectedRowis declared asint. While this works due to implicit conversion, an explicit cast or usingbooltype would improve readability.🧹 Suggested improvement
- const int bottomSelectedRow = isBottomRow(selectedRow); + const bool bottomSelectedRow = isBottomRow(selectedRow);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activities/util/KeyboardEntryActivity.cpp` at line 254, The variable bottomSelectedRow is declared as int but assigned the bool result of isBottomRow(selectedRow); change the declaration to use a bool for clarity: replace the line creating bottomSelectedRow with "const bool bottomSelectedRow = isBottomRow(selectedRow);" (alternatively, if an int is required elsewhere, use an explicit cast like static_cast<int>(isBottomRow(selectedRow))).
🤖 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/activities/util/KeyboardEntryActivity.cpp`:
- Line 254: The variable bottomSelectedRow is declared as int but assigned the
bool result of isBottomRow(selectedRow); change the declaration to use a bool
for clarity: replace the line creating bottomSelectedRow with "const bool
bottomSelectedRow = isBottomRow(selectedRow);" (alternatively, if an int is
required elsewhere, use an explicit cast like
static_cast<int>(isBottomRow(selectedRow))).
In `@src/components/themes/BaseTheme.cpp`:
- Line 826: The variable primaryOffset is dead because const int primaryOffset =
hasSecondary ? 0 : 0 always yields 0; remove the redundant primaryOffset
declaration from BaseTheme.cpp (and mirror the same cleanup in LyraTheme.cpp if
present) or replace the ternary with the correct non-zero value if there was an
intended offset based on hasSecondary; update any use-sites that relied on
primaryOffset to either use the literal 0 directly or the newly-corrected
expression in functions/methods referencing primaryOffset.
- Around line 831-833: There is a redundant renderer.fillRect(rect.x, rect.y,
rect.width, rect.height, true) call guarded by isSelected that causes double
drawing; remove this duplicated call (the one at the shown snippet) or change
its condition so the rectangle is only filled once when isSelected is true —
locate the duplicate fillRect usage tied to isSelected in BaseTheme.cpp and
delete the redundant block referencing renderer.fillRect and rect to eliminate
the overdraw.
In `@src/components/themes/lyra/LyraTheme.cpp`:
- Line 636: The variable primaryOffset is dead because const int primaryOffset =
hasSecondary ? 0 : 0 always yields 0; remove primaryOffset and either replace
its uses with the literal 0 or implement the intended offset logic depending on
hasSecondary (inspect surrounding code that references primaryOffset in
LyraTheme.cpp to decide whether the branch should produce a non-zero value).
Update references to primaryOffset accordingly and delete the redundant
declaration to avoid confusion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e883b0c7-12ef-4370-b25c-e4357298599e
📒 Files selected for processing (7)
src/activities/util/KeyboardEntryActivity.cppsrc/activities/util/KeyboardEntryActivity.hsrc/components/themes/BaseTheme.cppsrc/components/themes/BaseTheme.hsrc/components/themes/lyra/Lyra3CoversTheme.hsrc/components/themes/lyra/LyraTheme.cppsrc/components/themes/lyra/LyraTheme.h
📜 Review details
⏰ 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 context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: raygan
Repo: crosspoint-reader/crosspoint-reader PR: 1116
File: src/activities/settings/WallabagSettingsActivity.cpp:57-75
Timestamp: 2026-02-23T06:18:15.143Z
Learning: In settings activities (e.g., KOReaderSettingsActivity, WallabagSettingsActivity), the established pattern for launching KeyboardEntryActivity is to call exitActivity() on the parent settings activity before enterNewActivity(new KeyboardEntryActivity(...)). The lambda callbacks then call exitActivity() to exit the KeyboardEntryActivity and return to the parent. This differs from ConfirmationActivity which does not require exitActivity() before enterNewActivity().
📚 Learning: 2026-03-28T11:06:36.807Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/home/HomeActivity.cpp:92-95
Timestamp: 2026-03-28T11:06:36.807Z
Learning: In crosspoint-reader/crosspoint-reader, `BaseTheme::drawPopup()` (BaseTheme.cpp:558) calls `renderer.displayBuffer()` internally before returning. Any popup drawn via `GUI.drawPopup()` is therefore guaranteed to be visible on the e-ink panel before any subsequent blocking work begins. Do not flag missing `displayBuffer()` calls after `drawPopup()` as a bug; the flush is already included. `fillPopupProgress()` does NOT call `displayBuffer()`, so progress bar updates after the initial popup may not be shown — this is intentional and harmless when the update granularity doesn't warrant intermediate flushes.
Applied to files:
src/components/themes/lyra/LyraTheme.h
📚 Learning: 2026-02-23T06:18:15.143Z
Learnt from: raygan
Repo: crosspoint-reader/crosspoint-reader PR: 1116
File: src/activities/settings/WallabagSettingsActivity.cpp:57-75
Timestamp: 2026-02-23T06:18:15.143Z
Learning: In settings activities (e.g., KOReaderSettingsActivity, WallabagSettingsActivity), the established pattern for launching KeyboardEntryActivity is to call exitActivity() on the parent settings activity before enterNewActivity(new KeyboardEntryActivity(...)). The lambda callbacks then call exitActivity() to exit the KeyboardEntryActivity and return to the parent. This differs from ConfirmationActivity which does not require exitActivity() before enterNewActivity().
Applied to files:
src/activities/util/KeyboardEntryActivity.hsrc/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-25T01:15:04.951Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/components/themes/lyra/Lyra3CoversTheme.cpp:39-40
Timestamp: 2026-03-25T01:15:04.951Z
Learning: In crosspoint-reader/crosspoint-reader theme drawing code, `COVER_DISABLED_MODE` only controls whether the app generates new covers (e.g., on HOME entry) to avoid blocking the main thread. It must NOT be treated as a render-suppression switch for already-cached covers. Theme draw paths should only gate cached cover rendering on the per-book `RecentBook::coverDisabled` flag; if a cached cover BMP is available, the correct behavior is to render it even when `COVER_DISABLED_MODE` is set. Therefore, do not flag theme draw code for not checking `COVER_DISABLED_MODE`; checking only `coverDisabled` is correct by design.
Applied to files:
src/components/themes/lyra/LyraTheme.cppsrc/components/themes/BaseTheme.cpp
📚 Learning: 2026-02-27T22:49:59.600Z
Learnt from: ngxson
Repo: crosspoint-reader/crosspoint-reader PR: 1218
File: src/activities/ActivityManager.cpp:254-265
Timestamp: 2026-02-27T22:49:59.600Z
Learning: In this codebase, assertions are always enabled (no NDEBUG). Use assert() to crash on programmer errors and surface logic bugs during development and in production builds. Do not rely on asserts for runtime error handling; they should enforce invariants that must always hold. Keep asserts side-effect free and inexpensive, and avoid relying on them for user-visible failures. Include <cassert> where appropriate and document the invariant being tested.
Applied to files:
src/components/themes/lyra/LyraTheme.cppsrc/components/themes/BaseTheme.cppsrc/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-02T10:14:16.036Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1245
File: lib/Epub/Epub/Section.cpp:277-308
Timestamp: 2026-03-02T10:14:16.036Z
Learning: Guideline: Strengthen serialization::readString to defend against unbounded growth when reading from disk data. Implement and enforce a maximum allowed length (e.g., a configured or reasonable constant) and validate the incoming length before resizing or allocating. Audit all call sites (e.g., BookMetadataCache, TextBlock, KOReaderCredentialStore, Section cache readers) to ensure they do not rely on unbounded len-based resizing. If the readString API must remain, add internal safeguards (bounds checks, length validation, and error handling) so per-call-site validations are not required. Ensure Section cache files remain versioned (SECTION_FILE_VERSION) and parameter mismatches invalidate caches, but do not rely on unsafe allocations; prefer safe, bounded reads with explicit errors on invalid data.
Applied to files:
src/components/themes/lyra/LyraTheme.cppsrc/components/themes/BaseTheme.cppsrc/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-28T11:06:29.611Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/home/HomeActivity.cpp:92-95
Timestamp: 2026-03-28T11:06:29.611Z
Learning: When reviewing crosspoint-reader code, avoid flagging a missing `renderer.displayBuffer()` call immediately after `GUI.drawPopup()` / `BaseTheme::drawPopup()`: `BaseTheme::drawPopup()` already calls `renderer.displayBuffer()` before returning, so the popup is guaranteed to be flushed to the e-ink panel before subsequent blocking work begins. Conversely, do not require a `renderer.displayBuffer()` call after `fillPopupProgress()`; it intentionally does not flush, so intermediate progress-bar updates may not appear unless the update granularity warrants an explicit flush elsewhere.
Applied to files:
src/components/themes/lyra/LyraTheme.cppsrc/components/themes/BaseTheme.cppsrc/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-09T00:59:05.219Z
Learnt from: laird
Repo: crosspoint-reader/crosspoint-reader PR: 1331
File: src/components/themes/pulsr/PulsrTheme.h:58-117
Timestamp: 2026-03-09T00:59:05.219Z
Learning: In PulsrTheme (src/components/themes/pulsr/PulsrTheme.h/.cpp), `drawBatteryLeft` and `drawBatteryRight` are intentionally overridden as no-ops because PULSR displays battery state via pills in the left bar rather than the BaseTheme battery chrome. All of `drawBatteryLeft`, `drawBatteryRight`, `drawStatusBar`, and `drawHelpText` are overridden in PulsrTheme.
Applied to files:
src/components/themes/BaseTheme.cpp
📚 Learning: 2026-04-05T19:21:32.486Z
Learnt from: Zyd8
Repo: crosspoint-reader/crosspoint-reader PR: 1576
File: src/activities/home/RecentBooksActivity.cpp:65-90
Timestamp: 2026-04-05T19:21:32.486Z
Learning: In crosspoint-reader/crosspoint-reader (src/util/ButtonNavigator.cpp, shouldSuppressListNavForMidpointChord), the guard-clearing tick intentionally still returns `true` when it clears `s_midpointChordReleaseGuard`. This is correct because all list navigation in the activity uses `onNextRelease`/`onPreviousRelease`, which only fire on a fresh press+release cycle. The clearing tick therefore only swallows the stale release from the chord buttons themselves. Any subsequent Up/Down tap starts a new press+release cycle and will not be suppressed. Do not flag this as a dropped-input bug in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-05T19:31:31.692Z
Learnt from: Zyd8
Repo: crosspoint-reader/crosspoint-reader PR: 1576
File: src/util/ButtonNavigator.h:43-47
Timestamp: 2026-04-05T19:31:31.692Z
Learning: In crosspoint-reader/crosspoint-reader (src/util/ButtonNavigator.cpp), `clearMidpointChordReleaseGuard()` intentionally sets `s_suppressUntilChordRelease` in addition to resetting the post-release guard. This blocks `beginUpDownChord()` from firing on the first loop tick of a new activity if the midpoint chord was still physically held during the `Activity::onEnter()` transition. The fix prevents cross-screen midpoint chord leaks without requiring each activity's `onEnter()` to call `isMidpointChordHeld()`. Do not flag this dual-write as redundant in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
🔇 Additional comments (16)
src/components/themes/lyra/Lyra3CoversTheme.h (1)
39-42: LGTM!The new keyboard metrics are consistent with the parent
LyraThemeand theThemeMetricsstruct. ThekeyboardVerticalOffset = 0provides appropriate per-theme differentiation.src/components/themes/lyra/LyraTheme.h (2)
35-40: LGTM!The keyboard metrics are well-defined. The negative
keyboardVerticalOffset = -12correctly shifts the keyboard upward for better visual positioning on Lyra themes.
72-74: LGTM!The
drawKeyboardKeyoverride signature correctly matches the updated base class declaration withsecondaryLabelandkeyTypeparameters.src/components/themes/BaseTheme.h (3)
63-66: LGTM!The new
ThemeMetricsfieldskeyboardBottomKeySpacingandkeyboardVerticalOffsetare well-named and provide appropriate theme customization points for keyboard layout.
71-72: LGTM!The
KeyboardKeyTypeenum class cleanly categorizes key types for differentiated rendering. Usingenum classprovides proper scoping and type safety.
149-151: LGTM!The extended
drawKeyboardKeysignature with default parameters maintains backward compatibility while enabling new functionality.src/components/themes/lyra/LyraTheme.cpp (1)
603-645: LGTM overall!The
drawKeyboardKeyimplementation correctly handles allKeyboardKeyTypevariants with appropriate visual differentiation for selected/unselected states, special key glyphs (Space bar, Del arrow), and secondary label positioning.src/activities/util/KeyboardEntryActivity.h (3)
12-17: LGTM!The
KeyDefstruct andSpecialKeyTypeenum provide clean data structures for the grid-based keyboard. Using an unscoped enum forSpecialKeyTypeallows direct use in switch statements and array indexing.
55-58: LGTM!The grid dimension constants are well-defined.
BOTTOM_KEY_COUNT = 5correctly matches the number ofSpecialKeyTypeenum values, ensuring safe array indexing.
60-144: LGTM!The
abcLayoutandsymLayoutconstexpr arrays provide a clean data-driven approach. The design choice to set allsecondaryvalues to'\0'insymLayoutcorrectly disables long-press alternatives in symbol mode.src/components/themes/BaseTheme.cpp (1)
777-792: LGTM!The refactored
drawTextFielduses clean line-based geometry for the brackets. The calculations fortickLenand positioning are correct.src/activities/util/KeyboardEntryActivity.cpp (5)
49-55: LGTM!The
insertCharhelper correctly enforcesmaxLengthand handles the null character case. Returningtrueon both success and max-length-reached allows consistent control flow in the caller.
57-89: LGTM!The
handleKeyPressswitch correctly handles allSpecialKeyTypecases. Thestatic_cast<SpecialKeyType>(selectedCol)is safe becauseselectedColis constrained to[0, BOTTOM_KEY_COUNT-1]when on the bottom row.
94-118: LGTM!The Up/Down navigation correctly rescales
selectedColwhen transitioning between content rows (10 columns) and the bottom row (5 keys). The*2and/2mapping provides intuitive column correspondence.
132-155: LGTM!The long-press state machine is well-implemented:
confirmHeldtracks press stateconfirmLongHandledprevents double-insertion- Alternative character is only inserted once per long-press
- Normal key action fires only on short press (release before timeout)
261-275: LGTM!The bottom row rendering correctly maps
SpecialKeyTypetoKeyboardKeyTypefor theme rendering. ThebottomKeysarray order matches theSpecialKeyTypeenum ordering, ensuring correct key behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/activities/util/KeyboardEntryActivity.cpp (2)
254-254: Misleading variable name and type.
bottomSelectedRowis declared asintbut holds a boolean result fromisBottomRow(). The name suggests it might be a row index. Consider renaming for clarity:-const int bottomSelectedRow = isBottomRow(selectedRow); +const bool isBottomRowSelected = isBottomRow(selectedRow);And update the usage at line 270:
-const bool isSelected = bottomSelectedRow && i == selectedCol; +const bool isSelected = isBottomRowSelected && i == selectedCol;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activities/util/KeyboardEntryActivity.cpp` at line 254, The variable bottomSelectedRow holds a boolean from isBottomRow(selectedRow) but is declared as int and named like an index; change its type to bool and rename it to something clearer (e.g., isSelectedRowBottom or bottomSelected) in KeyboardEntryActivity.cpp where it's declared and in all places it's used (including the usage currently referred to at the later conditional around selectedRow handling). Update the declaration and every reference to the new boolean name to keep semantics identical.
57-86: Missing default case in switch statement.The switch on
static_cast<SpecialKeyType>(selectedCol)lacks a default case. While the navigation logic constrainsselectedColto valid indices (0-4) on the bottom row, a defensive default would make the code more robust and explicit about handling unexpected states.♻️ Add default case
case SpecOk: onComplete(text); return false; + default: + return true; // Unknown key, no action } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activities/util/KeyboardEntryActivity.cpp` around lines 57 - 86, The switch in KeyboardEntryActivity::handleKeyPress over static_cast<SpecialKeyType>(selectedCol) is missing a default branch; add a default case that defensively handles unexpected selectedCol values (e.g., log a warning or noop) and returns the same control flow as other handled cases (likely return true) to avoid falling through without a return. Reference symbols: KeyboardEntryActivity::handleKeyPress, isBottomRow, selectedCol, SpecialKeyType, BOTTOM_KEY_COUNT. Ensure behavior matches existing bottom-row handling (keep selected indices clamped elsewhere) but guard here against invalid enum casts.src/components/themes/BaseTheme.cpp (2)
777-793: UnusedtextWidthparameter.The
textWidthparameter is declared but not used in this implementation. The brackets are positioned at fixed offsets from the rect edges (fieldLeft = rect.x + 10,fieldRight = rect.x + rect.width - 15).This differs from
LyraTheme::drawTextField(lines 597-601 in LyraTheme.cpp), which usestextWidthto compute the underline width. If the fixed-position brackets are intentional for BaseTheme, consider marking the parameter as[[maybe_unused]]to suppress warnings and document the intent.♻️ Suggested fix
-void BaseTheme::drawTextField(const GfxRenderer& renderer, Rect rect, const int textWidth) const { +void BaseTheme::drawTextField(const GfxRenderer& renderer, Rect rect, [[maybe_unused]] const int textWidth) const {🤖 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 777 - 793, The textWidth parameter of BaseTheme::drawTextField is unused and triggers warnings; either use it to compute bracket/underline positions like LyraTheme::drawTextField or explicitly mark it as intentionally unused. Update the function signature for BaseTheme::drawTextField to annotate textWidth as [[maybe_unused]] (and add a short comment like "fixed-position brackets; textWidth unused") if you intend to keep fixed offsets, or modify the bracket calculations (fieldLeft/fieldRight/underline width) to incorporate textWidth to match LyraTheme behavior.
797-833: DuplicatefillRectcall for selected keys.For keys that don't early-return (Normal, Shift, Mode, Ok), when
isSelectedis true,fillRectis called twice: once at line 798 and again at line 832. This is functionally correct but wasteful.♻️ Remove duplicate fill
const int textX = rect.x + (rect.width - itemWidth) / 2; const int textY = rect.y + (rect.height - renderer.getLineHeight(UI_12_FONT_ID)) / 2 + primaryOffset; - if (isSelected) { - renderer.fillRect(rect.x, rect.y, rect.width, rect.height, true); - } renderer.drawText(UI_12_FONT_ID, textX, textY, label, !isSelected);🤖 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 797 - 833, The code calls renderer.fillRect(rect.x, rect.y, rect.width, rect.height, true) twice when isSelected is true; remove the duplicate by deleting the second fillRect (the one just before the end of the snippet) so selection is filled once (keep the initial fillRect that occurs before special-case returns for Space/Del), ensuring drawing of text/secondary labels that follow still uses the existing isSelected state and early-return logic for KeyboardKeyType::Space and ::Del remains unchanged.
🤖 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/BaseTheme.cpp`:
- Line 838: The secondary label in BaseTheme::drawKeyboardKey is drawn at rect.y
- 3 which places it above the key bounds; change the vertical position to align
with LyraTheme::drawKeyboardKey by using rect.y (or otherwise remove the
negative offset) when calling renderer.drawText for the secondaryLabel so it
sits within the key rectangle rather than above it.
---
Nitpick comments:
In `@src/activities/util/KeyboardEntryActivity.cpp`:
- Line 254: The variable bottomSelectedRow holds a boolean from
isBottomRow(selectedRow) but is declared as int and named like an index; change
its type to bool and rename it to something clearer (e.g., isSelectedRowBottom
or bottomSelected) in KeyboardEntryActivity.cpp where it's declared and in all
places it's used (including the usage currently referred to at the later
conditional around selectedRow handling). Update the declaration and every
reference to the new boolean name to keep semantics identical.
- Around line 57-86: The switch in KeyboardEntryActivity::handleKeyPress over
static_cast<SpecialKeyType>(selectedCol) is missing a default branch; add a
default case that defensively handles unexpected selectedCol values (e.g., log a
warning or noop) and returns the same control flow as other handled cases
(likely return true) to avoid falling through without a return. Reference
symbols: KeyboardEntryActivity::handleKeyPress, isBottomRow, selectedCol,
SpecialKeyType, BOTTOM_KEY_COUNT. Ensure behavior matches existing bottom-row
handling (keep selected indices clamped elsewhere) but guard here against
invalid enum casts.
In `@src/components/themes/BaseTheme.cpp`:
- Around line 777-793: The textWidth parameter of BaseTheme::drawTextField is
unused and triggers warnings; either use it to compute bracket/underline
positions like LyraTheme::drawTextField or explicitly mark it as intentionally
unused. Update the function signature for BaseTheme::drawTextField to annotate
textWidth as [[maybe_unused]] (and add a short comment like "fixed-position
brackets; textWidth unused") if you intend to keep fixed offsets, or modify the
bracket calculations (fieldLeft/fieldRight/underline width) to incorporate
textWidth to match LyraTheme behavior.
- Around line 797-833: The code calls renderer.fillRect(rect.x, rect.y,
rect.width, rect.height, true) twice when isSelected is true; remove the
duplicate by deleting the second fillRect (the one just before the end of the
snippet) so selection is filled once (keep the initial fillRect that occurs
before special-case returns for Space/Del), ensuring drawing of text/secondary
labels that follow still uses the existing isSelected state and early-return
logic for KeyboardKeyType::Space and ::Del remains unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b010592b-dcdb-41c5-97fb-83818649594a
📒 Files selected for processing (3)
src/activities/util/KeyboardEntryActivity.cppsrc/components/themes/BaseTheme.cppsrc/components/themes/lyra/LyraTheme.cpp
📜 Review details
⏰ 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 context used
🧠 Learnings (8)
📚 Learning: 2026-03-09T00:59:05.219Z
Learnt from: laird
Repo: crosspoint-reader/crosspoint-reader PR: 1331
File: src/components/themes/pulsr/PulsrTheme.h:58-117
Timestamp: 2026-03-09T00:59:05.219Z
Learning: In PulsrTheme (src/components/themes/pulsr/PulsrTheme.h/.cpp), `drawBatteryLeft` and `drawBatteryRight` are intentionally overridden as no-ops because PULSR displays battery state via pills in the left bar rather than the BaseTheme battery chrome. All of `drawBatteryLeft`, `drawBatteryRight`, `drawStatusBar`, and `drawHelpText` are overridden in PulsrTheme.
Applied to files:
src/components/themes/BaseTheme.cpp
📚 Learning: 2026-03-28T11:06:29.611Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/home/HomeActivity.cpp:92-95
Timestamp: 2026-03-28T11:06:29.611Z
Learning: When reviewing crosspoint-reader code, avoid flagging a missing `renderer.displayBuffer()` call immediately after `GUI.drawPopup()` / `BaseTheme::drawPopup()`: `BaseTheme::drawPopup()` already calls `renderer.displayBuffer()` before returning, so the popup is guaranteed to be flushed to the e-ink panel before subsequent blocking work begins. Conversely, do not require a `renderer.displayBuffer()` call after `fillPopupProgress()`; it intentionally does not flush, so intermediate progress-bar updates may not appear unless the update granularity warrants an explicit flush elsewhere.
Applied to files:
src/components/themes/BaseTheme.cppsrc/activities/util/KeyboardEntryActivity.cppsrc/components/themes/lyra/LyraTheme.cpp
📚 Learning: 2026-03-25T01:15:04.951Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/components/themes/lyra/Lyra3CoversTheme.cpp:39-40
Timestamp: 2026-03-25T01:15:04.951Z
Learning: In crosspoint-reader/crosspoint-reader theme drawing code, `COVER_DISABLED_MODE` only controls whether the app generates new covers (e.g., on HOME entry) to avoid blocking the main thread. It must NOT be treated as a render-suppression switch for already-cached covers. Theme draw paths should only gate cached cover rendering on the per-book `RecentBook::coverDisabled` flag; if a cached cover BMP is available, the correct behavior is to render it even when `COVER_DISABLED_MODE` is set. Therefore, do not flag theme draw code for not checking `COVER_DISABLED_MODE`; checking only `coverDisabled` is correct by design.
Applied to files:
src/components/themes/BaseTheme.cppsrc/components/themes/lyra/LyraTheme.cpp
📚 Learning: 2026-02-27T22:49:59.600Z
Learnt from: ngxson
Repo: crosspoint-reader/crosspoint-reader PR: 1218
File: src/activities/ActivityManager.cpp:254-265
Timestamp: 2026-02-27T22:49:59.600Z
Learning: In this codebase, assertions are always enabled (no NDEBUG). Use assert() to crash on programmer errors and surface logic bugs during development and in production builds. Do not rely on asserts for runtime error handling; they should enforce invariants that must always hold. Keep asserts side-effect free and inexpensive, and avoid relying on them for user-visible failures. Include <cassert> where appropriate and document the invariant being tested.
Applied to files:
src/components/themes/BaseTheme.cppsrc/activities/util/KeyboardEntryActivity.cppsrc/components/themes/lyra/LyraTheme.cpp
📚 Learning: 2026-03-02T10:14:16.036Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1245
File: lib/Epub/Epub/Section.cpp:277-308
Timestamp: 2026-03-02T10:14:16.036Z
Learning: Guideline: Strengthen serialization::readString to defend against unbounded growth when reading from disk data. Implement and enforce a maximum allowed length (e.g., a configured or reasonable constant) and validate the incoming length before resizing or allocating. Audit all call sites (e.g., BookMetadataCache, TextBlock, KOReaderCredentialStore, Section cache readers) to ensure they do not rely on unbounded len-based resizing. If the readString API must remain, add internal safeguards (bounds checks, length validation, and error handling) so per-call-site validations are not required. Ensure Section cache files remain versioned (SECTION_FILE_VERSION) and parameter mismatches invalidate caches, but do not rely on unsafe allocations; prefer safe, bounded reads with explicit errors on invalid data.
Applied to files:
src/components/themes/BaseTheme.cppsrc/activities/util/KeyboardEntryActivity.cppsrc/components/themes/lyra/LyraTheme.cpp
📚 Learning: 2026-02-23T06:18:15.143Z
Learnt from: raygan
Repo: crosspoint-reader/crosspoint-reader PR: 1116
File: src/activities/settings/WallabagSettingsActivity.cpp:57-75
Timestamp: 2026-02-23T06:18:15.143Z
Learning: In settings activities (e.g., KOReaderSettingsActivity, WallabagSettingsActivity), the established pattern for launching KeyboardEntryActivity is to call exitActivity() on the parent settings activity before enterNewActivity(new KeyboardEntryActivity(...)). The lambda callbacks then call exitActivity() to exit the KeyboardEntryActivity and return to the parent. This differs from ConfirmationActivity which does not require exitActivity() before enterNewActivity().
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-05T19:21:32.486Z
Learnt from: Zyd8
Repo: crosspoint-reader/crosspoint-reader PR: 1576
File: src/activities/home/RecentBooksActivity.cpp:65-90
Timestamp: 2026-04-05T19:21:32.486Z
Learning: In crosspoint-reader/crosspoint-reader (src/util/ButtonNavigator.cpp, shouldSuppressListNavForMidpointChord), the guard-clearing tick intentionally still returns `true` when it clears `s_midpointChordReleaseGuard`. This is correct because all list navigation in the activity uses `onNextRelease`/`onPreviousRelease`, which only fire on a fresh press+release cycle. The clearing tick therefore only swallows the stale release from the chord buttons themselves. Any subsequent Up/Down tap starts a new press+release cycle and will not be suppressed. Do not flag this as a dropped-input bug in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-05T19:31:31.692Z
Learnt from: Zyd8
Repo: crosspoint-reader/crosspoint-reader PR: 1576
File: src/util/ButtonNavigator.h:43-47
Timestamp: 2026-04-05T19:31:31.692Z
Learning: In crosspoint-reader/crosspoint-reader (src/util/ButtonNavigator.cpp), `clearMidpointChordReleaseGuard()` intentionally sets `s_suppressUntilChordRelease` in addition to resetting the post-release guard. This blocks `beginUpDownChord()` from firing on the first loop tick of a new activity if the midpoint chord was still physically held during the `Activity::onEnter()` transition. The fix prevents cross-screen midpoint chord leaks without requiring each activity's `onEnter()` to call `isMidpointChordHeld()`. Do not flag this dual-write as redundant in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
🔇 Additional comments (5)
src/components/themes/lyra/LyraTheme.cpp (1)
603-646: LGTM!The implementation cleanly handles the extended signature with
secondaryLabelandkeyType. The special rendering for Space (horizontal line) and Del (left arrow) keys with early returns keeps the logic clear. Selection inversion is handled consistently via theinvertvariable.Minor observation:
primaryOffsetat line 636 is declared but always 0 - could be removed if not planned for future use, but this is a trivial cleanup.src/activities/util/KeyboardEntryActivity.cpp (4)
18-55: LGTM!The helper methods cleanly encapsulate row count logic, bounds checking, and character selection. The
insertCharmethod correctly enforcesmaxLengthand handles the null character edge case.
91-130: LGTM!The navigation logic correctly handles the column count difference between content rows (10 columns) and the bottom row (5 columns) by rescaling
selectedColduring vertical transitions. The wrap-around behavior usingButtonNavigator::previousIndex/nextIndexis consistent.
137-145: Long-press on unsupported keys silently fails.When
getAlternativeChar()returns'\0'(e.g., on the bottom row or in symbol mode), theconfirmLongHandledflag is still set totrue(line 144), which prevents the normal key press from triggering on release.This means long-pressing on bottom row keys (shift, space, delete, etc.) or any key in symbol mode does nothing, without falling back to a normal press. Is this the intended behavior?
If long-press should fall back to normal press when no alternative exists:
♻️ Alternative: only set flag when action taken
if (confirmHeld && !confirmLongHandled && mappedInput.isPressed(MappedInputManager::Button::Confirm) && mappedInput.getHeldTime() > LONG_PRESS_MS) { char alt = getAlternativeChar(); if (alt != '\0') { insertChar(alt); requestUpdate(); + confirmLongHandled = true; } - confirmLongHandled = true; }
162-282: LGTM for overall render refactoring.The rendering logic correctly:
- Uses the new 10-column uniform grid with 95% screen width
- Computes separate bottom-row key widths based on
BOTTOM_KEY_COUNT- Passes
KeyboardKeyTypeto theme for proper special key rendering- Shows secondary labels only for row 0 in ABC mode
…sed textWidth, rename bottomSelectedRow
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/activities/util/KeyboardEntryActivity.cpp`:
- Around line 218-225: The bottom-aligned branch uses
getTotalRowCount()*(keyHeight+keySpacing) which overestimates the drawn
keyboard; instead compute the actual drawn keyboard height using contentRows and
the special bottom-row gap: keyboardHeight = contentRows*keyHeight +
(contentRows - 1)*keySpacing + bottomRowGap (use the variable/name used for the
extra gap), and subtract that keyboardHeight when computing keyboardStartY (and
the analogous adjustment at the other location around lines 251-252). Replace
references to getTotalRowCount() in the bottom-aligned calculation with this
computed keyboardHeight so the keyboard aligns to the bottom exactly as
rendered.
- Around line 139-156: The bug is that confirmLongHandled is set even when
getAlternativeChar() returns '\0', causing long-hold Confirm on non-ABC keys to
be swallowed; change the long-press branch so confirmLongHandled is only set
when an alternative char is actually inserted (move the assignment of
confirmLongHandled into the alt != '\0' block that calls insertChar and
requestUpdate), leaving the release-path logic (which calls handleKeyPress()
when confirmLongHandled is false) intact so OK/delete/space/mode/shift still
trigger on release.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43b0d99b-3f33-4329-92f1-eade8475f369
📒 Files selected for processing (2)
src/activities/util/KeyboardEntryActivity.cppsrc/components/themes/BaseTheme.cpp
📜 Review details
⏰ 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 context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1644
File: src/components/themes/BaseTheme.cpp:835-835
Timestamp: 2026-04-12T02:49:03.552Z
Learning: In crosspoint-reader/crosspoint-reader (src/components/themes/BaseTheme.cpp), the secondary label in `BaseTheme::drawKeyboardKey` is intentionally drawn at `rect.y - 3` (3px above the key rectangle's top edge). This uses part of BaseTheme's `keySpacing = 10` inter-row gap to visually separate the secondary symbol hint from the primary character. In contrast, `LyraTheme::drawKeyboardKey` uses `rect.y` (no offset) because LyraTheme has `keySpacing = 0`. Both placements are correct and deliberate for their respective themes. Do not flag the `-3` offset as a bug or suggest aligning it to `rect.y`.
📚 Learning: 2026-02-23T06:18:15.143Z
Learnt from: raygan
Repo: crosspoint-reader/crosspoint-reader PR: 1116
File: src/activities/settings/WallabagSettingsActivity.cpp:57-75
Timestamp: 2026-02-23T06:18:15.143Z
Learning: In settings activities (e.g., KOReaderSettingsActivity, WallabagSettingsActivity), the established pattern for launching KeyboardEntryActivity is to call exitActivity() on the parent settings activity before enterNewActivity(new KeyboardEntryActivity(...)). The lambda callbacks then call exitActivity() to exit the KeyboardEntryActivity and return to the parent. This differs from ConfirmationActivity which does not require exitActivity() before enterNewActivity().
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-12T02:49:03.552Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1644
File: src/components/themes/BaseTheme.cpp:835-835
Timestamp: 2026-04-12T02:49:03.552Z
Learning: In crosspoint-reader/crosspoint-reader (src/components/themes/BaseTheme.cpp), the secondary label in `BaseTheme::drawKeyboardKey` is intentionally drawn at `rect.y - 3` (3px above the key rectangle's top edge). This uses part of BaseTheme's `keySpacing = 10` inter-row gap to visually separate the secondary symbol hint from the primary character. In contrast, `LyraTheme::drawKeyboardKey` uses `rect.y` (no offset) because LyraTheme has `keySpacing = 0`. Both placements are correct and deliberate for their respective themes. Do not flag the `-3` offset as a bug or suggest aligning it to `rect.y`.
Applied to files:
src/activities/util/KeyboardEntryActivity.cppsrc/components/themes/BaseTheme.cpp
📚 Learning: 2026-04-05T19:21:32.486Z
Learnt from: Zyd8
Repo: crosspoint-reader/crosspoint-reader PR: 1576
File: src/activities/home/RecentBooksActivity.cpp:65-90
Timestamp: 2026-04-05T19:21:32.486Z
Learning: In crosspoint-reader/crosspoint-reader (src/util/ButtonNavigator.cpp, shouldSuppressListNavForMidpointChord), the guard-clearing tick intentionally still returns `true` when it clears `s_midpointChordReleaseGuard`. This is correct because all list navigation in the activity uses `onNextRelease`/`onPreviousRelease`, which only fire on a fresh press+release cycle. The clearing tick therefore only swallows the stale release from the chord buttons themselves. Any subsequent Up/Down tap starts a new press+release cycle and will not be suppressed. Do not flag this as a dropped-input bug in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-05T19:31:31.692Z
Learnt from: Zyd8
Repo: crosspoint-reader/crosspoint-reader PR: 1576
File: src/util/ButtonNavigator.h:43-47
Timestamp: 2026-04-05T19:31:31.692Z
Learning: In crosspoint-reader/crosspoint-reader (src/util/ButtonNavigator.cpp), `clearMidpointChordReleaseGuard()` intentionally sets `s_suppressUntilChordRelease` in addition to resetting the post-release guard. This blocks `beginUpDownChord()` from firing on the first loop tick of a new activity if the midpoint chord was still physically held during the `Activity::onEnter()` transition. The fix prevents cross-screen midpoint chord leaks without requiring each activity's `onEnter()` to call `isMidpointChordHeld()`. Do not flag this dual-write as redundant in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-02-27T22:49:59.600Z
Learnt from: ngxson
Repo: crosspoint-reader/crosspoint-reader PR: 1218
File: src/activities/ActivityManager.cpp:254-265
Timestamp: 2026-02-27T22:49:59.600Z
Learning: In this codebase, assertions are always enabled (no NDEBUG). Use assert() to crash on programmer errors and surface logic bugs during development and in production builds. Do not rely on asserts for runtime error handling; they should enforce invariants that must always hold. Keep asserts side-effect free and inexpensive, and avoid relying on them for user-visible failures. Include <cassert> where appropriate and document the invariant being tested.
Applied to files:
src/activities/util/KeyboardEntryActivity.cppsrc/components/themes/BaseTheme.cpp
📚 Learning: 2026-03-02T10:14:16.036Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1245
File: lib/Epub/Epub/Section.cpp:277-308
Timestamp: 2026-03-02T10:14:16.036Z
Learning: Guideline: Strengthen serialization::readString to defend against unbounded growth when reading from disk data. Implement and enforce a maximum allowed length (e.g., a configured or reasonable constant) and validate the incoming length before resizing or allocating. Audit all call sites (e.g., BookMetadataCache, TextBlock, KOReaderCredentialStore, Section cache readers) to ensure they do not rely on unbounded len-based resizing. If the readString API must remain, add internal safeguards (bounds checks, length validation, and error handling) so per-call-site validations are not required. Ensure Section cache files remain versioned (SECTION_FILE_VERSION) and parameter mismatches invalidate caches, but do not rely on unsafe allocations; prefer safe, bounded reads with explicit errors on invalid data.
Applied to files:
src/activities/util/KeyboardEntryActivity.cppsrc/components/themes/BaseTheme.cpp
📚 Learning: 2026-03-28T11:06:29.611Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/home/HomeActivity.cpp:92-95
Timestamp: 2026-03-28T11:06:29.611Z
Learning: When reviewing crosspoint-reader code, avoid flagging a missing `renderer.displayBuffer()` call immediately after `GUI.drawPopup()` / `BaseTheme::drawPopup()`: `BaseTheme::drawPopup()` already calls `renderer.displayBuffer()` before returning, so the popup is guaranteed to be flushed to the e-ink panel before subsequent blocking work begins. Conversely, do not require a `renderer.displayBuffer()` call after `fillPopupProgress()`; it intentionally does not flush, so intermediate progress-bar updates may not appear unless the update granularity warrants an explicit flush elsewhere.
Applied to files:
src/activities/util/KeyboardEntryActivity.cppsrc/components/themes/BaseTheme.cpp
📚 Learning: 2026-03-09T00:59:05.219Z
Learnt from: laird
Repo: crosspoint-reader/crosspoint-reader PR: 1331
File: src/components/themes/pulsr/PulsrTheme.h:58-117
Timestamp: 2026-03-09T00:59:05.219Z
Learning: In PulsrTheme (src/components/themes/pulsr/PulsrTheme.h/.cpp), `drawBatteryLeft` and `drawBatteryRight` are intentionally overridden as no-ops because PULSR displays battery state via pills in the left bar rather than the BaseTheme battery chrome. All of `drawBatteryLeft`, `drawBatteryRight`, `drawStatusBar`, and `drawHelpText` are overridden in PulsrTheme.
Applied to files:
src/components/themes/BaseTheme.cpp
📚 Learning: 2026-03-25T01:15:04.951Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/components/themes/lyra/Lyra3CoversTheme.cpp:39-40
Timestamp: 2026-03-25T01:15:04.951Z
Learning: In crosspoint-reader/crosspoint-reader theme drawing code, `COVER_DISABLED_MODE` only controls whether the app generates new covers (e.g., on HOME entry) to avoid blocking the main thread. It must NOT be treated as a render-suppression switch for already-cached covers. Theme draw paths should only gate cached cover rendering on the per-book `RecentBook::coverDisabled` flag; if a cached cover BMP is available, the correct behavior is to render it even when `COVER_DISABLED_MODE` is set. Therefore, do not flag theme draw code for not checking `COVER_DISABLED_MODE`; checking only `coverDisabled` is correct by design.
Applied to files:
src/components/themes/BaseTheme.cpp
🔇 Additional comments (2)
src/components/themes/BaseTheme.cpp (1)
805-835: Looks good.
Space/Delreturn before any text measurement, so thenullptrlabels used for those keys stay safe, and the secondary hint placement is correctly theme-specific here.Based on learnings, in crosspoint-reader/crosspoint-reader (
src/components/themes/BaseTheme.cpp), the secondary label inBaseTheme::drawKeyboardKeyis intentionally drawn atrect.y - 3to use part of BaseTheme's inter-row gap.src/activities/util/KeyboardEntryActivity.cpp (1)
226-247: Nice layout-driven rendering path.The primary/secondary swap stays in the
KeyDeftables instead of being hardcoded per row, andshowSecondarykeeps the top-right hints scoped to ABC row 0 cleanly.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/activities/util/KeyboardEntryActivity.cpp (1)
18-20: Future-proof content row count against mode drift.
getContentRowCount()always returnsABC_ROWSalthoughSYM_ROWSexists. If layouts diverge later, navigation/rendering will silently break in symbol mode. Consider deriving this fromsymMode(or asserting equality explicitly).♻️ Proposed refactor
-int KeyboardEntryActivity::getContentRowCount() const { return ABC_ROWS; } +int KeyboardEntryActivity::getContentRowCount() const { return symMode ? SYM_ROWS : ABC_ROWS; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activities/util/KeyboardEntryActivity.cpp` around lines 18 - 20, The current getContentRowCount() in KeyboardEntryActivity always returns ABC_ROWS which will break if symbol mode uses a different row count; update getContentRowCount() to return the correct count based on the activity's mode (e.g., return symMode ? SYM_ROWS : ABC_ROWS or call the existing method/flag that indicates symbol mode), or alternatively add an assert in getContentRowCount() that verifies ABC_ROWS == SYM_ROWS if you intend them to remain equal; ensure getTotalRowCount() continues to call getContentRowCount() so total updates automatically when content row count changes.
🤖 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/activities/util/KeyboardEntryActivity.cpp`:
- Around line 18-20: The current getContentRowCount() in KeyboardEntryActivity
always returns ABC_ROWS which will break if symbol mode uses a different row
count; update getContentRowCount() to return the correct count based on the
activity's mode (e.g., return symMode ? SYM_ROWS : ABC_ROWS or call the existing
method/flag that indicates symbol mode), or alternatively add an assert in
getContentRowCount() that verifies ABC_ROWS == SYM_ROWS if you intend them to
remain equal; ensure getTotalRowCount() continues to call getContentRowCount()
so total updates automatically when content row count changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b00b4d36-fbb6-4e20-9894-ebe190b4d36e
📒 Files selected for processing (1)
src/activities/util/KeyboardEntryActivity.cpp
📜 Review details
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1644
File: src/activities/util/KeyboardEntryActivity.cpp:218-225
Timestamp: 2026-04-12T03:00:46.785Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/util/KeyboardEntryActivity.cpp), the bottom-aligned keyboard start Y uses the simplified formula `getTotalRowCount() * (keyHeight + keySpacing)` intentionally. The small overestimation vs. the actual drawn height is compensated by per-theme `keyboardVerticalOffset` (BaseMetrics: -10, LyraMetrics: -12), manually tuned and verified on-device. Do not flag this approximation as a bug or suggest replacing it with an exact `keyboardHeight` calculation.
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1644
File: src/components/themes/BaseTheme.cpp:835-835
Timestamp: 2026-04-12T02:49:03.552Z
Learning: In crosspoint-reader/crosspoint-reader (src/components/themes/BaseTheme.cpp), the secondary label in `BaseTheme::drawKeyboardKey` is intentionally drawn at `rect.y - 3` (3px above the key rectangle's top edge). This uses part of BaseTheme's `keySpacing = 10` inter-row gap to visually separate the secondary symbol hint from the primary character. In contrast, `LyraTheme::drawKeyboardKey` uses `rect.y` (no offset) because LyraTheme has `keySpacing = 0`. Both placements are correct and deliberate for their respective themes. Do not flag the `-3` offset as a bug or suggest aligning it to `rect.y`.
📚 Learning: 2026-04-12T03:00:46.785Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1644
File: src/activities/util/KeyboardEntryActivity.cpp:218-225
Timestamp: 2026-04-12T03:00:46.785Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/util/KeyboardEntryActivity.cpp), the bottom-aligned keyboard start Y uses the simplified formula `getTotalRowCount() * (keyHeight + keySpacing)` intentionally. The small overestimation vs. the actual drawn height is compensated by per-theme `keyboardVerticalOffset` (BaseMetrics: -10, LyraMetrics: -12), manually tuned and verified on-device. Do not flag this approximation as a bug or suggest replacing it with an exact `keyboardHeight` calculation.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-12T02:49:03.552Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1644
File: src/components/themes/BaseTheme.cpp:835-835
Timestamp: 2026-04-12T02:49:03.552Z
Learning: In crosspoint-reader/crosspoint-reader (src/components/themes/BaseTheme.cpp), the secondary label in `BaseTheme::drawKeyboardKey` is intentionally drawn at `rect.y - 3` (3px above the key rectangle's top edge). This uses part of BaseTheme's `keySpacing = 10` inter-row gap to visually separate the secondary symbol hint from the primary character. In contrast, `LyraTheme::drawKeyboardKey` uses `rect.y` (no offset) because LyraTheme has `keySpacing = 0`. Both placements are correct and deliberate for their respective themes. Do not flag the `-3` offset as a bug or suggest aligning it to `rect.y`.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-02-23T06:18:15.143Z
Learnt from: raygan
Repo: crosspoint-reader/crosspoint-reader PR: 1116
File: src/activities/settings/WallabagSettingsActivity.cpp:57-75
Timestamp: 2026-02-23T06:18:15.143Z
Learning: In settings activities (e.g., KOReaderSettingsActivity, WallabagSettingsActivity), the established pattern for launching KeyboardEntryActivity is to call exitActivity() on the parent settings activity before enterNewActivity(new KeyboardEntryActivity(...)). The lambda callbacks then call exitActivity() to exit the KeyboardEntryActivity and return to the parent. This differs from ConfirmationActivity which does not require exitActivity() before enterNewActivity().
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-05T19:31:31.692Z
Learnt from: Zyd8
Repo: crosspoint-reader/crosspoint-reader PR: 1576
File: src/util/ButtonNavigator.h:43-47
Timestamp: 2026-04-05T19:31:31.692Z
Learning: In crosspoint-reader/crosspoint-reader (src/util/ButtonNavigator.cpp), `clearMidpointChordReleaseGuard()` intentionally sets `s_suppressUntilChordRelease` in addition to resetting the post-release guard. This blocks `beginUpDownChord()` from firing on the first loop tick of a new activity if the midpoint chord was still physically held during the `Activity::onEnter()` transition. The fix prevents cross-screen midpoint chord leaks without requiring each activity's `onEnter()` to call `isMidpointChordHeld()`. Do not flag this dual-write as redundant in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-05T19:21:32.486Z
Learnt from: Zyd8
Repo: crosspoint-reader/crosspoint-reader PR: 1576
File: src/activities/home/RecentBooksActivity.cpp:65-90
Timestamp: 2026-04-05T19:21:32.486Z
Learning: In crosspoint-reader/crosspoint-reader (src/util/ButtonNavigator.cpp, shouldSuppressListNavForMidpointChord), the guard-clearing tick intentionally still returns `true` when it clears `s_midpointChordReleaseGuard`. This is correct because all list navigation in the activity uses `onNextRelease`/`onPreviousRelease`, which only fire on a fresh press+release cycle. The clearing tick therefore only swallows the stale release from the chord buttons themselves. Any subsequent Up/Down tap starts a new press+release cycle and will not be suppressed. Do not flag this as a dropped-input bug in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-28T20:57:09.493Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 0
File: :0-0
Timestamp: 2026-03-28T20:57:09.493Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/home/HomeActivity.cpp, HomeActivity::onEnter), `coverRendered = false` and `coverBufferStored = false` ARE correctly reset in `onEnter()`. This was fixed in PR `#1488`. Do not flag their absence as a bug in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-09T20:05:09.146Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/reader/EpubReaderMenuActivity.cpp:39-45
Timestamp: 2026-04-09T20:05:09.146Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/reader/EpubReaderMenuActivity.cpp, EpubReaderMenuActivity::buildMenuItems), the `hasCoverForTheme` boolean (derived from `Storage.exists(coverPath)`) is intentionally used — not `!coverDisabled` — to choose between STR_HOME_COVER_ACTION_DISABLE and STR_HOME_COVER_ACTION_ENABLE in non-TIMEOUT global modes. The label reflects the action that will be performed: BMP exists → "Disable" (delete it); no BMP → "Enable" (generate it). Using `coverDisabled` would create confusing edge cases (e.g., coverDisabled=false with no BMP on disk would show "Disable" when there is nothing to disable). Do not flag this as a bug in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-28T11:08:53.531Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/home/HomeActivity.cpp:114-128
Timestamp: 2026-03-28T11:08:53.531Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/home/HomeActivity.cpp, HomeActivity::loadRecentCovers), the xtc.load() failure path intentionally does NOT set coverDisabled=true. XTC cover handling (including failure→disable behavior) is out of scope until the XTC reader gets its own cover menu actions. Do not flag the missing coverDisabled update on xtc.load() failure as a bug in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-25T01:06:03.725Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/home/HomeActivity.cpp:91-100
Timestamp: 2026-03-25T01:06:03.725Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/home/HomeActivity.cpp, HomeActivity::loadRecentCovers), marking coverDisabled=true on any generateThumbBmp() failure is intentional by design. This prevents HOME from retrying indefinitely. The user can always re-enable via Reader Menu → "Generate cover" / "Enable cover". Do not flag this pattern as a bug in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-09T18:26:10.522Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 0
File: :0-0
Timestamp: 2026-04-09T18:26:10.522Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/home/HomeActivity.cpp, HomeActivity::loadRecentCovers), the XTC generateThumbBmp() failure path uses master-style handling: it clears coverBmpPath by calling RECENT_BOOKS.updateBook(book.path, book.title, book.author, "") rather than setting coverDisabled=true. Only the EPUB path uses setCoverDisabled(true) on generateThumbBmp failure. The reason is intentional by design: XTC reader has no reader menu COVER_ACTION (no "Enable/Generate cover" option), so setting coverDisabled=true on XTC failure would permanently block regeneration with no user-accessible recovery path. Clearing the BMP path instead allows HOME to fall back to the classical placeholder, and a future successful generateThumbBmp() call will repopulate it naturally. Do not flag XTC failure→clear-bmpPath vs EPUB failure→coverDisabled as an inconsistency in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-28T11:06:29.611Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/home/HomeActivity.cpp:92-95
Timestamp: 2026-03-28T11:06:29.611Z
Learning: When reviewing crosspoint-reader code, avoid flagging a missing `renderer.displayBuffer()` call immediately after `GUI.drawPopup()` / `BaseTheme::drawPopup()`: `BaseTheme::drawPopup()` already calls `renderer.displayBuffer()` before returning, so the popup is guaranteed to be flushed to the e-ink panel before subsequent blocking work begins. Conversely, do not require a `renderer.displayBuffer()` call after `fillPopupProgress()`; it intentionally does not flush, so intermediate progress-bar updates may not appear unless the update granularity warrants an explicit flush elsewhere.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-08T13:08:46.521Z
Learnt from: zgredex
Repo: crosspoint-reader/crosspoint-reader PR: 1614
File: src/activities/boot_sleep/SleepActivity.cpp:186-212
Timestamp: 2026-04-08T13:08:46.521Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/boot_sleep/SleepActivity.cpp, SleepActivity::renderPxcSleepScreen and src/activities/util/PxcViewerActivity.cpp), a truncated PXC payload detected mid-render (short read inside the renderGrayscale callback) intentionally produces a partial frame rather than triggering a fallback. The pre-render failure paths (file open, header read, dimension mismatch) do fall back to renderDefaultSleepScreen() / error screen, but partial-read propagation inside the callback is considered acceptable by design. Do not flag this as a missing graceful fallback in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-25T01:15:06.744Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/components/themes/lyra/Lyra3CoversTheme.cpp:39-40
Timestamp: 2026-03-25T01:15:06.744Z
Learning: In crosspoint-reader/crosspoint-reader (src/components/themes/lyra/Lyra3CoversTheme.cpp and other theme drawing functions), `COVER_DISABLED_MODE` (global setting) only prevents new cover *generation* on HOME entry to avoid blocking the main thread. It does NOT suppress the display of already-cached covers. Only the per-book `coverDisabled` flag (RecentBook::coverDisabled) controls whether a cached cover BMP is rendered. Do not flag theme draw paths for not checking COVER_DISABLED_MODE; checking only coverDisabled is correct by design.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-02-27T22:49:59.600Z
Learnt from: ngxson
Repo: crosspoint-reader/crosspoint-reader PR: 1218
File: src/activities/ActivityManager.cpp:254-265
Timestamp: 2026-02-27T22:49:59.600Z
Learning: In this codebase, assertions are always enabled (no NDEBUG). Use assert() to crash on programmer errors and surface logic bugs during development and in production builds. Do not rely on asserts for runtime error handling; they should enforce invariants that must always hold. Keep asserts side-effect free and inexpensive, and avoid relying on them for user-visible failures. Include <cassert> where appropriate and document the invariant being tested.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-02T10:14:16.036Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1245
File: lib/Epub/Epub/Section.cpp:277-308
Timestamp: 2026-03-02T10:14:16.036Z
Learning: Guideline: Strengthen serialization::readString to defend against unbounded growth when reading from disk data. Implement and enforce a maximum allowed length (e.g., a configured or reasonable constant) and validate the incoming length before resizing or allocating. Audit all call sites (e.g., BookMetadataCache, TextBlock, KOReaderCredentialStore, Section cache readers) to ensure they do not rely on unbounded len-based resizing. If the readString API must remain, add internal safeguards (bounds checks, length validation, and error handling) so per-call-site validations are not required. Ensure Section cache files remain versioned (SECTION_FILE_VERSION) and parameter mismatches invalidate caches, but do not rely on unsafe allocations; prefer safe, bounded reads with explicit errors on invalid data.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
🔇 Additional comments (4)
src/activities/util/KeyboardEntryActivity.cpp (4)
57-91: Bottom-row dispatch and character insertion flow look solid.The
SpecialKeyTypeswitch cleanly separates control actions from content insertion, and routing insertions throughinsertChar()keeps max-length behavior consistent.
96-131: Navigation mapping between content rows and bottom row is well-handled.The
*2//2column mapping plus per-row max-column clamping keeps movement predictable with wrap-around.
139-146: Long-press confirm handling is correctly guarded now.
confirmLongHandledis only set when an alternative character is actually inserted, preserving normal release behavior for non-alt keys.
223-276: Rendering path aligns with the redesigned keyboard model.Uniform
COLScontent rendering, secondary-label gating, and typed bottom-row keys are consistent with the new theme/key abstractions.
Contributor
|
So...I have a request/question... I've always felt that because this keyboard is left right navigation dpad style it should actually be ABC format not qwerty. I find the qwerty style insanely hard to mentally use in a linear dpad interaction method and feel like it would be faster to input if it was just alphabetical. It would probably also localize better to other regions that way as well |
jpirnay
added a commit
to jpirnay/crosspoint-reader
that referenced
this pull request
Apr 12, 2026
refactor: redesign on-screen keyboard (Upstream crosspoint-reader#1644 by pablohc) + pwd show/hide toggle
CaptainFrito
previously approved these changes
Apr 13, 2026
Contributor
CaptainFrito
left a comment
CaptainFrito
left a comment
There was a problem hiding this comment.
I dig this! Clean implementation too. Thanks @pablohc
Contributor
Author
|
Thank you so much for the comment and review, @CaptainFrito. I’ve been testing some changes @jpirnay made to my PR, and they gave me some interesting ideas to implement. I’ll reach out to you again for a new review if that’s okay with you. @itsthisjustin Thanks for the feedback, but my intention was to keep QWERTY layout intact without causing friction for other users, while polishing the aesthetics and navigation between characters. The only “creative liberty” I took was moving the 0 to the beginning of the numbers row for visual preference. 
|
Contributor
Will get it tested tonight |
…ents, and layout unification
…plicate navigation
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/components/themes/BaseTheme.cpp (1)
843-845:⚠️ Potential issue | 🟡 MinorRestore the BaseTheme secondary hint offset.
Line 845 moves the secondary symbol down into the key body. In BaseTheme, that hint is intentionally drawn 3px above the key to use the 10px inter-row gap; switching to
rect.yremoves that separation and regresses the original spacing.🎯 Proposed fix
if (hasSecondary) { const int secWidth = renderer.getTextWidth(SMALL_FONT_ID, secondaryLabel); - renderer.drawText(SMALL_FONT_ID, rect.x + rect.width - secWidth - 1, rect.y, secondaryLabel, !invert); + renderer.drawText(SMALL_FONT_ID, rect.x + rect.width - secWidth - 1, rect.y - 3, secondaryLabel, !invert); }Based on learnings, in
src/components/themes/BaseTheme.cppthe secondary label placement atrect.y - 3is deliberate so BaseTheme uses its inter-row gap to separate the hint from the primary character.🤖 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 843 - 845, The secondary hint in BaseTheme was moved to rect.y which removes the intended 3px vertical gap; revert the placement so the secondary label is drawn at rect.y - 3 to restore the inter-row spacing. Update the hasSecondary / renderer.drawText call that uses SMALL_FONT_ID and secondaryLabel so the y coordinate is rect.y - 3 (keeping existing x calculation and invert logic) in BaseTheme.cpp.
🤖 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/activities/util/KeyboardEntryActivity.cpp`:
- Around line 50-63: The bug is that getAlternativeChar() always indexes
abcLayout so long-pressing a URL snippet returns an unrelated ABC alternate;
change the function to use the active content layout and bail out when the
active layout is not the ABC keyboard. Concretely, in getAlternativeChar() (and
keeping the existing symMode check) replace the hardcoded const
KeyDef(*layout)[COLS] = abcLayout; with something like const
KeyDef(*layout)[COLS] = getContentLayout(); then add an early return (return
'\0') when layout != abcLayout so alternative-char lookup is disabled for
URL/snippet layouts.
- Around line 309-321: When masking password characters in
KeyboardEntryActivity.cpp, avoid revealing the first character when cursorPos ==
0 by setting revealPos to an invalid index instead of 0; update the
non-cursorMode branch to compute revealPos = (text.length() > 0 && cursorPos >
0) ? cursorPos - 1 : std::string::npos (or SIZE_MAX) so the masking loop (which
compares i != revealPos) will not accidentally leave displayText[0] unmasked;
keep the rest of the logic (inputType, passwordVisible, displayText, text,
cursorMode, cursorPos) unchanged.
---
Duplicate comments:
In `@src/components/themes/BaseTheme.cpp`:
- Around line 843-845: The secondary hint in BaseTheme was moved to rect.y which
removes the intended 3px vertical gap; revert the placement so the secondary
label is drawn at rect.y - 3 to restore the inter-row spacing. Update the
hasSecondary / renderer.drawText call that uses SMALL_FONT_ID and secondaryLabel
so the y coordinate is rect.y - 3 (keeping existing x calculation and invert
logic) in BaseTheme.cpp.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 952edd67-565c-4fe2-8cb3-754d4d2ad405
📒 Files selected for processing (10)
src/activities/network/WifiSelectionActivity.cppsrc/activities/settings/CalibreSettingsActivity.cppsrc/activities/settings/KOReaderSettingsActivity.cppsrc/activities/util/KeyboardEntryActivity.cppsrc/activities/util/KeyboardEntryActivity.hsrc/components/themes/BaseTheme.cppsrc/components/themes/BaseTheme.hsrc/components/themes/lyra/Lyra3CoversTheme.hsrc/components/themes/lyra/LyraTheme.cppsrc/components/themes/lyra/LyraTheme.h
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/themes/lyra/Lyra3CoversTheme.h
- src/components/themes/lyra/LyraTheme.h
📜 Review details
🧰 Additional context used
🧠 Learnings (24)
📓 Common learnings
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1644
File: src/activities/util/KeyboardEntryActivity.cpp:218-225
Timestamp: 2026-04-12T03:00:48.309Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/util/KeyboardEntryActivity.cpp), the bottom-aligned keyboard start Y uses the simplified formula `getTotalRowCount() * (keyHeight + keySpacing)` intentionally. The small overestimation vs. the actual drawn height is compensated by per-theme `keyboardVerticalOffset` (BaseMetrics: -10, LyraMetrics: -12), manually tuned and verified on-device. Do not flag this approximation as a bug or suggest replacing it with an exact `keyboardHeight` calculation.
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1644
File: src/components/themes/BaseTheme.cpp:835-835
Timestamp: 2026-04-12T02:49:05.293Z
Learning: In crosspoint-reader/crosspoint-reader (src/components/themes/BaseTheme.cpp), the secondary label in `BaseTheme::drawKeyboardKey` is intentionally drawn at `rect.y - 3` (3px above the key rectangle's top edge). This uses part of BaseTheme's `keySpacing = 10` inter-row gap to visually separate the secondary symbol hint from the primary character. In contrast, `LyraTheme::drawKeyboardKey` uses `rect.y` (no offset) because LyraTheme has `keySpacing = 0`. Both placements are correct and deliberate for their respective themes. Do not flag the `-3` offset as a bug or suggest aligning it to `rect.y`.
Learnt from: raygan
Repo: crosspoint-reader/crosspoint-reader PR: 1116
File: src/activities/settings/WallabagSettingsActivity.cpp:57-75
Timestamp: 2026-02-23T06:18:15.143Z
Learning: In settings activities (e.g., KOReaderSettingsActivity, WallabagSettingsActivity), the established pattern for launching KeyboardEntryActivity is to call exitActivity() on the parent settings activity before enterNewActivity(new KeyboardEntryActivity(...)). The lambda callbacks then call exitActivity() to exit the KeyboardEntryActivity and return to the parent. This differs from ConfirmationActivity which does not require exitActivity() before enterNewActivity().
📚 Learning: 2026-02-23T06:18:15.143Z
Learnt from: raygan
Repo: crosspoint-reader/crosspoint-reader PR: 1116
File: src/activities/settings/WallabagSettingsActivity.cpp:57-75
Timestamp: 2026-02-23T06:18:15.143Z
Learning: In settings activities (e.g., KOReaderSettingsActivity, WallabagSettingsActivity), the established pattern for launching KeyboardEntryActivity is to call exitActivity() on the parent settings activity before enterNewActivity(new KeyboardEntryActivity(...)). The lambda callbacks then call exitActivity() to exit the KeyboardEntryActivity and return to the parent. This differs from ConfirmationActivity which does not require exitActivity() before enterNewActivity().
Applied to files:
src/activities/network/WifiSelectionActivity.cppsrc/activities/util/KeyboardEntryActivity.hsrc/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-02-27T22:49:59.600Z
Learnt from: ngxson
Repo: crosspoint-reader/crosspoint-reader PR: 1218
File: src/activities/ActivityManager.cpp:254-265
Timestamp: 2026-02-27T22:49:59.600Z
Learning: In this codebase, assertions are always enabled (no NDEBUG). Use assert() to crash on programmer errors and surface logic bugs during development and in production builds. Do not rely on asserts for runtime error handling; they should enforce invariants that must always hold. Keep asserts side-effect free and inexpensive, and avoid relying on them for user-visible failures. Include <cassert> where appropriate and document the invariant being tested.
Applied to files:
src/activities/network/WifiSelectionActivity.cppsrc/activities/settings/CalibreSettingsActivity.cppsrc/activities/settings/KOReaderSettingsActivity.cppsrc/components/themes/lyra/LyraTheme.cppsrc/components/themes/BaseTheme.cppsrc/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-02T10:14:16.036Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1245
File: lib/Epub/Epub/Section.cpp:277-308
Timestamp: 2026-03-02T10:14:16.036Z
Learning: Guideline: Strengthen serialization::readString to defend against unbounded growth when reading from disk data. Implement and enforce a maximum allowed length (e.g., a configured or reasonable constant) and validate the incoming length before resizing or allocating. Audit all call sites (e.g., BookMetadataCache, TextBlock, KOReaderCredentialStore, Section cache readers) to ensure they do not rely on unbounded len-based resizing. If the readString API must remain, add internal safeguards (bounds checks, length validation, and error handling) so per-call-site validations are not required. Ensure Section cache files remain versioned (SECTION_FILE_VERSION) and parameter mismatches invalidate caches, but do not rely on unsafe allocations; prefer safe, bounded reads with explicit errors on invalid data.
Applied to files:
src/activities/network/WifiSelectionActivity.cppsrc/activities/settings/CalibreSettingsActivity.cppsrc/activities/settings/KOReaderSettingsActivity.cppsrc/components/themes/lyra/LyraTheme.cppsrc/components/themes/BaseTheme.cppsrc/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-28T11:06:29.611Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/home/HomeActivity.cpp:92-95
Timestamp: 2026-03-28T11:06:29.611Z
Learning: When reviewing crosspoint-reader code, avoid flagging a missing `renderer.displayBuffer()` call immediately after `GUI.drawPopup()` / `BaseTheme::drawPopup()`: `BaseTheme::drawPopup()` already calls `renderer.displayBuffer()` before returning, so the popup is guaranteed to be flushed to the e-ink panel before subsequent blocking work begins. Conversely, do not require a `renderer.displayBuffer()` call after `fillPopupProgress()`; it intentionally does not flush, so intermediate progress-bar updates may not appear unless the update granularity warrants an explicit flush elsewhere.
Applied to files:
src/activities/network/WifiSelectionActivity.cppsrc/activities/settings/CalibreSettingsActivity.cppsrc/activities/settings/KOReaderSettingsActivity.cppsrc/components/themes/lyra/LyraTheme.cppsrc/components/themes/BaseTheme.cppsrc/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-12T12:28:33.205Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1629
File: src/activities/home/HomeActivity.cpp:119-120
Timestamp: 2026-04-12T12:28:33.205Z
Learning: When reviewing code in this repository (C/C++ sources), set review comment severity according to this policy:
- Use **Major** (🟠) only for defects with realistic risk of crash, out-of-memory (OOM), invalid pointer dereference, data corruption, or other severe issues that are unlikely to be caught in casual/manual device testing.
- Use **Minor** or informational for UX gaps, logic edge-cases, style issues, or missing feature completeness that the author can verify (or has verified) through normal device use.
- Do **not** escalate severity to Major based on behavioral/UX observations alone; assume the author has already tested the feature on their own device and only treat issues as Major if they match the high-risk defect categories above.
Applied to files:
src/activities/network/WifiSelectionActivity.cppsrc/activities/settings/CalibreSettingsActivity.cppsrc/activities/settings/KOReaderSettingsActivity.cppsrc/activities/util/KeyboardEntryActivity.hsrc/components/themes/lyra/LyraTheme.cppsrc/components/themes/BaseTheme.cppsrc/components/themes/BaseTheme.hsrc/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-02-23T06:18:08.408Z
Learnt from: raygan
Repo: crosspoint-reader/crosspoint-reader PR: 1116
File: src/activities/settings/WallabagSettingsActivity.cpp:57-75
Timestamp: 2026-02-23T06:18:08.408Z
Learning: When navigating from a settings activity to KeyboardEntryActivity (e.g., KOReaderSettingsActivity, WallabagSettingsActivity), call exitActivity() on the parent before calling enterNewActivity(new KeyboardEntryActivity(...)). The KeyboardEntryActivity should then call exitActivity() via its callbacks to return to the parent. Note: ConfirmationActivity does not require exitActivity() before entering KeyboardEntryActivity. Apply this pattern to similar settings activities in this module.
Applied to files:
src/activities/settings/CalibreSettingsActivity.cppsrc/activities/settings/KOReaderSettingsActivity.cpp
📚 Learning: 2026-02-12T06:57:35.955Z
Learnt from: CaptainFrito
Repo: crosspoint-reader/crosspoint-reader PR: 725
File: src/activities/network/CalibreConnectActivity.cpp:218-264
Timestamp: 2026-02-12T06:57:35.955Z
Learning: In src/activities/network/CalibreConnectActivity.cpp, button hints (« Exit) are intentionally omitted from the SERVER_STARTING and ERROR states—only the SERVER_RUNNING state displays navigation hints. This is a deliberate design decision by the author.
Applied to files:
src/activities/settings/CalibreSettingsActivity.cpp
📚 Learning: 2026-04-12T03:00:48.309Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1644
File: src/activities/util/KeyboardEntryActivity.cpp:218-225
Timestamp: 2026-04-12T03:00:48.309Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/util/KeyboardEntryActivity.cpp), the bottom-aligned keyboard start Y uses the simplified formula `getTotalRowCount() * (keyHeight + keySpacing)` intentionally. The small overestimation vs. the actual drawn height is compensated by per-theme `keyboardVerticalOffset` (BaseMetrics: -10, LyraMetrics: -12), manually tuned and verified on-device. Do not flag this approximation as a bug or suggest replacing it with an exact `keyboardHeight` calculation.
Applied to files:
src/activities/settings/CalibreSettingsActivity.cppsrc/activities/settings/KOReaderSettingsActivity.cppsrc/activities/util/KeyboardEntryActivity.hsrc/components/themes/lyra/LyraTheme.cppsrc/components/themes/BaseTheme.cppsrc/components/themes/BaseTheme.hsrc/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-08T13:05:57.197Z
Learnt from: zgredex
Repo: crosspoint-reader/crosspoint-reader PR: 1614
File: src/activities/util/PxcViewerActivity.cpp:23-29
Timestamp: 2026-04-08T13:05:57.197Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/util/BmpViewerActivity.cpp and src/activities/util/PxcViewerActivity.cpp), error-screen messages such as "Could not open file" and "Invalid BMP File" / "Invalid PXC file" are intentionally hardcoded English strings (not routed through tr()). Only button labels like STR_BACK use tr(). Do not flag these hardcoded viewer error strings as missing localization in future reviews — this is the established pattern for BmpViewerActivity and PxcViewerActivity.
Applied to files:
src/activities/settings/CalibreSettingsActivity.cpp
📚 Learning: 2026-02-22T19:13:22.166Z
Learnt from: ngxson
Repo: crosspoint-reader/crosspoint-reader PR: 1016
File: src/activities/reader/EpubReaderActivity.cpp:138-146
Timestamp: 2026-02-22T19:13:22.166Z
Learning: In src/activities/reader/EpubReaderActivity.cpp, the EpubReaderMenuActivity result callback intentionally applies orientation changes (via applyOrientation) before checking result.isCancelled. This differs from other callbacks in the file because orientation is treated as an immediate setting that should persist even when the user cancels the menu, rather than a deferred action requiring confirmation.
Applied to files:
src/activities/settings/KOReaderSettingsActivity.cpp
📚 Learning: 2026-04-12T02:49:05.293Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1644
File: src/components/themes/BaseTheme.cpp:835-835
Timestamp: 2026-04-12T02:49:05.293Z
Learning: In crosspoint-reader/crosspoint-reader (src/components/themes/BaseTheme.cpp), the secondary label in `BaseTheme::drawKeyboardKey` is intentionally drawn at `rect.y - 3` (3px above the key rectangle's top edge). This uses part of BaseTheme's `keySpacing = 10` inter-row gap to visually separate the secondary symbol hint from the primary character. In contrast, `LyraTheme::drawKeyboardKey` uses `rect.y` (no offset) because LyraTheme has `keySpacing = 0`. Both placements are correct and deliberate for their respective themes. Do not flag the `-3` offset as a bug or suggest aligning it to `rect.y`.
Applied to files:
src/activities/util/KeyboardEntryActivity.hsrc/components/themes/lyra/LyraTheme.cppsrc/components/themes/BaseTheme.cppsrc/components/themes/BaseTheme.hsrc/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-25T01:15:04.951Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/components/themes/lyra/Lyra3CoversTheme.cpp:39-40
Timestamp: 2026-03-25T01:15:04.951Z
Learning: In crosspoint-reader/crosspoint-reader theme drawing code, `COVER_DISABLED_MODE` only controls whether the app generates new covers (e.g., on HOME entry) to avoid blocking the main thread. It must NOT be treated as a render-suppression switch for already-cached covers. Theme draw paths should only gate cached cover rendering on the per-book `RecentBook::coverDisabled` flag; if a cached cover BMP is available, the correct behavior is to render it even when `COVER_DISABLED_MODE` is set. Therefore, do not flag theme draw code for not checking `COVER_DISABLED_MODE`; checking only `coverDisabled` is correct by design.
Applied to files:
src/components/themes/lyra/LyraTheme.cppsrc/components/themes/BaseTheme.cpp
📚 Learning: 2026-03-09T00:59:05.219Z
Learnt from: laird
Repo: crosspoint-reader/crosspoint-reader PR: 1331
File: src/components/themes/pulsr/PulsrTheme.h:58-117
Timestamp: 2026-03-09T00:59:05.219Z
Learning: In PulsrTheme (src/components/themes/pulsr/PulsrTheme.h/.cpp), `drawBatteryLeft` and `drawBatteryRight` are intentionally overridden as no-ops because PULSR displays battery state via pills in the left bar rather than the BaseTheme battery chrome. All of `drawBatteryLeft`, `drawBatteryRight`, `drawStatusBar`, and `drawHelpText` are overridden in PulsrTheme.
Applied to files:
src/components/themes/lyra/LyraTheme.cppsrc/components/themes/BaseTheme.cpp
📚 Learning: 2026-04-09T20:05:11.820Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/reader/EpubReaderMenuActivity.cpp:39-45
Timestamp: 2026-04-09T20:05:11.820Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/reader/EpubReaderMenuActivity.cpp, EpubReaderMenuActivity::buildMenuItems), the `hasCoverForTheme` boolean (derived from `Storage.exists(coverPath)`) is intentionally used — not `!coverDisabled` — to choose between STR_HOME_COVER_ACTION_DISABLE and STR_HOME_COVER_ACTION_ENABLE in non-TIMEOUT global modes. The label reflects the action that will be performed: BMP exists → "Disable" (delete it); no BMP → "Enable" (generate it). Using `coverDisabled` would create confusing edge cases (e.g., coverDisabled=false with no BMP on disk would show "Disable" when there is nothing to disable). Do not flag this as a bug in future reviews.
Applied to files:
src/components/themes/BaseTheme.cppsrc/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-28T20:57:09.493Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 0
File: :0-0
Timestamp: 2026-03-28T20:57:09.493Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/home/HomeActivity.cpp, HomeActivity::onEnter), `coverRendered = false` and `coverBufferStored = false` ARE correctly reset in `onEnter()`. This was fixed in PR `#1488`. Do not flag their absence as a bug in future reviews.
Applied to files:
src/components/themes/BaseTheme.cppsrc/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-25T01:06:03.725Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/home/HomeActivity.cpp:91-100
Timestamp: 2026-03-25T01:06:03.725Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/home/HomeActivity.cpp, HomeActivity::loadRecentCovers), marking coverDisabled=true on any generateThumbBmp() failure is intentional by design. This prevents HOME from retrying indefinitely. The user can always re-enable via Reader Menu → "Generate cover" / "Enable cover". Do not flag this pattern as a bug in future reviews.
Applied to files:
src/components/themes/BaseTheme.cppsrc/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-08T15:24:06.672Z
Learnt from: zgredex
Repo: crosspoint-reader/crosspoint-reader PR: 0
File: :0-0
Timestamp: 2026-04-08T15:24:06.672Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/reader/XtcReaderActivity.cpp, src/activities/util/BmpViewerActivity.cpp, src/activities/reader/EpubReaderActivity.cpp, and other callsites), renderGrayscale's callback type is `void (*renderFn)(const GfxRenderer&, const void*)`. Lambdas passed as this callback correctly use `const GfxRenderer& r` as the first parameter. Where the renderer must be mutated inside the callback (e.g., drawBitmap, drawButtonHints), `const_cast<GfxRenderer&>(r)` is used. No `cppcheck-suppress constParameterReference` is needed. Do not flag `const GfxRenderer& r` in these lambdas or suggest removing the `const_cast` in future reviews — this is the established pattern.
Applied to files:
src/components/themes/BaseTheme.cpp
📚 Learning: 2026-04-09T18:26:10.532Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 0
File: :0-0
Timestamp: 2026-04-09T18:26:10.532Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/home/HomeActivity.cpp, HomeActivity::loadRecentCovers), the XTC generateThumbBmp() failure path uses master-style handling: it clears coverBmpPath by calling RECENT_BOOKS.updateBook(book.path, book.title, book.author, "") rather than setting coverDisabled=true. Only the EPUB path uses setCoverDisabled(true) on generateThumbBmp failure. The reason is intentional by design: XTC reader has no reader menu COVER_ACTION (no "Enable/Generate cover" option), so setting coverDisabled=true on XTC failure would permanently block regeneration with no user-accessible recovery path. Clearing the BMP path instead allows HOME to fall back to the classical placeholder, and a future successful generateThumbBmp() call will repopulate it naturally. Do not flag XTC failure→clear-bmpPath vs EPUB failure→coverDisabled as an inconsistency in future reviews.
Applied to files:
src/components/themes/BaseTheme.cpp
📚 Learning: 2026-04-05T19:31:31.692Z
Learnt from: Zyd8
Repo: crosspoint-reader/crosspoint-reader PR: 1576
File: src/util/ButtonNavigator.h:43-47
Timestamp: 2026-04-05T19:31:31.692Z
Learning: In crosspoint-reader/crosspoint-reader (src/util/ButtonNavigator.cpp), `clearMidpointChordReleaseGuard()` intentionally sets `s_suppressUntilChordRelease` in addition to resetting the post-release guard. This blocks `beginUpDownChord()` from firing on the first loop tick of a new activity if the midpoint chord was still physically held during the `Activity::onEnter()` transition. The fix prevents cross-screen midpoint chord leaks without requiring each activity's `onEnter()` to call `isMidpointChordHeld()`. Do not flag this dual-write as redundant in future reviews.
Applied to files:
src/components/themes/BaseTheme.cppsrc/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-05T19:21:32.486Z
Learnt from: Zyd8
Repo: crosspoint-reader/crosspoint-reader PR: 1576
File: src/activities/home/RecentBooksActivity.cpp:65-90
Timestamp: 2026-04-05T19:21:32.486Z
Learning: In crosspoint-reader/crosspoint-reader (src/util/ButtonNavigator.cpp, shouldSuppressListNavForMidpointChord), the guard-clearing tick intentionally still returns `true` when it clears `s_midpointChordReleaseGuard`. This is correct because all list navigation in the activity uses `onNextRelease`/`onPreviousRelease`, which only fire on a fresh press+release cycle. The clearing tick therefore only swallows the stale release from the chord buttons themselves. Any subsequent Up/Down tap starts a new press+release cycle and will not be suppressed. Do not flag this as a dropped-input bug in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-28T11:08:53.531Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/home/HomeActivity.cpp:114-128
Timestamp: 2026-03-28T11:08:53.531Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/home/HomeActivity.cpp, HomeActivity::loadRecentCovers), the xtc.load() failure path intentionally does NOT set coverDisabled=true. XTC cover handling (including failure→disable behavior) is out of scope until the XTC reader gets its own cover menu actions. Do not flag the missing coverDisabled update on xtc.load() failure as a bug in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-08T13:08:48.852Z
Learnt from: zgredex
Repo: crosspoint-reader/crosspoint-reader PR: 1614
File: src/activities/boot_sleep/SleepActivity.cpp:186-212
Timestamp: 2026-04-08T13:08:48.852Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/boot_sleep/SleepActivity.cpp, SleepActivity::renderPxcSleepScreen and src/activities/util/PxcViewerActivity.cpp), a truncated PXC payload detected mid-render (short read inside the renderGrayscale callback) intentionally produces a partial frame rather than triggering a fallback. The pre-render failure paths (file open, header read, dimension mismatch) do fall back to renderDefaultSleepScreen() / error screen, but partial-read propagation inside the callback is considered acceptable by design. Do not flag this as a missing graceful fallback in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-25T01:15:06.744Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/components/themes/lyra/Lyra3CoversTheme.cpp:39-40
Timestamp: 2026-03-25T01:15:06.744Z
Learning: In crosspoint-reader/crosspoint-reader (src/components/themes/lyra/Lyra3CoversTheme.cpp and other theme drawing functions), `COVER_DISABLED_MODE` (global setting) only prevents new cover *generation* on HOME entry to avoid blocking the main thread. It does NOT suppress the display of already-cached covers. Only the per-book `coverDisabled` flag (RecentBook::coverDisabled) controls whether a cached cover BMP is rendered. Do not flag theme draw paths for not checking COVER_DISABLED_MODE; checking only coverDisabled is correct by design.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/activities/util/KeyboardEntryActivity.h (1)
17-17: Optional: makeSpecialKeyTypea scoped enum for consistency.
InputType(line 19) andKeyboardKeyType(BaseTheme.h) are bothenum class, whileSpecialKeyTypeis an unscoped enum using aSpecprefix to avoid collisions. Promoting it toenum class SpecialKeyType { Shift, Mode, Space, Del, Ok };would drop the prefix and match the style used elsewhere in this header.🧹 Proposed change (plus callsite updates in KeyboardEntryActivity.cpp)
-enum SpecialKeyType { SpecShift, SpecMode, SpecSpace, SpecDel, SpecOk }; +enum class SpecialKeyType { Shift, Mode, Space, Del, Ok };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activities/util/KeyboardEntryActivity.h` at line 17, Convert the unscoped enum SpecialKeyType to a scoped enum class named SpecialKeyType with members Shift, Mode, Space, Del, Ok (dropping the "Spec" prefix) to match InputType and KeyboardKeyType styles; update all usages and comparisons in KeyboardEntryActivity.cpp and any other callsites to qualify values as SpecialKeyType::Shift etc., and adjust any switch/case labels or implicit conversions that relied on the unscoped enum.src/components/themes/BaseTheme.cpp (1)
832-836: Optional: drop the unusedprimaryOffset.
primaryOffsetis aconst int = 0that's added totextYwith no effect, and the same dead local exists inLyraTheme::drawKeyboardKey(line 651). Either remove it or promote it to a real theme-specific offset if that's the original intent.🧹 Proposed cleanup
const bool hasSecondary = secondaryLabel != nullptr && secondaryLabel[0] != '\0'; - const int primaryOffset = 0; const int itemWidth = renderer.getTextWidth(UI_12_FONT_ID, label); const int textX = rect.x + (rect.width - itemWidth) / 2; - const int textY = rect.y + (rect.height - renderer.getLineHeight(UI_12_FONT_ID)) / 2 + primaryOffset; + const int textY = rect.y + (rect.height - renderer.getLineHeight(UI_12_FONT_ID)) / 2;🤖 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 832 - 836, Remove the unused primaryOffset and its addition to textY: delete the const int primaryOffset = 0 declaration and compute textY without adding primaryOffset in BaseTheme::draw... (the lines that set primaryOffset and textY). Do the same cleanup in LyraTheme::drawKeyboardKey (the analogous primaryOffset at line ~651) or replace these locals with a meaningful theme-specific offset variable if the intent was to support per-theme vertical adjustment.src/activities/settings/KOReaderSettingsActivity.cpp (1)
77-91: Optional: apply the same URL prefill/normalize to Calibre's OPDS URL.The
"https://"prefill + scheme-only-to-empty normalization here is a nice UX touch forInputType::Url.CalibreSettingsActivity::handleSelection()(lines 51–63) also accepts a URL but lacks both behaviors, so the two URL fields now diverge. Consider either lifting this into a small helper shared by URL fields, or replicating the two lines of logic in Calibre for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activities/settings/KOReaderSettingsActivity.cpp` around lines 77 - 91, CalibreSettingsActivity::handleSelection should mirror the UX in KOReaderSettingsActivity: when opening KeyboardEntryActivity for the Calibre OPDS URL, prefill the input with "https://" if the stored URL is empty and use InputType::Url, and before saving normalize scheme-only inputs ("https://" or "http://") to an empty string; update the code that constructs the KeyboardEntryActivity and the lambda that saves the result (the same pattern as in KOReaderSettingsActivity's startActivityForResult usage) so the Calibre OPDS URL is prefilled and scheme-only values are saved as empty.
🤖 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/activities/settings/KOReaderSettingsActivity.cpp`:
- Around line 77-91: CalibreSettingsActivity::handleSelection should mirror the
UX in KOReaderSettingsActivity: when opening KeyboardEntryActivity for the
Calibre OPDS URL, prefill the input with "https://" if the stored URL is empty
and use InputType::Url, and before saving normalize scheme-only inputs
("https://" or "http://") to an empty string; update the code that constructs
the KeyboardEntryActivity and the lambda that saves the result (the same pattern
as in KOReaderSettingsActivity's startActivityForResult usage) so the Calibre
OPDS URL is prefilled and scheme-only values are saved as empty.
In `@src/activities/util/KeyboardEntryActivity.h`:
- Line 17: Convert the unscoped enum SpecialKeyType to a scoped enum class named
SpecialKeyType with members Shift, Mode, Space, Del, Ok (dropping the "Spec"
prefix) to match InputType and KeyboardKeyType styles; update all usages and
comparisons in KeyboardEntryActivity.cpp and any other callsites to qualify
values as SpecialKeyType::Shift etc., and adjust any switch/case labels or
implicit conversions that relied on the unscoped enum.
In `@src/components/themes/BaseTheme.cpp`:
- Around line 832-836: Remove the unused primaryOffset and its addition to
textY: delete the const int primaryOffset = 0 declaration and compute textY
without adding primaryOffset in BaseTheme::draw... (the lines that set
primaryOffset and textY). Do the same cleanup in LyraTheme::drawKeyboardKey (the
analogous primaryOffset at line ~651) or replace these locals with a meaningful
theme-specific offset variable if the intent was to support per-theme vertical
adjustment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 16f3256b-1d75-49c1-9fdc-cb3a77878daa
📒 Files selected for processing (10)
src/activities/network/WifiSelectionActivity.cppsrc/activities/settings/CalibreSettingsActivity.cppsrc/activities/settings/KOReaderSettingsActivity.cppsrc/activities/util/KeyboardEntryActivity.cppsrc/activities/util/KeyboardEntryActivity.hsrc/components/themes/BaseTheme.cppsrc/components/themes/BaseTheme.hsrc/components/themes/lyra/Lyra3CoversTheme.hsrc/components/themes/lyra/LyraTheme.cppsrc/components/themes/lyra/LyraTheme.h
✅ Files skipped from review due to trivial changes (2)
- src/activities/network/WifiSelectionActivity.cpp
- src/components/themes/lyra/Lyra3CoversTheme.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/activities/util/KeyboardEntryActivity.cpp
📜 Review details
🧰 Additional context used
🧠 Learnings (20)
📓 Common learnings
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1644
File: src/activities/util/KeyboardEntryActivity.cpp:218-225
Timestamp: 2026-04-12T03:00:48.309Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/util/KeyboardEntryActivity.cpp), the bottom-aligned keyboard start Y uses the simplified formula `getTotalRowCount() * (keyHeight + keySpacing)` intentionally. The small overestimation vs. the actual drawn height is compensated by per-theme `keyboardVerticalOffset` (BaseMetrics: -10, LyraMetrics: -12), manually tuned and verified on-device. Do not flag this approximation as a bug or suggest replacing it with an exact `keyboardHeight` calculation.
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1644
File: src/components/themes/BaseTheme.cpp:835-835
Timestamp: 2026-04-12T02:49:05.293Z
Learning: In crosspoint-reader/crosspoint-reader (src/components/themes/BaseTheme.cpp), the secondary label in `BaseTheme::drawKeyboardKey` is intentionally drawn at `rect.y - 3` (3px above the key rectangle's top edge). This uses part of BaseTheme's `keySpacing = 10` inter-row gap to visually separate the secondary symbol hint from the primary character. In contrast, `LyraTheme::drawKeyboardKey` uses `rect.y` (no offset) because LyraTheme has `keySpacing = 0`. Both placements are correct and deliberate for their respective themes. Do not flag the `-3` offset as a bug or suggest aligning it to `rect.y`.
📚 Learning: 2026-02-23T06:18:08.408Z
Learnt from: raygan
Repo: crosspoint-reader/crosspoint-reader PR: 1116
File: src/activities/settings/WallabagSettingsActivity.cpp:57-75
Timestamp: 2026-02-23T06:18:08.408Z
Learning: When navigating from a settings activity to KeyboardEntryActivity (e.g., KOReaderSettingsActivity, WallabagSettingsActivity), call exitActivity() on the parent before calling enterNewActivity(new KeyboardEntryActivity(...)). The KeyboardEntryActivity should then call exitActivity() via its callbacks to return to the parent. Note: ConfirmationActivity does not require exitActivity() before entering KeyboardEntryActivity. Apply this pattern to similar settings activities in this module.
Applied to files:
src/activities/settings/CalibreSettingsActivity.cppsrc/activities/settings/KOReaderSettingsActivity.cpp
📚 Learning: 2026-02-12T06:57:35.955Z
Learnt from: CaptainFrito
Repo: crosspoint-reader/crosspoint-reader PR: 725
File: src/activities/network/CalibreConnectActivity.cpp:218-264
Timestamp: 2026-02-12T06:57:35.955Z
Learning: In src/activities/network/CalibreConnectActivity.cpp, button hints (« Exit) are intentionally omitted from the SERVER_STARTING and ERROR states—only the SERVER_RUNNING state displays navigation hints. This is a deliberate design decision by the author.
Applied to files:
src/activities/settings/CalibreSettingsActivity.cpp
📚 Learning: 2026-02-22T19:13:22.166Z
Learnt from: ngxson
Repo: crosspoint-reader/crosspoint-reader PR: 1016
File: src/activities/reader/EpubReaderActivity.cpp:138-146
Timestamp: 2026-02-22T19:13:22.166Z
Learning: In src/activities/reader/EpubReaderActivity.cpp, the EpubReaderMenuActivity result callback intentionally applies orientation changes (via applyOrientation) before checking result.isCancelled. This differs from other callbacks in the file because orientation is treated as an immediate setting that should persist even when the user cancels the menu, rather than a deferred action requiring confirmation.
Applied to files:
src/activities/settings/CalibreSettingsActivity.cpp
📚 Learning: 2026-02-27T22:49:59.600Z
Learnt from: ngxson
Repo: crosspoint-reader/crosspoint-reader PR: 1218
File: src/activities/ActivityManager.cpp:254-265
Timestamp: 2026-02-27T22:49:59.600Z
Learning: In this codebase, assertions are always enabled (no NDEBUG). Use assert() to crash on programmer errors and surface logic bugs during development and in production builds. Do not rely on asserts for runtime error handling; they should enforce invariants that must always hold. Keep asserts side-effect free and inexpensive, and avoid relying on them for user-visible failures. Include <cassert> where appropriate and document the invariant being tested.
Applied to files:
src/activities/settings/CalibreSettingsActivity.cppsrc/activities/settings/KOReaderSettingsActivity.cppsrc/components/themes/lyra/LyraTheme.cppsrc/components/themes/BaseTheme.cpp
📚 Learning: 2026-03-02T10:14:16.036Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1245
File: lib/Epub/Epub/Section.cpp:277-308
Timestamp: 2026-03-02T10:14:16.036Z
Learning: Guideline: Strengthen serialization::readString to defend against unbounded growth when reading from disk data. Implement and enforce a maximum allowed length (e.g., a configured or reasonable constant) and validate the incoming length before resizing or allocating. Audit all call sites (e.g., BookMetadataCache, TextBlock, KOReaderCredentialStore, Section cache readers) to ensure they do not rely on unbounded len-based resizing. If the readString API must remain, add internal safeguards (bounds checks, length validation, and error handling) so per-call-site validations are not required. Ensure Section cache files remain versioned (SECTION_FILE_VERSION) and parameter mismatches invalidate caches, but do not rely on unsafe allocations; prefer safe, bounded reads with explicit errors on invalid data.
Applied to files:
src/activities/settings/CalibreSettingsActivity.cppsrc/activities/settings/KOReaderSettingsActivity.cppsrc/components/themes/lyra/LyraTheme.cppsrc/components/themes/BaseTheme.cpp
📚 Learning: 2026-03-28T11:06:29.611Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/home/HomeActivity.cpp:92-95
Timestamp: 2026-03-28T11:06:29.611Z
Learning: When reviewing crosspoint-reader code, avoid flagging a missing `renderer.displayBuffer()` call immediately after `GUI.drawPopup()` / `BaseTheme::drawPopup()`: `BaseTheme::drawPopup()` already calls `renderer.displayBuffer()` before returning, so the popup is guaranteed to be flushed to the e-ink panel before subsequent blocking work begins. Conversely, do not require a `renderer.displayBuffer()` call after `fillPopupProgress()`; it intentionally does not flush, so intermediate progress-bar updates may not appear unless the update granularity warrants an explicit flush elsewhere.
Applied to files:
src/activities/settings/CalibreSettingsActivity.cppsrc/activities/settings/KOReaderSettingsActivity.cppsrc/components/themes/lyra/LyraTheme.cppsrc/components/themes/BaseTheme.cpp
📚 Learning: 2026-04-12T12:28:33.205Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1629
File: src/activities/home/HomeActivity.cpp:119-120
Timestamp: 2026-04-12T12:28:33.205Z
Learning: When reviewing code in this repository (C/C++ sources), set review comment severity according to this policy:
- Use **Major** (🟠) only for defects with realistic risk of crash, out-of-memory (OOM), invalid pointer dereference, data corruption, or other severe issues that are unlikely to be caught in casual/manual device testing.
- Use **Minor** or informational for UX gaps, logic edge-cases, style issues, or missing feature completeness that the author can verify (or has verified) through normal device use.
- Do **not** escalate severity to Major based on behavioral/UX observations alone; assume the author has already tested the feature on their own device and only treat issues as Major if they match the high-risk defect categories above.
Applied to files:
src/activities/settings/CalibreSettingsActivity.cppsrc/activities/settings/KOReaderSettingsActivity.cppsrc/components/themes/BaseTheme.hsrc/components/themes/lyra/LyraTheme.cppsrc/components/themes/BaseTheme.cppsrc/components/themes/lyra/LyraTheme.hsrc/activities/util/KeyboardEntryActivity.h
📚 Learning: 2026-04-12T03:00:48.309Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1644
File: src/activities/util/KeyboardEntryActivity.cpp:218-225
Timestamp: 2026-04-12T03:00:48.309Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/util/KeyboardEntryActivity.cpp), the bottom-aligned keyboard start Y uses the simplified formula `getTotalRowCount() * (keyHeight + keySpacing)` intentionally. The small overestimation vs. the actual drawn height is compensated by per-theme `keyboardVerticalOffset` (BaseMetrics: -10, LyraMetrics: -12), manually tuned and verified on-device. Do not flag this approximation as a bug or suggest replacing it with an exact `keyboardHeight` calculation.
Applied to files:
src/activities/settings/KOReaderSettingsActivity.cppsrc/components/themes/BaseTheme.hsrc/components/themes/lyra/LyraTheme.cppsrc/components/themes/BaseTheme.cppsrc/components/themes/lyra/LyraTheme.hsrc/activities/util/KeyboardEntryActivity.h
📚 Learning: 2026-04-12T02:49:05.293Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1644
File: src/components/themes/BaseTheme.cpp:835-835
Timestamp: 2026-04-12T02:49:05.293Z
Learning: In crosspoint-reader/crosspoint-reader (src/components/themes/BaseTheme.cpp), the secondary label in `BaseTheme::drawKeyboardKey` is intentionally drawn at `rect.y - 3` (3px above the key rectangle's top edge). This uses part of BaseTheme's `keySpacing = 10` inter-row gap to visually separate the secondary symbol hint from the primary character. In contrast, `LyraTheme::drawKeyboardKey` uses `rect.y` (no offset) because LyraTheme has `keySpacing = 0`. Both placements are correct and deliberate for their respective themes. Do not flag the `-3` offset as a bug or suggest aligning it to `rect.y`.
Applied to files:
src/components/themes/BaseTheme.hsrc/components/themes/lyra/LyraTheme.cppsrc/components/themes/BaseTheme.cppsrc/components/themes/lyra/LyraTheme.h
📚 Learning: 2026-03-09T00:59:05.219Z
Learnt from: laird
Repo: crosspoint-reader/crosspoint-reader PR: 1331
File: src/components/themes/pulsr/PulsrTheme.h:58-117
Timestamp: 2026-03-09T00:59:05.219Z
Learning: In PulsrTheme (src/components/themes/pulsr/PulsrTheme.h/.cpp), `drawBatteryLeft` and `drawBatteryRight` are intentionally overridden as no-ops because PULSR displays battery state via pills in the left bar rather than the BaseTheme battery chrome. All of `drawBatteryLeft`, `drawBatteryRight`, `drawStatusBar`, and `drawHelpText` are overridden in PulsrTheme.
Applied to files:
src/components/themes/BaseTheme.hsrc/components/themes/lyra/LyraTheme.cppsrc/components/themes/BaseTheme.cpp
📚 Learning: 2026-03-25T01:15:04.951Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/components/themes/lyra/Lyra3CoversTheme.cpp:39-40
Timestamp: 2026-03-25T01:15:04.951Z
Learning: In crosspoint-reader/crosspoint-reader theme drawing code, `COVER_DISABLED_MODE` only controls whether the app generates new covers (e.g., on HOME entry) to avoid blocking the main thread. It must NOT be treated as a render-suppression switch for already-cached covers. Theme draw paths should only gate cached cover rendering on the per-book `RecentBook::coverDisabled` flag; if a cached cover BMP is available, the correct behavior is to render it even when `COVER_DISABLED_MODE` is set. Therefore, do not flag theme draw code for not checking `COVER_DISABLED_MODE`; checking only `coverDisabled` is correct by design.
Applied to files:
src/components/themes/lyra/LyraTheme.cppsrc/components/themes/BaseTheme.cpp
📚 Learning: 2026-04-09T20:05:11.820Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/reader/EpubReaderMenuActivity.cpp:39-45
Timestamp: 2026-04-09T20:05:11.820Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/reader/EpubReaderMenuActivity.cpp, EpubReaderMenuActivity::buildMenuItems), the `hasCoverForTheme` boolean (derived from `Storage.exists(coverPath)`) is intentionally used — not `!coverDisabled` — to choose between STR_HOME_COVER_ACTION_DISABLE and STR_HOME_COVER_ACTION_ENABLE in non-TIMEOUT global modes. The label reflects the action that will be performed: BMP exists → "Disable" (delete it); no BMP → "Enable" (generate it). Using `coverDisabled` would create confusing edge cases (e.g., coverDisabled=false with no BMP on disk would show "Disable" when there is nothing to disable). Do not flag this as a bug in future reviews.
Applied to files:
src/components/themes/BaseTheme.cpp
📚 Learning: 2026-03-28T20:57:09.493Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 0
File: :0-0
Timestamp: 2026-03-28T20:57:09.493Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/home/HomeActivity.cpp, HomeActivity::onEnter), `coverRendered = false` and `coverBufferStored = false` ARE correctly reset in `onEnter()`. This was fixed in PR `#1488`. Do not flag their absence as a bug in future reviews.
Applied to files:
src/components/themes/BaseTheme.cpp
📚 Learning: 2026-03-25T01:06:03.725Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/home/HomeActivity.cpp:91-100
Timestamp: 2026-03-25T01:06:03.725Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/home/HomeActivity.cpp, HomeActivity::loadRecentCovers), marking coverDisabled=true on any generateThumbBmp() failure is intentional by design. This prevents HOME from retrying indefinitely. The user can always re-enable via Reader Menu → "Generate cover" / "Enable cover". Do not flag this pattern as a bug in future reviews.
Applied to files:
src/components/themes/BaseTheme.cpp
📚 Learning: 2026-04-08T15:24:06.672Z
Learnt from: zgredex
Repo: crosspoint-reader/crosspoint-reader PR: 0
File: :0-0
Timestamp: 2026-04-08T15:24:06.672Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/reader/XtcReaderActivity.cpp, src/activities/util/BmpViewerActivity.cpp, src/activities/reader/EpubReaderActivity.cpp, and other callsites), renderGrayscale's callback type is `void (*renderFn)(const GfxRenderer&, const void*)`. Lambdas passed as this callback correctly use `const GfxRenderer& r` as the first parameter. Where the renderer must be mutated inside the callback (e.g., drawBitmap, drawButtonHints), `const_cast<GfxRenderer&>(r)` is used. No `cppcheck-suppress constParameterReference` is needed. Do not flag `const GfxRenderer& r` in these lambdas or suggest removing the `const_cast` in future reviews — this is the established pattern.
Applied to files:
src/components/themes/BaseTheme.cpp
📚 Learning: 2026-04-09T18:26:10.532Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 0
File: :0-0
Timestamp: 2026-04-09T18:26:10.532Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/home/HomeActivity.cpp, HomeActivity::loadRecentCovers), the XTC generateThumbBmp() failure path uses master-style handling: it clears coverBmpPath by calling RECENT_BOOKS.updateBook(book.path, book.title, book.author, "") rather than setting coverDisabled=true. Only the EPUB path uses setCoverDisabled(true) on generateThumbBmp failure. The reason is intentional by design: XTC reader has no reader menu COVER_ACTION (no "Enable/Generate cover" option), so setting coverDisabled=true on XTC failure would permanently block regeneration with no user-accessible recovery path. Clearing the BMP path instead allows HOME to fall back to the classical placeholder, and a future successful generateThumbBmp() call will repopulate it naturally. Do not flag XTC failure→clear-bmpPath vs EPUB failure→coverDisabled as an inconsistency in future reviews.
Applied to files:
src/components/themes/BaseTheme.cpp
📚 Learning: 2026-04-05T19:31:31.692Z
Learnt from: Zyd8
Repo: crosspoint-reader/crosspoint-reader PR: 1576
File: src/util/ButtonNavigator.h:43-47
Timestamp: 2026-04-05T19:31:31.692Z
Learning: In crosspoint-reader/crosspoint-reader (src/util/ButtonNavigator.cpp), `clearMidpointChordReleaseGuard()` intentionally sets `s_suppressUntilChordRelease` in addition to resetting the post-release guard. This blocks `beginUpDownChord()` from firing on the first loop tick of a new activity if the midpoint chord was still physically held during the `Activity::onEnter()` transition. The fix prevents cross-screen midpoint chord leaks without requiring each activity's `onEnter()` to call `isMidpointChordHeld()`. Do not flag this dual-write as redundant in future reviews.
Applied to files:
src/components/themes/BaseTheme.cpp
📚 Learning: 2026-03-28T11:06:36.807Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/home/HomeActivity.cpp:92-95
Timestamp: 2026-03-28T11:06:36.807Z
Learning: In crosspoint-reader/crosspoint-reader, `BaseTheme::drawPopup()` (BaseTheme.cpp:558) calls `renderer.displayBuffer()` internally before returning. Any popup drawn via `GUI.drawPopup()` is therefore guaranteed to be visible on the e-ink panel before any subsequent blocking work begins. Do not flag missing `displayBuffer()` calls after `drawPopup()` as a bug; the flush is already included. `fillPopupProgress()` does NOT call `displayBuffer()`, so progress bar updates after the initial popup may not be shown — this is intentional and harmless when the update granularity doesn't warrant intermediate flushes.
Applied to files:
src/components/themes/lyra/LyraTheme.h
📚 Learning: 2026-02-23T06:18:15.143Z
Learnt from: raygan
Repo: crosspoint-reader/crosspoint-reader PR: 1116
File: src/activities/settings/WallabagSettingsActivity.cpp:57-75
Timestamp: 2026-02-23T06:18:15.143Z
Learning: In settings activities (e.g., KOReaderSettingsActivity, WallabagSettingsActivity), the established pattern for launching KeyboardEntryActivity is to call exitActivity() on the parent settings activity before enterNewActivity(new KeyboardEntryActivity(...)). The lambda callbacks then call exitActivity() to exit the KeyboardEntryActivity and return to the parent. This differs from ConfirmationActivity which does not require exitActivity() before enterNewActivity().
Applied to files:
src/activities/util/KeyboardEntryActivity.h
🔇 Additional comments (6)
src/activities/settings/CalibreSettingsActivity.cpp (1)
51-89: LGTM — cleanInputTypemigration.Each callsite passes the semantically correct
InputType, and the subsequentstrncpy+ null-terminate +saveToFile()flow is preserved. Nothing to add here (see the KOReader file for an optional prefill-consistency nit that also touches this activity).src/components/themes/BaseTheme.h (1)
60-115: LGTM — metrics and enum additions are cohesive.
KeyboardKeyTypeas a scoped enum withDisabledplus the new bottom-row/geometry fields give themes everything they need to render the refactored keyboard. Default values (keyboardBottomAligned = true,keyboardKeySpacing = 0, width %s) align with the PR's unified layout.src/components/themes/lyra/LyraTheme.h (1)
34-78: LGTM — overrides and metrics mirror BaseTheme cleanly.Defaults on the override signatures match the base declarations, so implicit calls from the activity are unambiguous. The reduced
keyboardKeyHeightplus the new bottom-row metrics are consistent with the unified layout.src/components/themes/lyra/LyraTheme.cpp (1)
597-661: LGTM — theme behaviors match BaseTheme contract.Underline thickness gating on
cursorMode, the content-aware vs centered fallback, and the key-type branching (Space/Del early-return, Disabled vs Normal selection fills) are all consistent withBaseTheme::drawKeyboardKey. Theinvert = isSelected && !inactiveSelectionderivation correctly propagates into both the key glyph lines and label rendering. (See the BaseTheme.cpp comment about theprimaryOffsetdead local — applies here too.)src/components/themes/BaseTheme.cpp (1)
777-844: LGTM — new geometry/cursor-aware rendering is consistent.
drawTextField's content-aware underline (cursor-thickness +contentStartX/contentWidthpositioning with centered fallback) anddrawKeyboardKey's key-type branches line up with LyraTheme and with howKeyboardEntryActivitynow drives them.src/activities/util/KeyboardEntryActivity.h (1)
21-182: LGTM — constructor/state additions look sound.
InputTypereplaces the bool flag cleanly, the layout tables are fully populated (4×10 for both ABC and SYM), and the expanded interaction state (cursor mode, long-press flags, URL/password toggles) is all default-initialized. No ownership/lifetime or bounds concerns at the header level.
… minimum width - Add kernOffset to cursorPixelX using string-difference calculation (getTextWidth(before+char) - getTextWidth(before) - getTextWidth(char)) to correctly position block cursor for characters with inter-character kerning (e.g. consecutive spaces) - Set minimum cursorCharWidth of 6px to ensure block cursor is visible as a block rather than a thin line for narrow characters like spaces - Convert SpecialKeyType to scoped enum class for type safety - Remove dead primaryOffset=0 from BaseTheme and LyraTheme drawKeyboardKey - Revert onExit to original (no half-refresh on exit) - Prefill OPDS URL with https:// when empty, normalize scheme-only inputs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/activities/util/KeyboardEntryActivity.cpp`:
- Around line 49-56: getSelectedChar currently honors shiftState even when
symMode is active, which can return key.secondary while the UI renders the
symbol key's primary; change the selection logic in getSelectedChar to ignore
shiftState when symMode is true (i.e., only use key.secondary when !symMode &&
shiftState > 0), and apply the same rule in the rendering code that computes the
displayed character for keys (the block that reads key.primary/key.secondary
around lines 650-659) so inserted and visible characters always match in
symMode.
- Around line 530-535: Update the hint strings assigned to passTip in
KeyboardEntryActivity.cpp to fix grammar and make password capitalization
consistent: when togglePos is true replace "Press < for go back current
position" with a grammatically correct variant (e.g., "Press '<' to move the
cursor back" or "Press '<' to go back to the previous position"), and when
togglePos is false use consistent lower-case "password" and clearer phrasing
such as "Hold '>' and press [***] to hide password" and "Hold '>' and press
[abc] to show password" (modify the strings where passTip is set, using the
togglePos and passwordVisible branches).
- Around line 3-4: In KeyboardEntryActivity.cpp, add the standard header
<algorithm> to ensure std::max is available, and replace the use of the macro
SIZE_MAX for the size_t sentinel with std::string::npos (the proper sentinel for
string/size_t operations); update any comparisons or assignments that use
SIZE_MAX to use std::string::npos and keep usages of std::max as std::max(...)
to fix the compile error.
In `@src/components/themes/lyra/LyraTheme.cpp`:
- Around line 616-627: The current logic sets const bool invert = isSelected &&
!inactiveSelection which still inverts text for selected but disabled keys;
change the condition so disabled keys are treated like inactive selections by
adding a check against KeyboardKeyType::Disabled (e.g., const bool invert =
isSelected && !inactiveSelection && keyType != KeyboardKeyType::Disabled) so
when keyType == KeyboardKeyType::Disabled the label/icon is not inverted; update
the use of invert where labels/icons are drawn to rely on this new boolean
(symbols: invert, isSelected, inactiveSelection, keyType,
KeyboardKeyType::Disabled).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18877a89-3630-4f71-894f-6e68e99ddb5a
📒 Files selected for processing (5)
src/activities/settings/CalibreSettingsActivity.cppsrc/activities/util/KeyboardEntryActivity.cppsrc/activities/util/KeyboardEntryActivity.hsrc/components/themes/BaseTheme.cppsrc/components/themes/lyra/LyraTheme.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/themes/BaseTheme.cpp
- src/activities/util/KeyboardEntryActivity.h
📜 Review details
🧰 Additional context used
🧠 Learnings (28)
📓 Common learnings
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1644
File: src/activities/util/KeyboardEntryActivity.cpp:218-225
Timestamp: 2026-04-12T03:00:48.309Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/util/KeyboardEntryActivity.cpp), the bottom-aligned keyboard start Y uses the simplified formula `getTotalRowCount() * (keyHeight + keySpacing)` intentionally. The small overestimation vs. the actual drawn height is compensated by per-theme `keyboardVerticalOffset` (BaseMetrics: -10, LyraMetrics: -12), manually tuned and verified on-device. Do not flag this approximation as a bug or suggest replacing it with an exact `keyboardHeight` calculation.
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1644
File: src/components/themes/BaseTheme.cpp:835-835
Timestamp: 2026-04-12T02:49:05.293Z
Learning: In crosspoint-reader/crosspoint-reader (src/components/themes/BaseTheme.cpp), the secondary label in `BaseTheme::drawKeyboardKey` is intentionally drawn at `rect.y - 3` (3px above the key rectangle's top edge). This uses part of BaseTheme's `keySpacing = 10` inter-row gap to visually separate the secondary symbol hint from the primary character. In contrast, `LyraTheme::drawKeyboardKey` uses `rect.y` (no offset) because LyraTheme has `keySpacing = 0`. Both placements are correct and deliberate for their respective themes. Do not flag the `-3` offset as a bug or suggest aligning it to `rect.y`.
Learnt from: Zyd8
Repo: crosspoint-reader/crosspoint-reader PR: 1576
File: src/util/ButtonNavigator.h:43-47
Timestamp: 2026-04-05T19:31:31.692Z
Learning: In crosspoint-reader/crosspoint-reader (src/util/ButtonNavigator.cpp), `clearMidpointChordReleaseGuard()` intentionally sets `s_suppressUntilChordRelease` in addition to resetting the post-release guard. This blocks `beginUpDownChord()` from firing on the first loop tick of a new activity if the midpoint chord was still physically held during the `Activity::onEnter()` transition. The fix prevents cross-screen midpoint chord leaks without requiring each activity's `onEnter()` to call `isMidpointChordHeld()`. Do not flag this dual-write as redundant in future reviews.
Learnt from: raygan
Repo: crosspoint-reader/crosspoint-reader PR: 1116
File: src/activities/settings/WallabagSettingsActivity.cpp:57-75
Timestamp: 2026-02-23T06:18:15.143Z
Learning: In settings activities (e.g., KOReaderSettingsActivity, WallabagSettingsActivity), the established pattern for launching KeyboardEntryActivity is to call exitActivity() on the parent settings activity before enterNewActivity(new KeyboardEntryActivity(...)). The lambda callbacks then call exitActivity() to exit the KeyboardEntryActivity and return to the parent. This differs from ConfirmationActivity which does not require exitActivity() before enterNewActivity().
📚 Learning: 2026-02-23T06:18:08.408Z
Learnt from: raygan
Repo: crosspoint-reader/crosspoint-reader PR: 1116
File: src/activities/settings/WallabagSettingsActivity.cpp:57-75
Timestamp: 2026-02-23T06:18:08.408Z
Learning: When navigating from a settings activity to KeyboardEntryActivity (e.g., KOReaderSettingsActivity, WallabagSettingsActivity), call exitActivity() on the parent before calling enterNewActivity(new KeyboardEntryActivity(...)). The KeyboardEntryActivity should then call exitActivity() via its callbacks to return to the parent. Note: ConfirmationActivity does not require exitActivity() before entering KeyboardEntryActivity. Apply this pattern to similar settings activities in this module.
Applied to files:
src/activities/settings/CalibreSettingsActivity.cpp
📚 Learning: 2026-02-12T06:57:35.955Z
Learnt from: CaptainFrito
Repo: crosspoint-reader/crosspoint-reader PR: 725
File: src/activities/network/CalibreConnectActivity.cpp:218-264
Timestamp: 2026-02-12T06:57:35.955Z
Learning: In src/activities/network/CalibreConnectActivity.cpp, button hints (« Exit) are intentionally omitted from the SERVER_STARTING and ERROR states—only the SERVER_RUNNING state displays navigation hints. This is a deliberate design decision by the author.
Applied to files:
src/activities/settings/CalibreSettingsActivity.cpp
📚 Learning: 2026-02-22T19:13:22.166Z
Learnt from: ngxson
Repo: crosspoint-reader/crosspoint-reader PR: 1016
File: src/activities/reader/EpubReaderActivity.cpp:138-146
Timestamp: 2026-02-22T19:13:22.166Z
Learning: In src/activities/reader/EpubReaderActivity.cpp, the EpubReaderMenuActivity result callback intentionally applies orientation changes (via applyOrientation) before checking result.isCancelled. This differs from other callbacks in the file because orientation is treated as an immediate setting that should persist even when the user cancels the menu, rather than a deferred action requiring confirmation.
Applied to files:
src/activities/settings/CalibreSettingsActivity.cpp
📚 Learning: 2026-02-27T22:49:59.600Z
Learnt from: ngxson
Repo: crosspoint-reader/crosspoint-reader PR: 1218
File: src/activities/ActivityManager.cpp:254-265
Timestamp: 2026-02-27T22:49:59.600Z
Learning: In this codebase, assertions are always enabled (no NDEBUG). Use assert() to crash on programmer errors and surface logic bugs during development and in production builds. Do not rely on asserts for runtime error handling; they should enforce invariants that must always hold. Keep asserts side-effect free and inexpensive, and avoid relying on them for user-visible failures. Include <cassert> where appropriate and document the invariant being tested.
Applied to files:
src/activities/settings/CalibreSettingsActivity.cppsrc/components/themes/lyra/LyraTheme.cppsrc/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-02T10:14:16.036Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1245
File: lib/Epub/Epub/Section.cpp:277-308
Timestamp: 2026-03-02T10:14:16.036Z
Learning: Guideline: Strengthen serialization::readString to defend against unbounded growth when reading from disk data. Implement and enforce a maximum allowed length (e.g., a configured or reasonable constant) and validate the incoming length before resizing or allocating. Audit all call sites (e.g., BookMetadataCache, TextBlock, KOReaderCredentialStore, Section cache readers) to ensure they do not rely on unbounded len-based resizing. If the readString API must remain, add internal safeguards (bounds checks, length validation, and error handling) so per-call-site validations are not required. Ensure Section cache files remain versioned (SECTION_FILE_VERSION) and parameter mismatches invalidate caches, but do not rely on unsafe allocations; prefer safe, bounded reads with explicit errors on invalid data.
Applied to files:
src/activities/settings/CalibreSettingsActivity.cppsrc/components/themes/lyra/LyraTheme.cppsrc/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-28T11:06:29.611Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/home/HomeActivity.cpp:92-95
Timestamp: 2026-03-28T11:06:29.611Z
Learning: When reviewing crosspoint-reader code, avoid flagging a missing `renderer.displayBuffer()` call immediately after `GUI.drawPopup()` / `BaseTheme::drawPopup()`: `BaseTheme::drawPopup()` already calls `renderer.displayBuffer()` before returning, so the popup is guaranteed to be flushed to the e-ink panel before subsequent blocking work begins. Conversely, do not require a `renderer.displayBuffer()` call after `fillPopupProgress()`; it intentionally does not flush, so intermediate progress-bar updates may not appear unless the update granularity warrants an explicit flush elsewhere.
Applied to files:
src/activities/settings/CalibreSettingsActivity.cppsrc/components/themes/lyra/LyraTheme.cppsrc/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-12T12:28:33.205Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1629
File: src/activities/home/HomeActivity.cpp:119-120
Timestamp: 2026-04-12T12:28:33.205Z
Learning: When reviewing code in this repository (C/C++ sources), set review comment severity according to this policy:
- Use **Major** (🟠) only for defects with realistic risk of crash, out-of-memory (OOM), invalid pointer dereference, data corruption, or other severe issues that are unlikely to be caught in casual/manual device testing.
- Use **Minor** or informational for UX gaps, logic edge-cases, style issues, or missing feature completeness that the author can verify (or has verified) through normal device use.
- Do **not** escalate severity to Major based on behavioral/UX observations alone; assume the author has already tested the feature on their own device and only treat issues as Major if they match the high-risk defect categories above.
Applied to files:
src/activities/settings/CalibreSettingsActivity.cppsrc/components/themes/lyra/LyraTheme.cppsrc/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-12T02:49:05.293Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1644
File: src/components/themes/BaseTheme.cpp:835-835
Timestamp: 2026-04-12T02:49:05.293Z
Learning: In crosspoint-reader/crosspoint-reader (src/components/themes/BaseTheme.cpp), the secondary label in `BaseTheme::drawKeyboardKey` is intentionally drawn at `rect.y - 3` (3px above the key rectangle's top edge). This uses part of BaseTheme's `keySpacing = 10` inter-row gap to visually separate the secondary symbol hint from the primary character. In contrast, `LyraTheme::drawKeyboardKey` uses `rect.y` (no offset) because LyraTheme has `keySpacing = 0`. Both placements are correct and deliberate for their respective themes. Do not flag the `-3` offset as a bug or suggest aligning it to `rect.y`.
Applied to files:
src/components/themes/lyra/LyraTheme.cppsrc/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-12T03:00:48.309Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1644
File: src/activities/util/KeyboardEntryActivity.cpp:218-225
Timestamp: 2026-04-12T03:00:48.309Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/util/KeyboardEntryActivity.cpp), the bottom-aligned keyboard start Y uses the simplified formula `getTotalRowCount() * (keyHeight + keySpacing)` intentionally. The small overestimation vs. the actual drawn height is compensated by per-theme `keyboardVerticalOffset` (BaseMetrics: -10, LyraMetrics: -12), manually tuned and verified on-device. Do not flag this approximation as a bug or suggest replacing it with an exact `keyboardHeight` calculation.
Applied to files:
src/components/themes/lyra/LyraTheme.cppsrc/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-25T01:15:04.951Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/components/themes/lyra/Lyra3CoversTheme.cpp:39-40
Timestamp: 2026-03-25T01:15:04.951Z
Learning: In crosspoint-reader/crosspoint-reader theme drawing code, `COVER_DISABLED_MODE` only controls whether the app generates new covers (e.g., on HOME entry) to avoid blocking the main thread. It must NOT be treated as a render-suppression switch for already-cached covers. Theme draw paths should only gate cached cover rendering on the per-book `RecentBook::coverDisabled` flag; if a cached cover BMP is available, the correct behavior is to render it even when `COVER_DISABLED_MODE` is set. Therefore, do not flag theme draw code for not checking `COVER_DISABLED_MODE`; checking only `coverDisabled` is correct by design.
Applied to files:
src/components/themes/lyra/LyraTheme.cpp
📚 Learning: 2026-03-09T00:59:05.219Z
Learnt from: laird
Repo: crosspoint-reader/crosspoint-reader PR: 1331
File: src/components/themes/pulsr/PulsrTheme.h:58-117
Timestamp: 2026-03-09T00:59:05.219Z
Learning: In PulsrTheme (src/components/themes/pulsr/PulsrTheme.h/.cpp), `drawBatteryLeft` and `drawBatteryRight` are intentionally overridden as no-ops because PULSR displays battery state via pills in the left bar rather than the BaseTheme battery chrome. All of `drawBatteryLeft`, `drawBatteryRight`, `drawStatusBar`, and `drawHelpText` are overridden in PulsrTheme.
Applied to files:
src/components/themes/lyra/LyraTheme.cpp
📚 Learning: 2026-02-23T06:18:15.143Z
Learnt from: raygan
Repo: crosspoint-reader/crosspoint-reader PR: 1116
File: src/activities/settings/WallabagSettingsActivity.cpp:57-75
Timestamp: 2026-02-23T06:18:15.143Z
Learning: In settings activities (e.g., KOReaderSettingsActivity, WallabagSettingsActivity), the established pattern for launching KeyboardEntryActivity is to call exitActivity() on the parent settings activity before enterNewActivity(new KeyboardEntryActivity(...)). The lambda callbacks then call exitActivity() to exit the KeyboardEntryActivity and return to the parent. This differs from ConfirmationActivity which does not require exitActivity() before enterNewActivity().
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-05T19:31:31.692Z
Learnt from: Zyd8
Repo: crosspoint-reader/crosspoint-reader PR: 1576
File: src/util/ButtonNavigator.h:43-47
Timestamp: 2026-04-05T19:31:31.692Z
Learning: In crosspoint-reader/crosspoint-reader (src/util/ButtonNavigator.cpp), `clearMidpointChordReleaseGuard()` intentionally sets `s_suppressUntilChordRelease` in addition to resetting the post-release guard. This blocks `beginUpDownChord()` from firing on the first loop tick of a new activity if the midpoint chord was still physically held during the `Activity::onEnter()` transition. The fix prevents cross-screen midpoint chord leaks without requiring each activity's `onEnter()` to call `isMidpointChordHeld()`. Do not flag this dual-write as redundant in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-05T19:21:32.486Z
Learnt from: Zyd8
Repo: crosspoint-reader/crosspoint-reader PR: 1576
File: src/activities/home/RecentBooksActivity.cpp:65-90
Timestamp: 2026-04-05T19:21:32.486Z
Learning: In crosspoint-reader/crosspoint-reader (src/util/ButtonNavigator.cpp, shouldSuppressListNavForMidpointChord), the guard-clearing tick intentionally still returns `true` when it clears `s_midpointChordReleaseGuard`. This is correct because all list navigation in the activity uses `onNextRelease`/`onPreviousRelease`, which only fire on a fresh press+release cycle. The clearing tick therefore only swallows the stale release from the chord buttons themselves. Any subsequent Up/Down tap starts a new press+release cycle and will not be suppressed. Do not flag this as a dropped-input bug in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-28T20:57:09.493Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 0
File: :0-0
Timestamp: 2026-03-28T20:57:09.493Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/home/HomeActivity.cpp, HomeActivity::onEnter), `coverRendered = false` and `coverBufferStored = false` ARE correctly reset in `onEnter()`. This was fixed in PR `#1488`. Do not flag their absence as a bug in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-09T20:05:11.820Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/reader/EpubReaderMenuActivity.cpp:39-45
Timestamp: 2026-04-09T20:05:11.820Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/reader/EpubReaderMenuActivity.cpp, EpubReaderMenuActivity::buildMenuItems), the `hasCoverForTheme` boolean (derived from `Storage.exists(coverPath)`) is intentionally used — not `!coverDisabled` — to choose between STR_HOME_COVER_ACTION_DISABLE and STR_HOME_COVER_ACTION_ENABLE in non-TIMEOUT global modes. The label reflects the action that will be performed: BMP exists → "Disable" (delete it); no BMP → "Enable" (generate it). Using `coverDisabled` would create confusing edge cases (e.g., coverDisabled=false with no BMP on disk would show "Disable" when there is nothing to disable). Do not flag this as a bug in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-28T11:08:53.531Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/home/HomeActivity.cpp:114-128
Timestamp: 2026-03-28T11:08:53.531Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/home/HomeActivity.cpp, HomeActivity::loadRecentCovers), the xtc.load() failure path intentionally does NOT set coverDisabled=true. XTC cover handling (including failure→disable behavior) is out of scope until the XTC reader gets its own cover menu actions. Do not flag the missing coverDisabled update on xtc.load() failure as a bug in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-25T01:06:03.725Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/home/HomeActivity.cpp:91-100
Timestamp: 2026-03-25T01:06:03.725Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/home/HomeActivity.cpp, HomeActivity::loadRecentCovers), marking coverDisabled=true on any generateThumbBmp() failure is intentional by design. This prevents HOME from retrying indefinitely. The user can always re-enable via Reader Menu → "Generate cover" / "Enable cover". Do not flag this pattern as a bug in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-08T13:08:48.852Z
Learnt from: zgredex
Repo: crosspoint-reader/crosspoint-reader PR: 1614
File: src/activities/boot_sleep/SleepActivity.cpp:186-212
Timestamp: 2026-04-08T13:08:48.852Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/boot_sleep/SleepActivity.cpp, SleepActivity::renderPxcSleepScreen and src/activities/util/PxcViewerActivity.cpp), a truncated PXC payload detected mid-render (short read inside the renderGrayscale callback) intentionally produces a partial frame rather than triggering a fallback. The pre-render failure paths (file open, header read, dimension mismatch) do fall back to renderDefaultSleepScreen() / error screen, but partial-read propagation inside the callback is considered acceptable by design. Do not flag this as a missing graceful fallback in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-25T01:15:06.744Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/components/themes/lyra/Lyra3CoversTheme.cpp:39-40
Timestamp: 2026-03-25T01:15:06.744Z
Learning: In crosspoint-reader/crosspoint-reader (src/components/themes/lyra/Lyra3CoversTheme.cpp and other theme drawing functions), `COVER_DISABLED_MODE` (global setting) only prevents new cover *generation* on HOME entry to avoid blocking the main thread. It does NOT suppress the display of already-cached covers. Only the per-book `coverDisabled` flag (RecentBook::coverDisabled) controls whether a cached cover BMP is rendered. Do not flag theme draw paths for not checking COVER_DISABLED_MODE; checking only coverDisabled is correct by design.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-17T15:27:17.468Z
Learnt from: znelson
Repo: crosspoint-reader/crosspoint-reader PR: 1413
File: lib/EpdFont/EpdFont.cpp:34-36
Timestamp: 2026-03-17T15:27:17.468Z
Learning: In crosspoint-reader/crosspoint-reader, `EpdFont::getGlyph()` falls back to `getGlyph(REPLACEMENT_GLYPH)` (U+FFFD) before returning `nullptr`. All fonts in this project are guaranteed to include a U+FFFD replacement glyph, so the `!glyph` null branch in `EpdFont::getTextBounds()` (and similar rendering paths) is unreachable in practice. Do not flag stale-state issues in that branch (e.g., leftover `lastBaseAdvanceFP`/`lastBaseTop` after a null glyph) as bugs in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 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/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-20T19:51:12.696Z
Learnt from: znelson
Repo: crosspoint-reader/crosspoint-reader PR: 1447
File: src/activities/reader/EpubReaderActivity.cpp:642-660
Timestamp: 2026-03-20T19:51:12.696Z
Learning: In crosspoint-reader/crosspoint-reader (EpubReaderActivity.cpp, prepareSection), when `epub->getTocIndexForSpineIndex(currentSpineIndex)` returns a negative value (pre-TOC spine with no TOC entry), `prepareSection` skips the entire chapter walk and leaves `chapterPageInfo` unset. The status bar correctly falls back to `section->pageCount` in this scenario. This means the `pageTocIndex == -1` path inside the `render()` per-page TOC update block is handled correctly by simply leaving `chapterPageInfo` unchanged — no explicit clear is needed.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-21T15:30:11.892Z
Learnt from: znelson
Repo: crosspoint-reader/crosspoint-reader PR: 1451
File: lib/EpdFont/FontDecompressor.cpp:248-252
Timestamp: 2026-03-21T15:30:11.892Z
Learning: In crosspoint-reader/crosspoint-reader, `FontDecompressor::prewarmCache()` returns -1 when all `MAX_PAGE_SLOTS` (= 4) page slots are exhausted. This is unreachable under normal use because the `PrewarmScope` architecture always calls `clearCache()` before iterating at most 4 font styles (matching `MAX_PAGE_SLOTS`). The -1 is a defensive guard against future misuse, not a live bug. The caller (`FontCacheManager::prewarmCache`) only checks `missed > 0`, which correctly handles the normal flow. Do not flag the undetected -1 return as a bug in future reviews; a header comment documenting the -1 error case is sufficient.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-08T10:24:33.238Z
Learnt from: jpirnay
Repo: crosspoint-reader/crosspoint-reader PR: 1342
File: lib/Serialization/Serialization.h:50-57
Timestamp: 2026-03-08T10:24:33.238Z
Learning: In crosspoint-reader/crosspoint-reader, every call site of `serialization::readString` (lib/Serialization/Serialization.h) checks the bool return value and immediately closes the file and returns false on failure. There is therefore no need to advance the file pointer (e.g. via seekCur) when rejecting an oversized string, because no subsequent read ever occurs after a failure. Additionally, calling seekCur with a potentially UINT32_MAX-sized offset on the ESP32 SdFat implementation is unsafe and should be avoided.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-29T19:50:44.877Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1534
File: src/activities/boot_sleep/SleepActivity.cpp:177-183
Timestamp: 2026-03-29T19:50:44.877Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/boot_sleep/SleepActivity.cpp, SleepActivity::getBookOverlayInfo), when reading totalPages from the TXT index cache (index.bin), the code must validate the cache header: check the magic number (0x54585449 / "TXTI") and version fields before trusting totalPages. This mirrors TxtReaderActivity::loadPageIndexCache(). If validation fails, totalPages stays 0 and the overlay falls back to "Page X" (no total) via STR_OVERLAY_READING_PROGRESS_NO_TOTAL. Do not flag this validation as unnecessary in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-16T03:08:28.963Z
Learnt from: znelson
Repo: crosspoint-reader/crosspoint-reader PR: 1635
File: lib/hal/HalPowerManager.cpp:118-123
Timestamp: 2026-04-16T03:08:28.963Z
Learning: In crosspoint-reader/crosspoint-reader (`lib/hal/HalPowerManager.cpp`, `HalPowerManager::getBatteryPercentage`), using `_batteryCachedPercent == 0` as the smoothing-filter initialization sentinel for the X4 (non-I2C) path is intentional and safe: the device cannot remain powered when the battery truly reads 0%, so the sentinel never collides with a real reading. Do not flag this as a correctness issue in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
🔇 Additional comments (3)
src/components/themes/lyra/LyraTheme.cpp (1)
597-608: LGTM — cursor-aware underline rendering is clean.The content-width fallback and cursor-mode thickness handling match the new theme API.
src/activities/settings/CalibreSettingsActivity.cpp (2)
53-67: LGTM — URL input mode and placeholder normalization fit the new keyboard flow.Prefilling
https://reduces typing, and clearing protocol-only values avoids saving a non-URL placeholder.
70-91: LGTM — text/password callers use the newInputTypeAPI correctly.The username and password entries now select the intended keyboard behavior without changing the existing save flow.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/activities/util/KeyboardEntryActivity.cpp`:
- Around line 61-63: The URL entry logic is incorrectly disabling
Shift/secondary letters whenever inputType == InputType::Url, preventing
uppercase path/query characters; change the checks to only disable Shift when
urlMode is active. Update KeyboardEntryActivity::getAlternativeChar (replace the
inputType==InputType::Url check with a urlMode check and keep the selectedRow
guard as appropriate), and make the same change in the other spots with the same
logic (the other methods/blocks that currently test inputType == InputType::Url
around Shift/symMode behavior) so that Shift remains available for URL input
unless urlMode (3×3 snippet) is active.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53b72bb5-74c6-4b58-8e0c-a358bec63437
📒 Files selected for processing (1)
src/activities/util/KeyboardEntryActivity.cpp
📜 Review details
⏰ 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 context used
🧠 Learnings (24)
📓 Common learnings
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1644
File: src/activities/util/KeyboardEntryActivity.cpp:218-225
Timestamp: 2026-04-12T03:00:48.309Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/util/KeyboardEntryActivity.cpp), the bottom-aligned keyboard start Y uses the simplified formula `getTotalRowCount() * (keyHeight + keySpacing)` intentionally. The small overestimation vs. the actual drawn height is compensated by per-theme `keyboardVerticalOffset` (BaseMetrics: -10, LyraMetrics: -12), manually tuned and verified on-device. Do not flag this approximation as a bug or suggest replacing it with an exact `keyboardHeight` calculation.
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1644
File: src/components/themes/BaseTheme.cpp:835-835
Timestamp: 2026-04-12T02:49:05.293Z
Learning: In crosspoint-reader/crosspoint-reader (src/components/themes/BaseTheme.cpp), the secondary label in `BaseTheme::drawKeyboardKey` is intentionally drawn at `rect.y - 3` (3px above the key rectangle's top edge). This uses part of BaseTheme's `keySpacing = 10` inter-row gap to visually separate the secondary symbol hint from the primary character. In contrast, `LyraTheme::drawKeyboardKey` uses `rect.y` (no offset) because LyraTheme has `keySpacing = 0`. Both placements are correct and deliberate for their respective themes. Do not flag the `-3` offset as a bug or suggest aligning it to `rect.y`.
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1629
File: src/activities/home/HomeActivity.cpp:119-120
Timestamp: 2026-04-12T12:28:36.371Z
Learning: In crosspoint-reader/crosspoint-reader, review comment severity should follow this policy:
- "Major" (🟠): reserve exclusively for issues with a realistic risk of crash, OOM, invalid pointer dereference, data corruption, or other defects that are unlikely to surface in casual device testing.
- "Minor" or informational: use for UX gaps, logic edge-cases, style issues, or missing feature completeness that the author can verify (or has verified) through normal device use.
- Assume the author has tested the feature on their device; do not escalate behavioral/UX observations to Major.
📚 Learning: 2026-04-12T03:00:48.309Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1644
File: src/activities/util/KeyboardEntryActivity.cpp:218-225
Timestamp: 2026-04-12T03:00:48.309Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/util/KeyboardEntryActivity.cpp), the bottom-aligned keyboard start Y uses the simplified formula `getTotalRowCount() * (keyHeight + keySpacing)` intentionally. The small overestimation vs. the actual drawn height is compensated by per-theme `keyboardVerticalOffset` (BaseMetrics: -10, LyraMetrics: -12), manually tuned and verified on-device. Do not flag this approximation as a bug or suggest replacing it with an exact `keyboardHeight` calculation.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-12T02:49:05.293Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1644
File: src/components/themes/BaseTheme.cpp:835-835
Timestamp: 2026-04-12T02:49:05.293Z
Learning: In crosspoint-reader/crosspoint-reader (src/components/themes/BaseTheme.cpp), the secondary label in `BaseTheme::drawKeyboardKey` is intentionally drawn at `rect.y - 3` (3px above the key rectangle's top edge). This uses part of BaseTheme's `keySpacing = 10` inter-row gap to visually separate the secondary symbol hint from the primary character. In contrast, `LyraTheme::drawKeyboardKey` uses `rect.y` (no offset) because LyraTheme has `keySpacing = 0`. Both placements are correct and deliberate for their respective themes. Do not flag the `-3` offset as a bug or suggest aligning it to `rect.y`.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-02-23T06:18:15.143Z
Learnt from: raygan
Repo: crosspoint-reader/crosspoint-reader PR: 1116
File: src/activities/settings/WallabagSettingsActivity.cpp:57-75
Timestamp: 2026-02-23T06:18:15.143Z
Learning: In settings activities (e.g., KOReaderSettingsActivity, WallabagSettingsActivity), the established pattern for launching KeyboardEntryActivity is to call exitActivity() on the parent settings activity before enterNewActivity(new KeyboardEntryActivity(...)). The lambda callbacks then call exitActivity() to exit the KeyboardEntryActivity and return to the parent. This differs from ConfirmationActivity which does not require exitActivity() before enterNewActivity().
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-05T19:31:31.692Z
Learnt from: Zyd8
Repo: crosspoint-reader/crosspoint-reader PR: 1576
File: src/util/ButtonNavigator.h:43-47
Timestamp: 2026-04-05T19:31:31.692Z
Learning: In crosspoint-reader/crosspoint-reader (src/util/ButtonNavigator.cpp), `clearMidpointChordReleaseGuard()` intentionally sets `s_suppressUntilChordRelease` in addition to resetting the post-release guard. This blocks `beginUpDownChord()` from firing on the first loop tick of a new activity if the midpoint chord was still physically held during the `Activity::onEnter()` transition. The fix prevents cross-screen midpoint chord leaks without requiring each activity's `onEnter()` to call `isMidpointChordHeld()`. Do not flag this dual-write as redundant in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-05T19:21:32.486Z
Learnt from: Zyd8
Repo: crosspoint-reader/crosspoint-reader PR: 1576
File: src/activities/home/RecentBooksActivity.cpp:65-90
Timestamp: 2026-04-05T19:21:32.486Z
Learning: In crosspoint-reader/crosspoint-reader (src/util/ButtonNavigator.cpp, shouldSuppressListNavForMidpointChord), the guard-clearing tick intentionally still returns `true` when it clears `s_midpointChordReleaseGuard`. This is correct because all list navigation in the activity uses `onNextRelease`/`onPreviousRelease`, which only fire on a fresh press+release cycle. The clearing tick therefore only swallows the stale release from the chord buttons themselves. Any subsequent Up/Down tap starts a new press+release cycle and will not be suppressed. Do not flag this as a dropped-input bug in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-28T20:57:09.493Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 0
File: :0-0
Timestamp: 2026-03-28T20:57:09.493Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/home/HomeActivity.cpp, HomeActivity::onEnter), `coverRendered = false` and `coverBufferStored = false` ARE correctly reset in `onEnter()`. This was fixed in PR `#1488`. Do not flag their absence as a bug in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-09T20:05:11.820Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/reader/EpubReaderMenuActivity.cpp:39-45
Timestamp: 2026-04-09T20:05:11.820Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/reader/EpubReaderMenuActivity.cpp, EpubReaderMenuActivity::buildMenuItems), the `hasCoverForTheme` boolean (derived from `Storage.exists(coverPath)`) is intentionally used — not `!coverDisabled` — to choose between STR_HOME_COVER_ACTION_DISABLE and STR_HOME_COVER_ACTION_ENABLE in non-TIMEOUT global modes. The label reflects the action that will be performed: BMP exists → "Disable" (delete it); no BMP → "Enable" (generate it). Using `coverDisabled` would create confusing edge cases (e.g., coverDisabled=false with no BMP on disk would show "Disable" when there is nothing to disable). Do not flag this as a bug in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-28T11:08:53.531Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/home/HomeActivity.cpp:114-128
Timestamp: 2026-03-28T11:08:53.531Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/home/HomeActivity.cpp, HomeActivity::loadRecentCovers), the xtc.load() failure path intentionally does NOT set coverDisabled=true. XTC cover handling (including failure→disable behavior) is out of scope until the XTC reader gets its own cover menu actions. Do not flag the missing coverDisabled update on xtc.load() failure as a bug in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-25T01:06:03.725Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/home/HomeActivity.cpp:91-100
Timestamp: 2026-03-25T01:06:03.725Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/home/HomeActivity.cpp, HomeActivity::loadRecentCovers), marking coverDisabled=true on any generateThumbBmp() failure is intentional by design. This prevents HOME from retrying indefinitely. The user can always re-enable via Reader Menu → "Generate cover" / "Enable cover". Do not flag this pattern as a bug in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-12T12:28:33.205Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1629
File: src/activities/home/HomeActivity.cpp:119-120
Timestamp: 2026-04-12T12:28:33.205Z
Learning: When reviewing code in this repository (C/C++ sources), set review comment severity according to this policy:
- Use **Major** (🟠) only for defects with realistic risk of crash, out-of-memory (OOM), invalid pointer dereference, data corruption, or other severe issues that are unlikely to be caught in casual/manual device testing.
- Use **Minor** or informational for UX gaps, logic edge-cases, style issues, or missing feature completeness that the author can verify (or has verified) through normal device use.
- Do **not** escalate severity to Major based on behavioral/UX observations alone; assume the author has already tested the feature on their own device and only treat issues as Major if they match the high-risk defect categories above.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-28T11:06:29.611Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/home/HomeActivity.cpp:92-95
Timestamp: 2026-03-28T11:06:29.611Z
Learning: When reviewing crosspoint-reader code, avoid flagging a missing `renderer.displayBuffer()` call immediately after `GUI.drawPopup()` / `BaseTheme::drawPopup()`: `BaseTheme::drawPopup()` already calls `renderer.displayBuffer()` before returning, so the popup is guaranteed to be flushed to the e-ink panel before subsequent blocking work begins. Conversely, do not require a `renderer.displayBuffer()` call after `fillPopupProgress()`; it intentionally does not flush, so intermediate progress-bar updates may not appear unless the update granularity warrants an explicit flush elsewhere.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-08T13:08:48.852Z
Learnt from: zgredex
Repo: crosspoint-reader/crosspoint-reader PR: 1614
File: src/activities/boot_sleep/SleepActivity.cpp:186-212
Timestamp: 2026-04-08T13:08:48.852Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/boot_sleep/SleepActivity.cpp, SleepActivity::renderPxcSleepScreen and src/activities/util/PxcViewerActivity.cpp), a truncated PXC payload detected mid-render (short read inside the renderGrayscale callback) intentionally produces a partial frame rather than triggering a fallback. The pre-render failure paths (file open, header read, dimension mismatch) do fall back to renderDefaultSleepScreen() / error screen, but partial-read propagation inside the callback is considered acceptable by design. Do not flag this as a missing graceful fallback in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-25T01:15:06.744Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/components/themes/lyra/Lyra3CoversTheme.cpp:39-40
Timestamp: 2026-03-25T01:15:06.744Z
Learning: In crosspoint-reader/crosspoint-reader (src/components/themes/lyra/Lyra3CoversTheme.cpp and other theme drawing functions), `COVER_DISABLED_MODE` (global setting) only prevents new cover *generation* on HOME entry to avoid blocking the main thread. It does NOT suppress the display of already-cached covers. Only the per-book `coverDisabled` flag (RecentBook::coverDisabled) controls whether a cached cover BMP is rendered. Do not flag theme draw paths for not checking COVER_DISABLED_MODE; checking only coverDisabled is correct by design.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-17T15:27:17.468Z
Learnt from: znelson
Repo: crosspoint-reader/crosspoint-reader PR: 1413
File: lib/EpdFont/EpdFont.cpp:34-36
Timestamp: 2026-03-17T15:27:17.468Z
Learning: In crosspoint-reader/crosspoint-reader, `EpdFont::getGlyph()` falls back to `getGlyph(REPLACEMENT_GLYPH)` (U+FFFD) before returning `nullptr`. All fonts in this project are guaranteed to include a U+FFFD replacement glyph, so the `!glyph` null branch in `EpdFont::getTextBounds()` (and similar rendering paths) is unreachable in practice. Do not flag stale-state issues in that branch (e.g., leftover `lastBaseAdvanceFP`/`lastBaseTop` after a null glyph) as bugs in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 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/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-20T19:51:12.696Z
Learnt from: znelson
Repo: crosspoint-reader/crosspoint-reader PR: 1447
File: src/activities/reader/EpubReaderActivity.cpp:642-660
Timestamp: 2026-03-20T19:51:12.696Z
Learning: In crosspoint-reader/crosspoint-reader (EpubReaderActivity.cpp, prepareSection), when `epub->getTocIndexForSpineIndex(currentSpineIndex)` returns a negative value (pre-TOC spine with no TOC entry), `prepareSection` skips the entire chapter walk and leaves `chapterPageInfo` unset. The status bar correctly falls back to `section->pageCount` in this scenario. This means the `pageTocIndex == -1` path inside the `render()` per-page TOC update block is handled correctly by simply leaving `chapterPageInfo` unchanged — no explicit clear is needed.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-21T15:30:11.892Z
Learnt from: znelson
Repo: crosspoint-reader/crosspoint-reader PR: 1451
File: lib/EpdFont/FontDecompressor.cpp:248-252
Timestamp: 2026-03-21T15:30:11.892Z
Learning: In crosspoint-reader/crosspoint-reader, `FontDecompressor::prewarmCache()` returns -1 when all `MAX_PAGE_SLOTS` (= 4) page slots are exhausted. This is unreachable under normal use because the `PrewarmScope` architecture always calls `clearCache()` before iterating at most 4 font styles (matching `MAX_PAGE_SLOTS`). The -1 is a defensive guard against future misuse, not a live bug. The caller (`FontCacheManager::prewarmCache`) only checks `missed > 0`, which correctly handles the normal flow. Do not flag the undetected -1 return as a bug in future reviews; a header comment documenting the -1 error case is sufficient.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-08T10:24:33.238Z
Learnt from: jpirnay
Repo: crosspoint-reader/crosspoint-reader PR: 1342
File: lib/Serialization/Serialization.h:50-57
Timestamp: 2026-03-08T10:24:33.238Z
Learning: In crosspoint-reader/crosspoint-reader, every call site of `serialization::readString` (lib/Serialization/Serialization.h) checks the bool return value and immediately closes the file and returns false on failure. There is therefore no need to advance the file pointer (e.g. via seekCur) when rejecting an oversized string, because no subsequent read ever occurs after a failure. Additionally, calling seekCur with a potentially UINT32_MAX-sized offset on the ESP32 SdFat implementation is unsafe and should be avoided.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-29T19:50:44.877Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1534
File: src/activities/boot_sleep/SleepActivity.cpp:177-183
Timestamp: 2026-03-29T19:50:44.877Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/boot_sleep/SleepActivity.cpp, SleepActivity::getBookOverlayInfo), when reading totalPages from the TXT index cache (index.bin), the code must validate the cache header: check the magic number (0x54585449 / "TXTI") and version fields before trusting totalPages. This mirrors TxtReaderActivity::loadPageIndexCache(). If validation fails, totalPages stays 0 and the overlay falls back to "Page X" (no total) via STR_OVERLAY_READING_PROGRESS_NO_TOTAL. Do not flag this validation as unnecessary in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-04-16T03:08:28.963Z
Learnt from: znelson
Repo: crosspoint-reader/crosspoint-reader PR: 1635
File: lib/hal/HalPowerManager.cpp:118-123
Timestamp: 2026-04-16T03:08:28.963Z
Learning: In crosspoint-reader/crosspoint-reader (`lib/hal/HalPowerManager.cpp`, `HalPowerManager::getBatteryPercentage`), using `_batteryCachedPercent == 0` as the smoothing-filter initialization sentinel for the X4 (non-I2C) path is intentional and safe: the device cannot remain powered when the battery truly reads 0%, so the sentinel never collides with a real reading. Do not flag this as a correctness issue in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-09T11:53:24.166Z
Learnt from: jpirnay
Repo: crosspoint-reader/crosspoint-reader PR: 1360
File: src/activities/boot_sleep/SleepActivity.cpp:83-86
Timestamp: 2026-03-09T11:53:24.166Z
Learning: In crosspoint-reader/crosspoint-reader (ESP32-C3 e-ink reader), `CrossPointState::lastSleepImage` is a `uint8_t` with `UINT8_MAX` used as an "unset" sentinel. A sentinel collision at index 255 would require 256+ full-screen BMP sleep images (~240 KB each minimum); at that count the heap is exhausted building the `std::vector<std::string> files` long before the sentinel can collide, so the collision is unreachable in practice. Widening the type would also require a binary state file version bump (`serialization::readPod` reads exactly `sizeof(uint8_t)`), adding unjustified churn. Do not flag this as a bug in future reviews.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-02-27T22:49:59.600Z
Learnt from: ngxson
Repo: crosspoint-reader/crosspoint-reader PR: 1218
File: src/activities/ActivityManager.cpp:254-265
Timestamp: 2026-02-27T22:49:59.600Z
Learning: In this codebase, assertions are always enabled (no NDEBUG). Use assert() to crash on programmer errors and surface logic bugs during development and in production builds. Do not rely on asserts for runtime error handling; they should enforce invariants that must always hold. Keep asserts side-effect free and inexpensive, and avoid relying on them for user-visible failures. Include <cassert> where appropriate and document the invariant being tested.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
📚 Learning: 2026-03-02T10:14:16.036Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1245
File: lib/Epub/Epub/Section.cpp:277-308
Timestamp: 2026-03-02T10:14:16.036Z
Learning: Guideline: Strengthen serialization::readString to defend against unbounded growth when reading from disk data. Implement and enforce a maximum allowed length (e.g., a configured or reasonable constant) and validate the incoming length before resizing or allocating. Audit all call sites (e.g., BookMetadataCache, TextBlock, KOReaderCredentialStore, Section cache readers) to ensure they do not rely on unbounded len-based resizing. If the readString API must remain, add internal safeguards (bounds checks, length validation, and error handling) so per-call-site validations are not required. Ensure Section cache files remain versioned (SECTION_FILE_VERSION) and parameter mismatches invalidate caches, but do not rely on unsafe allocations; prefer safe, bounded reads with explicit errors on invalid data.
Applied to files:
src/activities/util/KeyboardEntryActivity.cpp
Contributor
Author
|
Hi everyone, I've finished up the final tweaks and improvements I had left to do, so I'd say it's ready for review. |
Contributor
|
Really nice, major quality of life improvement! I like it a lot. I don't think I saw a tool-tip for getting into Cursor Mode, but other than that it's fabulous. I look forward to getting this working on the X3, too 😁 Great work! |
znelson
approved these changes
Apr 18, 2026
Contributor
Author
Thank you so much for your comments, @vjapolitzer. I would appreciate it if you could give me feedback on how well it works on the X3. I'm going in blind until I receive it, in case I need to tweak anything. The reason you didn't see the Cursor mode tooltip is due to a silly mistake on my part. I assumed that if the user enters text and doesn't make a mistake, they wouldn't need to access Cursor mode. When you make a mistake and start deleting, you are shown a tooltip on how to access Cursor mode, and if you enter it, you are shown a second tooltip explaining how to use it. Maybe I overthought it. 😂 |
Contributor
|
@pablohc I'm looking to you to respond to and fix any rising issues that come from keyboard changes, and address the missing localization needs. (I don't mean localize, just make the strings available to localization.) I'm going to move forward with this now, to give it some time to bake before a release. |
Contributor
Author
|
Wow @znelson, thanks for merge it! You didn't give me time to check the comments you left me!! 🤣 I'm halfway through implementing them. I will open a new PR with the fix! ;) |
Contributor
|
@pablohc Aha, I see! I thought perhaps it's a bit unclear at first on how to get to the hide/show password toggle. I wasn't making mistakes and deleting, and thus didn't get the tip for cursor mode! I just wanted to toggle the visibility, ha. Cherry-picked this into my |
Contributor
Author
|
It looks amazing @vjapolitzer!!! Do you think it makes sense for the instructions on how to enter cursor mode to be visible only when you need to correct/delete, or is it better for them to be visible all the time? I’m finishing up some details at #1697, but I still have time to change it. |
znelson
pushed a commit
that referenced
this pull request
Apr 20, 2026
Addresses reviewer feedback from #1644: - **Localize keyboard hint strings** — 13 hardcoded English strings replaced with \`tr()\` macro (\`STR_KB_HINT_*\`), making them translatable across all 22 languages (fallback to English when not yet translated) - **Deduplicate \`Lyra3CoversMetrics\`** — now derives from \`LyraMetrics\` via lambda copy, overriding only \`homeCoverTileHeight\` and \`homeRecentBooksCount\` (eliminates ~30 duplicated metric fields) - **Unify keyboard drawing in \`BaseTheme\`** — \`drawTextField\` and \`drawKeyboardKey\` overrides removed from \`LyraTheme\`; variability controlled via \`keyboardKeyCornerRadius\` metric (0=Base, 6=Lyra). Unified text field padding to 6, adopted Lyra's secondary label draw order (main first, then secondary) - **Add URL-optimized keyboard layout** — \`urlLayout\` with \`:\` and \`/\` replacing \`=\` and \`,\` for easier URL input without switching to SYM mode --- ### 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 **_
trilwu
pushed a commit
to trilwu/crosspet
that referenced
this pull request
Apr 23, 2026
# Refactor: Redesign On-Screen Keyboard ## Summary Complete redesign of the on-screen keyboard (used for WiFi password, KOReader, Calibre URLs) with improved layout, navigation, visual style, and new input features: **cursor mode** for text navigation, **password mode** with visibility toggle, and **URL mode** with pre-defined snippets. ## Screenshots ### Base Theme |**master** | **PR crosspoint-reader#1644** | |----------|-------------| | <img width="480" height="800" alt="image" src="https://github.com/user-attachments/assets/49125857-12d0-4020-b872-05d0ddbf1d94" /> | <img width="480" height="800" alt="image" src="https://github.com/user-attachments/assets/ad16656b-d66e-43dd-8697-2b85f709d7f8" /> | ### Lyra Theme | **master** | **PR crosspoint-reader#1644** | |----------|-------------| | <img width="480" height="800" alt="image" src="https://github.com/user-attachments/assets/d9901251-9154-48d2-83b4-376d3223d132" /> | <img width="480" height="800" alt="image" src="https://github.com/user-attachments/assets/84b45949-ed61-4924-af1b-d570c4c61e13" /> | ### Keyboard States | ABC Mode | Symbol Mode | URL Mode | |----------|-------------|----------| | <img width="480" height="280" alt="image" src="https://github.com/user-attachments/assets/6397a82e-50b8-4d03-92e0-f707ed7c9054" /> | <img width="480" height="280" alt="image" src="https://github.com/user-attachments/assets/205d34fe-0413-49e9-9db2-30569c297ba0" /> | <img width="480" height="280" alt="image" src="https://github.com/user-attachments/assets/801ddeab-e082-4a22-a9df-c66adb1afc16" /> | | Cursor Mode | Password Toggle | |-------------|-----------------| | <img width="480" height="800" alt="image" src="https://github.com/user-attachments/assets/841773e1-7a3c-45e5-aa89-e5787255608c" /> | <img width="480" height="800" alt="image" src="https://github.com/user-attachments/assets/9487a0b3-3f47-41ed-9cd3-b24e56e342a8" /> | ## Changes ### Layout (10-column uniform grid) - Reduced from 13/11/10 columns per row to **10 uniform columns** across all rows - Keyboard now uses **90% of screen width** (was ~66%) - Row 0: Numbers `1-9, 0` with secondary symbols (`!@#$%^&*()`) - Rows 1-3: Standard QWERTY letters - Bottom row: `shift` | `#@!` | `___` | `←` | `OK` ### New Symbol Mode (#@!) - New mode toggle key `#@!` / `abc` switches between letter and symbol layouts - Symbol layout: 4 rows (numbers, inverted symbols, paired symbols, loose symbols) - Covers all 95 printable ASCII characters - **No secondary hints, no long-press** in symbol mode (simple and direct) - SHIFT key remains visible but **disabled** in symbol mode ### URL Mode - In `InputType::Url`, the Space key becomes a **URL toggle** button - Activating URL mode replaces the 4 content rows with a **3×3 grid of URL snippets**: - Col 0 (protocols): `https://`, `http://`, `/opds` - Col 1 (hosts/ports): `www.`, `192.168.`, `:8080` - Col 2 (domains): `.com`, `.org`, `.net` - Snippets are inserted as full strings at the cursor position - URL mode **persists** after inserting a snippet (does not auto-deactivate) - Column alignment: col 0 over ABC, col 1 over URL, col 2 over Del - Up/Down navigation maps `bottomCol - 1` / `urlCol + 1` - SHIFT disabled in URL mode - SpecMode (`abc`) exits URL mode back to ABC - SpecSpace (`URL`) toggles URL mode on/off; selection always stays on the URL button - Button styled with `KeyboardKeyType::Mode` for consistent outline ### Cursor Mode - **Enter**: Long-press Up (500ms) while in keyboard mode - **Exit**: Short-press Down while in cursor mode (resets `passwordVisible`, clears toggle position) - **Navigate**: Left/Right move cursor position within text (one position per press, no continuous repeat) - **Visual**: - Keyboard mode: underline cursor (2px line + serifs) - Cursor mode: inverted block cursor (black fill + white character) - Block width adapts to the actual character width under cursor (minimum 6px for narrow chars like space) - Block position includes inter-character kerning offset for correct alignment (calculated via string-difference: `getTextWidth(before+char) - getTextWidth(before) - getTextWidth(char)`) - End-of-text: thin 6px block - Password hidden: 3-part drawing (Part 1 + block + Part 3) to prevent block overflow onto `*` characters - Toggle position: caret ("I") cursor at saved position, `[abc]`/`[***]` label with inverted selection - **Inactive key styling**: - BaseTheme: 2px outline rectangle - LyraTheme: gray filled rounded rectangle (`Color::LightGray`) - **Password toggle position**: in cursor mode (Password only), Hold Right (500ms) enters toggle — caret cursor shows saved position, `[abc]`/`[***]` label becomes selected. Press Confirm to toggle `passwordVisible`. Press Left to restore cursor to saved position. Right from toggle is a no-op. Down from toggle exits to keyboard. - `cursorPos` persists between keyboard and cursor modes ### Password Mode - `InputType::Password` enum replaces `bool isPassword` parameter - Text is masked with `*` except for one revealed character: - Keyboard mode: reveals character at `cursorPos - 1` - Cursor mode: no reveal in display text (block cursor draws actual char directly) - **Toggle `[abc]`/`[***]`**: accessible via cursor mode — Hold Right (500ms) enters toggle position, Confirm toggles visibility, Left exits back to cursor. Caret ("I") shown at saved position while in toggle. - `passwordVisible` resets to `false` when exiting cursor mode - **Long-press Del (1.5s)**: clears all text and resets cursor to 0 ### InputType Enum - Replaced `bool isPassword` constructor parameter with `enum class InputType { Text, Password, Url }` - Callers updated: `WifiSelectionActivity`, `KOReaderSettingsActivity`, `CalibreSettingsActivity` ### Contextual Tips - `"Tips:"` header followed by context-sensitive hints, centered between text field underline and keyboard as a block - ABC mode: `"Hold SELECT for UPPERCASE or secondary char"` (shift ON: `"lowercase"` variant) + `"Hold DEL to clear all text"` (only if text not empty) - ABC + `InputType::Url`: same + `"Press URL for snippets"` - Symbol mode: `"Hold DEL to clear all text"` (only if text not empty) - URL mode: `"Press ABC to exit URL mode"` + `"Hold DEL to clear all text"` (only if text not empty) - Cursor mode: `"Press DOWN to return to keyboard"` ### Hint Phases (cursor mode, Password only) - **Phase 1**: `"Hold UP to edit entry"` — shown after 2× DEL press, auto-hides after 4s, positioned below underline - **Phase 2**: `"Press < or > to move cursor"` + dynamic password toggle hint — shown when entering cursor mode, positioned below underline, visible until exit - When `!passwordVisible`: `"Hold > then press [abc] to show password"` - When `passwordVisible`: `"Hold > then press [***] to hide password"` - When in toggle position: `"Press < to return to cursor position"` ### Long-Press Alternative Character - Holding Confirm (>500ms) inserts the **alternative character** instead of the primary - Letters: long-press inserts opposite case (e.g., `a`→`A`, `A`→`a`) - Numbers/symbols (row 0): long-press inserts secondary (e.g., `0`→`)`, `)`→`0`) - Only active in ABC mode; disabled in Symbol mode and URL mode - **`InputType::Url`**: Hold SELECT on ABC rows 1+ (letters) returns primary character only (same as short press). Row 0 (symbols) still returns secondary character on Hold SELECT. ### Shift (2 sticky states) - Reduced from 3 states (shift/SHIFT/LOCK) to **2 sticky states** (shift/SHIFT) - Shift stays active after typing until manually toggled off - Label: `shift` (off) / `SHIFT` (on) ### SpecialKeyType Enum - `enum class SpecialKeyType { Shift, Mode, Space, Del, Ok }` replaces plain `enum` (`SpecShift`, `SpecMode`, etc.) for type safety - All switch cases updated to `SpecialKeyType::*` with `static_cast<int>()` for array indexing - `onExit()` reverted to simple `Activity::onExit()` call (half-refresh removed) - **Bottom row column mapping**: navigating up/down between content rows and bottom row uses `col/2` and `col*2` formulas for consistent positioning (10 cols ↔ 5 cols) - **URL mode column mapping**: `bottomCol - 1` / `urlCol + 1` (3 cols ↔ 5 cols) - **Wrap-around**: row 0 → up → bottom row and bottom row → down → row 0 both apply correct column mapping ### Visual Improvements (both Base and Lyra themes) - **Space key**: underscore-style horizontal line (60% of key width, 3px thick) - **Delete key**: arrow `←` drawn with lines (3px thick) instead of "DEL" text - **Secondary label** (ABC row 0): small hint in top-right corner with separation from primary number - **BaseTheme**: selection uses **inverted fill** (black rect + white text) instead of `[bracket]` markers - **BaseTheme**: text field brackets drawn as **stretchable lines** that adapt to multi-line input (1px normal, 3px cursor mode) - **LyraTheme**: text field uses **fixed-width underline** (16px margins, 8px each side) instead of stretchable line (2px normal, 3px cursor mode) - **Both themes**: special keys (shift, mode, space, del, OK) have bordered/bordered-rounded rectangles - **Font size**: keyboard uses `UI_12_FONT_ID` in both themes (was `UI_10` in Base) - **Key height**: 40px in all themes for better proportions - **Layout unification**: text and password toggle are left-aligned in all themes (`keyboardCenteredText = false` for Lyra/Lyra3Covers) - **`primaryOffset` removed**: dead code eliminated from BaseTheme and LyraTheme `drawKeyboardKey` ### New Theme Metrics - `keyboardVerticalOffset`: per-theme vertical adjustment of keyboard position - Base: `-13`, Lyra: `-7` - `keyboardBottomKeySpacing`: independent spacing for bottom row keys - Base: `5`, Lyra: `5` - Bottom-aligned keyboard in both themes for consistent vertical positioning - Bottom row total width calculated to match content rows width (10-col based, consistent across modes) - 4px extra gap between content rows and bottom row when `bkSpacing > 0` - `keyboardCenteredText`: `false` for all themes (unified left-aligned text) ### Defensive Improvements - **State reset on re-entry**: `onEnter()` resets all mutable state (`symMode`, `urlMode`, `cursorMode`, `togglePos`, `passwordVisible`, `shiftState`, `selectedRow`, `selectedCol`, `rightHeld`, `rightLongHandled`, `savedCursorPos`, `rightStartCursorPos`, `delPressCount`, `hintVisible`, `hintShowTime`) — prevents stale state when re-entering the keyboard - **Bounds checking**: `insertChar`/`insertString` clamp `cursorPos` to `text.length()` before inserting - **Empty string guard**: `insertString` returns early on empty string - **`std::string::npos`**: used instead of `SIZE_MAX` for size_t sentinel (proper C++ idiom) - **`<algorithm>` header**: included for `std::max` ## Files Modified | File | Changes | |------|---------| | `src/activities/util/KeyboardEntryActivity.h` | `InputType` enum, `KeyDef` struct, 10-col layouts, cursor/password/URL/toggle state, hints (`delPressCount`, `hintVisible`, `hintShowTime`), held vars (`rightHeld`, `rightLongHandled`, `savedCursorPos`, `rightStartCursorPos`), `mapColContentBottom` helper | | `src/activities/util/KeyboardEntryActivity.cpp` | Complete rewrite: layout rendering, symbol/cursor/password/URL modes, toggle position, long-press, contextual tips, hint phases, block cursor kerning alignment, defensive bounds checks, state reset | | `src/components/themes/BaseTheme.h` | `KeyboardKeyType` enum, new `drawTextField`/`drawKeyboardKey` signatures, `keyboardVerticalOffset`, `keyboardBottomKeySpacing` metrics | | `src/components/themes/BaseTheme.cpp` | Redesigned `drawTextField` (stretchable brackets), `drawKeyboardKey` (inverted selection, space/delete graphics, secondary label, inactive selection), removed `primaryOffset` dead code | | `src/components/themes/lyra/LyraTheme.h` | Override signatures, `keyboardVerticalOffset`, `keyboardBottomKeySpacing`, `keyboardKeyHeight` adjustments | | `src/components/themes/lyra/LyraTheme.cpp` | `drawTextField` (fixed underline), `drawKeyboardKey` (rounded rects for special keys, space/delete graphics, secondary label, inactive selection), removed `primaryOffset` dead code | | `src/components/themes/lyra/Lyra3CoversTheme.h` | `keyboardCenteredText = false`, `keyboardVerticalOffset = -7`, inherits Lyra overrides | | `src/activities/network/WifiSelectionActivity.cpp` | `bool isPassword` → `InputType::Password` | | `src/activities/settings/KOReaderSettingsActivity.cpp` | `bool isPassword` → `InputType::Text`/`InputType::Password`/`InputType::Url` | | `src/activities/settings/CalibreSettingsActivity.cpp` | `bool isPassword` → `InputType::Text`/`InputType::Password`/`InputType::Url` | ## Backward Compatibility - **API change**: Constructor parameter changed from `bool isPassword` to `InputType inputType` (default `InputType::Text`) - **All callers updated**: WiFi, KOReader, and Calibre integrations migrated to new `InputType` enum ## Testing ### Input & Text Handling - [x] Empty input → press OK (submit empty string) - [x] Back button → cancel (no text returned) - [x] Pre-filled initial text (e.g., editing existing WiFi password) - [x] Password mode: text masked with `*` characters, one character revealed - [x] Delete on empty text (no crash) - [x] Very long text near maxLength limit - [x] URL with path and port (~60 chars) - [x] Multi-line text wrapping in input field - [x] Space insert in middle of text (cursor mode) - [x] Delete last character repeatedly - [ ] Type all 95 printable ASCII characters ### Mode Switching - [x] ABC → #@! preserves typed text and cursor position - [x] #@! → ABC preserves typed text and cursor position - [x] Shift state preserved when switching modes - [x] Switch modes multiple times rapidly ### Shift Behavior - [x] Shift OFF → type letter → inserts lowercase, shift stays OFF - [x] Shift ON → type letter → inserts uppercase, shift stays ON - [x] Shift ON → type number → inserts symbol, shift stays ON - [x] Shift ON → navigate rows → shift stays ON - [x] Shift ON → switch to #@! → shift shows "shift" (disabled) - [x] Shift ON → switch to ABC → shift state preserved - [x] Shift ON → switch to URL → shift shows "shift" (disabled) - [x] Shift disabled in URL mode: pressing shift does nothing ### Long-Press - [x] Long-press letter with shift OFF → inserts uppercase - [x] Long-press letter with shift ON → inserts lowercase - [x] Long-press number → inserts secondary symbol - [x] Long-press symbol (row 0) → inserts opposite (number) - [x] Long-press key without secondary (e.g., `-`, `=` in rows 2-3) → inserts primary character on release - [x] Long-press on special keys (shift, mode, space, del, ok) → no alternative inserted - [x] Long-press in #@! mode → no effect (disabled) - [x] Long-press in URL mode → no effect (disabled) - [x] Long-press number in row 0 with InputType::Url → inserts secondary symbol (same as non-URL) - [x] Short press after cancelled long-press → normal behavior - [x] Long-press at maxLength → no character inserted - [x] Long-press Del (1.5s) → clears all text ### Cursor Mode - [x] Long-press Up → enters cursor mode - [x] Short-press Down → exits cursor mode (resets passwordVisible) - [x] Left/Right navigate within text - [x] Left at position 0 → no movement - [x] Right at end of text → no movement in Text mode, enters toggle in Password mode (Hold Right) - [x] Block cursor visual: correct width for character, thin block at end - [x] Underline cursor visual (keyboard mode): correct position with serifs - [x] Inactive key styling: outline (Base) or gray fill (Lyra) on selected key - [x] Typing with cursor mid-text → inserts at cursor position - [x] Deleting with cursor mid-text → deletes character before cursor - [x] Exit cursor mode → type at cursor position (inserts mid-text, not at end) - [x] Exit cursor mode from toggle → cursor at saved position (not end of text) ### Password Mode - [x] Masked text with one revealed character at `cursorPos - 1` - [x] Cursor mode: block shows actual character, display text all `*` - [x] Toggle `[abc]`/`[***]`: Hold Right (500ms) in cursor mode enters toggle, Confirm toggles visibility, Left exits back to cursor - [x] Exiting cursor mode resets `passwordVisible` to false - [x] Long-press Del clears all text ### URL Mode - [x] URL toggle activates/deactivates URL mode - [x] URL button stays selected after toggle (both on and off) - [x] Deactivating URL mode returns to ABC (not SYM) - [x] 3×3 snippet grid displays correctly - [x] Column alignment: col 0 over ABC, col 1 over URL, col 2 over Del - [x] Snippet insertion: inserts full string at cursor position - [x] URL mode persists after snippet insertion - [x] Shift disabled in InputType::Url - [x] SpecMode (`abc`) exits URL mode to ABC - [x] Up/Down navigation between URL grid and bottom row ### Re-entry State Reset - [x] Enter keyboard → activate URL mode → exit → re-enter → URL mode OFF - [x] Enter keyboard → switch to SYM → exit → re-enter → ABC mode - [x] Enter keyboard → enter cursor mode → exit → re-enter → keyboard mode - [x] Enter keyboard → enter toggle pos → exit → re-enter → togglePos OFF - [x] Enter keyboard → activate shift → exit → re-enter → shift OFF - [x] Enter password keyboard → toggle password visible → exit → re-enter → password hidden ### Navigation - [x] Left/right wrap-around within content rows - [x] Left/right wrap-around within bottom row - [x] Up from row 0 → bottom row (correct column mapping) - [x] Down from bottom row → row 0 (correct column mapping) - [x] Up from bottom row → last content row (correct column) - [x] Down from last content row → bottom row (correct column) - [x] Navigate horizontally in bottom row, then up → correct content column - [x] Navigate horizontally in bottom row, then down (wrap) → correct content column ### Visual (both themes) - [x] Secondary hints only on ABC row 0 - [x] No secondary hints in #@! mode or URL mode - [x] No secondary hints on letter rows (1-3) - [x] Space bar: horizontal line centered, not touching edges - [x] Delete: arrow `←` drawn correctly - [x] Selected key: inverted colors (black fill, white text) - [x] All special keys have border rectangles - [x] Fixed underline in text field (Both themes) - [x] Mode key label: `#@!` in ABC mode, `abc` in symbol mode, `abc` in URL mode - [x] URL key label: `URL` (only in InputType::Url), styled same as other bottom keys - [x] Shift label: `shift` when OFF, `SHIFT` when ON, `shift` when disabled (SYM/URL) - [x] Both themes: bottom row total width matches content rows width - [x] URL snippet grid centered over ABC/URL/Del buttons ### Device & Theme Coverage - [ ] Base Theme on X3 - [x] Base Theme on X4 - [ ] Lyra Theme on X3 - [x] Lyra Theme on X4 - [ ] Lyra Extended Theme on X3 - [x] Lyra Extended Theme on X4 ### Toggle Position - [x] Hold Right > 500ms in cursor mode (Password) → enters toggle, caret visible at saved position - [x] Short-press Right in cursor mode (Password) → advances cursor 1 position, does not jump to toggle - [x] Short-press Left in cursor mode (Password) → moves cursor left 1 position, from toggle returns to saved position - [x] Confirm in toggle → toggles `passwordVisible` - [x] Left from toggle → returns to saved position, caret disappears, block cursor appears - [x] Right from toggle → no-op - [x] Down from toggle → exits to keyboard, cursor at saved position - [x] Hold Right in cursor mode (InputType::Text) → no effect - [x] Hold Right in cursor mode (InputType::Url) → no effect - [x] Hold Right < 500ms released in cursor mode (Password) → short press, advances cursor 1 - [x] No continuous repeat when holding Left or Right in cursor mode ### Caret Visual in Toggle - [x] In toggle: caret "I" visible at saved cursor position - [x] Character under cursor visible (no gap) in password not-visible mode - [x] Character under cursor visible in password visible mode - [x] `[abc]`/`[***]` label with inverted selection in toggle ### Contextual Tips - [x] `"Tips:"` header centered above contextual hints - [x] Single tip → `"Tips:"` + one line - [x] Multiple tips → `"Tips:"` + multiple lines, all centered as block - [x] No tips shown when not applicable (e.g., ABC with empty text and non-URL) - [x] `"UPPERCASE"` shown when shift OFF - [x] `"lowercase"` shown when shift ON - [x] `"secondary char"` shown for InputType::Url ### Hint Phases - [x] 2× DEL → Phase 1 appears ("Hold UP to edit entry") - [x] Phase 1 auto-hides after 4s - [x] Phase 2 appears when entering cursor mode ("Press < or > to move cursor") - [x] Phase 2 shows "Hold > then press [abc] to show password" when `!passwordVisible` - [x] Phase 2 shows "Hold > then press [***] to hide password" when `passwordVisible` - [x] Phase 2 shows "Press < to return to cursor position" when in toggle - [x] Phase 2 disappears when exiting cursor mode ### Long-Press `InputType::Url` Behavior - [x] Hold SELECT on letter rows (rows 1+) with InputType::Url → same character as short press - [x] Hold SELECT on row 0 with InputType::Url → secondary character works normally ### Number Row Reorder - [x] Number row order: 1-9, 0 left to right - [x] `(` and `)` are adjacent (positions 8 and 9) via secondary labels - [x] Long-press on row 0 returns correct secondary symbols in new order - [x] SYM row 1: `(` and `)` also adjacent (positions 8 and 9) ### Block Cursor Alignment - [x] Block cursor correctly positioned for consecutive spaces (kerning offset applied) - [x] Block cursor correctly positioned for mixed characters (letters, numbers, symbols) - [x] Block width minimum 6px for narrow characters (space) — visible as block, not thin line - [x] Password hidden: 3-part drawing prevents block overflow onto `*` characters - [x] Password visible: block post-loop draws correctly on continuous text (no 3-part needed) - [x] End-of-text block: thin 6px block at correct position ### Integration - [x] WiFi password entry (connect to network) - [ ] KOReader username, password, and sync server URL - [ ] Calibre OPDS URL, username, and password - [ ] Calibre OPDS URL: empty → opens with "https://" prefilled - [ ] Calibre OPDS URL: type "http://" or "https://" only → saved as empty - [ ] Calibre OPDS URL: type full URL → saved correctly - [ ] Calibre OPDS URL: existing URL → opens with existing URL (not "https://" prefill) (cherry picked from commit 77b2c31)
trilwu
added a commit
to trilwu/crosspet
that referenced
this pull request
Apr 23, 2026
…sign Wallpaper recency buffer (crosspoint-reader#1606): - SleepActivity::renderOverlaySleepScreen was a fork-only second render path not touched by upstream's migration. Port it to the same recency-buffer pick (isRecentSleep/pushRecentSleep) so overlay mode gets the same anti-clustering behaviour as the primary sleep screen. Keyboard API redesign (crosspoint-reader#1644): - KeyboardEntryActivity constructor's trailing bool replaced with InputType enum. Update fork-only call sites: ConferenceBadgeActivity, VirtualPetActivity (rename + hatch flows). - BaseTheme::drawKeyboardKey gained secondaryLabel + keyType + inactiveSelection params. CrossPetTheme was still declaring the old 4-arg override, breaking vtable linkage. Reimplement with the new signature: keep LightGray-selection aesthetic but handle Space/Del key glyphs, outlined special keys, and top-right secondary label like BaseTheme. HTML header regen is a by-product of the pre-build script picking up the upstream FilesPage/SettingsPage template edits from the cherry-picks.
trilwu
pushed a commit
to trilwu/crosspet
that referenced
this pull request
Apr 23, 2026
Addresses reviewer feedback from crosspoint-reader#1644: - **Localize keyboard hint strings** — 13 hardcoded English strings replaced with \`tr()\` macro (\`STR_KB_HINT_*\`), making them translatable across all 22 languages (fallback to English when not yet translated) - **Deduplicate \`Lyra3CoversMetrics\`** — now derives from \`LyraMetrics\` via lambda copy, overriding only \`homeCoverTileHeight\` and \`homeRecentBooksCount\` (eliminates ~30 duplicated metric fields) - **Unify keyboard drawing in \`BaseTheme\`** — \`drawTextField\` and \`drawKeyboardKey\` overrides removed from \`LyraTheme\`; variability controlled via \`keyboardKeyCornerRadius\` metric (0=Base, 6=Lyra). Unified text field padding to 6, adopted Lyra's secondary label draw order (main first, then secondary) - **Add URL-optimized keyboard layout** — \`urlLayout\` with \`:\` and \`/\` replacing \`=\` and \`,\` for easier URL input without switching to SYM mode --- 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 **_ (cherry picked from commit 1a145fe)
trilwu
pushed a commit
to trilwu/crosspet
that referenced
this pull request
Apr 23, 2026
Complete redesign of the on-screen keyboard (used for WiFi password, KOReader, Calibre URLs) with improved layout, navigation, visual style, and new input features: **cursor mode** for text navigation, **password mode** with visibility toggle, and **URL mode** with pre-defined snippets. |**master** | **PR crosspoint-reader#1644** | |----------|-------------| | <img width="480" height="800" alt="image" src="https://github.com/user-attachments/assets/49125857-12d0-4020-b872-05d0ddbf1d94" /> | <img width="480" height="800" alt="image" src="https://github.com/user-attachments/assets/ad16656b-d66e-43dd-8697-2b85f709d7f8" /> | | **master** | **PR crosspoint-reader#1644** | |----------|-------------| | <img width="480" height="800" alt="image" src="https://github.com/user-attachments/assets/d9901251-9154-48d2-83b4-376d3223d132" /> | <img width="480" height="800" alt="image" src="https://github.com/user-attachments/assets/84b45949-ed61-4924-af1b-d570c4c61e13" /> | | ABC Mode | Symbol Mode | URL Mode | |----------|-------------|----------| | <img width="480" height="280" alt="image" src="https://github.com/user-attachments/assets/6397a82e-50b8-4d03-92e0-f707ed7c9054" /> | <img width="480" height="280" alt="image" src="https://github.com/user-attachments/assets/205d34fe-0413-49e9-9db2-30569c297ba0" /> | <img width="480" height="280" alt="image" src="https://github.com/user-attachments/assets/801ddeab-e082-4a22-a9df-c66adb1afc16" /> | | Cursor Mode | Password Toggle | |-------------|-----------------| | <img width="480" height="800" alt="image" src="https://github.com/user-attachments/assets/841773e1-7a3c-45e5-aa89-e5787255608c" /> | <img width="480" height="800" alt="image" src="https://github.com/user-attachments/assets/9487a0b3-3f47-41ed-9cd3-b24e56e342a8" /> | - Reduced from 13/11/10 columns per row to **10 uniform columns** across all rows - Keyboard now uses **90% of screen width** (was ~66%) - Row 0: Numbers `1-9, 0` with secondary symbols (`!@#$%^&*()`) - Rows 1-3: Standard QWERTY letters - Bottom row: `shift` | `#@!` | `___` | `←` | `OK` - New mode toggle key `#@!` / `abc` switches between letter and symbol layouts - Symbol layout: 4 rows (numbers, inverted symbols, paired symbols, loose symbols) - Covers all 95 printable ASCII characters - **No secondary hints, no long-press** in symbol mode (simple and direct) - SHIFT key remains visible but **disabled** in symbol mode - In `InputType::Url`, the Space key becomes a **URL toggle** button - Activating URL mode replaces the 4 content rows with a **3×3 grid of URL snippets**: - Col 0 (protocols): `https://`, `http://`, `/opds` - Col 1 (hosts/ports): `www.`, `192.168.`, `:8080` - Col 2 (domains): `.com`, `.org`, `.net` - Snippets are inserted as full strings at the cursor position - URL mode **persists** after inserting a snippet (does not auto-deactivate) - Column alignment: col 0 over ABC, col 1 over URL, col 2 over Del - Up/Down navigation maps `bottomCol - 1` / `urlCol + 1` - SHIFT disabled in URL mode - SpecMode (`abc`) exits URL mode back to ABC - SpecSpace (`URL`) toggles URL mode on/off; selection always stays on the URL button - Button styled with `KeyboardKeyType::Mode` for consistent outline - **Enter**: Long-press Up (500ms) while in keyboard mode - **Exit**: Short-press Down while in cursor mode (resets `passwordVisible`, clears toggle position) - **Navigate**: Left/Right move cursor position within text (one position per press, no continuous repeat) - **Visual**: - Keyboard mode: underline cursor (2px line + serifs) - Cursor mode: inverted block cursor (black fill + white character) - Block width adapts to the actual character width under cursor (minimum 6px for narrow chars like space) - Block position includes inter-character kerning offset for correct alignment (calculated via string-difference: `getTextWidth(before+char) - getTextWidth(before) - getTextWidth(char)`) - End-of-text: thin 6px block - Password hidden: 3-part drawing (Part 1 + block + Part 3) to prevent block overflow onto `*` characters - Toggle position: caret ("I") cursor at saved position, `[abc]`/`[***]` label with inverted selection - **Inactive key styling**: - BaseTheme: 2px outline rectangle - LyraTheme: gray filled rounded rectangle (`Color::LightGray`) - **Password toggle position**: in cursor mode (Password only), Hold Right (500ms) enters toggle — caret cursor shows saved position, `[abc]`/`[***]` label becomes selected. Press Confirm to toggle `passwordVisible`. Press Left to restore cursor to saved position. Right from toggle is a no-op. Down from toggle exits to keyboard. - `cursorPos` persists between keyboard and cursor modes - `InputType::Password` enum replaces `bool isPassword` parameter - Text is masked with `*` except for one revealed character: - Keyboard mode: reveals character at `cursorPos - 1` - Cursor mode: no reveal in display text (block cursor draws actual char directly) - **Toggle `[abc]`/`[***]`**: accessible via cursor mode — Hold Right (500ms) enters toggle position, Confirm toggles visibility, Left exits back to cursor. Caret ("I") shown at saved position while in toggle. - `passwordVisible` resets to `false` when exiting cursor mode - **Long-press Del (1.5s)**: clears all text and resets cursor to 0 - Replaced `bool isPassword` constructor parameter with `enum class InputType { Text, Password, Url }` - Callers updated: `WifiSelectionActivity`, `KOReaderSettingsActivity`, `CalibreSettingsActivity` - `"Tips:"` header followed by context-sensitive hints, centered between text field underline and keyboard as a block - ABC mode: `"Hold SELECT for UPPERCASE or secondary char"` (shift ON: `"lowercase"` variant) + `"Hold DEL to clear all text"` (only if text not empty) - ABC + `InputType::Url`: same + `"Press URL for snippets"` - Symbol mode: `"Hold DEL to clear all text"` (only if text not empty) - URL mode: `"Press ABC to exit URL mode"` + `"Hold DEL to clear all text"` (only if text not empty) - Cursor mode: `"Press DOWN to return to keyboard"` - **Phase 1**: `"Hold UP to edit entry"` — shown after 2× DEL press, auto-hides after 4s, positioned below underline - **Phase 2**: `"Press < or > to move cursor"` + dynamic password toggle hint — shown when entering cursor mode, positioned below underline, visible until exit - When `!passwordVisible`: `"Hold > then press [abc] to show password"` - When `passwordVisible`: `"Hold > then press [***] to hide password"` - When in toggle position: `"Press < to return to cursor position"` - Holding Confirm (>500ms) inserts the **alternative character** instead of the primary - Letters: long-press inserts opposite case (e.g., `a`→`A`, `A`→`a`) - Numbers/symbols (row 0): long-press inserts secondary (e.g., `0`→`)`, `)`→`0`) - Only active in ABC mode; disabled in Symbol mode and URL mode - **`InputType::Url`**: Hold SELECT on ABC rows 1+ (letters) returns primary character only (same as short press). Row 0 (symbols) still returns secondary character on Hold SELECT. - Reduced from 3 states (shift/SHIFT/LOCK) to **2 sticky states** (shift/SHIFT) - Shift stays active after typing until manually toggled off - Label: `shift` (off) / `SHIFT` (on) - `enum class SpecialKeyType { Shift, Mode, Space, Del, Ok }` replaces plain `enum` (`SpecShift`, `SpecMode`, etc.) for type safety - All switch cases updated to `SpecialKeyType::*` with `static_cast<int>()` for array indexing - `onExit()` reverted to simple `Activity::onExit()` call (half-refresh removed) - **Bottom row column mapping**: navigating up/down between content rows and bottom row uses `col/2` and `col*2` formulas for consistent positioning (10 cols ↔ 5 cols) - **URL mode column mapping**: `bottomCol - 1` / `urlCol + 1` (3 cols ↔ 5 cols) - **Wrap-around**: row 0 → up → bottom row and bottom row → down → row 0 both apply correct column mapping - **Space key**: underscore-style horizontal line (60% of key width, 3px thick) - **Delete key**: arrow `←` drawn with lines (3px thick) instead of "DEL" text - **Secondary label** (ABC row 0): small hint in top-right corner with separation from primary number - **BaseTheme**: selection uses **inverted fill** (black rect + white text) instead of `[bracket]` markers - **BaseTheme**: text field brackets drawn as **stretchable lines** that adapt to multi-line input (1px normal, 3px cursor mode) - **LyraTheme**: text field uses **fixed-width underline** (16px margins, 8px each side) instead of stretchable line (2px normal, 3px cursor mode) - **Both themes**: special keys (shift, mode, space, del, OK) have bordered/bordered-rounded rectangles - **Font size**: keyboard uses `UI_12_FONT_ID` in both themes (was `UI_10` in Base) - **Key height**: 40px in all themes for better proportions - **Layout unification**: text and password toggle are left-aligned in all themes (`keyboardCenteredText = false` for Lyra/Lyra3Covers) - **`primaryOffset` removed**: dead code eliminated from BaseTheme and LyraTheme `drawKeyboardKey` - `keyboardVerticalOffset`: per-theme vertical adjustment of keyboard position - Base: `-13`, Lyra: `-7` - `keyboardBottomKeySpacing`: independent spacing for bottom row keys - Base: `5`, Lyra: `5` - Bottom-aligned keyboard in both themes for consistent vertical positioning - Bottom row total width calculated to match content rows width (10-col based, consistent across modes) - 4px extra gap between content rows and bottom row when `bkSpacing > 0` - `keyboardCenteredText`: `false` for all themes (unified left-aligned text) - **State reset on re-entry**: `onEnter()` resets all mutable state (`symMode`, `urlMode`, `cursorMode`, `togglePos`, `passwordVisible`, `shiftState`, `selectedRow`, `selectedCol`, `rightHeld`, `rightLongHandled`, `savedCursorPos`, `rightStartCursorPos`, `delPressCount`, `hintVisible`, `hintShowTime`) — prevents stale state when re-entering the keyboard - **Bounds checking**: `insertChar`/`insertString` clamp `cursorPos` to `text.length()` before inserting - **Empty string guard**: `insertString` returns early on empty string - **`std::string::npos`**: used instead of `SIZE_MAX` for size_t sentinel (proper C++ idiom) - **`<algorithm>` header**: included for `std::max` | File | Changes | |------|---------| | `src/activities/util/KeyboardEntryActivity.h` | `InputType` enum, `KeyDef` struct, 10-col layouts, cursor/password/URL/toggle state, hints (`delPressCount`, `hintVisible`, `hintShowTime`), held vars (`rightHeld`, `rightLongHandled`, `savedCursorPos`, `rightStartCursorPos`), `mapColContentBottom` helper | | `src/activities/util/KeyboardEntryActivity.cpp` | Complete rewrite: layout rendering, symbol/cursor/password/URL modes, toggle position, long-press, contextual tips, hint phases, block cursor kerning alignment, defensive bounds checks, state reset | | `src/components/themes/BaseTheme.h` | `KeyboardKeyType` enum, new `drawTextField`/`drawKeyboardKey` signatures, `keyboardVerticalOffset`, `keyboardBottomKeySpacing` metrics | | `src/components/themes/BaseTheme.cpp` | Redesigned `drawTextField` (stretchable brackets), `drawKeyboardKey` (inverted selection, space/delete graphics, secondary label, inactive selection), removed `primaryOffset` dead code | | `src/components/themes/lyra/LyraTheme.h` | Override signatures, `keyboardVerticalOffset`, `keyboardBottomKeySpacing`, `keyboardKeyHeight` adjustments | | `src/components/themes/lyra/LyraTheme.cpp` | `drawTextField` (fixed underline), `drawKeyboardKey` (rounded rects for special keys, space/delete graphics, secondary label, inactive selection), removed `primaryOffset` dead code | | `src/components/themes/lyra/Lyra3CoversTheme.h` | `keyboardCenteredText = false`, `keyboardVerticalOffset = -7`, inherits Lyra overrides | | `src/activities/network/WifiSelectionActivity.cpp` | `bool isPassword` → `InputType::Password` | | `src/activities/settings/KOReaderSettingsActivity.cpp` | `bool isPassword` → `InputType::Text`/`InputType::Password`/`InputType::Url` | | `src/activities/settings/CalibreSettingsActivity.cpp` | `bool isPassword` → `InputType::Text`/`InputType::Password`/`InputType::Url` | - **API change**: Constructor parameter changed from `bool isPassword` to `InputType inputType` (default `InputType::Text`) - **All callers updated**: WiFi, KOReader, and Calibre integrations migrated to new `InputType` enum - [x] Empty input → press OK (submit empty string) - [x] Back button → cancel (no text returned) - [x] Pre-filled initial text (e.g., editing existing WiFi password) - [x] Password mode: text masked with `*` characters, one character revealed - [x] Delete on empty text (no crash) - [x] Very long text near maxLength limit - [x] URL with path and port (~60 chars) - [x] Multi-line text wrapping in input field - [x] Space insert in middle of text (cursor mode) - [x] Delete last character repeatedly - [ ] Type all 95 printable ASCII characters - [x] ABC → #@! preserves typed text and cursor position - [x] #@! → ABC preserves typed text and cursor position - [x] Shift state preserved when switching modes - [x] Switch modes multiple times rapidly - [x] Shift OFF → type letter → inserts lowercase, shift stays OFF - [x] Shift ON → type letter → inserts uppercase, shift stays ON - [x] Shift ON → type number → inserts symbol, shift stays ON - [x] Shift ON → navigate rows → shift stays ON - [x] Shift ON → switch to #@! → shift shows "shift" (disabled) - [x] Shift ON → switch to ABC → shift state preserved - [x] Shift ON → switch to URL → shift shows "shift" (disabled) - [x] Shift disabled in URL mode: pressing shift does nothing - [x] Long-press letter with shift OFF → inserts uppercase - [x] Long-press letter with shift ON → inserts lowercase - [x] Long-press number → inserts secondary symbol - [x] Long-press symbol (row 0) → inserts opposite (number) - [x] Long-press key without secondary (e.g., `-`, `=` in rows 2-3) → inserts primary character on release - [x] Long-press on special keys (shift, mode, space, del, ok) → no alternative inserted - [x] Long-press in #@! mode → no effect (disabled) - [x] Long-press in URL mode → no effect (disabled) - [x] Long-press number in row 0 with InputType::Url → inserts secondary symbol (same as non-URL) - [x] Short press after cancelled long-press → normal behavior - [x] Long-press at maxLength → no character inserted - [x] Long-press Del (1.5s) → clears all text - [x] Long-press Up → enters cursor mode - [x] Short-press Down → exits cursor mode (resets passwordVisible) - [x] Left/Right navigate within text - [x] Left at position 0 → no movement - [x] Right at end of text → no movement in Text mode, enters toggle in Password mode (Hold Right) - [x] Block cursor visual: correct width for character, thin block at end - [x] Underline cursor visual (keyboard mode): correct position with serifs - [x] Inactive key styling: outline (Base) or gray fill (Lyra) on selected key - [x] Typing with cursor mid-text → inserts at cursor position - [x] Deleting with cursor mid-text → deletes character before cursor - [x] Exit cursor mode → type at cursor position (inserts mid-text, not at end) - [x] Exit cursor mode from toggle → cursor at saved position (not end of text) - [x] Masked text with one revealed character at `cursorPos - 1` - [x] Cursor mode: block shows actual character, display text all `*` - [x] Toggle `[abc]`/`[***]`: Hold Right (500ms) in cursor mode enters toggle, Confirm toggles visibility, Left exits back to cursor - [x] Exiting cursor mode resets `passwordVisible` to false - [x] Long-press Del clears all text - [x] URL toggle activates/deactivates URL mode - [x] URL button stays selected after toggle (both on and off) - [x] Deactivating URL mode returns to ABC (not SYM) - [x] 3×3 snippet grid displays correctly - [x] Column alignment: col 0 over ABC, col 1 over URL, col 2 over Del - [x] Snippet insertion: inserts full string at cursor position - [x] URL mode persists after snippet insertion - [x] Shift disabled in InputType::Url - [x] SpecMode (`abc`) exits URL mode to ABC - [x] Up/Down navigation between URL grid and bottom row - [x] Enter keyboard → activate URL mode → exit → re-enter → URL mode OFF - [x] Enter keyboard → switch to SYM → exit → re-enter → ABC mode - [x] Enter keyboard → enter cursor mode → exit → re-enter → keyboard mode - [x] Enter keyboard → enter toggle pos → exit → re-enter → togglePos OFF - [x] Enter keyboard → activate shift → exit → re-enter → shift OFF - [x] Enter password keyboard → toggle password visible → exit → re-enter → password hidden - [x] Left/right wrap-around within content rows - [x] Left/right wrap-around within bottom row - [x] Up from row 0 → bottom row (correct column mapping) - [x] Down from bottom row → row 0 (correct column mapping) - [x] Up from bottom row → last content row (correct column) - [x] Down from last content row → bottom row (correct column) - [x] Navigate horizontally in bottom row, then up → correct content column - [x] Navigate horizontally in bottom row, then down (wrap) → correct content column - [x] Secondary hints only on ABC row 0 - [x] No secondary hints in #@! mode or URL mode - [x] No secondary hints on letter rows (1-3) - [x] Space bar: horizontal line centered, not touching edges - [x] Delete: arrow `←` drawn correctly - [x] Selected key: inverted colors (black fill, white text) - [x] All special keys have border rectangles - [x] Fixed underline in text field (Both themes) - [x] Mode key label: `#@!` in ABC mode, `abc` in symbol mode, `abc` in URL mode - [x] URL key label: `URL` (only in InputType::Url), styled same as other bottom keys - [x] Shift label: `shift` when OFF, `SHIFT` when ON, `shift` when disabled (SYM/URL) - [x] Both themes: bottom row total width matches content rows width - [x] URL snippet grid centered over ABC/URL/Del buttons - [ ] Base Theme on X3 - [x] Base Theme on X4 - [ ] Lyra Theme on X3 - [x] Lyra Theme on X4 - [ ] Lyra Extended Theme on X3 - [x] Lyra Extended Theme on X4 - [x] Hold Right > 500ms in cursor mode (Password) → enters toggle, caret visible at saved position - [x] Short-press Right in cursor mode (Password) → advances cursor 1 position, does not jump to toggle - [x] Short-press Left in cursor mode (Password) → moves cursor left 1 position, from toggle returns to saved position - [x] Confirm in toggle → toggles `passwordVisible` - [x] Left from toggle → returns to saved position, caret disappears, block cursor appears - [x] Right from toggle → no-op - [x] Down from toggle → exits to keyboard, cursor at saved position - [x] Hold Right in cursor mode (InputType::Text) → no effect - [x] Hold Right in cursor mode (InputType::Url) → no effect - [x] Hold Right < 500ms released in cursor mode (Password) → short press, advances cursor 1 - [x] No continuous repeat when holding Left or Right in cursor mode - [x] In toggle: caret "I" visible at saved cursor position - [x] Character under cursor visible (no gap) in password not-visible mode - [x] Character under cursor visible in password visible mode - [x] `[abc]`/`[***]` label with inverted selection in toggle - [x] `"Tips:"` header centered above contextual hints - [x] Single tip → `"Tips:"` + one line - [x] Multiple tips → `"Tips:"` + multiple lines, all centered as block - [x] No tips shown when not applicable (e.g., ABC with empty text and non-URL) - [x] `"UPPERCASE"` shown when shift OFF - [x] `"lowercase"` shown when shift ON - [x] `"secondary char"` shown for InputType::Url - [x] 2× DEL → Phase 1 appears ("Hold UP to edit entry") - [x] Phase 1 auto-hides after 4s - [x] Phase 2 appears when entering cursor mode ("Press < or > to move cursor") - [x] Phase 2 shows "Hold > then press [abc] to show password" when `!passwordVisible` - [x] Phase 2 shows "Hold > then press [***] to hide password" when `passwordVisible` - [x] Phase 2 shows "Press < to return to cursor position" when in toggle - [x] Phase 2 disappears when exiting cursor mode - [x] Hold SELECT on letter rows (rows 1+) with InputType::Url → same character as short press - [x] Hold SELECT on row 0 with InputType::Url → secondary character works normally - [x] Number row order: 1-9, 0 left to right - [x] `(` and `)` are adjacent (positions 8 and 9) via secondary labels - [x] Long-press on row 0 returns correct secondary symbols in new order - [x] SYM row 1: `(` and `)` also adjacent (positions 8 and 9) - [x] Block cursor correctly positioned for consecutive spaces (kerning offset applied) - [x] Block cursor correctly positioned for mixed characters (letters, numbers, symbols) - [x] Block width minimum 6px for narrow characters (space) — visible as block, not thin line - [x] Password hidden: 3-part drawing prevents block overflow onto `*` characters - [x] Password visible: block post-loop draws correctly on continuous text (no 3-part needed) - [x] End-of-text block: thin 6px block at correct position - [x] WiFi password entry (connect to network) - [ ] KOReader username, password, and sync server URL - [ ] Calibre OPDS URL, username, and password - [ ] Calibre OPDS URL: empty → opens with "https://" prefilled - [ ] Calibre OPDS URL: type "http://" or "https://" only → saved as empty - [ ] Calibre OPDS URL: type full URL → saved correctly - [ ] Calibre OPDS URL: existing URL → opens with existing URL (not "https://" prefill)
trilwu
pushed a commit
to trilwu/crosspet
that referenced
this pull request
Apr 23, 2026
Addresses reviewer feedback from crosspoint-reader#1644: - **Localize keyboard hint strings** — 13 hardcoded English strings replaced with \`tr()\` macro (\`STR_KB_HINT_*\`), making them translatable across all 22 languages (fallback to English when not yet translated) - **Deduplicate \`Lyra3CoversMetrics\`** — now derives from \`LyraMetrics\` via lambda copy, overriding only \`homeCoverTileHeight\` and \`homeRecentBooksCount\` (eliminates ~30 duplicated metric fields) - **Unify keyboard drawing in \`BaseTheme\`** — \`drawTextField\` and \`drawKeyboardKey\` overrides removed from \`LyraTheme\`; variability controlled via \`keyboardKeyCornerRadius\` metric (0=Base, 6=Lyra). Unified text field padding to 6, adopted Lyra's secondary label draw order (main first, then secondary) - **Add URL-optimized keyboard layout** — \`urlLayout\` with \`:\` and \`/\` replacing \`=\` and \`,\` for easier URL input without switching to SYM mode --- ### 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 **_
trilwu
added a commit
to trilwu/crosspet
that referenced
this pull request
Apr 23, 2026
Cherry-picked upstream commits: - ecca1e0 refactor: redesign on-screen keyboard (crosspoint-reader#1644) - cec1977 feat: Support for proportional numeral spacing (crosspoint-reader#1414) - c693308 fix: Switch to xpath map for paragraph level syncing in KOSync (crosspoint-reader#1686) - ee33879 fix: keyboard feedback crosspoint-reader#1644 (crosspoint-reader#1697) - d54a02d style: put page name first in browser titles (crosspoint-reader#1703) - 4adcf68 fix: Erroneous navigation with long filenames in footnote links (crosspoint-reader#1723) - 6a46687 fix: Replaced Bookerly with Noto Serif for licensing reasons (crosspoint-reader#1736) - 75aff1e feat: Support for multiple OPDS servers (crosspoint-reader#1209) Fork features preserved: BLE, flashcards, pet system, Vietnamese strings, Lexend/Bokerlam fonts, keyboard hints (STR_KB_HINT_*), screen refresh short-press, text darkness, extended sleep screen modes, urlLayout keyboard, weather widget, game scores, WiFi reconnect in performSyncAfterWifi.
6 tasks
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
8 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.
Refactor: Redesign On-Screen Keyboard
Summary
Complete redesign of the on-screen keyboard (used for WiFi password, KOReader, Calibre URLs) with improved layout, navigation, visual style, and new input features: cursor mode for text navigation, password mode with visibility toggle, and URL mode with pre-defined snippets.
Screenshots
Base Theme
Lyra Theme
Keyboard States
Changes
Layout (10-column uniform grid)
1-9, 0with secondary symbols (!@#$%^&*())shift|#@!|___|←|OKNew Symbol Mode (#@!)
#@!/abcswitches between letter and symbol layoutsURL Mode
InputType::Url, the Space key becomes a URL toggle buttonhttps://,http://,/opdswww.,192.168.,:8080.com,.org,.netbottomCol - 1/urlCol + 1abc) exits URL mode back to ABCURL) toggles URL mode on/off; selection always stays on the URL buttonKeyboardKeyType::Modefor consistent outlineCursor Mode
passwordVisible, clears toggle position)getTextWidth(before+char) - getTextWidth(before) - getTextWidth(char))*characters[abc]/[***]label with inverted selectionColor::LightGray)[abc]/[***]label becomes selected. Press Confirm to togglepasswordVisible. Press Left to restore cursor to saved position. Right from toggle is a no-op. Down from toggle exits to keyboard.cursorPospersists between keyboard and cursor modesPassword Mode
InputType::Passwordenum replacesbool isPasswordparameter*except for one revealed character:cursorPos - 1[abc]/[***]: accessible via cursor mode — Hold Right (500ms) enters toggle position, Confirm toggles visibility, Left exits back to cursor. Caret ("I") shown at saved position while in toggle.passwordVisibleresets tofalsewhen exiting cursor modeInputType Enum
bool isPasswordconstructor parameter withenum class InputType { Text, Password, Url }WifiSelectionActivity,KOReaderSettingsActivity,CalibreSettingsActivityContextual Tips
"Tips:"header followed by context-sensitive hints, centered between text field underline and keyboard as a block"Hold SELECT for UPPERCASE or secondary char"(shift ON:"lowercase"variant) +"Hold DEL to clear all text"(only if text not empty)InputType::Url: same +"Press URL for snippets""Hold DEL to clear all text"(only if text not empty)"Press ABC to exit URL mode"+"Hold DEL to clear all text"(only if text not empty)"Press DOWN to return to keyboard"Hint Phases (cursor mode, Password only)
"Hold UP to edit entry"— shown after 2× DEL press, auto-hides after 4s, positioned below underline"Press < or > to move cursor"+ dynamic password toggle hint — shown when entering cursor mode, positioned below underline, visible until exit!passwordVisible:"Hold > then press [abc] to show password"passwordVisible:"Hold > then press [***] to hide password""Press < to return to cursor position"Long-Press Alternative Character
a→A,A→a)0→),)→0)InputType::Url: Hold SELECT on ABC rows 1+ (letters) returns primary character only (same as short press). Row 0 (symbols) still returns secondary character on Hold SELECT.Shift (2 sticky states)
shift(off) /SHIFT(on)SpecialKeyType Enum
enum class SpecialKeyType { Shift, Mode, Space, Del, Ok }replaces plainenum(SpecShift,SpecMode, etc.) for type safetySpecialKeyType::*withstatic_cast<int>()for array indexingonExit()reverted to simpleActivity::onExit()call (half-refresh removed)col/2andcol*2formulas for consistent positioning (10 cols ↔ 5 cols)bottomCol - 1/urlCol + 1(3 cols ↔ 5 cols)Visual Improvements (both Base and Lyra themes)
←drawn with lines (3px thick) instead of "DEL" text[bracket]markersUI_12_FONT_IDin both themes (wasUI_10in Base)keyboardCenteredText = falsefor Lyra/Lyra3Covers)primaryOffsetremoved: dead code eliminated from BaseTheme and LyraThemedrawKeyboardKeyNew Theme Metrics
keyboardVerticalOffset: per-theme vertical adjustment of keyboard position-13, Lyra:-7keyboardBottomKeySpacing: independent spacing for bottom row keys5, Lyra:5bkSpacing > 0keyboardCenteredText:falsefor all themes (unified left-aligned text)Defensive Improvements
onEnter()resets all mutable state (symMode,urlMode,cursorMode,togglePos,passwordVisible,shiftState,selectedRow,selectedCol,rightHeld,rightLongHandled,savedCursorPos,rightStartCursorPos,delPressCount,hintVisible,hintShowTime) — prevents stale state when re-entering the keyboardinsertChar/insertStringclampcursorPostotext.length()before insertinginsertStringreturns early on empty stringstd::string::npos: used instead ofSIZE_MAXfor size_t sentinel (proper C++ idiom)<algorithm>header: included forstd::maxFiles Modified
src/activities/util/KeyboardEntryActivity.hInputTypeenum,KeyDefstruct, 10-col layouts, cursor/password/URL/toggle state, hints (delPressCount,hintVisible,hintShowTime), held vars (rightHeld,rightLongHandled,savedCursorPos,rightStartCursorPos),mapColContentBottomhelpersrc/activities/util/KeyboardEntryActivity.cppsrc/components/themes/BaseTheme.hKeyboardKeyTypeenum, newdrawTextField/drawKeyboardKeysignatures,keyboardVerticalOffset,keyboardBottomKeySpacingmetricssrc/components/themes/BaseTheme.cppdrawTextField(stretchable brackets),drawKeyboardKey(inverted selection, space/delete graphics, secondary label, inactive selection), removedprimaryOffsetdead codesrc/components/themes/lyra/LyraTheme.hkeyboardVerticalOffset,keyboardBottomKeySpacing,keyboardKeyHeightadjustmentssrc/components/themes/lyra/LyraTheme.cppdrawTextField(fixed underline),drawKeyboardKey(rounded rects for special keys, space/delete graphics, secondary label, inactive selection), removedprimaryOffsetdead codesrc/components/themes/lyra/Lyra3CoversTheme.hkeyboardCenteredText = false,keyboardVerticalOffset = -7, inherits Lyra overridessrc/activities/network/WifiSelectionActivity.cppbool isPassword→InputType::Passwordsrc/activities/settings/KOReaderSettingsActivity.cppbool isPassword→InputType::Text/InputType::Password/InputType::Urlsrc/activities/settings/CalibreSettingsActivity.cppbool isPassword→InputType::Text/InputType::Password/InputType::UrlBackward Compatibility
bool isPasswordtoInputType inputType(defaultInputType::Text)InputTypeenumTesting
Input & Text Handling
*characters, one character revealedMode Switching
Shift Behavior
Long-Press
-,=in rows 2-3) → inserts primary character on releaseCursor Mode
Password Mode
cursorPos - 1*[abc]/[***]: Hold Right (500ms) in cursor mode enters toggle, Confirm toggles visibility, Left exits back to cursorpasswordVisibleto falseURL Mode
abc) exits URL mode to ABCRe-entry State Reset
Navigation
Visual (both themes)
←drawn correctly#@!in ABC mode,abcin symbol mode,abcin URL modeURL(only in InputType::Url), styled same as other bottom keysshiftwhen OFF,SHIFTwhen ON,shiftwhen disabled (SYM/URL)Device & Theme Coverage
Toggle Position
passwordVisibleCaret Visual in Toggle
[abc]/[***]label with inverted selection in toggleContextual Tips
"Tips:"header centered above contextual hints"Tips:"+ one line"Tips:"+ multiple lines, all centered as block"UPPERCASE"shown when shift OFF"lowercase"shown when shift ON"secondary char"shown for InputType::UrlHint Phases
!passwordVisiblepasswordVisibleLong-Press
InputType::UrlBehaviorNumber Row Reorder
(and)are adjacent (positions 8 and 9) via secondary labels(and)also adjacent (positions 8 and 9)Block Cursor Alignment
*charactersIntegration