feat: Improve pointer exclusivity handling#1152
Conversation
📝 WalkthroughWalkthroughExtracts pointer-capture logic into a new InputCaptureManager, integrates it with TouchpadView lifecycle, and updates XServerScreen to call the manager for refresh/disable/toggle actions and a new Ctrl+Shift+Alt+Z toggle; removes motion-triggered recapture. ChangesInput capture refactor & UI integration
Sequence Diagram(s)sequenceDiagram
participant User
participant XServerScreen
participant TouchpadView
participant InputCaptureManager
participant AndroidOS
User->>XServerScreen: Press Ctrl+Shift+Alt+Z (ACTION_UP)
XServerScreen->>TouchpadView: getInputCaptureManager()
TouchpadView->>InputCaptureManager: togglePointerCapture()
InputCaptureManager->>AndroidOS: requestPointerCapture() / releasePointerCapture()
AndroidOS-->>InputCaptureManager: capture state updated
User->>XServerScreen: Show Quick Menu
XServerScreen->>TouchpadView: inputCaptureManager.disablePointerCapture()
InputCaptureManager->>AndroidOS: releasePointerCapture()
AndroidOS-->>InputCaptureManager: released
User->>XServerScreen: Dismiss Quick Menu
XServerScreen->>TouchpadView: post { inputCaptureManager.refreshPointerCapture() }
InputCaptureManager->>AndroidOS: requestPointerCapture() (if needed)
AndroidOS-->>InputCaptureManager: capture state updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: Turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![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 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/com/winlator/widget/TouchpadView.java">
<violation number="1" location="app/src/main/java/com/winlator/widget/TouchpadView.java:180">
P2: Focus-loss release clears the request flag, preventing automatic pointer recapture when window focus returns.</violation>
</file>
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 1125-1128: The current branch sends physical-device events to
Keyboard.onKeyEvent which loses modifier/shifted-character handling; change the
routing so both virtual and physical devices call Keyboard.onVirtualKeyEvent
until onKeyEvent is feature-parity-ready. In the block that checks
it.event.device?.isVirtual and sets handled via keyboard?.onVirtualKeyEvent(...)
or keyboard?.onKeyEvent(...), remove the else branch and invoke
keyboard?.onVirtualKeyEvent(it.event) for all cases (preserving the handled
assignment) so physical keyboard input continues to include modifier/keysym
data.
- Around line 1101-1108: The special-key chord handling leaks the Z key event
and pointer recapture; update the chord interception to consume both ACTION_DOWN
and ACTION_UP for KeyEvent.KEYCODE_Z (not only ACTION_UP) so the guest never
sees the unmatched press/release, and set handled=true for both. Also change the
direct pointer-capture call in tryCapturePointer() (and any direct
requestPointerCapture() usages) to route through the TouchpadView helper APIs
(e.g., PluviaApp.touchpadView?.togglePointerExclusive() / a new
touchpadView.requestPointerCaptureHelper()) so pointer capture is
granted/revoked only via TouchpadView and cannot immediately reacquire after the
user toggles it off. Ensure you update the matching event branch names
(areAllSpecialKeysUp / chord logic) to reflect handling both actions.
In `@app/src/main/java/com/winlator/widget/TouchpadView.java`:
- Around line 173-180: The current releasePointerExclusive() method
unconditionally clears pointerCaptureRequested, which conflates a transient
suspension with a permanent opt-out and prevents determinePointerExclusivity()
from restoring capture after temporary releases; change the logic by introducing
a new suspendPointerExclusive() method for transient cases (used by
quick-menu/focus-loss paths) that calls releasePointerCapture() but does NOT set
pointerCaptureRequested = false, and reserve releasePointerExclusive() to
perform the permanent opt-out by releasing capture and setting
pointerCaptureRequested = false so determinePointerExclusivity() can correctly
recapture after suspensions.
🪄 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: 5232eb80-b0a3-4f87-b455-f34aa310b9e9
📒 Files selected for processing (2)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/com/winlator/widget/TouchpadView.java
Unnvaldr
force-pushed
the
feat-improve-pointer-exclusivity-handling
branch
2 times, most recently
from
9d4bb3e to
f7573a7
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/src/main/java/com/winlator/inputcontrols/InputCaptureManager.java (1)
45-55:⚠️ Potential issue | 🟠 MajorDon't clear
pointerCaptureRequestedduring transient refreshes.
refreshPointerCapture()is now the restore path used after quick-menu dismissal and focus regain. ClearingpointerCaptureRequestedon the!shouldCapture && hasCapturebranch turns a temporary release into a permanent opt-out, so the laterrefreshPointerCapture()call has nothing left to restore.Suggested fix
public void refreshPointerCapture() { boolean shouldCapture = targetView.hasFocus() && pointerCaptureRequested; boolean hasCapture = targetView.hasPointerCapture(); if (shouldCapture && !hasCapture) { enablePointerCapture(); - setPointerCaptureRequested(true); } else if (!shouldCapture && hasCapture) { disablePointerCapture(); - setPointerCaptureRequested(false); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/inputcontrols/InputCaptureManager.java` around lines 45 - 55, refreshPointerCapture currently clears the persistent flag pointerCaptureRequested by calling setPointerCaptureRequested(false) when temporarily releasing capture, which turns transient releases into permanent opt-outs; change refreshPointerCapture so the branch that calls disablePointerCapture() does NOT clear pointerCaptureRequested (remove or guard the setPointerCaptureRequested(false) call) so that transient refreshes can be restored later by subsequent refreshPointerCapture calls; leave enablePointerCapture(), setPointerCaptureRequested(true) behavior unchanged.app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (1)
1225-1254:⚠️ Potential issue | 🟠 MajorConsume
Ctrl+Shift+Alt+Zon both key-down and key-up.Right now only the
ACTION_UPhalf of the chord is intercepted. TheACTION_DOWNstill falls through to the guest keyboard handler, so games see a strayZpress whenever the user toggles pointer capture.Suggested fix
- val areAllSpecialKeysUp = it.event.isCtrlPressed and it.event.isShiftPressed and it.event.isAltPressed && it.event.action == KeyEvent.ACTION_UP - if (areAllSpecialKeysUp) { - // Handing special key combination - when (it.event.keyCode) { - KeyEvent.KEYCODE_Z -> { - // Toggles pointer exclusivity when in game view - PluviaApp.touchpadView?.inputCaptureManager?.togglePointerCapture() - handled = true - } - else -> { - handled = false - } - } + val isPointerCaptureChord = + it.event.keyCode == KeyEvent.KEYCODE_Z && + it.event.isCtrlPressed && + it.event.isShiftPressed && + it.event.isAltPressed + if (isPointerCaptureChord) { + if (it.event.action == KeyEvent.ACTION_DOWN && it.event.repeatCount == 0) { + PluviaApp.touchpadView?.inputCaptureManager?.togglePointerCapture() + } + handled = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt` around lines 1225 - 1254, The code only detects the Ctrl+Shift+Alt+Z chord on ACTION_UP (variable areAllSpecialKeysUp), letting the ACTION_DOWN fall through to keyboard handlers; modify the chord detection logic to detect the same chord on both ACTION_DOWN and ACTION_UP (or check event.action == KeyEvent.ACTION_DOWN || event.action == KeyEvent.ACTION_UP) and when the chord is matched call PluviaApp.touchpadView?.inputCaptureManager?.togglePointerCapture() and set handled = true for both actions so the guest handlers (keyboard?.onKeyEvent / keyboard?.onVirtualKeyEvent) never receive the stray 'Z' press.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 1225-1254: The code only detects the Ctrl+Shift+Alt+Z chord on
ACTION_UP (variable areAllSpecialKeysUp), letting the ACTION_DOWN fall through
to keyboard handlers; modify the chord detection logic to detect the same chord
on both ACTION_DOWN and ACTION_UP (or check event.action == KeyEvent.ACTION_DOWN
|| event.action == KeyEvent.ACTION_UP) and when the chord is matched call
PluviaApp.touchpadView?.inputCaptureManager?.togglePointerCapture() and set
handled = true for both actions so the guest handlers (keyboard?.onKeyEvent /
keyboard?.onVirtualKeyEvent) never receive the stray 'Z' press.
In `@app/src/main/java/com/winlator/inputcontrols/InputCaptureManager.java`:
- Around line 45-55: refreshPointerCapture currently clears the persistent flag
pointerCaptureRequested by calling setPointerCaptureRequested(false) when
temporarily releasing capture, which turns transient releases into permanent
opt-outs; change refreshPointerCapture so the branch that calls
disablePointerCapture() does NOT clear pointerCaptureRequested (remove or guard
the setPointerCaptureRequested(false) call) so that transient refreshes can be
restored later by subsequent refreshPointerCapture calls; leave
enablePointerCapture(), setPointerCaptureRequested(true) behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1aab3f4-7684-4c3f-bc3a-374aaadcf021
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/com/winlator/inputcontrols/InputCaptureManager.javaapp/src/main/java/com/winlator/widget/TouchpadView.java
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt:1225">
P2: Ctrl+Shift+Alt+Z shortcut is consumed only on key-up, allowing key-down to propagate and potentially leaving guest input in an inconsistent state.</violation>
</file>
<file name="app/src/main/java/com/winlator/inputcontrols/InputCaptureManager.java">
<violation number="1" location="app/src/main/java/com/winlator/inputcontrols/InputCaptureManager.java:85">
P2: Inverted pointer-capture skip condition causes inconsistent state handling and redundant capture requests.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
I don't fully understand what we are fixing here, and the changes look quite extensive in critical parts of the app. What's going on? |
I do not agree that it is critical section, worst case scenario is that the pointer capture does not work. Other than that, basic functionality is preserved. There was already a quite extensive logic split between TouchpadView (a.k.a. mouse and touchpad) and XServerScreen, new manager reduces this complexity a little bit. Other stuff addressed:
|
|
Thanks - but what issue does this fix? Can you elaborate or share a video? If there's no actual issue being fixed then this is not worth merging - the benefits are outweighed by the potential of breaking something major |
I have just listed these problems above, what exactly needs an elaboration? Current support for multi-window/display setups is not were it should be, hence some of the fixes and features were made. Not counting that the whole input handling pipeline is a mess ATM.
For any DeX/Android 16+ user it would be quite an improvement. Is not Release Preview build exactly for the purpose to prevent such regressions, before it reaches the wider audience? |
Extract pointer capture logic to a separate manager, introduce state based pointer capture, recapture on overlay menu dismissal
Unnvaldr
force-pushed
the
feat-improve-pointer-exclusivity-handling
branch
from
f7573a7 to
fabad00
Compare
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/app/gamenative/ui/screen/xserver/XServerScreen.kt (1)
947-963:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip touchpad refocus when the quick menu was closed to open the keyboard.
This path always calls
requestFocus()+refreshPointerCapture(), even whenQuickMenuAction.KEYBOARDtriggered the dismiss. That can immediately pull focus back off the IME target, so the menu closes but typing still doesn’t work.Suggested fix
- val resumeImmediatelyForKeyboard = keyboardRequestedFromOverlay && manualResumeMode - keyboardRequestedFromOverlay = false + val openedKeyboard = keyboardRequestedFromOverlay + val resumeImmediatelyForKeyboard = openedKeyboard && manualResumeMode + keyboardRequestedFromOverlay = false if (!keepPausedForEditor) { if (resumeImmediatelyForKeyboard) { forceResumeIfSuspended() @@ - PluviaApp.touchpadView?.postDelayed({ - val view = PluviaApp.touchpadView ?: return@postDelayed - view.requestFocus() - view.inputCaptureManager.refreshPointerCapture() - }, 100) + if (!openedKeyboard) { + val touchpadView = PluviaApp.touchpadView + touchpadView?.postDelayed({ + if (PluviaApp.touchpadView !== touchpadView || !touchpadView.isAttachedToWindow) return@postDelayed + touchpadView.requestFocus() + touchpadView.inputCaptureManager.refreshPointerCapture() + }, 100) + } showQuickMenu = false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt` around lines 947 - 963, Capture the current keyboardRequestedFromOverlay into a local val before it is reset and use that flag to skip the touchpad refocus in the delayed runnable; specifically, store val keyboardRequested = keyboardRequestedFromOverlay before setting keyboardRequestedFromOverlay = false, keep resumeImmediatelyForKeyboard as computed, and inside the PluviaApp.touchpadView?.postDelayed runnable return early (skip requestFocus() and refreshPointerCapture()) if keyboardRequested is true so closing the quick menu to open the IME won’t steal focus.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 841-844: The delayed Runnable must re-check pointer-capture
eligibility before calling refreshPointerCapture(): inside the postDelayed block
(where PluviaApp.touchpadView is captured and
view.inputCaptureManager.refreshPointerCapture() is invoked) call the same
eligibility check used by tryCapturePointer()—e.g., verify
showQuickMenu/editor/touchscreen state or invoke a boolean helper (or
tryCapturePointer() if it returns a pure eligibility result) and return early if
capture is no longer allowed (to avoid reacquiring capture after
gameBack()/edit-mode changes).
---
Outside diff comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 947-963: Capture the current keyboardRequestedFromOverlay into a
local val before it is reset and use that flag to skip the touchpad refocus in
the delayed runnable; specifically, store val keyboardRequested =
keyboardRequestedFromOverlay before setting keyboardRequestedFromOverlay =
false, keep resumeImmediatelyForKeyboard as computed, and inside the
PluviaApp.touchpadView?.postDelayed runnable return early (skip requestFocus()
and refreshPointerCapture()) if keyboardRequested is true so closing the quick
menu to open the IME won’t steal focus.
🪄 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: 39748ca6-3c63-49b8-a627-137dc3f8742c
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/com/winlator/inputcontrols/InputCaptureManager.javaapp/src/main/java/com/winlator/widget/TouchpadView.java
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/winlator/inputcontrols/InputCaptureManager.java
| PluviaApp.touchpadView?.postDelayed({ | ||
| val view = PluviaApp.touchpadView | ||
| if (view != null) { | ||
| view.requestFocus() | ||
| view.requestPointerCapture() | ||
| } | ||
| val view = PluviaApp.touchpadView ?: return@postDelayed | ||
| view.inputCaptureManager.refreshPointerCapture() | ||
| }, 100) |
There was a problem hiding this comment.
Re-check capture eligibility inside the delayed refresh.
tryCapturePointer() validates showQuickMenu/editor/touchscreen state before posting, but those flags can change during the 100ms delay. A device event that races with gameBack() or edit-mode entry can still reacquire pointer capture over the overlay.
Suggested fix
- PluviaApp.touchpadView?.postDelayed({
- val view = PluviaApp.touchpadView ?: return@postDelayed
- view.inputCaptureManager.refreshPointerCapture()
- }, 100)
+ val touchpadView = PluviaApp.touchpadView
+ touchpadView?.postDelayed({
+ if (PluviaApp.touchpadView !== touchpadView || !touchpadView.isAttachedToWindow) return@postDelayed
+ if ((hasPhysicalMouse || hasInternalTouchpad) &&
+ !showElementEditor && !keepPausedForEditor && !showQuickMenu && !isEditMode &&
+ !container.isTouchscreenMode
+ ) {
+ touchpadView.inputCaptureManager.refreshPointerCapture()
+ }
+ }, 100)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt` around
lines 841 - 844, The delayed Runnable must re-check pointer-capture eligibility
before calling refreshPointerCapture(): inside the postDelayed block (where
PluviaApp.touchpadView is captured and
view.inputCaptureManager.refreshPointerCapture() is invoked) call the same
eligibility check used by tryCapturePointer()—e.g., verify
showQuickMenu/editor/touchscreen state or invoke a boolean helper (or
tryCapturePointer() if it returns a pure eligibility result) and return early if
capture is no longer allowed (to avoid reacquiring capture after
gameBack()/edit-mode changes).
Because it's not clear to me what's being fixed - I'd like to see what kind of multi-window/display setups you mean, what the problem is and what this fix does.
Perhaps it's not critical to you, but I will have to be the one doing a hotfix if pointer capture is broken. Please do provide a recording as you mentioned you will at the top, this issue has not been discussed so I need to understand it better before I can review and merge. Hope that makes sense. |
3 participants
Description
Current implementation of the pointer capture does not take into account, that implicit requesting on every external mouse event is not desireable, since it locks your mouse to the GameNative's window on any type of a mouse event, not just mouse presses.
I have rebuilt it in a way, so that the exclusivity is requested on window focus regain (which can be reached through many means, but most importantly, explicit mouse click on the window), but it only happens if the exclusivity was lost through other actions than usage of special key combination (see below).
Also clean-up of this subsystem was moved to attached/detached lifecycle events.
Additionaly, the user is now able to request/release exclusivity via special key combination (Ctrl + Shift + Alt + , in this case it was binded to Z key). Especially helpful in the multi-window mode.
Recording
I will create a showcase in the oncoming days.
Checklist
#code-changes, I have discussed this change there and it has been green-lighted. If I do not have access, I have still provided clear context in this PR. If I skip both, I accept that this change may face delays in review, may not be reviewed at all, or may be closed.CONTRIBUTING.md.Summary by cubic
Make pointer exclusivity predictable and user-controlled.
InputCaptureManagernow owns state and lifecycle, replaces direct pointer capture calls, and adds a Ctrl+Shift+Alt+Z toggle; quick menu releases capture and recaptures on close with focus restored.New Features
Bug Fixes
InputCaptureManagerfor state-based control and focus handling; replaced direct request/release calls with refresh/enable/disable.Written for commit fabad00. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes