Dual screen support #402
Dual screen support #402
Conversation
This commit refactors the external display input handling. It removes the `ExternalKeyboardView` and replaces it with a new `ExternalOnScreenKeyboardView` for a cleaner implementation. The `winHandler` and `inputControlsViewProvider` dependencies have been removed from the `ExternalDisplayInputController` and related classes. The "Hybrid" mode UI is updated to use a floating action button to toggle the on-screen keyboard visibility over the touchpad, instead of switching between two full-screen views.
# Conflicts: # app/src/main/res/values/strings.xml
📝 WalkthroughWalkthroughAdds external display input support: new controller and presentation for secondary displays, an on-screen keyboard view, UI options to select mode, preference and container persistence for externalDisplayMode, and XServerScreen integration and lifecycle handling. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as ContainerConfigDialog
participant Screen as XServerScreen
participant Controller as ExternalDisplayInputController
participant DisplayMgr as Android DisplayManager
participant XServer as XServer
participant Touchpad as TouchpadView
UI->>Screen: user selects externalDisplayMode
Screen->>Controller: setMode(mode)
Controller->>DisplayMgr: get displays / register DisplayListener
Note right of Controller `#DFF7DF`: find external presentation display\ncreate ExternalInputPresentation
Controller->>DisplayMgr: show presentation on external display
Controller->>XServer: bind input targets (touch/keyboard)
Controller->>Touchpad: request TouchpadView via provider
Touchpad->>XServer: deliver touch events
DisplayMgr-->>Controller: display removed/changed (listener)
Controller->>Controller: dismiss or recreate presentation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-25T10:56:46.337ZApplied to files:
🔇 Additional comments (7)
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}
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)
app/src/main/java/com/winlator/container/ContainerData.kt (1)
75-84: AlignexternalDisplayModedefaults across property and Saver.restore
externalDisplayModedefaults to"hybrid"in the data class, butSaver.restorefalls back to"touchpad"when the key is missing:val externalDisplayMode: String = "hybrid", ... externalDisplayMode = (savedMap["externalDisplayMode"] as? String) ?: "touchpad",Given
PrefManager.externalDisplayInputModealso defaults to"hybrid", this creates two effective defaults depending on the code path (freshContainerData()vs restored state / older saved maps). If"hybrid"is meant to be the canonical default, consider using it inrestoreas well, or add a brief comment explaining why"touchpad"is preferred here.Also applies to: 130-138, 183-191
🤖 Fix all issues with AI agents
In @app/src/main/java/com/winlator/container/Container.java:
- Around line 952-959: The getter getExternalDisplayMode, the setter
setExternalDisplayMode, and the externalDisplayMode field initializer use
inconsistent fallback values ("hybrid" vs "touchpad"); pick a single canonical
default (prefer "hybrid" to match the existing field initializer) and update
setExternalDisplayMode to use that default when null is passed, ensuring
getExternalDisplayMode, setExternalDisplayMode, and the externalDisplayMode
field all agree on the same fallback value.
🧹 Nitpick comments (3)
app/src/main/java/app/gamenative/PrefManager.kt (1)
426-430: External display input pref looks correct; consider centralizing allowed valuesThe key name and
"hybrid"default fit existing prefs and the documentedoff|touchpad|keyboard|hybriddomain. To reduce drift with other callers (e.g.,ContainerData.externalDisplayModeandExternalDisplayInputController.fromConfig), consider centralizing the canonical string values in one place (or mapping via the controller’s enum) instead of repeating literals.app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (1)
63-63: ExternalDisplayInputController lifecycle wiring is sound; consider making the no-op callback explicitThe controller is correctly scoped to the
FrameLayoutlifecycle: it’s created with the rightcontext/xServer, starts immediately, and is stopped via an attach-state listener on view detach, which should ensure the display listener is unregistered.The only nit is the empty
onViewAttachedToWindowoverride:override fun onViewAttachedToWindow(v: View) {}Static analysis flags this as an empty function block. If you don’t need any attach-time behavior, consider making the no-op explicit (e.g., adding a
// no-opcomment or using= Unit) so the intent is clearer and the rule can be suppressed or configured accordingly.Also applies to: 815-829
app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.kt (1)
163-168: External display mode dropdown wiring is correct; consider sharing config-value mappingThe UI/state wiring for external display modes looks consistent:
- Display labels come from
externalDisplayModes(off, touchpad, keyboard, hybrid).externalDisplayModeIndexis initialized fromconfig.externalDisplayMode.lowercase()with a sensible fallback to 0 (“off”) for unknown values.- The
SettingsListDropdownwrites back toconfig.externalDisplayModeusing the same index→string mapping.Functionally this should behave as intended. To reduce duplication and the chance of typos drifting, you might introduce a single parallel list of config values, e.g.:
val externalDisplayConfigValues = listOf("off", "touchpad", "keyboard", "hybrid") val externalDisplayModes = externalDisplayConfigValues.map { /* map to stringResource label */ }Then:
- Initialize
externalDisplayModeIndexviaexternalDisplayConfigValues.indexOf(config.externalDisplayMode.lowercase()).coerceAtLeast(0).- On selection, write back
externalDisplayMode = externalDisplayConfigValues[index].That keeps the mapping in one place while preserving current behavior.
Also applies to: 687-695, 1722-1740
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
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/ui/component/dialog/ContainerConfigDialog.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/ContainerUtils.ktapp/src/main/java/com/winlator/container/Container.javaapp/src/main/java/com/winlator/container/ContainerData.ktapp/src/main/res/values/strings.xml
🧰 Additional context used
🧬 Code graph analysis (3)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (1)
app/src/main/java/app/gamenative/externaldisplay/ExternalDisplayInputController.kt (2)
setMode(76-79)start(63-66)
app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.kt (2)
app/src/main/java/app/gamenative/ui/component/settings/SettingsListDropdown.kt (1)
SettingsListDropdown(40-125)app/src/main/java/app/gamenative/ui/theme/Color.kt (1)
settingsTileColors(27-32)
app/src/main/java/app/gamenative/externaldisplay/ExternalDisplayInputController.kt (1)
app/src/main/java/com/winlator/widget/TouchpadView.java (1)
TouchpadView(28-728)
🪛 detekt (1.23.8)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
[warning] 824-824: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
🔇 Additional comments (15)
app/src/main/res/values/strings.xml (1)
524-529: LGTM!The new string resources are well-structured and follow the existing naming conventions. The descriptions are clear and user-friendly.
app/src/main/java/com/winlator/container/Container.java (2)
115-116: LGTM!The new field follows the existing pattern in the class and the default value of "hybrid" aligns with the feature design.
651-651: LGTM!The persistence of
externalDisplayModefollows the established pattern for other container fields.app/src/main/java/app/gamenative/externaldisplay/ExternalOnScreenKeyboardView.kt (4)
17-50: LGTM!Well-structured Kotlin class with clean separation of concerns. The use of data classes for
KeySpecandKeyButton, along with theShiftStateenum, makes the code readable and maintainable.
52-134: LGTM!The keyboard layout implementation is well-structured with appropriate key weights for a balanced visual appearance. The five-row layout covers standard QWERTY keys plus modifiers and navigation arrows.
136-196: LGTM!The key handling logic is well-implemented. The shift state management correctly handles the three states (OFF, ON, CAPS) with appropriate behavior for letters vs. symbols.
236-241: LGTM!The
tapKeymethod correctly handles shift key injection around the actual key press/release sequence. The implementation properly injects shift press before the key and releases it after.app/src/main/java/app/gamenative/utils/ContainerUtils.kt (3)
124-124: LGTM!The external display mode is correctly sourced from
PrefManager.externalDisplayInputModefollowing the established pattern.
162-162: LGTM!The persistence of
externalDisplayModeto preferences follows the established pattern.
227-227: LGTM!The external display mode is correctly wired through all three data flow points: reading from container, including in ContainerData construction, and applying back to container.
Also applies to: 270-270, 391-391
app/src/main/java/app/gamenative/externaldisplay/ExternalDisplayInputController.kt (5)
24-38: LGTM!Clean class structure with well-defined
Modeenum. ThefromConfigcompanion function provides a safe default (OFF) for unknown or null values.
63-79: LGTM!The lifecycle management is well-implemented. The
start()method correctly registers the display listener on the main looper for thread safety, andstop()properly cleans up resources. The silent exception catch during unregistration is acceptable for cleanup operations.
81-121: LGTM!The presentation management logic correctly handles mode changes and display availability. The
findPresentationDisplay()appropriately filters for presentation-capable displays while excluding the default and hidden displays.
123-145: LGTM!The presentation correctly sets
FLAG_NOT_FOCUSABLEandFLAG_NOT_TOUCH_MODALwindow flags to ensure the external display doesn't steal focus from the main window. This is essential for the feature's usability as noted in the PR objectives.
210-276: LGTM!The
HybridInputLayoutimplementation is well-designed. The floating action button toggle with dynamic positioning based on keyboard visibility provides a good UX. The keyboard starting hidden (line 233) and the toggle button repositioning logic (lines 269-275) work together smoothly.
| // External display mode | ||
| public String getExternalDisplayMode() { | ||
| return externalDisplayMode != null ? externalDisplayMode : "hybrid"; | ||
| } | ||
|
|
||
| public void setExternalDisplayMode(String externalDisplayMode) { | ||
| this.externalDisplayMode = externalDisplayMode != null ? externalDisplayMode : "touchpad"; | ||
| } |
There was a problem hiding this comment.
Inconsistent fallback values for externalDisplayMode.
The getter returns "hybrid" as fallback (line 954), but the setter defaults to "touchpad" when null is passed (line 958). The field initializer also uses "hybrid" (line 116). This inconsistency could lead to unexpected behavior.
🔧 Proposed fix
public void setExternalDisplayMode(String externalDisplayMode) {
- this.externalDisplayMode = externalDisplayMode != null ? externalDisplayMode : "touchpad";
+ this.externalDisplayMode = externalDisplayMode != null ? externalDisplayMode : "hybrid";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // External display mode | |
| public String getExternalDisplayMode() { | |
| return externalDisplayMode != null ? externalDisplayMode : "hybrid"; | |
| } | |
| public void setExternalDisplayMode(String externalDisplayMode) { | |
| this.externalDisplayMode = externalDisplayMode != null ? externalDisplayMode : "touchpad"; | |
| } | |
| // External display mode | |
| public String getExternalDisplayMode() { | |
| return externalDisplayMode != null ? externalDisplayMode : "hybrid"; | |
| } | |
| public void setExternalDisplayMode(String externalDisplayMode) { | |
| this.externalDisplayMode = externalDisplayMode != null ? externalDisplayMode : "hybrid"; | |
| } |
🤖 Prompt for AI Agents
In @app/src/main/java/com/winlator/container/Container.java around lines 952 -
959, The getter getExternalDisplayMode, the setter setExternalDisplayMode, and
the externalDisplayMode field initializer use inconsistent fallback values
("hybrid" vs "touchpad"); pick a single canonical default (prefer "hybrid" to
match the existing field initializer) and update setExternalDisplayMode to use
that default when null is passed, ensuring getExternalDisplayMode,
setExternalDisplayMode, and the externalDisplayMode field all agree on the same
fallback value.
|
Cool, any device was tested to work? |
Ayn Thor! |
| // Touchscreen mode | ||
| private boolean touchscreenMode = false; | ||
| // External display input handling | ||
| private String externalDisplayMode = "hybrid"; |
There was a problem hiding this comment.
I'd recommend creating a CONST for this that can be passed around so we're not using a hard-coded string everywhere for the different displayInput modes.
There was a problem hiding this comment.
Correct. Also it shouldn't be hybrid by default right?
| "touchpad" -> 1 | ||
| "keyboard" -> 2 | ||
| "hybrid" -> 3 |
| private const val EXTERNAL_TOUCHPAD_BG: Int = 0xFF2B2B2B.toInt() | ||
| private const val EXTERNAL_KEYBOARD_BG: Int = 0xFF2B2B2B.toInt() |
There was a problem hiding this comment.
Might be good for you to check if we have a theme/colors file and bring it in from there rather than hardcode the hexvalue
There was a problem hiding this comment.
Correct, we do have colors constants
|
Overall looks good, just added a few QoL changes for maintainability. Will do a final test with my Thor tonight |
|
Tested on the Thor, looks and works great. I think we can iterate and adjust it as we go based on any community feedback. Thanks for the contribution! |
| val externalDisplayController = ExternalDisplayInputController( | ||
| context = context, | ||
| xServer = xServerView.getxServer(), | ||
| touchpadViewProvider = { PluviaApp.touchpadView }, | ||
| ).apply { | ||
| setMode(ExternalDisplayInputController.fromConfig(container.externalDisplayMode)) | ||
| start() | ||
| } | ||
| frameLayout.addOnAttachStateChangeListener(object : View.OnAttachStateChangeListener { | ||
| override fun onViewAttachedToWindow(v: View) {} | ||
|
|
||
| override fun onViewDetachedFromWindow(v: View) { | ||
| externalDisplayController.stop() | ||
| } | ||
| }) |
There was a problem hiding this comment.
Should this be in an if statement? We shouldn't do this if there's only a single screen, right?
4 participants
Added Support to use the external screen as a Touchpad, Keyboard, or Touchpad+Keyboard
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.