feat: Added theme support for External Display Input#497
feat: Added theme support for External Display Input#497MayRedBeWithYou wants to merge 3 commits intoutkarshdalal:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds a theme system for external display input: new Theme enum with colors, PrefManager.externalDisplayTheme preference, settings UI to select theme, ExternalDisplayInputController.setTheme API, and views/presentation updated to render and propagate theme changes. Changes
Sequence DiagramsequenceDiagram
actor User
participant Settings as "Settings UI"
participant PrefMgr as "PrefManager"
participant XScreen as "XServerScreen"
participant DisplayCtrl as "ExternalDisplayInputController"
participant Presentation as "ExternalInputPresentation"
participant Views as "External Views"
User->>Settings: select theme option
Settings->>PrefMgr: save externalDisplayTheme
PrefMgr-->>Settings: confirm persisted
XScreen->>PrefMgr: read externalDisplayTheme
PrefMgr-->>XScreen: return theme
XScreen->>DisplayCtrl: start / setTheme(theme)
DisplayCtrl->>Presentation: updateTheme(theme)
Presentation->>Views: re-render using theme colors
Views-->>User: show themed external UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 8 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupInterface.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupInterface.kt:419">
P2: User-visible dropdown labels are hardcoded instead of using string resources, preventing localization and inconsistent with the rest of the settings UI.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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 (2)
app/src/main/java/app/gamenative/externaldisplay/SwapInputOverlayView.kt (1)
55-60:⚠️ Potential issue | 🟠 MajorBug:
keyboardToggleButtonbackground color is not themed.Line 57 uses hardcoded
R.color.external_display_key_backgroundwhile every other color in this file and the equivalent button inHybridInputLayout(line 291 ofExternalDisplayInputController.kt) usestheme.keyBg. This means the AMOLED theme won't apply to this button's background inSwapInputOverlayView.🐛 Proposed fix
background = GradientDrawable().apply { shape = GradientDrawable.OVAL - setColor(ContextCompat.getColor(context, R.color.external_display_key_background)) + setColor(ContextCompat.getColor(context, theme.keyBg)) }app/src/main/java/app/gamenative/externaldisplay/ExternalDisplayInputController.kt (1)
213-222:⚠️ Potential issue | 🟡 MinorInconsistent icon styling:
alpha = 0.35fhere vs. no alpha inSwapInputOverlayView.In the
ExternalInputPresentationKEYBOARD mode, the hint icon uses bothsetColorFilter(theme.iconColor)(line 220) andalpha = 0.35f(line 221). InSwapInputOverlayView(line 30), onlysetColorFilter(theme.iconColor)is applied—no alpha. This means the hint icon will look different depending on which code path renders it.If the intent is for the theme's
iconColorto fully control opacity, remove the hardcoded alpha here. If the alpha is intentional for the external display, consider documenting why.
🤖 Fix all issues with AI agents
In
`@app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupInterface.kt`:
- Around line 415-429: The dropdown is comparing localized labels ("Default",
"AMOLED") to a stored enum name (PrefManager.externalDisplayTheme, e.g.,
"DEFAULT"), causing indexOf to return -1 and wrong persistence; change the
dropdown to be enum-backed: build a list of enum values (e.g., Theme.values())
or enum-name strings to use as the internal items for value/indexing and
persistence, and separately map those enum entries to localized display labels
for title/visible text; initialize selectedExternalDisplayTheme from
PrefManager.externalDisplayTheme (the enum name), use the enum-name list for
SettingsListDropdown.value/indexOf, and onItemSelected persist the selected
enum.name back to PrefManager.externalDisplayTheme while showing the
corresponding localized label.
🧹 Nitpick comments (2)
app/src/main/java/app/gamenative/externaldisplay/ExternalOnScreenKeyboardView.kt (1)
160-165: HardcodedColor.WHITEfor key text.Key text color is
Color.WHITEregardless of theme. This works for both current dark themes, but if a light theme is added later, this won't adapt. Consider adding atextColorproperty to theThemeenum for forward-compatibility.app/src/main/java/app/gamenative/externaldisplay/ExternalDisplayInputController.kt (1)
47-52:fromConfigrelies on raw string matching—consider using enum name.
fromConfigmatches"amoled"as a magic string. If a new theme is added, the developer must remember to update this function. UsingTheme.valueOf()(with a fallback) orentries.find { it.name.equals(value, ignoreCase = true) }would auto-extend to new enum values.♻️ Suggested refactor
companion object { - fun fromConfig(value: String?): Theme = when (value?.lowercase()) { - "amoled" -> AMOLED - else -> DEFAULT - } + fun fromConfig(value: String?): Theme = + entries.find { it.name.equals(value, ignoreCase = true) } ?: DEFAULT }
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupInterface.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupInterface.kt:433">
P2: Dropdown selection state is no longer updated, so the displayed selection can remain stale after the user picks a theme.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="app/src/main/java/app/gamenative/externaldisplay/ExternalDisplayInputController.kt">
<violation number="1" location="app/src/main/java/app/gamenative/externaldisplay/ExternalDisplayInputController.kt:49">
P2: Theme.fromConfig no longer recognizes the previously stored "AMOLED" preference value, so existing users with that saved theme will fall back to DEFAULT after the rename.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
in effort to reduce complexity, could we just revise grey default to be black for amoled instead of adding as a secondary option? |
MayRedBeWithYou
force-pushed
the
feature/external-display-input-themes
branch
from
d62c7d1 to
07a0187
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/PrefManager.kt (1)
959-961: Persist a canonical theme value in the setter.Line 961 currently stores raw input; normalizing to a canonical enum name avoids invalid/stale values being persisted and silently mapped back to default later.
♻️ Suggested change
var externalDisplayTheme: String get() = getPref(EXTERNAL_DISPLAY_THEME, ExternalDisplayInputController.Theme.DEFAULT.name) - set(value) = setPref(EXTERNAL_DISPLAY_THEME, value) + set(value) = setPref( + EXTERNAL_DISPLAY_THEME, + ExternalDisplayInputController.Theme.fromConfig(value).name, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/PrefManager.kt` around lines 959 - 961, The setter for externalDisplayTheme currently stores the raw input string which can persist invalid/stale values; change the setter to normalize the input to a canonical enum name from ExternalDisplayInputController.Theme (e.g., resolve the incoming value to the matching Theme entry—using valueOf or a case-insensitive lookup—and fall back to ExternalDisplayInputController.Theme.DEFAULT.name if it doesn't match) before calling setPref(EXTERNAL_DISPLAY_THEME, ...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/app/gamenative/PrefManager.kt`:
- Around line 959-961: The setter for externalDisplayTheme currently stores the
raw input string which can persist invalid/stale values; change the setter to
normalize the input to a canonical enum name from
ExternalDisplayInputController.Theme (e.g., resolve the incoming value to the
matching Theme entry—using valueOf or a case-insensitive lookup—and fall back to
ExternalDisplayInputController.Theme.DEFAULT.name if it doesn't match) before
calling setPref(EXTERNAL_DISPLAY_THEME, ...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 41efaf38-9770-4531-a903-d8a4034d6920
📒 Files selected for processing (6)
app/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/externaldisplay/ExternalDisplayInputController.ktapp/src/main/java/app/gamenative/externaldisplay/ExternalOnScreenKeyboardView.ktapp/src/main/java/app/gamenative/externaldisplay/SwapInputOverlayView.ktapp/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupInterface.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/app/gamenative/externaldisplay/SwapInputOverlayView.kt
- app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupInterface.kt
|
Closing this one in favour of #838 |
2 participants
I recently posted a #feature-request regarding adding AMOLED theme for External Display Input mode. I decided to give it a shot - let me know what you think.
Summary by cubic
Adds theme support to External Display Input with a new Black theme for true blacks and reduced glare. Themes apply across touchpad, keyboard, hybrid modes, and overlays, with consistent key label and icon colors.
New Features
PrefManager.Bug Fixes
Written for commit 07a0187. Summary will update on new commits.
Summary by CodeRabbit
New Features
Settings
UI