Fix: Robust External Mouse/Trackpad Handling & Coordinates Overflow#717
Fix: Robust External Mouse/Trackpad Handling & Coordinates Overflow#717CrisCW1990 wants to merge 6 commits intoutkarshdalal:masterfrom
Conversation
…1 pointer overflow This commit addresses several critical input bugs related to trackpads, external mice, and captured pointer behavior in TouchpadView.java. These changes significantly improve aim stability in games, prevent rapid clicking issues, and protect XServer from variables overflowing.
…1 pointer overflow This commit addresses several critical input bugs related to trackpads, external mice, and captured pointer behavior in TouchpadView.java. These changes significantly improve aim stability in games, prevent rapid clicking issues, and protect XServer from variables overflowing.
# Conflicts: # app/src/main/java/com/winlator/widget/TouchpadView.java
📝 WalkthroughWalkthroughEnhanced TouchpadView's pointer-capture and input handling with mechanisms to track fractional movement remainders, manage pending button releases, and introduce a delayed-release safety mechanism. Added onPointerCaptureChange callback and external button mapping to unify external mouse event handling while preserving relative-axis preferences. Changes
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 unit tests (beta)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues 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:1358">
P1: Captured-button inference is recomputed on release, so finger-count changes can release a different mouse button than was pressed.</violation>
<violation number="2" location="app/src/main/java/com/winlator/widget/TouchpadView.java:1383">
P2: Captured-pointer button handling converts unmapped mouse buttons to left-click, causing unintended primary clicks for unsupported button types.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // If the trackpad physical click is pressed, but it doesn't specify secondary/tertiary, | ||
| // we can infer it from the number of fingers resting on the trackpad. | ||
| if (actionButton == MotionEvent.BUTTON_PRIMARY || actionButton == 0) { | ||
| int pointerCount = event.getPointerCount(); |
There was a problem hiding this comment.
P1: Captured-button inference is recomputed on release, so finger-count changes can release a different mouse button than was pressed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/com/winlator/widget/TouchpadView.java, line 1358:
<comment>Captured-button inference is recomputed on release, so finger-count changes can release a different mouse button than was pressed.</comment>
<file context>
@@ -1241,25 +1264,150 @@ public float[] computeDeltaPoint(float lastX, float lastY, float x, float y) {
+ // If the trackpad physical click is pressed, but it doesn't specify secondary/tertiary,
+ // we can infer it from the number of fingers resting on the trackpad.
+ if (actionButton == MotionEvent.BUTTON_PRIMARY || actionButton == 0) {
+ int pointerCount = event.getPointerCount();
+ if (pointerCount == 2) actionButton = MotionEvent.BUTTON_SECONDARY;
+ else if (pointerCount == 3) actionButton = MotionEvent.BUTTON_TERTIARY;
</file context>
| else if (actionButton == MotionEvent.BUTTON_TERTIARY) btn = Pointer.Button.BUTTON_MIDDLE; | ||
|
|
||
| // Ensure we always have a default button if the actionButton was not explicitly set (e.g. raw finger tap) | ||
| if (btn == null) btn = Pointer.Button.BUTTON_LEFT; |
There was a problem hiding this comment.
P2: Captured-pointer button handling converts unmapped mouse buttons to left-click, causing unintended primary clicks for unsupported button types.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/com/winlator/widget/TouchpadView.java, line 1383:
<comment>Captured-pointer button handling converts unmapped mouse buttons to left-click, causing unintended primary clicks for unsupported button types.</comment>
<file context>
@@ -1241,25 +1264,150 @@ public float[] computeDeltaPoint(float lastX, float lastY, float x, float y) {
+ else if (actionButton == MotionEvent.BUTTON_TERTIARY) btn = Pointer.Button.BUTTON_MIDDLE;
+
+ // Ensure we always have a default button if the actionButton was not explicitly set (e.g. raw finger tap)
+ if (btn == null) btn = Pointer.Button.BUTTON_LEFT;
+
+ if (pressed) {
</file context>
| if (btn == null) btn = Pointer.Button.BUTTON_LEFT; | |
| if (btn == null) return; |
![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
🤖 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/com/winlator/widget/TouchpadView.java`:
- Around line 1355-1369: The inference logic using event.getPointerCount()
should be done only for the press and the inferred button reused on release to
avoid mismatches; add a field (e.g., lastInferredActionButton) to TouchpadView,
when actionButton is MotionEvent.BUTTON_PRIMARY or 0 and you infer
secondary/tertiary in the ACTION_BUTTON_PRESS branch set
lastInferredActionButton to the inferred value and call
handleExternalMouseButton(lastInferredActionButton, true), and in the
ACTION_BUTTON_RELEASE branch use lastInferredActionButton (fall back to
actionButton if unset) when calling handleExternalMouseButton(..., false) and
then clear/reset lastInferredActionButton to avoid stuck state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4e7136f-3704-4d7c-800d-7e780082bc9e
📒 Files selected for processing (2)
app/src/main/java/com/winlator/widget/TouchpadView.javaapp/src/main/java/com/winlator/xserver/Pointer.java
| // If the trackpad physical click is pressed, but it doesn't specify secondary/tertiary, | ||
| // we can infer it from the number of fingers resting on the trackpad. | ||
| if (actionButton == MotionEvent.BUTTON_PRIMARY || actionButton == 0) { | ||
| int pointerCount = event.getPointerCount(); | ||
| if (pointerCount == 2) actionButton = MotionEvent.BUTTON_SECONDARY; | ||
| else if (pointerCount == 3) actionButton = MotionEvent.BUTTON_TERTIARY; | ||
| else actionButton = MotionEvent.BUTTON_PRIMARY; | ||
| } | ||
|
|
||
| if (action == MotionEvent.ACTION_BUTTON_PRESS) { | ||
| handleExternalMouseButton(actionButton, true); | ||
| return true; | ||
| } else if (action == MotionEvent.ACTION_BUTTON_RELEASE) { | ||
| handleExternalMouseButton(actionButton, false); | ||
| return true; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Android MotionEvent getActionButton() 0 ACTION_BUTTON_PRESS RELEASE touchpad trackpad
💡 Result:
MotionEvent.getActionButton() only has a defined meaning when the action is ACTION_BUTTON_PRESS or ACTION_BUTTON_RELEASE. For any other action (for example ACTION_DOWN/UP/MOVE that many touchpads generate for tap/gesture contact), the returned value is undefined—and commonly shows up as 0 (BUTTON_NONE). [1]
So if you’re seeing:
getActionButton() == 0- and your event is not
ACTION_BUTTON_PRESS/RELEASE
…that’s expected.
What to do instead
- Always branch on the masked action first
int a = event.getActionMasked();
if (a == MotionEvent.ACTION_BUTTON_PRESS || a == MotionEvent.ACTION_BUTTON_RELEASE) {
int changed = event.getActionButton(); // which button changed
int down = event.getButtonState(); // buttons currently held down
}getButtonState() is “buttons currently pressed”, so on a release it can legitimately be 0 because the button is no longer down. [2]
-
Detect whether it’s a touchpad vs mouse
Android explicitly recommends usinggetToolType(0)to distinguish touchpad events from mouse events. [1] -
If you want “tap/click” from a touchpad gesture
Many touchpads report a “tap” as touch contact (ACTION_DOWN/UP) withbuttonState == 0(no physical button). In that case, treat it like touch: handleACTION_DOWN/UPrather than expectingACTION_BUTTON_PRESS/RELEASE. (Android’s mouse input guidance focuses on button events for mice, but touchpads can generate touch-style events depending on device/driver.) [3]
Sources: Android MotionEvent reference (notes about getActionButton() being undefined except for button press/release; toolType guidance). [1] Stack Overflow explanation of why button state can be 0 on release. [2] Android mouse input guide example usage around button handling. [3]
🏁 Script executed:
# First, let me examine the TouchpadView.java file to understand the context
fd "TouchpadView.java" --type fRepository: utkarshdalal/GameNative
Length of output: 122
🏁 Script executed:
# Read the TouchpadView.java file around the problematic lines
wc -l app/src/main/java/com/winlator/widget/TouchpadView.javaRepository: utkarshdalal/GameNative
Length of output: 127
🏁 Script executed:
# Get the specific section mentioned in the review (lines 1340-1400 for context)
sed -n '1340,1400p' app/src/main/java/com/winlator/widget/TouchpadView.javaRepository: utkarshdalal/GameNative
Length of output: 2841
🏁 Script executed:
# Check what's at lines 1376-1384 mentioned in "Also applies to"
sed -n '1370,1390p' app/src/main/java/com/winlator/widget/TouchpadView.javaRepository: utkarshdalal/GameNative
Length of output: 1062
🏁 Script executed:
# Look for any related tests or documentation
fd -t f "TouchpadView\|touchpad\|trackpad" app/ | grep -i testRepository: utkarshdalal/GameNative
Length of output: 49
Button inference can mismatch on release and leave buttons stuck.
Lines 1357-1362 infer secondary/tertiary from pointerCount for both press and release. If finger count changes between down/up events, the release may target a different button than was pressed (e.g., right-click pressed with 2 fingers, then 1 finger lifts but physical click still held, release sees 1 finger and infers left-click), leaving the originally pressed button in stuck state.
🛠️ Suggested fix (track inferred button from press and reuse on release)
+ private Pointer.Button inferredCapturedButtonDown;
private boolean onExternalMouseCapturedButtonEvent(MotionEvent event) {
int action = event.getActionMasked();
int actionButton = event.getActionButton();
-
- if (actionButton == MotionEvent.BUTTON_PRIMARY || actionButton == 0) {
- int pointerCount = event.getPointerCount();
- if (pointerCount == 2) actionButton = MotionEvent.BUTTON_SECONDARY;
- else if (pointerCount == 3) actionButton = MotionEvent.BUTTON_TERTIARY;
- else actionButton = MotionEvent.BUTTON_PRIMARY;
- }
-
- if (action == MotionEvent.ACTION_BUTTON_PRESS) {
- handleExternalMouseButton(actionButton, true);
+
+ if (action == MotionEvent.ACTION_BUTTON_PRESS) {
+ if (actionButton == MotionEvent.BUTTON_PRIMARY || actionButton == 0) {
+ int pointerCount = event.getPointerCount();
+ if (pointerCount == 2) actionButton = MotionEvent.BUTTON_SECONDARY;
+ else if (pointerCount == 3) actionButton = MotionEvent.BUTTON_TERTIARY;
+ else actionButton = MotionEvent.BUTTON_PRIMARY;
+ }
+ Pointer.Button btn = externalToXButton(actionButton);
+ inferredCapturedButtonDown = (event.getActionButton() == 0) ? btn : null;
+ handleExternalMouseButton(btn, true);
return true;
} else if (action == MotionEvent.ACTION_BUTTON_RELEASE) {
- handleExternalMouseButton(actionButton, false);
+ Pointer.Button btn = (event.getActionButton() == 0 && inferredCapturedButtonDown != null)
+ ? inferredCapturedButtonDown
+ : externalToXButton(actionButton);
+ inferredCapturedButtonDown = null;
+ handleExternalMouseButton(btn, false);
return true;
} else if (action == MotionEvent.ACTION_SCROLL) {
return handleExternalMouseScroll(event);
}
return false;
}
- private void handleExternalMouseButton(int actionButton, boolean pressed) {
- Pointer.Button btn = null;
- if (actionButton == MotionEvent.BUTTON_PRIMARY) btn = Pointer.Button.BUTTON_LEFT;
- else if (actionButton == MotionEvent.BUTTON_SECONDARY) btn = Pointer.Button.BUTTON_RIGHT;
- else if (actionButton == MotionEvent.BUTTON_TERTIARY) btn = Pointer.Button.BUTTON_MIDDLE;
-
- if (btn == null) btn = Pointer.Button.BUTTON_LEFT;
+ private void handleExternalMouseButton(Pointer.Button btn, boolean pressed) {
+ if (btn == null) btn = Pointer.Button.BUTTON_LEFT;
if (pressed) {
cancelPendingRelease(btn);
...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/winlator/widget/TouchpadView.java` around lines 1355 -
1369, The inference logic using event.getPointerCount() should be done only for
the press and the inferred button reused on release to avoid mismatches; add a
field (e.g., lastInferredActionButton) to TouchpadView, when actionButton is
MotionEvent.BUTTON_PRIMARY or 0 and you infer secondary/tertiary in the
ACTION_BUTTON_PRESS branch set lastInferredActionButton to the inferred value
and call handleExternalMouseButton(lastInferredActionButton, true), and in the
ACTION_BUTTON_RELEASE branch use lastInferredActionButton (fall back to
actionButton if unset) when calling handleExternalMouseButton(..., false) and
then clear/reset lastInferredActionButton to avoid stuck state.
1 participant
Description
This PR addresses several critical input bugs related to trackpads, external mice, and captured pointer behavior in TouchpadView.java. These changes significantly improve aim stability in games, prevent rapid clicking issues, and protect XServer from variables overflowing in unconstrained camera environments.
Key Changes:
Button Event Sequencing Guard (Rapid Clicking)
postDelayedreleases that could fire out of order, interrupting dragging and continuous firing mechanics.pendingButtonReleasesto track and aggressively cancel any stale releases immediately when a new corresponding button event (press or release) occurs.Phantom Drift on Capture Reacquisition
capturedPointerRemainderX/Y), causing the crosshair/mouse to jump unexpectedly.hasCapture == true.Expanded Hover Movement Support
ACTION_HOVER_MOVEinstead ofACTION_MOVE, which bypassed the captured pointer logic.ACTION_HOVER_MOVEalongsideACTION_MOVEusingevent.getActionMasked()to ensure calculations like sensitivity, relative deltas, and X11 injections behave correctly for all pointer events without falling back to absolute screens.X11 Pointer Short Overflow Prevention
xServer.pointerrelentlessly. Eventually overflowing the internalshortcoordinates (past limits of 32,767) causing erratic cursor teleports in X11 memory.Short.MIN_VALUEandShort.MAX_VALUEbefore calling injectPointerMoveDelta. (Note: We intentionally avoid clamping to physical screen sizes here[0..width]to allow raw original deltas to be safely accumulated for infinite game camera movement).Testing
Summary by cubic
Improves external mouse and trackpad input stability in captured/relative mode. Fixes rapid click ordering, removes cursor jumps on capture, and prevents X11 coordinate overflow to stabilize aiming in games.
Written for commit ba939f5. Summary will update on new commits.
Summary by CodeRabbit