Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughIntroduce non-selectable separator rows in menus, add selectable-aware navigation to ButtonNavigator, provide UITheme separator helpers and themed separator rendering, refactor reader and clock settings to runtime menu models, and update English translations (restore "Sun", add reader UI keys). Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/activities/settings/SettingsActivity.cpp (1)
161-181:⚠️ Potential issue | 🟠 MajorAdvance to the first selectable row after category changes.
Line 162 forces
selectedSettingIndexto1when switching categories from inside the list, but the System category now always starts withSettingInfo::Separator(...)at Line 40. That leaves the focus on a non-selectable row, so Confirm becomes a no-op until the user navigates again.💡 Suggested fix
if (hasChangedCategory) { selectedSettingIndex = (selectedSettingIndex == 0) ? 0 : 1; switch (selectedCategoryIndex) { case 0: currentSettings = &displaySettings; break; @@ } settingsCount = static_cast<int>(currentSettings->size()); - buttonNavigator.setSelectablePredicate(makeSelectablePredicate(*currentSettings, 1, true), settingsCount + 1); + auto predicate = buildSelectablePredicate(); + buttonNavigator.setSelectablePredicate(predicate, settingsCount + 1); + if (selectedSettingIndex > 0 && !predicate(selectedSettingIndex)) { + selectedSettingIndex = buttonNavigator.nextIndex(selectedSettingIndex); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activities/settings/SettingsActivity.cpp` around lines 161 - 181, When the category changes we must advance focus to the first selectable setting (not assume index 1); after assigning currentSettings (based on selectedCategoryIndex) compute the first selectable index in *currentSettings (skip entries like SettingInfo::Separator) and set selectedSettingIndex to that index (or 0 if none) before updating settingsCount and calling buttonNavigator.setSelectablePredicate with makeSelectablePredicate(*currentSettings, 1, true). Use the same selectable check used by makeSelectablePredicate to locate the first selectable row so Confirm is active immediately in categories such as System that start with a Separator.
🤖 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`:
- Around line 306-307: The code contains a duplicated renderer.drawText(...)
call (the identical call using font, rect.x +
BaseMetrics::values.contentSidePadding, itemY, item.c_str(), true) which will
render the same text twice; remove the redundant second call so only a single
renderer.drawText(...) invocation remains (refer to the renderer.drawText call
that uses font, rect, BaseMetrics::values.contentSidePadding, itemY and
item.c_str()).
---
Outside diff comments:
In `@src/activities/settings/SettingsActivity.cpp`:
- Around line 161-181: When the category changes we must advance focus to the
first selectable setting (not assume index 1); after assigning currentSettings
(based on selectedCategoryIndex) compute the first selectable index in
*currentSettings (skip entries like SettingInfo::Separator) and set
selectedSettingIndex to that index (or 0 if none) before updating settingsCount
and calling buttonNavigator.setSelectablePredicate with
makeSelectablePredicate(*currentSettings, 1, true). Use the same selectable
check used by makeSelectablePredicate to locate the first selectable row so
Confirm is active immediately in categories such as System that start with a
Separator.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3c86648-2aa2-4cf5-8a11-7005dd725b1c
📒 Files selected for processing (15)
lib/I18n/translations/english.yamlsrc/activities/reader/EpubReaderMenuActivity.cppsrc/activities/reader/EpubReaderMenuActivity.hsrc/activities/settings/ClockSettingsActivity.cppsrc/activities/settings/ClockSettingsActivity.hsrc/activities/settings/SettingsActivity.cppsrc/activities/settings/SettingsActivity.hsrc/components/UITheme.cppsrc/components/UITheme.hsrc/components/themes/BaseTheme.cppsrc/components/themes/BaseTheme.hsrc/components/themes/lyra/LyraTheme.cppsrc/util/ButtonNavigator.cppsrc/util/ButtonNavigator.hsrc/util/MenuItemHelpers.h
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/components/themes/BaseTheme.cpp (1)
306-307:⚠️ Potential issue | 🟡 MinorDuplicate
drawTextcall - likely copy-paste error.Line 307 duplicates the exact same
drawTextcall from Line 306, which will render the same text twice at the same position.🐛 Proposed fix to remove duplicate line
auto item = renderer.truncatedText(font, itemName.c_str(), textWidth); renderer.drawText(font, rect.x + BaseMetrics::values.contentSidePadding, itemY, item.c_str(), true); - renderer.drawText(font, rect.x + BaseMetrics::values.contentSidePadding, itemY, item.c_str(), true);🤖 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 306 - 307, Remove the duplicate draw call: the two consecutive calls to renderer.drawText(font, rect.x + BaseMetrics::values.contentSidePadding, itemY, item.c_str(), true) render the same text twice; keep a single invocation and delete the redundant one (search for renderer.drawText usage in BaseTheme::... where font, rect, itemY, and item.c_str() are used to locate the duplicated lines).
🤖 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/reader/EpubReaderMenuActivity.cpp`:
- Around line 71-75: The current onEnter() always advances selectedIndex by
calling buttonNavigator.nextIndex(selectedIndex), shifting a restored valid
selection; change it to only normalize when the current row is not selectable by
checking the selectable predicate first (use the same predicate passed to
buttonNavigator.setSelectablePredicate — or
buttonNavigator.isSelectable(selectedIndex) if available) and only call
buttonNavigator.nextIndex(selectedIndex) and assign selectedIndex when that
check fails so that valid selections are preserved.
In `@src/components/themes/BaseTheme.cpp`:
- Around line 291-302: The duplicate `if (isSeparator)` check should be merged:
compute bool isSeparator = UITheme::isSeparatorTitle(itemName), and if true call
UITheme::stripSeparatorTitle(itemName) and immediately call
drawListSeparator(...) then continue; remove the second separate `if
(isSeparator)` block. Update the block surrounding UITheme::isSeparatorTitle,
UITheme::stripSeparatorTitle, drawListSeparator, and the variables itemName,
itemY, contentWidth, rowHeight, textWidth, rect, renderer so the strip and draw
happen inside a single conditional.
In `@src/components/themes/lyra/LyraTheme.cpp`:
- Around line 446-457: The duplicate check of isSeparator should be removed by
combining the logic: call UITheme::isSeparatorTitle(itemName) into isSeparator,
and if true then immediately set itemName =
UITheme::stripSeparatorTitle(itemName), call drawListSeparator(...) with the
same parameters (using itemY, contentWidth, rowHeight, textX, rowTextWidth,
LyraMetrics::values.contentSidePadding), and continue; i.e. replace the two
separate if (isSeparator) blocks with a single if (isSeparator) { itemName =
UITheme::stripSeparatorTitle(itemName); drawListSeparator(...); continue; }
ensuring drawListSeparator and the strip call remain intact.
---
Duplicate comments:
In `@src/components/themes/BaseTheme.cpp`:
- Around line 306-307: Remove the duplicate draw call: the two consecutive calls
to renderer.drawText(font, rect.x + BaseMetrics::values.contentSidePadding,
itemY, item.c_str(), true) render the same text twice; keep a single invocation
and delete the redundant one (search for renderer.drawText usage in
BaseTheme::... where font, rect, itemY, and item.c_str() are used to locate the
duplicated lines).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b6e8902d-7e99-458c-b019-5be33b365e4e
📒 Files selected for processing (12)
lib/I18n/translations/english.yamlsrc/activities/reader/EpubReaderMenuActivity.cppsrc/activities/reader/EpubReaderMenuActivity.hsrc/activities/settings/ClockSettingsActivity.cppsrc/activities/settings/ClockSettingsActivity.hsrc/components/UITheme.cppsrc/components/UITheme.hsrc/components/themes/BaseTheme.cppsrc/components/themes/BaseTheme.hsrc/components/themes/lyra/LyraTheme.cppsrc/util/ButtonNavigator.cppsrc/util/ButtonNavigator.h
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Additional Context
specific areas to focus on).
AI Usage
While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it
helps set the right context for reviewers.
Did you use AI tools to help write this code? < YES | PARTIALLY | NO >
Summary by CodeRabbit
New Features
Refactor