feat: Themed language screen#1020
Conversation
📝 WalkthroughWalkthroughNavigation in LanguageSelectActivity moved from direct input polling to an event-driven ButtonNavigator; rendering switched from manual per-item drawing to GUI.drawList and GUI.drawHeader, with layout computed from shared metrics and lambdas supplying item text and selection marker. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Activity as LanguageSelectActivity
participant Navigator as ButtonNavigator
participant GUI
participant Renderer as drawList
User->>Activity: press navigation button
Activity->>Navigator: forward button event
Navigator->>Activity: onNextRelease / onPreviousRelease
Activity->>Activity: update currentIndex (nextIndex/previousIndex)
Activity->>Activity: requestUpdate()
Activity->>GUI: drawHeader(), compute contentTop/contentHeight
GUI->>Renderer: drawList(items lambda, selection lambda)
Renderer->>Activity: call item-label lambda for visible items
Renderer->>Activity: call selection-marker lambda for currentIndex
GUI-->>Activity: rendered frame
Activity-->>User: updated UI shown
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/activities/settings/LanguageSelectActivity.cppsrc/activities/settings/LanguageSelectActivity.h
🧰 Additional context used
🧬 Code graph analysis (1)
src/activities/settings/LanguageSelectActivity.cpp (1)
lib/I18n/I18n.cpp (2)
getInstance(15-18)getInstance(15-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
src/activities/settings/LanguageSelectActivity.h (1)
10-33: LGTM: ButtonNavigator integration is clean.
The private member addition and include are straightforward and don’t alter public APIs.src/activities/settings/LanguageSelectActivity.cpp (1)
33-41: No action needed.onNextRelease/onPreviousReleaseare polling functions, not callback registrations. They check button state each frame and immediately invoke the lambda if released—they do not accumulate handlers. This per-frame polling pattern is correct and intended for the activity loop.Likely an incorrect or invalid review comment.
🤖 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/settings/LanguageSelectActivity.cpp`:
- Around line 64-70: Add a defensive guard for empty language lists before
calling GUI.drawList: check the total item count (use I18N.getLanguageCount() or
the existing totalItems) and if it is 0, avoid calling GUI.drawList (either
return early or render the same empty-list handling used in
RecentBooksActivity/MyLibraryActivity) instead of attempting to draw the list;
keep the selectedIndex handling unchanged and preserve the currentLang logic for
non-empty cases.
CaptainFrito
force-pushed
the
feature/lyra-language-screen
branch
from
9eef16f to
000a43c
Compare
There was a problem hiding this comment.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/activities/settings/LanguageSelectActivity.cppsrc/activities/settings/LanguageSelectActivity.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/activities/settings/LanguageSelectActivity.h
🧰 Additional context used
🧠 Learnings (1)
📚 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/LanguageSelectActivity.cpp
🧬 Code graph analysis (1)
src/activities/settings/LanguageSelectActivity.cpp (1)
lib/I18n/I18n.cpp (2)
getInstance(15-18)getInstance(15-15)
⏰ 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: build
- GitHub Check: cppcheck
🔇 Additional comments (1)
src/activities/settings/LanguageSelectActivity.cpp (1)
33-42: LGTM!The event-driven navigation using
ButtonNavigatoris cleanly implemented. The lambdas correctly capturethisand update the selection index with proper boundary handling vianextIndex/previousIndex.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/activities/settings/LanguageSelectActivity.cpp`:
- Around line 59-70: Add a defensive guard before calling GUI.drawList in
LanguageSelectActivity so an empty language list is handled like
RecentBooksActivity/MyLibraryActivity: check the language count (use
getLanguageCount() or totalItems) and if it is zero, skip or render an
appropriate empty-state instead of calling GUI.drawList; keep existing header
rendering (GUI.drawHeader) and preserve
selectedIndex/I18N.getLanguageName/I18N.getLanguage usage when the list is
non-empty.
|
No code review but tested on device and seems to work great! |
CaptainFrito
force-pushed
the
feature/lyra-language-screen
branch
from
000a43c to
f1999f8
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/activities/settings/LanguageSelectActivity.cpp (1)
67-70: Clean drawList implementation with appropriate lambdas.The rendering correctly uses
GUI.drawListwith lambdas to supply language names and the "Set" marker for the current selection. This approach cleanly separates data retrieval from rendering logic.Regarding the empty-list guard suggested in a previous review: since
getLanguageCount()returns a compile-time constant from a hardcoded enum (always 8 languages), adding a guard provides minimal practical value here. If you prefer consistency with other list-based activities, you could add it, but it's not strictly necessary for this case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activities/settings/LanguageSelectActivity.cpp` around lines 67 - 70, The current GUI.drawList call using lambdas (GUI.drawList, I18N.getLanguageName) is fine; no code change required because getLanguageCount() is a compile-time constant (always 8). If you prefer consistency with other activities, add an optional empty-list guard: call getLanguageCount() (or a wrapper) into totalItems, if totalItems == 0 skip calling GUI.drawList and render an empty-state message instead; otherwise invoke GUI.drawList exactly as currently written, keeping the lambdas for names and the "Set" marker.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/activities/settings/LanguageSelectActivity.cpp`:
- Around line 67-70: The current GUI.drawList call using lambdas (GUI.drawList,
I18N.getLanguageName) is fine; no code change required because
getLanguageCount() is a compile-time constant (always 8). If you prefer
consistency with other activities, add an optional empty-list guard: call
getLanguageCount() (or a wrapper) into totalItems, if totalItems == 0 skip
calling GUI.drawList and render an empty-state message instead; otherwise invoke
GUI.drawList exactly as currently written, keeping the lambdas for names and the
"Set" marker.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/activities/settings/LanguageSelectActivity.cppsrc/activities/settings/LanguageSelectActivity.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/activities/settings/LanguageSelectActivity.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: build
- GitHub Check: cppcheck
🧰 Additional context used
🧠 Learnings (2)
📚 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/LanguageSelectActivity.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/LanguageSelectActivity.cpp
🧬 Code graph analysis (1)
src/activities/settings/LanguageSelectActivity.cpp (1)
lib/I18n/I18n.cpp (2)
getInstance(15-18)getInstance(15-15)
🔇 Additional comments (2)
src/activities/settings/LanguageSelectActivity.cpp (2)
33-42: LGTM!Clean migration to event-driven navigation using
ButtonNavigator. The lambda-based handlers with static index calculation methods follow a good pattern for list navigation with wrapping support.
59-65: LGTM!The metrics-driven layout calculations correctly position the content area between the header and button hints. Using
UITheme::getInstance().getMetrics()properly integrates with the theming system.
## Summary Added theme support to the language screen  ## Additional Context --- ### AI Usage Did you use AI tools to help write this code? _**NO**_
|
@CaptainFrito, thanks for the new screen, it looks very nice! I noticed you changed the localisation string form Because the new string is translated quite diffferently in the translations, since "set" is a bit ambiguous . Currently this is also the only place in the code this is used as far as I could tell. So translators also lacked context maybe. At least for the languages I understand it looks a bit off. I thought I'd check with you before I open a PR. My proposal would be to make a string called |
Thanks for the feedback! I only changed it to try to reduce the number of different strings we have in the app, and Set is already used elsewhere. But I understand if it's not suitable we can change it back, or change STR_ON_MARKER to something else :) |
|
Ok, thank you that helps. I'll open a PR later today and we can discuss the rest there |
## Summary Added theme support to the language screen  ## Additional Context --- ### AI Usage Did you use AI tools to help write this code? _**NO**_
## Summary Added theme support to the language screen  ## Additional Context --- ### AI Usage Did you use AI tools to help write this code? _**NO**_
## Summary Added theme support to the language screen  ## Additional Context --- ### AI Usage Did you use AI tools to help write this code? _**NO**_
## Summary Added theme support to the language screen  ## Additional Context --- ### AI Usage Did you use AI tools to help write this code? _**NO**_
## Summary * **What is the goal of this PR?** (e.g., Implements the new feature for file uploading.) Fixes a clarity issue regarding the translation string `STR_SET`. The issue lies in the fact that the english word can have different meanings. The only time the string is used is in the language selectio screen, where it has the meaning of _selected_. (As in _The language has been **set** to French_). Another meaning can be _configured_. (As in _The KOReader username has been __set__). This is the meaning many of the translations have taken. The reason that the string is right above `STR_NOT_SET` (which is meant as _not configured_). With this PR I propose to explicitly use the term "_Selected_". There are two good reasons for this: + it removes the confusion and the misleading translations + it is consistent with the button label `Select`, communicating the link between the two (the row will be marked `Selected` if you press the buttpn `Select`. Much clearer than now) * **What changes are included?** Removed the unused strings and added translations for the new string `STR_SELECTED` for the languages I know. tagging the translators for feedback: fr: @Spigaw @CaptainFrito de: @DavidOrtmann cs: @brbla pt: @yagofarias it: @andreaturchet @fargolinux ru: @madebyKir @mrtnvgr es: @yeyeto2788 @Skrzakk @pablohc sv: @dawiik ca: @angeldenom uj: @mirus-ua be: @dexif ## Additional Context * Add any other information that might be helpful for the reviewer (e.g., performance implications, potential risks, specific areas to focus on). the Issue was introduced in #1020. Previously, if a language was selected it was marked with `[ON]` (`STR_ON_MARKER`). I considered reverting it back to that, but the solution I described above seemed superior. --- ### 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? _**< NO >**_ --------- Co-authored-by: Егор Мартынов <[email protected]> Co-authored-by: Mirus <[email protected]>
5 participants
Summary
Added theme support to the language screen
Additional Context
AI Usage
Did you use AI tools to help write this code? NO