Add PC-accurate controller vibration with per-container settings#1214
Open
TideGear wants to merge 7 commits into
Open
Add PC-accurate controller vibration with per-container settings#1214TideGear wants to merge 7 commits into
TideGear wants to merge 7 commits into
Conversation
Contributor
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds multi-slot per-player rumble with keepalive: native evshim caches per-player rumble, uses inotify+poll wakeups and re-sends SDL rumble; Java expands to 4 player slots, routes input device→slot, provides vibration mode/intensity prefs and UI, propagates settings to launcher/WinHandler, and updates controller assignment locking. ChangesNative evshim (keepalive) & SDL stub / build
WinHandler, input routing & launcher
Preferences, ContainerData & UI
Sequence Diagram(s)sequenceDiagram
participant Game as Game Process
participant Evshim as evshim.c
participant SDL as SDL (p_SDL_JoystickRumble)
participant Updater as vjoy_updater
participant Cache as Rumble Cache
Game->>Evshim: OnRumble(low,high)
Evshim->>Cache: cache per-player low/high (normal path)
Evshim->>SDL: write rumble to player's fd
Note over Updater,Evshim: inotify+poll wakes on shared-memory MODIFY
Updater->>Cache: read last non-zero cached rumble
Updater->>SDL: p_SDL_JoystickRumble(re-send keepalive)
sequenceDiagram
participant FileObs as FileObserver/inotify
participant Win as WinHandler
participant Scheduler as Deadline Scheduler
participant VibMgr as VibratorManager / Vibrator
participant ControllerMgr as ControllerManager
FileObs->>Win: shared-memory MODIFY event
Win->>Scheduler: evaluate per-player deadlines
Scheduler->>Win: deadline reached / refresh needed
Win->>ControllerMgr: resolve assigned controller/device
alt vibrationMode == controller
Win->>VibMgr: CombinedVibration per-player (or vibrator fallback)
else vibrationMode == device
Win->>VibMgr: compute combined amplitude -> device vibrator
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 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: The best way for teams to 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. 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}
Contributor
There was a problem hiding this comment.
5 issues found across 12 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/PrefManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/PrefManager.kt:223">
P2: `vibrationMode` persists arbitrary strings despite a closed set of supported modes, creating a cross-file contract bug where invalid values silently disable vibration behavior.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.kt:75">
P2: New user-visible vibration mode labels are hardcoded in Compose instead of string resources, breaking localization/resource centralization.</violation>
</file>
<file name="app/src/main/cpp/extras/evshim.c">
<violation number="1" location="app/src/main/cpp/extras/evshim.c:163">
P1: Unsynchronized shared keepalive rumble state (`last_rumble_low/high`) is read/written across callback/updater contexts, creating a C data race (undefined behavior).</violation>
</file>
<file name="app/src/main/java/com/winlator/winhandler/WinHandler.java">
<violation number="1" location="app/src/main/java/com/winlator/winhandler/WinHandler.java:776">
P2: Rumble routing falls back to the first detected gamepad, which can vibrate the wrong controller when multiple devices are connected.</violation>
<violation number="2" location="app/src/main/java/com/winlator/winhandler/WinHandler.java:876">
P1: Stopping rumble depends on current mode, so changing mode can skip canceling previously-started device vibration (up to 60s one-shot).</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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
app/src/main/java/app/gamenative/PrefManager.kt (1)
219-231: Normalize vibration preference values at the persistence boundary.
vibrationModecurrently persists arbitrary strings, andvibrationIntensityis only bounded on write. Normalizing on read+write avoids undefined behavior from stale/corrupt prefs.♻️ Suggested hardening patch
+ private val VALID_VIBRATION_MODES = setOf("off", "controller", "device", "both") + private val VIBRATION_MODE = stringPreferencesKey("vibration_mode") var vibrationMode: String - get() = getPref(VIBRATION_MODE, "controller") + get() = getPref(VIBRATION_MODE, "controller") + .takeIf { it in VALID_VIBRATION_MODES } ?: "controller" set(value) { - setPref(VIBRATION_MODE, value) + setPref(VIBRATION_MODE, value.takeIf { it in VALID_VIBRATION_MODES } ?: "controller") } private val VIBRATION_INTENSITY = intPreferencesKey("vibration_intensity") var vibrationIntensity: Int - get() = getPref(VIBRATION_INTENSITY, 100) + get() = getPref(VIBRATION_INTENSITY, 100).coerceIn(0, 100) set(value) { setPref(VIBRATION_INTENSITY, value.coerceIn(0, 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/PrefManager.kt` around lines 219 - 231, Persisted vibration preferences aren't normalized on read and only partially bounded on write, so stale or corrupt values can cause undefined behavior; update vibrationMode and vibrationIntensity accessors to validate and normalize both on get and set: for vibrationMode (symbol VIBRATION_MODE and property vibrationMode) accept only a defined set of allowed values (e.g., include "controller" as the default) and on get return the default if stored value is missing/unknown, and on set constrain input to the allowed set before calling setPref; for vibrationIntensity (symbol VIBRATION_INTENSITY and property vibrationIntensity) apply value.coerceIn(0,100) on both set and when reading from getPref (or fallback to the default 100 if stored value is out of range) so getPref/getters never return invalid values; use existing getPref and setPref helpers to centralize this normalization.app/src/main/java/com/winlator/winhandler/WinHandler.java (1)
104-107: ValidatevibrationModebefore storing it.This is persisted as a free-form string, and an unexpected value or casing currently falls through every branch in
startVibrationForPlayer()while still marking the slot as rumbling. Normalizing to the supported set here avoids silent no-op sessions from malformed container data.Suggested normalization
public void setVibrationMode(String mode) { - this.vibrationMode = mode != null ? mode : "controller"; + String normalized = mode == null ? "controller" : mode.toLowerCase(java.util.Locale.ROOT); + switch (normalized) { + case "off": + case "controller": + case "device": + case "both": + this.vibrationMode = normalized; + break; + default: + Log.w(TAG, "Unknown vibration mode '" + mode + "', defaulting to controller"); + this.vibrationMode = "controller"; + break; + } Log.i(TAG, "Vibration mode set to: " + this.vibrationMode); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 104 - 107, Validate and normalize the incoming vibrationMode in setVibrationMode before storing: create a canonical set (e.g., SUPPORTED_VIBRATION_MODES) and normalize the input (trim + toLowerCase) and map known aliases to canonical values; if the normalized value is not in SUPPORTED_VIBRATION_MODES, fall back to "controller" and log the normalization. Update setVibrationMode to use this check/mapping so vibrationMode only ever contains a supported value and startVibrationForPlayer will not receive unexpected free-form strings.
🤖 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/cpp/extras/evshim.c`:
- Around line 78-90: OnRumble() is writing last_rumble_low/last_rumble_high
concurrently with vjoy_updater() reading them, causing a data race; acquire
shm_mutex around updates and the pwrite snapshot so the pair of low/high values
are written atomically (i.e., lock shm_mutex, update last_rumble_low[idx] and
last_rumble_high[idx] and prepare vals[], call pwrite while still holding the
mutex, then unlock), but move any call to SDL_JoystickRumble() to after
unlocking; apply the same mutex-protected snapshot fix to the other occurrence
that updates last_rumble_* (the block referenced at lines 161-167) so both sites
use shm_mutex for consistent reads/writes.
In `@app/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.kt`:
- Around line 77-104: The UI shows a mismatch because vibrationModeIndex is
derived from vibrationModeValues using config.vibrationMode but the slider
visibility checks the raw config.vibrationMode; normalize the vibration mode
once (e.g., map/normalize config.vibrationMode to a valid value from
vibrationModeValues) before using it in the UI and persist that normalized value
back into state.config (or use a local normalizedMode for both the dropdown and
the slider visibility check). Update the logic around vibrationModeIndex, the
remember block for intensitySlider, and the conditional "if
(config.vibrationMode != \"off\")" to use the normalized value (reference:
vibrationModeIndex, vibrationModeValues, config.vibrationMode,
state.config.value, intensitySlider).
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 1546-1547: The code passes raw extras to
WinHandler.setVibrationMode via
handler.setVibrationMode(container.getExtra("vibrationMode", "controller")),
which can store variants like "Controller" and later break
startVibrationForPlayer that only recognizes "off/controller/device/both";
normalize the value before calling setVibrationMode (e.g. read
container.getExtra("vibrationMode", "controller"), trim and toLowerCase it, then
map common synonyms/capitalizations to the canonical tokens "off", "controller",
"device", or "both") so WinHandler.setVibrationMode always receives one of the
accepted lowercase values.
In `@app/src/main/java/app/gamenative/utils/ContainerUtils.kt`:
- Around line 146-147: The new container creation is bypassing the updated
defaults: update createNewContainer(...) so the ContainerData it constructs
includes vibrationMode and vibrationIntensity from PrefManager (or simply
initialize it from getDefaultContainerData()) instead of hardcoding
"controller"/100; modify the ContainerData construction inside
createNewContainer to set vibrationMode = PrefManager.vibrationMode and
vibrationIntensity = PrefManager.vibrationIntensity (or replace the manual
builder with a call to getDefaultContainerData()) to ensure fresh containers
respect saved defaults.
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java`:
- Around line 616-645: The keepalive logic is re-triggering the 60s phone
one-shot because unchanged rumble calls startVibrationForPlayer(p, lowFreq,
highFreq) which routes "device"/"both" into vibrateDevice(); change the
keepalive path so device vibration is not restarted every keepalive tick—either
add a new method like refreshDeviceVibrationKeepalive(playerId, lowFreq,
highFreq) or add a flag/overload to startVibrationForPlayer to distinguish
initial/start vs keepalive; ensure the initial start still calls vibrateDevice()
with DEVICE_RUMBLE_MS but keepalive calls a separate device refresh with a much
longer cadence (or a no-op for device) and adjust rumbleKeepaliveCtr handling
accordingly so controller rumble continues to be refreshed while device
vibration is not restarted every ~240ms.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/PrefManager.kt`:
- Around line 219-231: Persisted vibration preferences aren't normalized on read
and only partially bounded on write, so stale or corrupt values can cause
undefined behavior; update vibrationMode and vibrationIntensity accessors to
validate and normalize both on get and set: for vibrationMode (symbol
VIBRATION_MODE and property vibrationMode) accept only a defined set of allowed
values (e.g., include "controller" as the default) and on get return the default
if stored value is missing/unknown, and on set constrain input to the allowed
set before calling setPref; for vibrationIntensity (symbol VIBRATION_INTENSITY
and property vibrationIntensity) apply value.coerceIn(0,100) on both set and
when reading from getPref (or fallback to the default 100 if stored value is out
of range) so getPref/getters never return invalid values; use existing getPref
and setPref helpers to centralize this normalization.
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java`:
- Around line 104-107: Validate and normalize the incoming vibrationMode in
setVibrationMode before storing: create a canonical set (e.g.,
SUPPORTED_VIBRATION_MODES) and normalize the input (trim + toLowerCase) and map
known aliases to canonical values; if the normalized value is not in
SUPPORTED_VIBRATION_MODES, fall back to "controller" and log the normalization.
Update setVibrationMode to use this check/mapping so vibrationMode only ever
contains a supported value and startVibrationForPlayer will not receive
unexpected free-form strings.
🪄 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: 2ea17b0f-27f1-4d46-b5d7-819da35201fa
⛔ Files ignored due to path filters (1)
app/src/main/jniLibs/arm64-v8a/libevshim.sois excluded by!**/*.so
📒 Files selected for processing (11)
app/src/main/cpp/extras/CMakeLists.txtapp/src/main/cpp/extras/evshim.capp/src/main/cpp/extras/sdl2_stub/SDL2/SDL.happ/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.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/ContainerData.ktapp/src/main/java/com/winlator/winhandler/WinHandler.javaapp/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.javaapp/src/main/res/values/strings.xml
Author
|
I'll address the conflicts and AI points tonight. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
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)
1008-1023:⚠️ Potential issue | 🟠 MajorRequest USB permissions on
onInputDeviceChanged()too.The comment in this file already notes that some devices only expose their full capabilities on
onInputDeviceChanged(). Right now those controllers can become eligible for USB rumble there without ever triggering the permission request, so the new controller-vibration path stays unavailable until reconnect/relaunch.Suggested fix
override fun onInputDeviceChanged(deviceId: Int) { scanForExternalDevices() val device = InputDevice.getDevice(deviceId) ?: return evaluateDevice(device) + xServerView?.getxServer()?.winHandler?.requestUsbPermissionsForControllers() }🤖 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 1008 - 1023, onInputDeviceChanged currently rescans and evaluates the device but does not request USB permissions, so controllers that only reveal capabilities on change never get permission for USB rumble; update the onInputDeviceChanged handler to call xServerView?.getxServer()?.winHandler?.requestUsbPermissionsForControllers() (same as in onInputDeviceAdded) after evaluating the device (or immediately after scanForExternalDevices/evaluateDevice) so the permission flow runs when a device's capabilities change; modify the onInputDeviceChanged method (refer to function name onInputDeviceChanged and usages of xServerView/getxServer/winHandler/requestUsbPermissionsForControllers/evaluateDevice/scanForExternalDevices) to include that call.
🤖 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/cpp/extras/evshim.c`:
- Around line 69-115: The current init_ff_rumble fills ff_fd[]/ff_effect_id[] by
scan order which can mismatch virtual players; instead associate rumble fds with
the actual virtual player slots (used by OnRumble) by matching devices to the
gamepad each slot opened. Change init_ff_rumble / or move effect creation into
the same place that opens a gamepad so you call EVIOCGBIT/EVIOCGNAME/EVIOCGUNIQ
on the specific device path you opened and then store the resulting fd and
eff.id into ff_fd[player_index] and ff_effect_id[player_index] (rather than
using a monotonically incremented found index). Use the same device-identifying
value your gamepad-open code uses (device path/event number or EVIOCGUNIQ) to
map device -> player slot so OnRumble indexes the correct physical device.
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java`:
- Around line 1010-1026: The phone Vibrator is global so per-player start/stop
in vibrateDevice() and related stop paths cancels or overwrites other players;
change the logic to maintain a shared, aggregated device rumble state (e.g., a
map of active player amplitudes or a combinedAmplitude counter) in WinHandler,
compute a single aggregated amplitude (cap to 255) using the existing
scaleAmplitude() logic, and call vibrateSingle(phoneVibrator, aggregatedAmp,
DEVICE_RUMBLE_MS) only once for the device; only call phoneVibrator.cancel()
when the aggregate indicates no active players; update the code paths that
currently call vibrateDevice(), vibrateSingle(), or cancel() per-slot so they
update the shared state and then recompute/apply the aggregated vibration
instead of touching the Vibrator directly per player.
- Around line 164-176: The current matching uses only vendor/product and can
bind the wrong physical controller; instead, when mapping an InputDevice to a
UsbDevice (in WinHandler around the block using
device.getVendorId()/getProductId(), usbManager.getDeviceList(),
hasPermission(), requestPermission(), USB_PERMISSION_ACTION), match and cache
the exact UsbDevice (e.g., compare UsbDevice.getDeviceName() or a stable
descriptor/ID with InputDevice.getName()/getDescriptor()) and store that
UsbDevice (or its deviceId) in a per-player map keyed by the player or
InputDevice.getId(); on subsequent permission/rumble operations use the cached
UsbDevice mapping rather than VID/PID-only lookup. Make the same change for the
other similar lookup locations mentioned (lines ~909-917).
- Around line 144-178: requestUsbPermissionsForControllers currently issues USB
permission requests without consulting or updating the usbPermissionPending[]
guard, which allows duplicate requests; update
requestUsbPermissionsForControllers to check usbPermissionPending[player] before
calling usbManager.requestPermission and set usbPermissionPending[player] = true
when dispatching a request to deduplicate, and ensure the
BroadcastReceiver/USB_PERMISSION_ACTION handling used by onInputDeviceAdded and
vibrateViaUsb clears usbPermissionPending[player] for that player on both grant
and denial so the pending state is removed regardless of outcome.
---
Outside diff comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 1008-1023: onInputDeviceChanged currently rescans and evaluates
the device but does not request USB permissions, so controllers that only reveal
capabilities on change never get permission for USB rumble; update the
onInputDeviceChanged handler to call
xServerView?.getxServer()?.winHandler?.requestUsbPermissionsForControllers()
(same as in onInputDeviceAdded) after evaluating the device (or immediately
after scanForExternalDevices/evaluateDevice) so the permission flow runs when a
device's capabilities change; modify the onInputDeviceChanged method (refer to
function name onInputDeviceChanged and usages of
xServerView/getxServer/winHandler/requestUsbPermissionsForControllers/evaluateDevice/scanForExternalDevices)
to include that call.
🪄 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: 7e6f9036-446a-4dcc-aa93-84ad2de72b3c
⛔ Files ignored due to path filters (1)
app/src/main/jniLibs/arm64-v8a/libevshim.sois excluded by!**/*.so
📒 Files selected for processing (11)
app/src/main/cpp/extras/CMakeLists.txtapp/src/main/cpp/extras/evshim.capp/src/main/cpp/extras/sdl2_stub/SDL2/SDL.happ/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.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/ContainerData.ktapp/src/main/java/com/winlator/winhandler/WinHandler.javaapp/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.javaapp/src/main/res/values/strings.xml
✅ Files skipped from review due to trivial changes (2)
- app/src/main/res/values/strings.xml
- app/src/main/cpp/extras/sdl2_stub/SDL2/SDL.h
🚧 Files skipped from review as they are similar to previous changes (4)
- app/src/main/java/app/gamenative/utils/ContainerUtils.kt
- app/src/main/cpp/extras/CMakeLists.txt
- app/src/main/java/app/gamenative/PrefManager.kt
- app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java
Comment on lines
+144
to
+178
| public void requestUsbPermissionsForControllers() { | ||
| if (activity == null) return; | ||
| UsbManager usbManager = (UsbManager) activity.getSystemService(Context.USB_SERVICE); | ||
| if (usbManager == null) return; | ||
|
|
||
| for (int p = 0; p < MAX_PLAYERS; p++) { | ||
| InputDevice device = resolveInputDeviceForPlayer(p); | ||
| if (device == null) continue; | ||
|
|
||
| boolean hasStandardVibrator = false; | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { | ||
| VibratorManager vm = device.getVibratorManager(); | ||
| if (vm != null && vm.getVibratorIds().length > 0) hasStandardVibrator = true; | ||
| } | ||
| if (!hasStandardVibrator) { | ||
| Vibrator v = device.getVibrator(); | ||
| if (v != null && v.hasVibrator()) hasStandardVibrator = true; | ||
| } | ||
| if (hasStandardVibrator) continue; | ||
|
|
||
| int vid = device.getVendorId(); | ||
| int pid = device.getProductId(); | ||
| for (UsbDevice ud : usbManager.getDeviceList().values()) { | ||
| if (ud.getVendorId() == vid && ud.getProductId() == pid) { | ||
| if (!usbManager.hasPermission(ud)) { | ||
| PendingIntent pi = PendingIntent.getBroadcast(activity, 0, | ||
| new Intent(USB_PERMISSION_ACTION), PendingIntent.FLAG_IMMUTABLE); | ||
| usbManager.requestPermission(ud, pi); | ||
| Log.i(TAG, "Proactively requesting USB permission for '" + | ||
| device.getName() + "' (no standard vibrator)"); | ||
| } | ||
| break; | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Deduplicate proactive USB permission requests.
requestUsbPermissionsForControllers() bypasses usbPermissionPending[], so repeated onInputDeviceAdded() callbacks or a quick fallback into vibrateViaUsb() can queue duplicate permission requests before the first one resolves.
🛠 Suggested guard
- if (!usbManager.hasPermission(ud)) {
+ if (!usbManager.hasPermission(ud) && !usbPermissionPending[p]) {
+ usbPermissionPending[p] = true;
PendingIntent pi = PendingIntent.getBroadcast(activity, 0,
new Intent(USB_PERMISSION_ACTION), PendingIntent.FLAG_IMMUTABLE);
usbManager.requestPermission(ud, pi);
Log.i(TAG, "Proactively requesting USB permission for '" +
device.getName() + "' (no standard vibrator)");
}Make sure the USB_PERMISSION_ACTION result path clears usbPermissionPending[player] on both grant and denial.
Also applies to: 920-927
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 144 -
178, requestUsbPermissionsForControllers currently issues USB permission
requests without consulting or updating the usbPermissionPending[] guard, which
allows duplicate requests; update requestUsbPermissionsForControllers to check
usbPermissionPending[player] before calling usbManager.requestPermission and set
usbPermissionPending[player] = true when dispatching a request to deduplicate,
and ensure the BroadcastReceiver/USB_PERMISSION_ACTION handling used by
onInputDeviceAdded and vibrateViaUsb clears usbPermissionPending[player] for
that player on both grant and denial so the pending state is removed regardless
of outcome.
Comment on lines
+164
to
+176
| int vid = device.getVendorId(); | ||
| int pid = device.getProductId(); | ||
| for (UsbDevice ud : usbManager.getDeviceList().values()) { | ||
| if (ud.getVendorId() == vid && ud.getProductId() == pid) { | ||
| if (!usbManager.hasPermission(ud)) { | ||
| PendingIntent pi = PendingIntent.getBroadcast(activity, 0, | ||
| new Intent(USB_PERMISSION_ACTION), PendingIntent.FLAG_IMMUTABLE); | ||
| usbManager.requestPermission(ud, pi); | ||
| Log.i(TAG, "Proactively requesting USB permission for '" + | ||
| device.getName() + "' (no standard vibrator)"); | ||
| } | ||
| break; | ||
| } |
Contributor
There was a problem hiding this comment.
VID/PID-only matching can bind rumble to the wrong controller.
These lookups pick the first UsbDevice whose vendor/product pair matches the InputDevice. With two identical pads connected, both players can attach to the same USB device, so permission and rumble traffic go to the wrong controller.
Track a per-player USB identity stronger than vendorId/productId alone, or cache the exact UsbDevice selected for that player once the mapping is known.
Also applies to: 909-917
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 164 -
176, The current matching uses only vendor/product and can bind the wrong
physical controller; instead, when mapping an InputDevice to a UsbDevice (in
WinHandler around the block using device.getVendorId()/getProductId(),
usbManager.getDeviceList(), hasPermission(), requestPermission(),
USB_PERMISSION_ACTION), match and cache the exact UsbDevice (e.g., compare
UsbDevice.getDeviceName() or a stable descriptor/ID with
InputDevice.getName()/getDescriptor()) and store that UsbDevice (or its
deviceId) in a per-player map keyed by the player or InputDevice.getId(); on
subsequent permission/rumble operations use the cached UsbDevice mapping rather
than VID/PID-only lookup. Make the same change for the other similar lookup
locations mentioned (lines ~909-917).
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
app/src/main/java/app/gamenative/utils/ContainerUtils.kt (1)
146-147:⚠️ Potential issue | 🟠 MajorNew-container path still bypasses these vibration defaults.
Even with Lines 146-147 set correctly,
createNewContainer(...)still manually constructsContainerDataat Line 811 withoutvibrationMode/vibrationIntensity, so fresh containers fall back to model defaults instead of currentPrefManagervalues.Suggested fix
ContainerData( screenSize = PrefManager.screenSize, envVars = PrefManager.envVars, cpuList = PrefManager.cpuList, cpuListWoW64 = PrefManager.cpuListWoW64, @@ portraitMode = PrefManager.portraitMode, externalDisplayMode = PrefManager.externalDisplayInputMode, externalDisplaySwap = PrefManager.externalDisplaySwap, + vibrationMode = PrefManager.vibrationMode, + vibrationIntensity = PrefManager.vibrationIntensity, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/ContainerUtils.kt` around lines 146 - 147, createNewContainer currently builds a fresh ContainerData manually and omits vibrationMode/vibrationIntensity so new containers use model defaults; modify the ContainerData construction inside createNewContainer to set vibrationMode = PrefManager.vibrationMode and vibrationIntensity = PrefManager.vibrationIntensity (or call the existing ContainerData factory/constructor that already applies PrefManager values) so new containers inherit the current user preferences.app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (1)
1531-1532:⚠️ Potential issue | 🟡 MinorNormalize
vibrationModebefore handing it toWinHandler.Line 1531 still forwards the raw extra, but downstream only recognizes lowercase
"off","controller","device", and"both". A migrated/imported value like"Controller"will silently disable rumble.🔧 Suggested fix
- handler.setVibrationMode(container.getExtra("vibrationMode", "controller")) + val supportedVibrationModes = setOf("off", "controller", "device", "both") + val vibrationMode = container.getExtra("vibrationMode", "controller") + .trim() + .lowercase(Locale.ROOT) + .takeIf { it in supportedVibrationModes } + ?: "controller" + handler.setVibrationMode(vibrationMode)🤖 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 1531 - 1532, Normalize the vibrationMode string from container.getExtra before calling handler.setVibrationMode: read the raw value via container.getExtra("vibrationMode", "controller"), convert it to lowercase using Kotlin's lowercase(Locale.ROOT) (or .lowercase()), and map known variants to the accepted tokens "off", "controller", "device", or "both" (falling back to "controller" if unrecognized) so that handler.setVibrationMode receives only the downstream-recognized values; keep the existing handler.setVibrationIntensity call as-is.app/src/main/java/com/winlator/winhandler/WinHandler.java (2)
826-842:⚠️ Potential issue | 🟠 MajorAggregate handset rumble across players instead of cancelling it per slot.
The phone
Vibratoris global, but Lines 858-862 and 889-894 treat it as player-local. In"device"/"both"mode, one player stopping rumble cancels everyone else, and another player starting rumble overwrites the current handset amplitude.Also applies to: 848-900
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 826 - 842, vibrateDevice treats the global Vibrator as player-local causing one player's stop to cancel everyone and new starts to overwrite amplitude; change this by maintaining a shared aggregation of per-player amplitudes (e.g., a concurrent map or array keyed by player/slot) and update an aggregated amplitude (max or summed clamped to 255) whenever vibrateDevice or the player-stop path changes a player's value. Replace direct phoneVibrator.cancel() calls and direct vibrateSingle(phoneVibrator, phoneAmp, ...) writes with updates to the global aggregated value: only call cancel when the aggregate becomes zero, and call vibrateSingle with the aggregated amplitude when it increases/changes. Update references in vibrateDevice and any stop/clear code paths that currently call phoneVibrator.cancel() or write phoneAmp directly so they use the shared aggregation logic instead.
645-650:⚠️ Potential issue | 🟠 MajorDon’t route keepalive back through handset vibration.
Line 649 replays
startVibrationForPlayer(...)for unchanged rumble. In"device"/"both"mode that re-triggers the 60s phone vibration every keepalive tick, which is the same regression the earlier feedback fixed.🔧 Suggested fix
- startVibrationForPlayer(p, lowFreq, highFreq); + if ("controller".equals(vibrationMode) || "both".equals(vibrationMode)) { + vibrateController(p, lowFreq, highFreq); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 645 - 650, The keepalive path uses startVibrationForPlayer(...) which re-triggers phone vibration in "device"/"both" mode; change the keepalive logic (the isRumbling[p] / rumbleKeepaliveCtr[p] branch) to call a controller-only rumble helper instead of startVibrationForPlayer (e.g., use or add a method like startControllerRumbleForPlayer or sendControllerRumble) so the keepalive only pulses the controller; if no controller-only helper exists, add one that mirrors controller rumble behavior without invoking phone vibration, and call that from the keepalive branch instead of startVibrationForPlayer.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/utils/ContainerUtils.kt (1)
324-325: Defensively clamp parsedvibrationIntensityon read.Reading extras can produce out-of-range values (stale/manual/corrupt data). Clamping at deserialization keeps
ContainerDatainternally consistent.Suggested hardening
- vibrationIntensity = container.getExtra("vibrationIntensity", "100").toIntOrNull() ?: 100, + vibrationIntensity = (container.getExtra("vibrationIntensity", "100").toIntOrNull() ?: 100).coerceIn(0, 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/utils/ContainerUtils.kt` around lines 324 - 325, When deserializing vibrationIntensity from the container extra (the line using container.getExtra("vibrationIntensity", "100") and assigning to vibrationIntensity in ContainerData), parse the value defensively and clamp it to a safe range (e.g., 0..100) so out-of-range or corrupt values can't be stored; replace the current .toIntOrNull() ?: 100 logic with a parse-then-coerce (Kotlin's coerceIn) pattern to ensure vibrationIntensity is always within the defined bounds.
🤖 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 1531-1532: Normalize the vibrationMode string from
container.getExtra before calling handler.setVibrationMode: read the raw value
via container.getExtra("vibrationMode", "controller"), convert it to lowercase
using Kotlin's lowercase(Locale.ROOT) (or .lowercase()), and map known variants
to the accepted tokens "off", "controller", "device", or "both" (falling back to
"controller" if unrecognized) so that handler.setVibrationMode receives only the
downstream-recognized values; keep the existing handler.setVibrationIntensity
call as-is.
In `@app/src/main/java/app/gamenative/utils/ContainerUtils.kt`:
- Around line 146-147: createNewContainer currently builds a fresh ContainerData
manually and omits vibrationMode/vibrationIntensity so new containers use model
defaults; modify the ContainerData construction inside createNewContainer to set
vibrationMode = PrefManager.vibrationMode and vibrationIntensity =
PrefManager.vibrationIntensity (or call the existing ContainerData
factory/constructor that already applies PrefManager values) so new containers
inherit the current user preferences.
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java`:
- Around line 826-842: vibrateDevice treats the global Vibrator as player-local
causing one player's stop to cancel everyone and new starts to overwrite
amplitude; change this by maintaining a shared aggregation of per-player
amplitudes (e.g., a concurrent map or array keyed by player/slot) and update an
aggregated amplitude (max or summed clamped to 255) whenever vibrateDevice or
the player-stop path changes a player's value. Replace direct
phoneVibrator.cancel() calls and direct vibrateSingle(phoneVibrator, phoneAmp,
...) writes with updates to the global aggregated value: only call cancel when
the aggregate becomes zero, and call vibrateSingle with the aggregated amplitude
when it increases/changes. Update references in vibrateDevice and any stop/clear
code paths that currently call phoneVibrator.cancel() or write phoneAmp directly
so they use the shared aggregation logic instead.
- Around line 645-650: The keepalive path uses startVibrationForPlayer(...)
which re-triggers phone vibration in "device"/"both" mode; change the keepalive
logic (the isRumbling[p] / rumbleKeepaliveCtr[p] branch) to call a
controller-only rumble helper instead of startVibrationForPlayer (e.g., use or
add a method like startControllerRumbleForPlayer or sendControllerRumble) so the
keepalive only pulses the controller; if no controller-only helper exists, add
one that mirrors controller rumble behavior without invoking phone vibration,
and call that from the keepalive branch instead of startVibrationForPlayer.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/utils/ContainerUtils.kt`:
- Around line 324-325: When deserializing vibrationIntensity from the container
extra (the line using container.getExtra("vibrationIntensity", "100") and
assigning to vibrationIntensity in ContainerData), parse the value defensively
and clamp it to a safe range (e.g., 0..100) so out-of-range or corrupt values
can't be stored; replace the current .toIntOrNull() ?: 100 logic with a
parse-then-coerce (Kotlin's coerceIn) pattern to ensure vibrationIntensity is
always within the defined bounds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7bca9ac-190a-47ab-a909-86a6845ed85e
📒 Files selected for processing (7)
app/src/main/cpp/extras/evshim.capp/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/ContainerUtils.ktapp/src/main/java/com/winlator/winhandler/WinHandler.javaapp/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/res/values/strings.xml
- app/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.kt
- app/src/main/cpp/extras/evshim.c
Contributor
There was a problem hiding this comment.
3 issues found across 7 files (changes from recent commits).
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:1531">
P2: Removed normalization for vibration mode can cause mode parsing mismatches and incorrect rumble routing.</violation>
</file>
<file name="app/src/main/java/com/winlator/winhandler/WinHandler.java">
<violation number="1" location="app/src/main/java/com/winlator/winhandler/WinHandler.java:649">
P2: Keepalive now re-triggers long device vibration repeatedly, causing unnecessary repeated one-shot vibration calls during sustained rumble.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.kt:77">
P2: Removed vibrationMode normalization can produce inconsistent UI: dropdown shows Off for unknown values while intensity controls remain visible.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/src/main/java/com/winlator/winhandler/WinHandler.java (1)
852-868:⚠️ Potential issue | 🟠 MajorAggregate handset rumble across players instead of driving it per slot.
vibrateDevice()uses the system vibrator fromContext.VIBRATOR_SERVICE, so these per-player start/stop paths are still mutating one shared handset vibration state. A stop from one slot cancels the phone rumble for every other active slot, and another slot’svibrateDevice()call overwrites the current amplitude. Because the keepalive path intentionally no-ops for"device"mode, that canceled rumble will not be re-armed until the game’s rumble values change again. (developer.android.com)Also applies to: 878-899, 902-932
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 852 - 868, vibrateDevice is mutating the single system vibrator per-player which causes one slot stopping to cancel others and amplitude overwrites; change the design to aggregate per-slot rumble values centrally (e.g., add a RumbleManager with a Map<slotId, int amplitude>), expose an updateSlot(slotId, amplitude) and removeSlot(slotId) API that calls a single internal method to compute the aggregated amplitude (use a deterministic aggregation like max or capped sum of scaleAmplitude results), then call vibrateSingle(phoneVibrator, aggregatedAmp, DEVICE_RUMBLE_MS) or phoneVibrator.cancel() only when the aggregated amplitude transitions to zero; update calls that currently call vibrateDevice(...) to instead call RumbleManager.updateSlot/removeSlot and keep symbols vibrateDevice, vibrateSingle, scaleAmplitude, DEVICE_RUMBLE_MS and activity.getSystemService referenced when locating where to replace per-slot starts/stops.
🤖 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 1538-1543: The code currently reads vibrationIntensity from
container.getExtra and converts it with toIntOrNull() ?: 100 but does not
enforce bounds; clamp the parsed intensity to the expected 0–100 range before
calling handler.setVibrationIntensity. Locate the vibration intensity flow
around container.getExtra("vibrationIntensity", "100") and replace the raw parse
with a guarded value (e.g., val intensity =
container.getExtra(...).toIntOrNull() ?: 100; val clamped =
intensity.coerceIn(0, 100)) and pass clamped to handler.setVibrationIntensity so
downstream code always receives a valid 0–100 value.
In `@app/src/main/java/com/winlator/inputcontrols/ControllerManager.java`:
- Around line 98-124: evictDisconnectedAndCompact() currently forces all kept
controllers to enabled and leaves old tail enabled flags untouched; update it to
preserve each kept controller's prior enabled state by recording enabledSlots
for identifiers kept (use slotAssignments and enabledSlots to build a parallel
keptEnabled list), then when repopulating slotAssignments set enabledSlots[i] to
the corresponding keptEnabled value for each new compacted index, and for any
remaining slot indices up to WinHandler.MAX_PLAYERS clear/disable them (set
false) so vacated tail flags don't persist; ensure saveAssignments() still runs
after these updates.
---
Duplicate comments:
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java`:
- Around line 852-868: vibrateDevice is mutating the single system vibrator
per-player which causes one slot stopping to cancel others and amplitude
overwrites; change the design to aggregate per-slot rumble values centrally
(e.g., add a RumbleManager with a Map<slotId, int amplitude>), expose an
updateSlot(slotId, amplitude) and removeSlot(slotId) API that calls a single
internal method to compute the aggregated amplitude (use a deterministic
aggregation like max or capped sum of scaleAmplitude results), then call
vibrateSingle(phoneVibrator, aggregatedAmp, DEVICE_RUMBLE_MS) or
phoneVibrator.cancel() only when the aggregated amplitude transitions to zero;
update calls that currently call vibrateDevice(...) to instead call
RumbleManager.updateSlot/removeSlot and keep symbols vibrateDevice,
vibrateSingle, scaleAmplitude, DEVICE_RUMBLE_MS and activity.getSystemService
referenced when locating where to replace per-slot starts/stops.
🪄 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: 42e41256-d01d-480b-b821-b31ad868070b
⛔ Files ignored due to path filters (1)
app/src/main/jniLibs/arm64-v8a/libevshim.sois excluded by!**/*.so
📒 Files selected for processing (10)
app/src/main/cpp/extras/evshim.capp/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.ktapp/src/main/java/app/gamenative/ui/screen/xserver/PhysicalControllerHandler.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/ContainerUtils.ktapp/src/main/java/com/winlator/inputcontrols/ControllerManager.javaapp/src/main/java/com/winlator/winhandler/WinHandler.javaapp/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.javaapp/src/main/res/values/strings.xml
✅ Files skipped from review due to trivial changes (1)
- app/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (4)
- app/src/main/java/app/gamenative/utils/ContainerUtils.kt
- app/src/main/cpp/extras/evshim.c
- app/src/main/java/app/gamenative/PrefManager.kt
- app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java
Contributor
There was a problem hiding this comment.
4 issues found across 11 files (changes from recent commits).
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:1063">
P2: Expensive interpolated debug logging is executed for every key event in a hot input path, causing avoidable per-event overhead even when debug logs are disabled.</violation>
</file>
<file name="app/src/main/java/com/winlator/inputcontrols/ControllerManager.java">
<violation number="1" location="app/src/main/java/com/winlator/inputcontrols/ControllerManager.java:121">
P2: Compaction corrupts persisted slot enabled state by forcing kept slots to `true` and not clearing emptied slots.</violation>
</file>
<file name="app/src/main/java/com/winlator/winhandler/WinHandler.java">
<violation number="1" location="app/src/main/java/com/winlator/winhandler/WinHandler.java:880">
P1: Keepalive refresh skips device vibration, so continuous rumble in `device`/`both` modes can stop after the 60s one-shot timeout.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/screen/xserver/PhysicalControllerHandler.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/xserver/PhysicalControllerHandler.kt:248">
P2: Unconditional debug logging was added in the controller input hot path, causing avoidable per-event overhead from eagerly interpolated log messages.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
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/PhysicalControllerHandler.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/xserver/PhysicalControllerHandler.kt:38">
P1: Axis cleanup ignores tracked deviceId and sends all release events to slot 0, breaking multi-controller state release.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/winhandler/WinHandler.java (1)
631-636:⚠️ Potential issue | 🔴 CriticalDuplicate
running = trueandstartSendThread()calls spawn two send threads.Lines 613-614 already set
running = trueand callstartSendThread(). The duplicates at lines 634-635 will spawn a second send thread, causing potential race conditions on theactionsqueue.🐛 Proposed fix: remove duplicate lines
startRumblePoller(); - running = true; - startSendThread(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 631 - 636, There are duplicate invocations of running = true and startSendThread() that will spawn two send threads; remove the second pair of calls (the ones after startRumblePoller()) so that only the original running = true and startSendThread() remain (referencing the running field and startSendThread() method in WinHandler) to prevent a second send thread and race conditions on the actions queue.
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (1)
1528-1533:⚠️ Potential issue | 🟡 MinorClamp
vibrationIntensityto the supported 0–100 range.
toIntOrNull() ?: 100still lets stale extras like-1or500through, so the contract is only enforced by the UI and not at launch time.🔧 Proposed fix
handler.setVibrationMode( PrefManager.normalizeVibrationModeInput( container.getExtra("vibrationMode", "controller"), ), ) - handler.setVibrationIntensity(container.getExtra("vibrationIntensity", "100").toIntOrNull() ?: 100) + val vibrationIntensity = container.getExtra("vibrationIntensity", "100") + .toIntOrNull() + ?.coerceIn(0, 100) + ?: 100 + handler.setVibrationIntensity(vibrationIntensity)🤖 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 1528 - 1533, The vibrationIntensity value read from container.getExtra("vibrationIntensity", "100") must be parsed and clamped to 0–100 before calling handler.setVibrationIntensity to prevent stale/surprising extras like -1 or 500; replace the current toIntOrNull() ?: 100 usage by parsing the string, defaulting to 100 on parse failure, then apply Kotlin's coerceIn(0, 100) (or equivalent clamp) and pass that clamped Int into handler.setVibrationIntensity; keep the vibrationMode handling (PrefManager.normalizeVibrationModeInput and handler.setVibrationMode) unchanged.
🤖 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/PhysicalControllerHandler.kt`:
- Around line 36-45: releaseActiveAxes currently resolves bindings against
profile?.getController("*") and sends releases without the originating
device/slot; iterate activeAxisBindings by deviceId (for ((deviceId, bindings)
in activeAxisBindings)) and resolve the controller with
profile?.getController(deviceId) so releases are sent to the correct virtual
controller/slot. In the releaseActiveAxes method, replace the wildcard lookup
with a lookup using the deviceId key, skip when controller is null, and then
call controller.getControllerBinding(keyCode) and handleInputEvent as before;
keep the final activeAxisBindings.clear() to fully clean up.
In
`@app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java`:
- Around line 182-193: The controller counting logic in
BionicProgramLauncherComponent currently only checks InputDevice.SOURCE_GAMEPAD
and thus can miss joystick-class controllers; update the predicate in the loop
that iterates android.view.InputDevice.getDeviceIds() to match the same
acceptance used in ExternalController (e.g., treat a device as a controller if
dev != null && !dev.isVirtual() &&
(dev.supportsSource(InputDevice.SOURCE_GAMEPAD) ||
dev.supportsSource(InputDevice.SOURCE_JOYSTICK))), so connectedControllers and
enabledPlayerCount reflect both gamepad and joystick-class pads.
---
Outside diff comments:
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java`:
- Around line 631-636: There are duplicate invocations of running = true and
startSendThread() that will spawn two send threads; remove the second pair of
calls (the ones after startRumblePoller()) so that only the original running =
true and startSendThread() remain (referencing the running field and
startSendThread() method in WinHandler) to prevent a second send thread and race
conditions on the actions queue.
---
Duplicate comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 1528-1533: The vibrationIntensity value read from
container.getExtra("vibrationIntensity", "100") must be parsed and clamped to
0–100 before calling handler.setVibrationIntensity to prevent stale/surprising
extras like -1 or 500; replace the current toIntOrNull() ?: 100 usage by parsing
the string, defaulting to 100 on parse failure, then apply Kotlin's coerceIn(0,
100) (or equivalent clamp) and pass that clamped Int into
handler.setVibrationIntensity; keep the vibrationMode handling
(PrefManager.normalizeVibrationModeInput and handler.setVibrationMode)
unchanged.
🪄 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: 135e0eaf-fc76-437c-a7ee-dc2f4e58beb9
📒 Files selected for processing (5)
app/src/main/java/app/gamenative/ui/screen/xserver/PhysicalControllerHandler.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/com/winlator/inputcontrols/ControllerManager.javaapp/src/main/java/com/winlator/winhandler/WinHandler.javaapp/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/winlator/inputcontrols/ControllerManager.java
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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/xenvironment/components/BionicProgramLauncherComponent.java (1)
306-323:⚠️ Potential issue | 🟠 MajorOnly add
libevshim.sotoLD_PRELOADwhen it is actually present.If the APK copy misses or fails on a fresh install,
LD_PRELOADstill points atimagefs/.../libevshim.so. That turns a recoverable rumble-deploy failure into a guest-launch failure because the dynamic loader aborts before Wine starts.Proposed fix
File apkEvshim = new File(context.getApplicationInfo().nativeLibraryDir, "libevshim.so"); File ifsEvshim = new File(evshimPath); + boolean hasEvshim = ifsEvshim.exists(); if (apkEvshim.exists()) { if (FileUtils.copy(apkEvshim, ifsEvshim)) { FileUtils.chmod(ifsEvshim, 0755); Log.i("EvshimDeploy", "Copied APK evshim -> " + evshimPath); + hasEvshim = true; } else { Log.e("EvshimDeploy", "Failed to copy APK evshim to " + evshimPath); } } @@ - ld_preload += ":" + evshimPath; + if (hasEvshim) { + ld_preload += ":" + evshimPath; + } else { + Log.w("EvshimDeploy", "libevshim.so unavailable; continuing without evshim preload"); + } ld_preload += ":" + replacePath;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java` around lines 306 - 323, The current logic always appends evshimPath to ld_preload even if the copy failed; change it so evshimPath is only appended when the file actually exists or the copy succeeded: after attempting the copy of apkEvshim to ifsEvshim (variables apkEvshim, ifsEvshim and call to FileUtils.copy) check if ifsEvshim.exists() (or the copy returned true) and only then append ":" + evshimPath to ld_preload and log the copy success; otherwise do not modify ld_preload with evshimPath and ensure envVars.put("LD_PRELOAD", ld_preload) only contains valid existing entries.
♻️ Duplicate comments (2)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (1)
1533-1533:⚠️ Potential issue | 🟡 MinorClamp imported vibration intensity before handing it to
WinHandler.This still accepts stale/container values outside the UI contract (
0..100). A bad import like-1or500makes rumble behavior depend on downstream validation instead of enforcing the boundary here.Proposed fix
- handler.setVibrationIntensity(container.getExtra("vibrationIntensity", "100").toIntOrNull() ?: 100) + val vibrationIntensity = container.getExtra("vibrationIntensity", "100") + .toIntOrNull() + ?.coerceIn(0, 100) + ?: 100 + handler.setVibrationIntensity(vibrationIntensity)🤖 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` at line 1533, The call to handler.setVibrationIntensity currently forwards an imported value that may be outside the UI-contract 0..100; parse the extra as you do now (container.getExtra("vibrationIntensity", "100").toIntOrNull() ?: 100) but then clamp it to the allowed range before calling handler.setVibrationIntensity (e.g., use Kotlin's coerceIn(0, 100) or an equivalent clamp) so that XServerScreen / handler.setVibrationIntensity always receives a value within 0..100.app/src/main/java/com/winlator/winhandler/WinHandler.java (1)
817-850:⚠️ Potential issue | 🟠 MajorAggregate phone rumble across slots before vibrating.
The handset
Vibratoris still treated as slot-local here. In"device"/"both"mode, each player’s start/refresh writes its own amplitude directly, so the last active slot overwrites the phone effect for everyone else. Multiplayer device rumble will be wrong whenever two slots are rumbling at the same time. Keep per-slot device amplitudes and apply one aggregated phone effect instead.Also applies to: 860-869
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 817 - 850, The phone vibrator is being written per-slot causing last-writer-wins; change the logic so vibrateDevice only applies an aggregated amplitude computed from all players' per-slot device amplitudes instead of each slot writing directly: keep per-player device amplitude state (e.g., add/maintain an array like deviceAmplitude[player] updated in startVibrationForPlayer/refresh code paths when vibrationMode is "device" or "both"), compute a single combined amplitude (e.g., max or summed/normalized) from that array, and call vibrateDevice once with the aggregated amplitude (instead of calling vibrateDevice from each slot); update stop/timeout paths to clear the per-player entry and reapply the aggregated vibration; touch vibrateDevice, startVibrationForPlayer, and any refresh/stop methods that currently call vibrateDevice.
🤖 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/PhysicalControllerHandler.kt`:
- Around line 35-37: The current devicesWithConfirmedHat set caches transient
deviceId ints, which breaks hotplug because Android can recycle IDs; switch the
key to a stable per-device identifier (e.g. InputDevice.getDescriptor() or
another persistent UID) wherever devicesWithConfirmedHat is used (the
mutableSetOf<Int>() declaration and the code paths that add/check/remove entries
in PhysicalControllerHandler), update the set type from Int to String (or
appropriate type), and ensure you add on first confirmed HAT using the stable
descriptor and remove/ignore entries on device disconnects so recycled numeric
deviceId values no longer cause incorrect DPAD suppression.
In `@app/src/main/java/com/winlator/inputcontrols/ControllerManager.java`:
- Around line 80-89: scanForDevices(), autoAssignDevice(), and other mutators
update detectedDevices and slotAssignments without synchronization while
rumblePollerThread (via WinHandler.resolveInputDeviceForPlayer and
getAssignedDeviceForSlot) reads them, causing ConcurrentModificationException;
fix by protecting this shared state with a single lock (e.g. add a private final
Object deviceStateLock) and wrap all reads and writes to detectedDevices and
slotAssignments (including in scanForDevices, autoAssignDevice,
getAssignedDeviceForSlot, and any code paths used by rumblePollerThread) in
synchronized(deviceStateLock) blocks, or alternatively publish immutable
snapshots (create unmodifiable copies into volatile fields) that the rumble
thread reads atomically; choose one approach and apply it consistently across
the referenced methods and the rumblePollerThread usage.
- Around line 98-123: evictDisconnectedAndCompact relies on getDeviceIdentifier
which can return non-unique fallback IDs causing identical-model controllers to
collapse; change the slot assignment to use a truly per-physical-device key
(e.g., a stable unique id generated from the InputDevice instance or a persisted
device-UUID mapped to the device instance) instead of the fallback string so two
same-model controllers keep separate entries; update usages of slotAssignments,
enabledSlots and the compaction/assignment code paths (including the other
compaction/assignment locations that also call getDeviceIdentifier) to look up
and store this physical-device-unique key and fall back to the non-unique
identifier only when guarded by a check that prevents reuse/eviction of an
already-mapped physical device.
---
Outside diff comments:
In
`@app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java`:
- Around line 306-323: The current logic always appends evshimPath to ld_preload
even if the copy failed; change it so evshimPath is only appended when the file
actually exists or the copy succeeded: after attempting the copy of apkEvshim to
ifsEvshim (variables apkEvshim, ifsEvshim and call to FileUtils.copy) check if
ifsEvshim.exists() (or the copy returned true) and only then append ":" +
evshimPath to ld_preload and log the copy success; otherwise do not modify
ld_preload with evshimPath and ensure envVars.put("LD_PRELOAD", ld_preload) only
contains valid existing entries.
---
Duplicate comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Line 1533: The call to handler.setVibrationIntensity currently forwards an
imported value that may be outside the UI-contract 0..100; parse the extra as
you do now (container.getExtra("vibrationIntensity", "100").toIntOrNull() ?:
100) but then clamp it to the allowed range before calling
handler.setVibrationIntensity (e.g., use Kotlin's coerceIn(0, 100) or an
equivalent clamp) so that XServerScreen / handler.setVibrationIntensity always
receives a value within 0..100.
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java`:
- Around line 817-850: The phone vibrator is being written per-slot causing
last-writer-wins; change the logic so vibrateDevice only applies an aggregated
amplitude computed from all players' per-slot device amplitudes instead of each
slot writing directly: keep per-player device amplitude state (e.g.,
add/maintain an array like deviceAmplitude[player] updated in
startVibrationForPlayer/refresh code paths when vibrationMode is "device" or
"both"), compute a single combined amplitude (e.g., max or summed/normalized)
from that array, and call vibrateDevice once with the aggregated amplitude
(instead of calling vibrateDevice from each slot); update stop/timeout paths to
clear the per-player entry and reapply the aggregated vibration; touch
vibrateDevice, startVibrationForPlayer, and any refresh/stop methods that
currently call vibrateDevice.
🪄 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: bebaa3b8-c517-4310-8747-13582f31524d
📒 Files selected for processing (5)
app/src/main/java/app/gamenative/ui/screen/xserver/PhysicalControllerHandler.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/com/winlator/inputcontrols/ControllerManager.javaapp/src/main/java/com/winlator/winhandler/WinHandler.javaapp/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java
Contributor
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/winhandler/WinHandler.java`:
- Around line 736-776: The Javadoc for rumbleViaVibratorManager incorrectly
claims DS4/DualSense motor order is swapped while the method always maps
lowFreq→ids[0] and highFreq→ids[1]; fix this by updating the comment to remove
or correct the DS4/DualSense swap claim (or, if you prefer to implement proper
swapping, add controller-type detection and swap the assignments of
lowFreq/highFreq before adding vibrators). Locate rumbleViaVibratorManager and
adjust the Javadoc description (or implement controller-type check and swap the
low/high assignments to ids[] accordingly) so the code and comment match.
🪄 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: ce583838-6508-4cad-b13d-e2c5f2ce4499
📒 Files selected for processing (4)
app/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/ui/screen/xserver/PhysicalControllerHandler.ktapp/src/main/java/com/winlator/inputcontrols/ControllerManager.javaapp/src/main/java/com/winlator/winhandler/WinHandler.java
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/app/gamenative/PrefManager.kt
- app/src/main/java/com/winlator/inputcontrols/ControllerManager.java
Contributor
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
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/winhandler/WinHandler.java">
<violation number="1" location="app/src/main/java/com/winlator/winhandler/WinHandler.java:739">
P3: Javadoc claims motor order is "swapped for DS4/DualSense controllers," but the method has no controller-type parameter and always assigns `lowFreq` → `ids[0]`, `highFreq` → `ids[1]` unconditionally. Either implement the swap or remove the inaccurate claim to avoid misleading future maintainers.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/main/java/com/winlator/inputcontrols/ControllerManager.java (1)
362-396: Nested locking inautoAssignDevicecauses potential deadlock.
autoAssignDeviceacquiresdeviceStateLockat line 373, then callsassignDeviceToSlot()(line 385) andsetSlotEnabled()(line 386), which also try to acquire the same lock. While Java's intrinsic locks are reentrant, the real issue is thatassignDeviceToSlot()andsetSlotEnabled()callsaveAssignments()outside the lock, potentially causing inconsistent state if another thread reads between the assignment and the save.Consider refactoring to avoid calling these public methods from within the synchronized block:
♻️ Suggested refactor
public int autoAssignDevice(int deviceId) { int existingSlot = getSlotForDevice(deviceId); if (existingSlot >= 0) { return isSlotEnabled(existingSlot) ? existingSlot : -1; } InputDevice device = inputManager.getInputDevice(deviceId); if (device == null || !isGameController(device)) { return -1; } + int assignedSlot = -1; synchronized (deviceStateLock) { - // Keep detectedDevices in sync so getAssignedDeviceForSlot can - // resolve this device without waiting for the next scanForDevices(). String identifier = getDeviceIdentifier(device); boolean alreadyDetected = false; for (InputDevice d : detectedDevices) { if (identifier.equals(getDeviceIdentifier(d))) { alreadyDetected = true; break; } } if (!alreadyDetected) detectedDevices.add(device); for (int i = 0; i < WinHandler.MAX_PLAYERS; i++) { if (slotAssignments.get(i) == null) { - assignDeviceToSlot(i, device); - setSlotEnabled(i, true); + // Remove from any other slot first + for (int j = 0; j < WinHandler.MAX_PLAYERS; j++) { + if (identifier.equals(slotAssignments.get(j))) { + slotAssignments.remove(j); + } + } + slotAssignments.put(i, identifier); + enabledSlots[i] = true; + assignedSlot = i; android.util.Log.i("ControllerSlot", "autoAssign: '" + device.getName() + "' -> slot=" + i); - return i; + break; } } } + if (assignedSlot >= 0) { + saveAssignments(); + return assignedSlot; + } android.util.Log.w("ControllerSlot", "autoAssign: no slot available for '" + device.getName() + "'"); return -1; }🤖 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/ControllerManager.java` around lines 362 - 396, autoAssignDevice currently synchronizes on deviceStateLock then calls assignDeviceToSlot() and setSlotEnabled(), which themselves acquire the same lock and call saveAssignments() outside the lock leading to a window of inconsistent state; fix by avoiding nested public calls inside the synchronized block: inside autoAssignDevice, perform the slot assignment and enabled-flag update directly on the backing data structures (the same fields modified by assignDeviceToSlot and setSlotEnabled) while holding deviceStateLock, and then call saveAssignments() while still synchronized or call a private helper that updates state without additional locking; reference methods/fields: autoAssignDevice, assignDeviceToSlot, setSlotEnabled, saveAssignments, deviceStateLock, and slotAssignments.app/src/main/java/com/winlator/winhandler/WinHandler.java (1)
736-776: Javadoc accurately reflects motor assignment; consider clarifying the underlying assumption.The Javadoc now correctly describes the motor assignment (low-frequency to
ids[0], high-frequency toids[1]) and no longer contains the inaccurate DS4/DualSense swap claim. This assignment aligns with Android's standard InputDevice vibrator ordering, where the left motor is at index 0 and the right motor is at index 1. The mapping of low-frequency bass rumble to the left (stronger) motor and high-frequency effects to the right (weaker) motor is sensible.However, add an inline comment clarifying that this assignment depends on controllers exposing vibrators in Android's standard order, as different manufacturers may vary their motor exposure or ordering. This documents the implicit assumption for future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 736 - 776, Add a short inline comment in the rumbleViaVibratorManager method (near the Javadoc or immediately before using ids[0] and ids[1]) stating that the mapping of low-frequency to ids[0] and high-frequency to ids[1] relies on controllers exposing vibrators in Android's standard order (left at index 0, right at index 1) and that some devices/vendors may expose or order multiple motors differently; reference ids[], vm.getVibrator(ids[0])/ids[1], and the blended fallback so maintainers know the assumption and where to adjust if a controller behaves differently.
🤖 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/com/winlator/inputcontrols/ControllerManager.java`:
- Around line 362-396: autoAssignDevice currently synchronizes on
deviceStateLock then calls assignDeviceToSlot() and setSlotEnabled(), which
themselves acquire the same lock and call saveAssignments() outside the lock
leading to a window of inconsistent state; fix by avoiding nested public calls
inside the synchronized block: inside autoAssignDevice, perform the slot
assignment and enabled-flag update directly on the backing data structures (the
same fields modified by assignDeviceToSlot and setSlotEnabled) while holding
deviceStateLock, and then call saveAssignments() while still synchronized or
call a private helper that updates state without additional locking; reference
methods/fields: autoAssignDevice, assignDeviceToSlot, setSlotEnabled,
saveAssignments, deviceStateLock, and slotAssignments.
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java`:
- Around line 736-776: Add a short inline comment in the
rumbleViaVibratorManager method (near the Javadoc or immediately before using
ids[0] and ids[1]) stating that the mapping of low-frequency to ids[0] and
high-frequency to ids[1] relies on controllers exposing vibrators in Android's
standard order (left at index 0, right at index 1) and that some devices/vendors
may expose or order multiple motors differently; reference ids[],
vm.getVibrator(ids[0])/ids[1], and the blended fallback so maintainers know the
assumption and where to adjust if a controller behaves differently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43a223ee-30ba-410e-ade6-3f1f345d9205
📒 Files selected for processing (4)
app/src/main/java/app/gamenative/ui/screen/xserver/PhysicalControllerHandler.ktapp/src/main/java/com/winlator/inputcontrols/ControllerManager.javaapp/src/main/java/com/winlator/winhandler/WinHandler.javaapp/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/src/main/java/com/winlator/winhandler/WinHandler.java (1)
860-892:⚠️ Potential issue | 🟠 MajorAggregate handset rumble before touching the phone vibrator.
Lines 869, 872, 885, and 890 still drive the global phone
Vibratorfrom a single player's amplitudes. In"device"/"both"mode that means last-writer-wins: one player's update overwrites another's phone intensity, and when the overwriting player stops, Lines 917-931 intentionally skipcancel()but never restore the remaining players' amplitude until the next long device refresh window. Multiplayer device rumble will stay stale or wrong for up to ~55s.Track per-player phone-rumble state and recompute one aggregated handset amplitude on every start/update/stop instead of calling
vibrateDevice(...)per slot.Also applies to: 894-932
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 860 - 892, startVibrationForPlayer and refreshVibrationForPlayer currently call vibrateDevice per player, causing last-writer-wins and stale handset intensity; instead maintain per-player handset state (e.g. arrays lowFreqPerPlayer[], highFreqPerPlayer[] indexed by player and reuse existing isRumbling[]) and replace direct calls to vibrateDevice(...) with a single helper (e.g. recomputeAndVibrateDevice()) that scans all players to compute the aggregated handset amplitude (max or appropriate merge of low/high) and invokes vibrateDevice once; update that helper from startVibrationForPlayer, refreshVibrationForPlayer, and any stop/clear logic so stopping a player clears its per-player entries and immediately recomputes the aggregate (respecting skipDevice and deviceRefresh flags).app/src/main/java/com/winlator/inputcontrols/ControllerManager.java (1)
106-138:⚠️ Potential issue | 🟠 MajorIdentical pads still collapse into one logical device on pre-Q.
These new compaction and auto-assignment paths treat
getDeviceIdentifier(...)as unique, but on older Android the fallback is onlyvendorId/productId. Two same-model controllers will therefore share the same key: the second pad can be dropped by thealreadyDetectedcheck on Lines 378-382, and compaction/assignment can merge or steal the other pad's slot. That breaks the multiplayer routing this PR adds.Please switch these paths to a truly per-physical-device key when
descriptoris unavailable, or at least avoid deduping/compacting controllers that only match on the non-unique fallback key.Also applies to: 373-399
🤖 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/ControllerManager.java` around lines 106 - 138, The compaction/dedup logic (evictDisconnectedAndCompact, the alreadyDetected check) relies on getDeviceIdentifier(...) which on pre-Q can return a non-unique vendorId/productId fallback; change the flow so controllers whose identifier came from the fallback are NOT treated as globally unique: detect when getDeviceIdentifier returned the fallback (i.e. absence of a proper descriptor/unique id) and then (1) in the alreadyDetected path avoid rejecting a new physical device that only matches a fallback id, and (2) in evictDisconnectedAndCompact avoid merging/evicting slots based solely on fallback-id matches (only compact when identifiers are truly unique or when you can derive a per-physical-device key such as device descriptor/uniqueId); update logic around slotAssignments, detectedDevices and enabledSlots to preserve distinct physical devices when identifier is the fallback.
🤖 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/com/winlator/inputcontrols/ControllerManager.java`:
- Around line 106-138: The compaction/dedup logic (evictDisconnectedAndCompact,
the alreadyDetected check) relies on getDeviceIdentifier(...) which on pre-Q can
return a non-unique vendorId/productId fallback; change the flow so controllers
whose identifier came from the fallback are NOT treated as globally unique:
detect when getDeviceIdentifier returned the fallback (i.e. absence of a proper
descriptor/unique id) and then (1) in the alreadyDetected path avoid rejecting a
new physical device that only matches a fallback id, and (2) in
evictDisconnectedAndCompact avoid merging/evicting slots based solely on
fallback-id matches (only compact when identifiers are truly unique or when you
can derive a per-physical-device key such as device descriptor/uniqueId); update
logic around slotAssignments, detectedDevices and enabledSlots to preserve
distinct physical devices when identifier is the fallback.
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java`:
- Around line 860-892: startVibrationForPlayer and refreshVibrationForPlayer
currently call vibrateDevice per player, causing last-writer-wins and stale
handset intensity; instead maintain per-player handset state (e.g. arrays
lowFreqPerPlayer[], highFreqPerPlayer[] indexed by player and reuse existing
isRumbling[]) and replace direct calls to vibrateDevice(...) with a single
helper (e.g. recomputeAndVibrateDevice()) that scans all players to compute the
aggregated handset amplitude (max or appropriate merge of low/high) and invokes
vibrateDevice once; update that helper from startVibrationForPlayer,
refreshVibrationForPlayer, and any stop/clear logic so stopping a player clears
its per-player entries and immediately recomputes the aggregate (respecting
skipDevice and deviceRefresh flags).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d31fe50-1d95-43e4-8962-18d5a2237bd0
📒 Files selected for processing (2)
app/src/main/java/com/winlator/inputcontrols/ControllerManager.javaapp/src/main/java/com/winlator/winhandler/WinHandler.java
Author
|
All current AI bot comments addressed. The two remaining CodeRabbit duplicates are concerns already triaged. |
Author
|
Doing an optimization pass now. |
TideGear
added a commit
to TideGear/GameNative
that referenced
this pull request
Apr 16, 2026
Fixes all issues currently identified in the PR utkarshdalal#1214 code review. Changes span evshim.c, WinHandler.java, PhysicalControllerHandler.kt, XServerScreen.kt, and ControllerManager.java. Vibration correctness: - scaleAmplitude: replace MSB-only scaling ((rawFreq >> 8) & 0xFF) with full 16-bit-to-255 mapping (Math.round(unsigned * 255.0 / 65535.0)) so XInput rumble values 1-255 are no longer silently dropped - evshim keepalive race fix: add per-player atomic_int g_keepalive_active[] flag; OnRumble() is now a no-op when called re-entrantly from the SDL keepalive, preventing stuck-on vibration after the game sends stop (0, 0) - stopVibrationForPlayer: cancel only via VibratorManager on API >= 31 (not both VibratorManager and the legacy Vibrator, which double-cancels the same hardware and can cause motor jitter) - vibrationMode / vibrationIntensity declared volatile so the rumble poller thread always sees updates written by the UI thread - rumbleViaVibratorManager: identify low-freq vs high-freq motor by sorting vibrator IDs ascending (kernel enumerates in HID descriptor order; for XInput-compatible controllers this is heavy-then-light = ascending); log the chosen motor IDs for field verification Input isolation / disconnect cleanup: - PhysicalControllerHandler: add deviceIdToDescriptor reverse map populated when a HAT MotionEvent is first confirmed; add onDeviceDisconnected(deviceId) that releases held axis directions and removes the descriptor from devicesWithConfirmedHat so stale state cannot affect a new controller that reuses the same recycled deviceId - XServerScreen: register InputManager.InputDeviceListener in the existing DisposableEffect(Unit) block; forward onInputDeviceRemoved to physicalControllerHandler.onDeviceDisconnected(); unregister on dispose Crash / correctness fixes: - ControllerManager.autoAssignDevice: null-guard getDeviceIdentifier() result before use to prevent NPE on devices where getDescriptor() returns null Code quality: - Pre-build AudioAttributes (static final) and VibrationAttributes (instance final, built once in constructor with API check for USAGE_MEDIA on API 33+); remove buildVibrationAttrs() and buildAudioAttrs() helper methods that allocated new objects on every 240 ms keepalive tick - evshim OnRumble: simplify redundant if/else for last_rumble assignment to a direct unconditional write (both branches assigned the arguments) - WinHandler.startRumblePoller: remove stale comment claiming phone vibration is unimplemented (it is implemented) and fix stray backslash character Build: - Add build-evshim.ps1: PowerShell script to recompile evshim.c into app/src/main/jniLibs/arm64-v8a/libevshim.so using NDK 26.1.10909125; uses args-array splatting to avoid PowerShell backtick-continuation parse errors - Recompile libevshim.so with all evshim.c changes applied
Contributor
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
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/winhandler/WinHandler.java">
<violation number="1" location="app/src/main/java/com/winlator/winhandler/WinHandler.java:716">
P2: 0% vibration intensity cannot fully disable rumble because `scaleAmplitude()` forces a minimum amplitude of 1 for any non-zero input.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
app/src/main/java/com/winlator/inputcontrols/ControllerManager.java (1)
377-383:⚠️ Potential issue | 🟠 MajorFallback IDs still collapse identical controllers on pre-Q.
When
getDeviceIdentifier()falls back tovendor/product, a second same-model pad is treated as already detected here, so it can reuse the first pad's slot and misroute multiplayer input/rumble on older Android versions.🤖 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/ControllerManager.java` around lines 377 - 383, The code treats two identical-model controllers as the same when getDeviceIdentifier() returns the vendor/product fallback; update ControllerManager so when getDeviceIdentifier(device) returns a fallback value you also compare a per-instance unique token (e.g., InputDevice.getDescriptor() or device.getId()) to distinguish distinct devices. Concretely, change the detectedDevices duplicate-check around getDeviceIdentifier(...) to: compute id = getDeviceIdentifier(device); if id is the fallback form, compare both id and device.getDescriptor() (or device.getId()) against each detected device (using the same descriptor/id for detected devices), otherwise keep the existing identifier-only comparison.app/src/main/java/com/winlator/winhandler/WinHandler.java (1)
848-865:⚠️ Potential issue | 🟠 MajorThe handset vibrator is still being driven per player.
vibrateDevice()applies one slot's amplitude directly to the shared phone vibrator. In"device"/"both"mode, another player's start/refresh overwrites the current handset rumble instead of combining with it, so multiplayer device haptics are still wrong even though the stop-path now avoids global cancels.Also applies to: 872-903
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 848 - 865, vibrateDevice currently writes a single player's amplitude to the shared handset vibrator causing overwrites when multiple players are active; change the logic so instead of directly calling vibrateSingle(phoneVibrator, ...) with one slot's computed phoneAmp, maintain a concurrent per-player amplitude map (keyed by player ID used by start/refresh/stop paths) and compute a combined handset amplitude (e.g., clamp(sum of active amplitudes or use max, then apply the same curvature scaling) before calling vibrateSingle); update the map on player start/refresh and remove/zero on player stop so vibrateDevice (or the code that currently calls it) reads the aggregated value and applies a single vibrator command, ensuring thread-safety around the map updates and clamping final phoneAmp to 1..255 and cancelling only when the combined amplitude is zero.
🧹 Nitpick comments (3)
build-evshim.ps1 (2)
24-31: Avoid build-flag drift with the CMakeevshimtarget.The clang flags here duplicate
app/src/main/cpp/extras/CMakeLists.txt(add_library(evshim ...), includes, anddllinkage). Keeping two build definitions risks silent divergence over time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build-evshim.ps1` around lines 24 - 31, The PowerShell script duplicates compiler/linker flags already declared in the CMake evshim target (see add_library(evshim ...) and its include paths and dl linkage in app/src/main/cpp/extras/CMakeLists.txt), which risks drift; change build-evshim.ps1 to invoke CMake to configure/build the evshim target (or read flags from CMake output) instead of hardcoding "-shared", "-fPIC", "-O2", include paths, and "-ldl" in $args so the single source of truth remains the CMake add_library(evshim ...) definition.
16-16: Ensure output directory exists before invoking clang.Line 16 writes into
app\src\main\jniLibs\arm64-v8a\...; this can fail on a fresh clone if the directory is absent.♻️ Proposed hardening
$OUT = "app\src\main\jniLibs\arm64-v8a\libevshim.so" + +$OUT_DIR = Split-Path -Parent $OUT +if (-not (Test-Path $OUT_DIR)) { + New-Item -ItemType Directory -Path $OUT_DIR -Force | Out-Null +}Also applies to: 33-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build-evshim.ps1` at line 16, Ensure the output directory exists before invoking clang by creating the parent directory of the output path referenced by $OUT (and the other output variable used later) if it doesn't exist; locate the usages of $OUT in build-evshim.ps1 (and the second occurrence around the later clang invocation) and add a pre-flight step that gets the parent directory (using Split-Path -Parent on $OUT) and creates it with New-Item -ItemType Directory -Force (or Test-Path + New-Item) so writes to the target lib directory cannot fail on a fresh clone.app/src/main/java/com/winlator/winhandler/WinHandler.java (1)
758-760: Log the motor mapping once, not on every refresh.This branch is on the keepalive path, so sustained rumble will emit
Log.dseveral times per second per controller. The comment says the IDs are logged on first use; a one-time guard keyed by device/descriptor would keep this out of the hot path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 758 - 760, The Log.d call inside the keepalive branch is executed on every refresh; change it to log the mapping only once per device by adding a one-time guard keyed by the device/descriptor (e.g., use a Set or Map of seen device identifiers in the WinHandler class) and check that before logging; keep the existing logic that sets lowMotorId/highMotorId when ids.length >= 2 and only call Log.d(TAG, ...) when the device id/descriptor is not yet recorded, then mark it as logged so subsequent keepalive cycles skip logging.
🤖 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/inputcontrols/ControllerManager.java`:
- Around line 385-397: When auto-assigning a controller in the loop over
WinHandler.MAX_PLAYERS, do not flip persisted enabledSlots back to true; instead
only consider slots where enabledSlots[i] is already true (or leave enabledSlots
untouched) and remove the assignment enabledSlots[i] = true. Update the block
that mutates slotAssignments and assignedSlot (the loop that checks
identifier.equals(slotAssignments.get(j)) and then does slotAssignments.put(i,
identifier); enabledSlots[i] = true; assignedSlot = i;) to skip disabled slots
(e.g., if (!enabledSlots[i]) continue;) and delete the enabledSlots mutation so
the user’s saved enabledSlots preference is preserved and saveAssignments()
remains called once.
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java`:
- Around line 221-230: When swapping controllers in setControllerForSlot, you
clear lastLowFreqs/lastHighFreqs and deviceRumbleTickCtr but forget to zero the
slot's rumble bytes in the underlying output buffer (the bytes at offsets 32 and
34), which allows stale rumble to be applied to the new controller; update
setControllerForSlot to also write zero to those rumble buffer positions for the
given slot (in the same place you call stopVibrationForPlayer and reset
lastLowFreqs/lastHighFreqs/deviceRumbleTickCtr) so the next poll won't treat old
rumble as a change.
- Around line 913-926: In stopVibrationForPlayer, when API >= S you only cancel
via VibratorManager if getVibratorIds().length > 0, but vibrateController may
have fallen back to device.getVibrator() when rumbleViaVibratorManager()
returned false; update stopVibrationForPlayer so that on API >= S you still call
device.getVibrator().cancel() when vm.getVibratorIds().length == 0 (i.e.,
fallback case). Locate methods stopVibrationForPlayer and vibrateController and
add the fallback vibrator cancellation in the branch handling
Build.VERSION.SDK_INT >= Build.VERSION_CODES.S so the one-shot pulse is stopped
immediately.
In `@build-evshim.ps1`:
- Line 24: The script assigns to the automatic PowerShell variable $args which
clobbers positional parameters; rename the variable (e.g. $evshimArgs or
$scriptArgs) wherever it’s defined and referenced (the assignment at $args =
@(...) and the later occurrence around line 33) and update all uses in
build-evshim.ps1 (functions, invocations, splatting, etc.) to the new name to
avoid interfering with PowerShell’s automatic $args.
---
Duplicate comments:
In `@app/src/main/java/com/winlator/inputcontrols/ControllerManager.java`:
- Around line 377-383: The code treats two identical-model controllers as the
same when getDeviceIdentifier() returns the vendor/product fallback; update
ControllerManager so when getDeviceIdentifier(device) returns a fallback value
you also compare a per-instance unique token (e.g., InputDevice.getDescriptor()
or device.getId()) to distinguish distinct devices. Concretely, change the
detectedDevices duplicate-check around getDeviceIdentifier(...) to: compute id =
getDeviceIdentifier(device); if id is the fallback form, compare both id and
device.getDescriptor() (or device.getId()) against each detected device (using
the same descriptor/id for detected devices), otherwise keep the existing
identifier-only comparison.
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java`:
- Around line 848-865: vibrateDevice currently writes a single player's
amplitude to the shared handset vibrator causing overwrites when multiple
players are active; change the logic so instead of directly calling
vibrateSingle(phoneVibrator, ...) with one slot's computed phoneAmp, maintain a
concurrent per-player amplitude map (keyed by player ID used by
start/refresh/stop paths) and compute a combined handset amplitude (e.g.,
clamp(sum of active amplitudes or use max, then apply the same curvature
scaling) before calling vibrateSingle); update the map on player start/refresh
and remove/zero on player stop so vibrateDevice (or the code that currently
calls it) reads the aggregated value and applies a single vibrator command,
ensuring thread-safety around the map updates and clamping final phoneAmp to
1..255 and cancelling only when the combined amplitude is zero.
---
Nitpick comments:
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java`:
- Around line 758-760: The Log.d call inside the keepalive branch is executed on
every refresh; change it to log the mapping only once per device by adding a
one-time guard keyed by the device/descriptor (e.g., use a Set or Map of seen
device identifiers in the WinHandler class) and check that before logging; keep
the existing logic that sets lowMotorId/highMotorId when ids.length >= 2 and
only call Log.d(TAG, ...) when the device id/descriptor is not yet recorded,
then mark it as logged so subsequent keepalive cycles skip logging.
In `@build-evshim.ps1`:
- Around line 24-31: The PowerShell script duplicates compiler/linker flags
already declared in the CMake evshim target (see add_library(evshim ...) and its
include paths and dl linkage in app/src/main/cpp/extras/CMakeLists.txt), which
risks drift; change build-evshim.ps1 to invoke CMake to configure/build the
evshim target (or read flags from CMake output) instead of hardcoding "-shared",
"-fPIC", "-O2", include paths, and "-ldl" in $args so the single source of truth
remains the CMake add_library(evshim ...) definition.
- Line 16: Ensure the output directory exists before invoking clang by creating
the parent directory of the output path referenced by $OUT (and the other output
variable used later) if it doesn't exist; locate the usages of $OUT in
build-evshim.ps1 (and the second occurrence around the later clang invocation)
and add a pre-flight step that gets the parent directory (using Split-Path
-Parent on $OUT) and creates it with New-Item -ItemType Directory -Force (or
Test-Path + New-Item) so writes to the target lib directory cannot fail on a
fresh clone.
🪄 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: 5c92cfd8-1ea3-4323-b9b9-06b8f335164e
⛔ Files ignored due to path filters (1)
app/src/main/jniLibs/arm64-v8a/libevshim.sois excluded by!**/*.so
📒 Files selected for processing (6)
app/src/main/cpp/extras/evshim.capp/src/main/java/app/gamenative/ui/screen/xserver/PhysicalControllerHandler.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/com/winlator/inputcontrols/ControllerManager.javaapp/src/main/java/com/winlator/winhandler/WinHandler.javabuild-evshim.ps1
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/cpp/extras/evshim.c
- app/src/main/java/app/gamenative/ui/screen/xserver/PhysicalControllerHandler.kt
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
build-evshim.ps1 (1)
10-10: Use SDK env-var fallbacks before defaulting toLOCALAPPDATA.Hardcoding only
$env:LOCALAPPDATA\Android\Sdkmakes the script fail on machines usingANDROID_SDK_ROOTorANDROID_HOME.Suggested refactor
-$SDK_ROOT = "$env:LOCALAPPDATA\Android\Sdk" +$SDK_ROOT = if ($env:ANDROID_SDK_ROOT) { + $env:ANDROID_SDK_ROOT +} elseif ($env:ANDROID_HOME) { + $env:ANDROID_HOME +} else { + "$env:LOCALAPPDATA\Android\Sdk" +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build-evshim.ps1` at line 10, The script hardcodes $SDK_ROOT to "$env:LOCALAPPDATA\Android\Sdk" which breaks machines that set ANDROID_SDK_ROOT or ANDROID_HOME; update the assignment of $SDK_ROOT to prefer environment fallbacks in order (use $env:ANDROID_SDK_ROOT, then $env:ANDROID_HOME) and only default to "$env:LOCALAPPDATA\Android\Sdk" if the other vars are empty or unset; reference and update the $SDK_ROOT variable assignment in build-evshim.ps1 and ensure the chosen value is validated (non-empty) before continuing.app/src/main/java/com/winlator/inputcontrols/ControllerManager.java (1)
161-177: Snapshot slot state underdeviceStateLock.
saveAssignments()is now the only accessor that still readsslotAssignmentsandenabledSlotsunlocked. Taking a local snapshot under the lock would keep the new concurrency boundary consistent and avoid ever persisting a mixed snapshot if a future caller invokes this off the main thread.♻️ Suggested cleanup
public void saveAssignments() { + String[] assigned = new String[WinHandler.MAX_PLAYERS]; + boolean[] enabled = new boolean[WinHandler.MAX_PLAYERS]; + synchronized (deviceStateLock) { + for (int i = 0; i < WinHandler.MAX_PLAYERS; i++) { + assigned[i] = slotAssignments.get(i); + enabled[i] = enabledSlots[i]; + } + } + SharedPreferences.Editor editor = preferences.edit(); for (int i = 0; i < WinHandler.MAX_PLAYERS; i++) { // Save the assigned device identifier - String deviceIdentifier = slotAssignments.get(i); + String deviceIdentifier = assigned[i]; String prefKey = PREF_PLAYER_SLOT_PREFIX + i; if (deviceIdentifier != null) { editor.putString(prefKey, deviceIdentifier); } else { editor.remove(prefKey); } // Save the enabled state String enabledKey = PREF_ENABLED_SLOTS_PREFIX + i; - editor.putBoolean(enabledKey, enabledSlots[i]); + editor.putBoolean(enabledKey, enabled[i]); } editor.apply(); }🤖 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/ControllerManager.java` around lines 161 - 177, saveAssignments reads shared slotAssignments and enabledSlots without synchronization; take a consistent snapshot under deviceStateLock and persist that snapshot instead. Inside saveAssignments, acquire deviceStateLock and copy slotAssignments into a local List<String> (or String[] sized to WinHandler.MAX_PLAYERS) and copy enabledSlots with Arrays.copyOf into a local boolean[]; release the lock, then create SharedPreferences.Editor and loop over the local snapshots writing PREF_PLAYER_SLOT_PREFIX + i and PREF_ENABLED_SLOTS_PREFIX + i, and finally editor.apply(); use the existing method name saveAssignments and existing fields slotAssignments, enabledSlots and deviceStateLock to locate where to add the snapshot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build-evshim.ps1`:
- Around line 31-32: The comment containing a non-ASCII em dash should be
changed to use plain ASCII (e.g., replace the em dash with a regular
hyphen/minus) or the file saved as UTF-8 with BOM to avoid
PSUseBOMForUnicodeEncodedFile warnings; update the comment near the reference to
$clangArgs and $args so it reads using only ASCII characters (e.g., "Use
$clangArgs (not $args) - $args is a PowerShell automatic variable...") to
resolve the encoding warning.
- Around line 9-13: The NDK version constant NDK_VERSION in build-evshim.ps1 is
pinned to 26.1.10909125 which mismatches the Gradle pinned version; update
NDK_VERSION to 22.1.7171670 to match app/build.gradle.kts and adjust any derived
values (NDK_ROOT and CLANG) accordingly so the computed paths (NDK_ROOT and
CLANG) point to the correct NDK installation for that version. Ensure the CLANG
tool path still targets the aarch64-linux-android21-clang.cmd under the new
NDK_ROOT or update the tool name/ABI number if the 22.1 NDK uses a different
prebuilt layout.
---
Nitpick comments:
In `@app/src/main/java/com/winlator/inputcontrols/ControllerManager.java`:
- Around line 161-177: saveAssignments reads shared slotAssignments and
enabledSlots without synchronization; take a consistent snapshot under
deviceStateLock and persist that snapshot instead. Inside saveAssignments,
acquire deviceStateLock and copy slotAssignments into a local List<String> (or
String[] sized to WinHandler.MAX_PLAYERS) and copy enabledSlots with
Arrays.copyOf into a local boolean[]; release the lock, then create
SharedPreferences.Editor and loop over the local snapshots writing
PREF_PLAYER_SLOT_PREFIX + i and PREF_ENABLED_SLOTS_PREFIX + i, and finally
editor.apply(); use the existing method name saveAssignments and existing fields
slotAssignments, enabledSlots and deviceStateLock to locate where to add the
snapshot.
In `@build-evshim.ps1`:
- Line 10: The script hardcodes $SDK_ROOT to "$env:LOCALAPPDATA\Android\Sdk"
which breaks machines that set ANDROID_SDK_ROOT or ANDROID_HOME; update the
assignment of $SDK_ROOT to prefer environment fallbacks in order (use
$env:ANDROID_SDK_ROOT, then $env:ANDROID_HOME) and only default to
"$env:LOCALAPPDATA\Android\Sdk" if the other vars are empty or unset; reference
and update the $SDK_ROOT variable assignment in build-evshim.ps1 and ensure the
chosen value is validated (non-empty) before continuing.
🪄 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: 968b5300-684c-4d5a-a57f-a14836e201f6
📒 Files selected for processing (3)
app/src/main/java/com/winlator/inputcontrols/ControllerManager.javaapp/src/main/java/com/winlator/winhandler/WinHandler.javabuild-evshim.ps1
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/winlator/winhandler/WinHandler.java
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Author
|
Looks like the bots have calmed down. |
Contributor
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
Author
|
@cubic-dev-ai review |
Author
|
@coderabbitai autofix |
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Failed to clone repository into sandbox. Please try again. |
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Failed to clone repository into sandbox. Please try again. |
Author
|
Should be all set except for that last bot comment. Can't fix that right now. Are you able to do it for me @utkarshdalal ? If not, I'll try to get to it in a day or so. |
Owner
|
Thanks - this comment? |
Author
|
@utkarshdalal I desperately crave the digital approval of CodeRabbit, so I did it anyway. Haha, should be good to go. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java (2)
282-299:⚠️ Potential issue | 🟠 MajorDon’t preload a stale evshim after a failed APK copy.
If
apkEvshimexists butFileUtils.copy(...)fails,ifsEvshim.exists()can still be true from an older deployment, so this still preloads a stalelibevshim.so. Track whether the refresh succeeded and only appendevshimPathwhen the copy succeeded, or when no APK refresh was attempted.Suggested fix
File apkEvshim = new File(context.getApplicationInfo().nativeLibraryDir, "libevshim.so"); File ifsEvshim = new File(evshimPath); + boolean evshimReady = false; if (apkEvshim.exists()) { if (FileUtils.copy(apkEvshim, ifsEvshim)) { FileUtils.chmod(ifsEvshim, 0755); + evshimReady = ifsEvshim.exists(); Log.i("EvshimDeploy", "Copied APK evshim -> " + evshimPath); } else { Log.e("EvshimDeploy", "Failed to copy APK evshim to " + evshimPath); } + } else if (ifsEvshim.exists()) { + evshimReady = true; } if (new File(sysvPath).exists()) ld_preload += sysvPath; - if (ifsEvshim.exists()) { + if (evshimReady) { ld_preload += (ld_preload.isEmpty() ? "" : ":") + evshimPath; } else { Log.w("EvshimDeploy", "evshim not present at " + evshimPath + "; skipping LD_PRELOAD entry"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java` around lines 282 - 299, The code can preload a stale evshim if apkEvshim exists but FileUtils.copy(apkEvshim, ifsEvshim) fails; modify the logic in BionicProgramLauncherComponent to track whether a refresh was attempted and whether it succeeded (e.g., a boolean copyAttempted / copySucceeded), set copyAttempted = true when apkEvshim.exists() and set copySucceeded to the FileUtils.copy result, only append evshimPath to ld_preload when either no APK refresh was attempted (apkEvshim did not exist) or when copySucceeded is true; update the condition that currently checks ifsEvshim.exists() before adding evshimPath (and keep the Log.w path for the skipped case).
182-190:⚠️ Potential issue | 🟠 MajorRestore dynamic evshim player provisioning.
This change regresses evshim back to single-player. In
app/src/main/cpp/extras/evshim.c, startup only opensgamepad*.memforEVSHIM_MAX_PLAYERS, so forcing"1"and creating onlygamepad.memmeans player 2+ never gets mapped even if the rest of the input stack detects them. Please restore the dynamic controller count here and pre-create the matchinggamepad1.mem,gamepad2.mem, etc. files before launch.Also applies to: 214-214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java` around lines 182 - 190, The code currently pre-creates only a single "gamepad.mem" file which regresses evshim to single-player; update BionicProgramLauncherComponent to create the full set of mem files that evshim expects by iterating from 1..EVSHIM_MAX_PLAYERS and pre-creating "gamepad1.mem", "gamepad2.mem", etc. (and keep the original "gamepad.mem" if required by evshim) instead of a single hard-coded path; use the existing EVSHIM_MAX_PLAYERS constant (or the same numeric value used in app/src/main/cpp/extras/evshim.c / any Java constant) to drive the loop and perform the same mkdirs/RandomAccessFile length setup for each filename.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/PrefManager.kt (1)
219-231: Consider exposing a canonical ordered vibration-mode list.Mode values are duplicated in UI and prefs code. Exposing one shared ordered source (for dropdown + validation) would prevent drift if options change later.
Refactor sketch
- private val VALID_VIBRATION_MODES = setOf("off", "controller", "device", "both") + val VIBRATION_MODE_VALUES = listOf("off", "controller", "device", "both") + private val VALID_VIBRATION_MODES = VIBRATION_MODE_VALUES.toSet()- val vibrationModeValues = listOf("off", "controller", "device", "both") + val vibrationModeValues = PrefManager.VIBRATION_MODE_VALUES🤖 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 219 - 231, VALID_VIBRATION_MODES and DEFAULT_VIBRATION_MODE are duplicated sources of truth; extract and expose a single ordered canonical list (e.g., VIBRATION_MODE_ORDER or a VibrationMode enum/list) to be used by normalizeVibrationMode, normalizeVibrationModeInput and the UI dropdown so the displayed order and validation stay consistent; change VALID_VIBRATION_MODES to derive from that canonical list (or replace it), keep DEFAULT_VIBRATION_MODE as the first/explicit element of that canonical list, and update normalizeVibrationMode to validate against the canonical list.
🤖 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/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java`:
- Around line 282-299: The code can preload a stale evshim if apkEvshim exists
but FileUtils.copy(apkEvshim, ifsEvshim) fails; modify the logic in
BionicProgramLauncherComponent to track whether a refresh was attempted and
whether it succeeded (e.g., a boolean copyAttempted / copySucceeded), set
copyAttempted = true when apkEvshim.exists() and set copySucceeded to the
FileUtils.copy result, only append evshimPath to ld_preload when either no APK
refresh was attempted (apkEvshim did not exist) or when copySucceeded is true;
update the condition that currently checks ifsEvshim.exists() before adding
evshimPath (and keep the Log.w path for the skipped case).
- Around line 182-190: The code currently pre-creates only a single
"gamepad.mem" file which regresses evshim to single-player; update
BionicProgramLauncherComponent to create the full set of mem files that evshim
expects by iterating from 1..EVSHIM_MAX_PLAYERS and pre-creating "gamepad1.mem",
"gamepad2.mem", etc. (and keep the original "gamepad.mem" if required by evshim)
instead of a single hard-coded path; use the existing EVSHIM_MAX_PLAYERS
constant (or the same numeric value used in app/src/main/cpp/extras/evshim.c /
any Java constant) to drive the loop and perform the same
mkdirs/RandomAccessFile length setup for each filename.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/PrefManager.kt`:
- Around line 219-231: VALID_VIBRATION_MODES and DEFAULT_VIBRATION_MODE are
duplicated sources of truth; extract and expose a single ordered canonical list
(e.g., VIBRATION_MODE_ORDER or a VibrationMode enum/list) to be used by
normalizeVibrationMode, normalizeVibrationModeInput and the UI dropdown so the
displayed order and validation stay consistent; change VALID_VIBRATION_MODES to
derive from that canonical list (or replace it), keep DEFAULT_VIBRATION_MODE as
the first/explicit element of that canonical list, and update
normalizeVibrationMode to validate against the canonical list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 41b709da-848b-4c60-8868-aa2aea46b334
⛔ Files ignored due to path filters (1)
app/src/main/jniLibs/arm64-v8a/libevshim.sois excluded by!**/*.so
📒 Files selected for processing (6)
app/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/ContainerUtils.ktapp/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.javaapp/src/main/res/values/strings.xml
✅ Files skipped from review due to trivial changes (2)
- app/src/main/res/values/strings.xml
- app/src/main/java/app/gamenative/utils/ContainerUtils.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
…ontroller Layered the vibration improvements from the pre-cleanup branch (commit 878469f^) onto the contributor's multi-controller PR (utkarshdalal#1261, merged upstream as dacbfa0). Upstream's slot infrastructure (getBufferForSlot/setControllerForSlot/resolveControllerSlot) is preserved unchanged; the additions are purely vibration quality. What this changes: - WinHandler.java: per-slot rumble poller wakes via FileObserver (inotify) instead of a 20 ms sleep loop, dispatches dual-motor rumble via VibratorManager (API 31+) with ascending-ID motor sort, applies a 240 ms keepalive cycle so long rumbles don't get cut off, refreshes the InputDevice handle periodically to handle hot-swap mid-rumble, and scales amplitude by the user's intensity preference. - evshim.c: per-player atomic g_keepalive_active flag + per-player last_rumble cache + inotify-based wake replace the prior 5 ms poll fallback, eliminating dropped/stuck rumbles on rapid game-side pulses. Includes the narrow-rumble re-entry guard (4a08598). - libevshim.so: rebuilt against new evshim.c (26288 bytes). Settings UI (ControllerTab): - Vibration Mode dropdown: off / controller / device. (Pre-cleanup had a 'both' option; dropped per design — pick one target, not both.) - Vibration Intensity slider 0-100%, hidden when mode is 'off'. Persistence: - ContainerData: vibrationMode + vibrationIntensity fields with saver / restorer wiring. - ContainerUtils: roundtrips both fields between PrefManager defaults, ContainerData, and Container.extras. - PrefManager: VIBRATION_MODE + VIBRATION_INTENSITY prefs with normalizeVibrationModeInput() helper for safe deserialization. - XServerScreen: pushes per-container vibration mode + intensity into WinHandler at game launch. What is intentionally left as upstream: - All multi-controller routing, slot assignment, hot-plug, and the ControllerManager / PhysicalControllerHandler integration. The contributor's design for those is what the PR keeps. build-evshim.ps1 and the SDL2 stub header are added to support recompiling libevshim.so without an SDL2 SDK install.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/winhandler/WinHandler.java (1)
597-615:⚠️ Potential issue | 🔴 CriticalRecompute the enabled controller state after rejecting a mismatched
gamepadId.
enabled3is captured before Line 602 clearscurrentController, so a stale request can still enter the enabled branch and then dereferencethis.currentController.stateinside the async send action. That can NPE the sender thread and report an enabled pad when no matching controller exists.🛠 Suggested fix
- ExternalController externalController2 = this.currentController; - final boolean enabled3 = externalController2 != null || useVirtualGamepad2; - if (externalController2 != null && externalController2.getDeviceId() != gamepadId) { - this.currentController = null; - } + ExternalController matchedController = this.currentController; + if (matchedController != null && matchedController.getDeviceId() != gamepadId) { + matchedController = null; + this.currentController = null; + } + final ExternalController controllerForState = matchedController; + final boolean enabled3 = controllerForState != null || useVirtualGamepad2; addAction(() -> { sendData.rewind(); sendData.put(RequestCodes.GET_GAMEPAD_STATE); this.sendData.put((byte)(enabled3 ? 1 : 0)); if (enabled3) { this.sendData.putInt(gamepadId); if (useVirtualGamepad2) { inputControlsView.getProfile().getGamepadState().writeTo(this.sendData); } else { - this.currentController.state.writeTo(this.sendData); + controllerForState.state.writeTo(this.sendData); } } sendPacket(port); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 597 - 615, enabled3 is computed before currentController may be cleared, which can let the async addAction lambda enter the enabled branch and dereference this.currentController.state causing NPE; fix by moving or recomputing the enabled check inside the addAction closure (or after the currentController nulling) so it uses the up-to-date value—specifically, inside the lambda used with addAction (the block that writes RequestCodes.GET_GAMEPAD_STATE to sendData and accesses this.currentController.state), re-evaluate whether a controller is enabled (using inputControlsView, profile2 / useVirtualGamepad2 and this.currentController/deviceId/gamepadId) and only access this.currentController.state when the recomputed enabled result confirms a valid currentController, or prefer using useVirtualGamepad2 path when appropriate.
♻️ Duplicate comments (4)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (2)
1840-1845:⚠️ Potential issue | 🟡 MinorClamp
vibrationIntensitybefore callingWinHandler.Line 1845 still forwards raw extras, so stale/imported values like
-1or500can escape the0..100contract introduced by this feature.🔧 Proposed fix
handler.setVibrationMode( PrefManager.normalizeVibrationModeInput( container.getExtra("vibrationMode", "controller"), ), ) - handler.setVibrationIntensity(container.getExtra("vibrationIntensity", "100").toIntOrNull() ?: 100) + val vibrationIntensity = container.getExtra("vibrationIntensity", "100") + .toIntOrNull() + ?.coerceIn(0, 100) + ?: 100 + handler.setVibrationIntensity(vibrationIntensity)🤖 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 1840 - 1845, The code passes unvalidated vibrationIntensity from container.getExtra into handler.setVibrationIntensity, allowing values outside 0..100; before calling handler.setVibrationIntensity (and anywhere WinHandler expects the 0..100 contract), parse the extra into an Int and clamp it into the valid range (e.g., using Int.coerceIn(0, 100)) and then pass that clamped value; update the call site that currently does container.getExtra("vibrationIntensity", "100").toIntOrNull() ?: 100 to ensure the parsed value is coerced into 0..100 before invoking handler.setVibrationIntensity (and keep PrefManager.normalizeVibrationModeInput usage unchanged for vibrationMode).
1187-1193:⚠️ Potential issue | 🟠 MajorGate
game_closedbehind the analytics opt-in.This event still bypasses
PrefManager.usageAnalyticsEnabled, so users who opted out will emit close telemetry from the quick menu.🔧 Proposed fix
- PostHog.capture( - event = "game_closed", - properties = mapOf( - "game_name" to ContainerUtils.resolveGameName(appId), - "game_store" to ContainerUtils.extractGameSourceFromContainerId(appId).name, - ), - ) + if (PrefManager.usageAnalyticsEnabled) { + PostHog.capture( + event = "game_closed", + properties = mapOf( + "game_name" to ContainerUtils.resolveGameName(appId), + "game_store" to ContainerUtils.extractGameSourceFromContainerId(appId).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/ui/screen/xserver/XServerScreen.kt` around lines 1187 - 1193, The game_closed telemetry call should be skipped when the user has opted out; wrap the PostHog.capture(...) call in a guard that checks PrefManager.usageAnalyticsEnabled (or the equivalent getter) and only invokes PostHog.capture when it returns true; update the XServerScreen code path that currently calls PostHog.capture (referencing PostHog.capture and ContainerUtils.resolveGameName/ContainerUtils.extractGameSourceFromContainerId) to consult PrefManager.usageAnalyticsEnabled before emitting the event.app/src/main/java/com/winlator/winhandler/WinHandler.java (2)
837-850:⚠️ Potential issue | 🟡 MinorDon't rely on
getVibratorIds()already being sorted.The motor-role selection assumes ascending ID order, but the implementation only swaps the first two elements. If Android returns an unsorted array, low/high motor assignment is wrong. Sort
idsbefore selectinglowMotorIdandhighMotorId.🛠 Suggested fix
int[] ids = vm.getVibratorIds(); if (ids.length == 0) return false; + Arrays.sort(ids); int highAmp = scaleAmplitude(highFreq, vibrationIntensity); int lowAmp = scaleAmplitude(lowFreq, vibrationIntensity); if (lowAmp == 0 && highAmp == 0) { vm.cancel(); return true; } // Determine which ID drives the low-freq (heavy/left) motor and which drives // the high-freq (light/right) motor by sorting IDs ascending. int lowMotorId = ids[0]; int highMotorId = ids.length >= 2 ? ids[1] : ids[0]; if (ids.length >= 2) { - if (ids[0] > ids[1]) { lowMotorId = ids[1]; highMotorId = ids[0]; } String motorKey = lowMotorId + "_" + highMotorId; if (loggedRumbleMotorIds.add(motorKey)) { Log.d(TAG, "Rumble motors: lowMotor=" + lowMotorId + " highMotor=" + highMotorId); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 837 - 850, The code assumes getVibratorIds() returns a sorted array which is unsafe; before computing lowMotorId/highMotorId, sort the ids array (e.g., Arrays.sort(ids)) so the ascending order is guaranteed, then select lowMotorId = ids[0] and highMotorId = ids.length >= 2 ? ids[1] : ids[0]; remove the manual conditional swap logic (the if (ids[0] > ids[1]) block) and keep existing checks around ids.length, scaleAmplitude(), and vm.cancel() intact.
782-785:⚠️ Potential issue | 🟠 MajorRe-check
runningafter takingrumbleNotifyLock.
stop()can notify between the outerwhile (running)check and thewait(0)call. If that happens while no player is rumbling, the notification is lost, the poller sleeps indefinitely, and theFileObservers never get stopped.🛠 Suggested fix
try { synchronized (rumbleNotifyLock) { // wait(0) means indefinite; cap at keepalive interval when rumbling. + if (!running) { + break; + } rumbleNotifyLock.wait(waitMs == Long.MAX_VALUE ? 0 : waitMs); } } catch (InterruptedException e) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 782 - 785, The poller must re-check the running flag after acquiring rumbleNotifyLock to avoid missing a stop() notification: inside the synchronized (rumbleNotifyLock) block (where rumbleNotifyLock.wait(...) is called) add a check like if (!running) break/return (or loop so waiting only occurs while running) so the thread does not call wait(0) when stop() was already signaled; ensure the condition protects against spurious wakeups by using a while (running && /* other condition */) around the wait call in the method that contains rumbleNotifyLock.wait(...).
🤖 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/utils/ContainerUtils.kt`:
- Around line 324-325: Normalize the vibrationMode right when reading container
extras in toContainerData(): replace the raw container.getExtra("vibrationMode",
...) value with the canonicalized value (use the same normalization helper used
at runtime, e.g. VibrationMode.normalize/parse or whatever utility method
exists) so vibrationMode stored in the returned ContainerData is always
canonical; ensure the same change is applied at the other site mentioned (lines
~492-493) and leave vibrationIntensity parsing as-is.
In `@app/src/main/java/com/winlator/container/ContainerData.kt`:
- Around line 185-186: The restored vibrationMode and vibrationIntensity are
used verbatim from savedMap which allows invalid/old values to propagate; in the
mapSaver.restore logic (the code that reads savedMap into ContainerData fields
like vibrationMode and vibrationIntensity) validate and normalize these values:
map vibrationMode to a known whitelist of allowed strings (e.g., default to
"controller" for any unknown value) and clamp vibrationIntensity to the valid
range (e.g., 0..100, default 100 on invalid or missing), or encapsulate this in
small helpers (e.g., normalizeVibrationMode/normalizeVibrationIntensity) and
call them when reading savedMap["vibrationMode"] and
savedMap["vibrationIntensity"] so only valid values are stored back into
ContainerData.
---
Outside diff comments:
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java`:
- Around line 597-615: enabled3 is computed before currentController may be
cleared, which can let the async addAction lambda enter the enabled branch and
dereference this.currentController.state causing NPE; fix by moving or
recomputing the enabled check inside the addAction closure (or after the
currentController nulling) so it uses the up-to-date value—specifically, inside
the lambda used with addAction (the block that writes
RequestCodes.GET_GAMEPAD_STATE to sendData and accesses
this.currentController.state), re-evaluate whether a controller is enabled
(using inputControlsView, profile2 / useVirtualGamepad2 and
this.currentController/deviceId/gamepadId) and only access
this.currentController.state when the recomputed enabled result confirms a valid
currentController, or prefer using useVirtualGamepad2 path when appropriate.
---
Duplicate comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 1840-1845: The code passes unvalidated vibrationIntensity from
container.getExtra into handler.setVibrationIntensity, allowing values outside
0..100; before calling handler.setVibrationIntensity (and anywhere WinHandler
expects the 0..100 contract), parse the extra into an Int and clamp it into the
valid range (e.g., using Int.coerceIn(0, 100)) and then pass that clamped value;
update the call site that currently does
container.getExtra("vibrationIntensity", "100").toIntOrNull() ?: 100 to ensure
the parsed value is coerced into 0..100 before invoking
handler.setVibrationIntensity (and keep PrefManager.normalizeVibrationModeInput
usage unchanged for vibrationMode).
- Around line 1187-1193: The game_closed telemetry call should be skipped when
the user has opted out; wrap the PostHog.capture(...) call in a guard that
checks PrefManager.usageAnalyticsEnabled (or the equivalent getter) and only
invokes PostHog.capture when it returns true; update the XServerScreen code path
that currently calls PostHog.capture (referencing PostHog.capture and
ContainerUtils.resolveGameName/ContainerUtils.extractGameSourceFromContainerId)
to consult PrefManager.usageAnalyticsEnabled before emitting the event.
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java`:
- Around line 837-850: The code assumes getVibratorIds() returns a sorted array
which is unsafe; before computing lowMotorId/highMotorId, sort the ids array
(e.g., Arrays.sort(ids)) so the ascending order is guaranteed, then select
lowMotorId = ids[0] and highMotorId = ids.length >= 2 ? ids[1] : ids[0]; remove
the manual conditional swap logic (the if (ids[0] > ids[1]) block) and keep
existing checks around ids.length, scaleAmplitude(), and vm.cancel() intact.
- Around line 782-785: The poller must re-check the running flag after acquiring
rumbleNotifyLock to avoid missing a stop() notification: inside the synchronized
(rumbleNotifyLock) block (where rumbleNotifyLock.wait(...) is called) add a
check like if (!running) break/return (or loop so waiting only occurs while
running) so the thread does not call wait(0) when stop() was already signaled;
ensure the condition protects against spurious wakeups by using a while (running
&& /* other condition */) around the wait call in the method that contains
rumbleNotifyLock.wait(...).
🪄 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: e164bf5c-d0b5-4d36-b72a-f7c81db7098d
⛔ Files ignored due to path filters (1)
app/src/main/jniLibs/arm64-v8a/libevshim.sois excluded by!**/*.so
📒 Files selected for processing (10)
app/src/main/cpp/extras/evshim.capp/src/main/cpp/extras/sdl2_stub/SDL2/SDL.happ/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.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/ContainerData.ktapp/src/main/java/com/winlator/winhandler/WinHandler.javaapp/src/main/res/values/strings.xmlbuild-evshim.ps1
✅ Files skipped from review due to trivial changes (2)
- app/src/main/cpp/extras/sdl2_stub/SDL2/SDL.h
- app/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- build-evshim.ps1
Bot review (PR utkarshdalal#1214): - ContainerUtils.toContainerData / applyToContainer: normalize vibrationMode via PrefManager.normalizeVibrationModeInput and clamp vibrationIntensity to 0..100 at the read/write boundary. Stops a stale 'both' (or any other invalid value) carried over from the prior schema from propagating into ContainerData and back to disk. - ContainerData.Saver/restorer: same defence on the Compose Saver round-trip via a private VALID_RESTORED_VIBRATION_MODES whitelist plus an Int.coerceIn(0, 100) on intensity. Rebuilt invalid mode strings fall back to 'controller'. - WinHandler.startRumblePoller: re-check 'running' after acquiring rumbleNotifyLock so a stop() that runs between the outer while(running) check and the synchronized block is observed before the poller goes into wait(0). Without this, stop()'s notifyAll() is the only chance to wake the poller and a tight race could miss it. - WinHandler.rumbleViaVibratorManager: Arrays.sort(ids) instead of a two-element manual swap. Generalises the heavy/light motor selection to 3+ vibrator controllers (DualSense edition variants); 2-vibrator cases produce the same result as before. Broader review (dropped from upstream PR utkarshdalal#1261 when copying pre-cleanup's WinHandler wholesale): - WinHandler GET_GAMEPAD adoption: when no current controller is set, consult ControllerManager.getAssignedDeviceForSlot(0) instead of ExternalController.getController(0). The latter queries InputDevice ID 0 — an arbitrary number that rarely corresponds to the user's slot-0 assignment. - WinHandler.start(): call initializeAssignedControllers() so saved slot assignments are pre-registered before the first GET_GAMEPAD packet arrives. Pre-cleanup defined the method but never called it (dead code); upstream's PR called it from start(), and games that probe controllers at startup were depending on that pre-registration.
TideGear
added a commit
to TideGear/GameNative
that referenced
this pull request
Apr 29, 2026
Bot review (PR utkarshdalal#1214): - ContainerUtils.toContainerData / applyToContainer: normalize vibrationMode via PrefManager.normalizeVibrationModeInput and clamp vibrationIntensity to 0..100 at the read/write boundary. Stops a stale 'both' (or any other invalid value) carried over from the prior schema from propagating into ContainerData and back to disk. - ContainerData.Saver/restorer: same defence on the Compose Saver round-trip via a private VALID_RESTORED_VIBRATION_MODES whitelist plus an Int.coerceIn(0, 100) on intensity. Rebuilt invalid mode strings fall back to 'controller'. - WinHandler.startRumblePoller: re-check 'running' after acquiring rumbleNotifyLock so a stop() that runs between the outer while(running) check and the synchronized block is observed before the poller goes into wait(0). Without this, stop()'s notifyAll() is the only chance to wake the poller and a tight race could miss it. - WinHandler.rumbleViaVibratorManager: Arrays.sort(ids) instead of a two-element manual swap. Generalises the heavy/light motor selection to 3+ vibrator controllers (DualSense edition variants); 2-vibrator cases produce the same result as before. Broader review (dropped from upstream PR utkarshdalal#1261 when copying pre-cleanup's WinHandler wholesale): - WinHandler GET_GAMEPAD adoption: when no current controller is set, consult ControllerManager.getAssignedDeviceForSlot(0) instead of ExternalController.getController(0). The latter queries InputDevice ID 0 — an arbitrary number that rarely corresponds to the user's slot-0 assignment. - WinHandler.start(): call initializeAssignedControllers() so saved slot assignments are pre-registered before the first GET_GAMEPAD packet arrives. Pre-cleanup defined the method but never called it (dead code); upstream's PR called it from start(), and games that probe controllers at startup were depending on that pre-registration.
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
TideGear
added a commit
to TideGear/GameNative
that referenced
this pull request
Apr 30, 2026
Bot review (PR utkarshdalal#1214): - ContainerUtils.toContainerData / applyToContainer: normalize vibrationMode via PrefManager.normalizeVibrationModeInput and clamp vibrationIntensity to 0..100 at the read/write boundary. Stops a stale 'both' (or any other invalid value) carried over from the prior schema from propagating into ContainerData and back to disk. - ContainerData.Saver/restorer: same defence on the Compose Saver round-trip via a private VALID_RESTORED_VIBRATION_MODES whitelist plus an Int.coerceIn(0, 100) on intensity. Rebuilt invalid mode strings fall back to 'controller'. - WinHandler.startRumblePoller: re-check 'running' after acquiring rumbleNotifyLock so a stop() that runs between the outer while(running) check and the synchronized block is observed before the poller goes into wait(0). Without this, stop()'s notifyAll() is the only chance to wake the poller and a tight race could miss it. - WinHandler.rumbleViaVibratorManager: Arrays.sort(ids) instead of a two-element manual swap. Generalises the heavy/light motor selection to 3+ vibrator controllers (DualSense edition variants); 2-vibrator cases produce the same result as before. Broader review (dropped from upstream PR utkarshdalal#1261 when copying pre-cleanup's WinHandler wholesale): - WinHandler GET_GAMEPAD adoption: when no current controller is set, consult ControllerManager.getAssignedDeviceForSlot(0) instead of ExternalController.getController(0). The latter queries InputDevice ID 0 — an arbitrary number that rarely corresponds to the user's slot-0 assignment. - WinHandler.start(): call initializeAssignedControllers() so saved slot assignments are pre-registered before the first GET_GAMEPAD packet arrives. Pre-cleanup defined the method but never called it (dead code); upstream's PR called it from start(), and games that probe controllers at startup were depending on that pre-registration.
EVSHIM_MAX_PLAYERS was hardcoded to WinHandler.MAX_PLAYERS (=4), so evshim always registered four SDL virtual joysticks regardless of how many physical controllers were actually connected. Games (ToS, etc.) would see one or more phantom unbound gamepads — they didn't respond to input because no slot had a controller, and rumble routed at them went nowhere because getControllerForSlot returned null. Cap to controllerManager.getDetectedDevices().size() with a floor of 1 so the virtual on-screen gamepad still has a vjoy when no physical controller is present at launch. Mem files for all four slots are still pre-created; only the SDL vjoy registration is bounded.
Contributor
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/xenvironment/components/BionicProgramLauncherComponent.java`:
- Around line 225-235: The env var EVSHIM_MAX_PLAYERS is computed once inside
execGuestProgram() using
ControllerManager.getInstance().getDetectedDevices().size(), which prevents
hot-plugged controllers from getting vjoy slots; either set EVSHIM_MAX_PLAYERS
to the constant WinHandler.MAX_PLAYERS (so evshim always exposes all vjoy
slots), or implement a runtime reconfiguration path in the component that
watches ControllerManager for increases and calls the evshim reconfigure/restart
routine (or restarts the guest process) to update EVSHIM_MAX_PLAYERS when
detected device count grows; locate the logic in execGuestProgram(), update
envVars.put("EVSHIM_MAX_PLAYERS", ...) accordingly or add the
watcher/reconfigure using ControllerManager and the existing evshim/start/stop
helpers.
🪄 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: 368a3291-a529-4949-af42-d11d30e1e263
📒 Files selected for processing (1)
app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java
Comment on lines
+225
to
235
| // Tell evshim how many SDL virtual joysticks to register, capped at the count of | ||
| // currently-detected physical controllers. Without the cap we'd always spawn | ||
| // MAX_PLAYERS vjoys regardless of how many were connected, and games (e.g. ToS | ||
| // controller tester) would see phantom unbound gamepads — they don't respond to | ||
| // input and rumble routed at them goes nowhere. Floor at 1 so the virtual | ||
| // on-screen gamepad still has a vjoy when no physical controller is present. | ||
| final int connectedControllerCount = | ||
| com.winlator.inputcontrols.ControllerManager.getInstance().getDetectedDevices().size(); | ||
| final int enabledPlayerCount = | ||
| Math.max(1, Math.min(connectedControllerCount, WinHandler.MAX_PLAYERS)); | ||
| envVars.put("EVSHIM_MAX_PLAYERS", String.valueOf(enabledPlayerCount)); |
Contributor
There was a problem hiding this comment.
EVSHIM_MAX_PLAYERS is fixed at launch, so additional hot-plugged controllers won’t get vjoy slots.
enabledPlayerCount is computed once in execGuestProgram() (Line 231–Line 235), while ControllerManager’s detected-device list can grow at runtime. If the session starts with 1 controller, plugging in a second later still leaves evshim capped at 1 slot, despite pre-created gamepad2.mem.
Consider either keeping EVSHIM_MAX_PLAYERS at WinHandler.MAX_PLAYERS or adding a runtime reconfigure/restart path when connected count increases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java`
around lines 225 - 235, The env var EVSHIM_MAX_PLAYERS is computed once inside
execGuestProgram() using
ControllerManager.getInstance().getDetectedDevices().size(), which prevents
hot-plugged controllers from getting vjoy slots; either set EVSHIM_MAX_PLAYERS
to the constant WinHandler.MAX_PLAYERS (so evshim always exposes all vjoy
slots), or implement a runtime reconfiguration path in the component that
watches ControllerManager for increases and calls the evshim reconfigure/restart
routine (or restarts the guest process) to update EVSHIM_MAX_PLAYERS when
detected device count grows; locate the logic in execGuestProgram(), update
envVars.put("EVSHIM_MAX_PLAYERS", ...) accordingly or add the
watcher/reconfigure using ControllerManager and the existing evshim/start/stop
helpers.
Author
There was a problem hiding this comment.
With the bot's suggestion we always have phantom controllers connected. Now, hot-plugging doesn't work, but at least we don't have controllers that aren't there detected by games. Controllers must be connected before a game is launched, and that's acceptable.
Contributor
There was a problem hiding this comment.
@TideGear, that's a clear and reasonable trade-off — phantom unbound gamepads in games (e.g. showing up as extra disconnected controllers) is a more user-visible problem than requiring controllers to be connected before launch. The current approach of capping EVSHIM_MAX_PLAYERS at the detected controller count at launch time is a sensible design decision given that constraint.
I'll note this for future reviews.
✏️ Learnings added
Learnt from: TideGear
Repo: utkarshdalal/GameNative PR: 1214
File: app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java:225-235
Timestamp: 2026-04-30T04:32:08.135Z
Learning: In `app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java`, `EVSHIM_MAX_PLAYERS` is intentionally set to the number of currently-connected physical controllers at launch time (floored at 1), rather than `WinHandler.MAX_PLAYERS`. This avoids phantom/unbound virtual gamepad slots appearing to games when fewer than MAX_PLAYERS controllers are connected. The accepted trade-off is that hot-plugging additional controllers after game launch won't get new vjoy slots — controllers must be connected before launching a game. Do not flag this as a bug or suggest always using MAX_PLAYERS in future reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: unbelievableflavour
Repo: utkarshdalal/GameNative PR: 698
File: app/src/main/java/app/gamenative/utils/LaunchSteps.kt:134-142
Timestamp: 2026-03-03T11:45:46.529Z
Learning: In the LaunchSteps orchestration (app/src/main/java/app/gamenative/utils/LaunchSteps.kt), the `wineserver -k` command in the runLauncher function is necessary to prevent the launcher from closing after each chained step. Without it, the launcher terminates prematurely between steps.
Learnt from: bllendev
Repo: utkarshdalal/GameNative PR: 1261
File: app/src/main/java/com/winlator/inputcontrols/ControllerManager.java:376-383
Timestamp: 2026-04-26T15:00:17.977Z
Learning: In `app/src/main/java/com/winlator/inputcontrols/ControllerManager.java`, the `autoAssignDevice` method's first-empty-slot loop does NOT need to check `enabledSlots[i]` when selecting a slot for a never-before-seen controller, because no settings UI for enabling/disabling slots is present in the codebase yet. This guard should be revisited only when a slot-management settings view is added.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 1005
File: app/src/main/java/app/gamenative/ui/component/dialog/PhysicalControllerConfigSection.kt:706-738
Timestamp: 2026-03-24T12:56:02.025Z
Learning: In `app/src/main/java/app/gamenative/ui/component/dialog/PhysicalControllerConfigSection.kt`, the `needsCopy` heuristic in `copyElementsIfNeeded` (around lines 706-738) treats any destination layout lacking `GAMEPAD_*` bindings as safe to overwrite with source elements, even if it contains non-GAMEPAD custom bindings. This imperfect heuristic is a pre-existing limitation acknowledged by the maintainer and intentionally out of scope. Do not flag this overwrite-without-merge behavior as an issue in future reviews of this file.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 1001
File: app/src/main/java/app/gamenative/ui/screen/xserver/PhysicalControllerHandler.kt:30-31
Timestamp: 2026-03-23T21:41:08.751Z
Learning: In `app/src/main/java/app/gamenative/ui/screen/xserver/PhysicalControllerHandler.kt`, `activeAxisBindings` is a plain `mutableSetOf<Int>()` and is safe without synchronization because all accesses occur on the main thread: Android dispatches MotionEvents (and thus `onGenericMotionEvent` → `processJoystickInput`) on the main thread, and `setProfile` and `cleanup` are called from Compose, which also runs on the main thread. Do not flag this as a thread-safety issue in future reviews.
Learnt from: Catpotatos
Repo: utkarshdalal/GameNative PR: 1245
File: app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt:188-191
Timestamp: 2026-04-21T20:12:43.546Z
Learning: In `app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`, `ALWAYS_REEXTRACT = true` is intentionally kept for stability. The key improvement in PR `#1245` is that for x86_64 containers, DXVK/VKD3D no longer re-extracts on every boot — `needReextract` in `setupWineSystemFiles()` only triggers unconditional re-extract via `ALWAYS_REEXTRACT` for arm64ec containers (`isArm64EcContainer`). Graphics drivers and the Wrapper/Fex/cnc-ddraw path still use `ALWAYS_REEXTRACT` unconditionally for all containers. Do not flag `ALWAYS_REEXTRACT = true` as incorrect or suggest setting it to `false` in future reviews of this file.
TideGear
added a commit
to TideGear/GameNative
that referenced
this pull request
May 2, 2026
Bot review (PR utkarshdalal#1214): - ContainerUtils.toContainerData / applyToContainer: normalize vibrationMode via PrefManager.normalizeVibrationModeInput and clamp vibrationIntensity to 0..100 at the read/write boundary. Stops a stale 'both' (or any other invalid value) carried over from the prior schema from propagating into ContainerData and back to disk. - ContainerData.Saver/restorer: same defence on the Compose Saver round-trip via a private VALID_RESTORED_VIBRATION_MODES whitelist plus an Int.coerceIn(0, 100) on intensity. Rebuilt invalid mode strings fall back to 'controller'. - WinHandler.startRumblePoller: re-check 'running' after acquiring rumbleNotifyLock so a stop() that runs between the outer while(running) check and the synchronized block is observed before the poller goes into wait(0). Without this, stop()'s notifyAll() is the only chance to wake the poller and a tight race could miss it. - WinHandler.rumbleViaVibratorManager: Arrays.sort(ids) instead of a two-element manual swap. Generalises the heavy/light motor selection to 3+ vibrator controllers (DualSense edition variants); 2-vibrator cases produce the same result as before. Broader review (dropped from upstream PR utkarshdalal#1261 when copying pre-cleanup's WinHandler wholesale): - WinHandler GET_GAMEPAD adoption: when no current controller is set, consult ControllerManager.getAssignedDeviceForSlot(0) instead of ExternalController.getController(0). The latter queries InputDevice ID 0 — an arbitrary number that rarely corresponds to the user's slot-0 assignment. - WinHandler.start(): call initializeAssignedControllers() so saved slot assignments are pre-registered before the first GET_GAMEPAD packet arrives. Pre-cleanup defined the method but never called it (dead code); upstream's PR called it from start(), and games that probe controllers at startup were depending on that pre-registration.
TideGear
added a commit
to TideGear/GameNative
that referenced
this pull request
May 3, 2026
Bot review (PR utkarshdalal#1214): - ContainerUtils.toContainerData / applyToContainer: normalize vibrationMode via PrefManager.normalizeVibrationModeInput and clamp vibrationIntensity to 0..100 at the read/write boundary. Stops a stale 'both' (or any other invalid value) carried over from the prior schema from propagating into ContainerData and back to disk. - ContainerData.Saver/restorer: same defence on the Compose Saver round-trip via a private VALID_RESTORED_VIBRATION_MODES whitelist plus an Int.coerceIn(0, 100) on intensity. Rebuilt invalid mode strings fall back to 'controller'. - WinHandler.startRumblePoller: re-check 'running' after acquiring rumbleNotifyLock so a stop() that runs between the outer while(running) check and the synchronized block is observed before the poller goes into wait(0). Without this, stop()'s notifyAll() is the only chance to wake the poller and a tight race could miss it. - WinHandler.rumbleViaVibratorManager: Arrays.sort(ids) instead of a two-element manual swap. Generalises the heavy/light motor selection to 3+ vibrator controllers (DualSense edition variants); 2-vibrator cases produce the same result as before. Broader review (dropped from upstream PR utkarshdalal#1261 when copying pre-cleanup's WinHandler wholesale): - WinHandler GET_GAMEPAD adoption: when no current controller is set, consult ControllerManager.getAssignedDeviceForSlot(0) instead of ExternalController.getController(0). The latter queries InputDevice ID 0 — an arbitrary number that rarely corresponds to the user's slot-0 assignment. - WinHandler.start(): call initializeAssignedControllers() so saved slot assignments are pre-registered before the first GET_GAMEPAD packet arrives. Pre-cleanup defined the method but never called it (dead code); upstream's PR called it from start(), and games that probe controllers at startup were depending on that pre-registration.
TideGear
added a commit
to TideGear/GameNative
that referenced
this pull request
May 4, 2026
Bot review (PR utkarshdalal#1214): - ContainerUtils.toContainerData / applyToContainer: normalize vibrationMode via PrefManager.normalizeVibrationModeInput and clamp vibrationIntensity to 0..100 at the read/write boundary. Stops a stale 'both' (or any other invalid value) carried over from the prior schema from propagating into ContainerData and back to disk. - ContainerData.Saver/restorer: same defence on the Compose Saver round-trip via a private VALID_RESTORED_VIBRATION_MODES whitelist plus an Int.coerceIn(0, 100) on intensity. Rebuilt invalid mode strings fall back to 'controller'. - WinHandler.startRumblePoller: re-check 'running' after acquiring rumbleNotifyLock so a stop() that runs between the outer while(running) check and the synchronized block is observed before the poller goes into wait(0). Without this, stop()'s notifyAll() is the only chance to wake the poller and a tight race could miss it. - WinHandler.rumbleViaVibratorManager: Arrays.sort(ids) instead of a two-element manual swap. Generalises the heavy/light motor selection to 3+ vibrator controllers (DualSense edition variants); 2-vibrator cases produce the same result as before. Broader review (dropped from upstream PR utkarshdalal#1261 when copying pre-cleanup's WinHandler wholesale): - WinHandler GET_GAMEPAD adoption: when no current controller is set, consult ControllerManager.getAssignedDeviceForSlot(0) instead of ExternalController.getController(0). The latter queries InputDevice ID 0 — an arbitrary number that rarely corresponds to the user's slot-0 assignment. - WinHandler.start(): call initializeAssignedControllers() so saved slot assignments are pre-registered before the first GET_GAMEPAD packet arrives. Pre-cleanup defined the method but never called it (dead code); upstream's PR called it from start(), and games that probe controllers at startup were depending on that pre-registration.
# Conflicts: # app/src/main/java/app/gamenative/utils/ContainerUtils.kt # app/src/main/java/com/winlator/winhandler/WinHandler.java # app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java
…s dropped during merge The libevshim.so baked into imagefs_bionic.txz has hardcoded /data/data/com.winlator.cmod/... gamepad-mem paths from before the package rename to app.gamenative. Every open() fails, OnRumble bails before writing, and the FileObserver/poller chain never wakes. Input still works because xinput uses the UDP path (winhandler.exe <-> WinHandler.java), which is independent of evshim — so the regression presented as silent rumble only. Re-add the APK->imagefs copy block (originally in b8d7a49, removed during PR review) so the freshly built jniLibs/arm64-v8a/libevshim.so (built from evshim.c via build-evshim.ps1) overwrites the stale imagefs copy on every launch. Gate the LD_PRELOAD entry on the file existing so a copy failure doesn't poison every spawned process. Also enable EVSHIM_DEBUG=1 to surface the per-rumble logs. Restore three multi-controller fixes that the upstream/master merge dropped: * WinHandler.setControllerForSlot: actually store the new controller for the slot. Without these two lines getControllerForSlot returns null and the rumble poller has no device to vibrate. * WinHandler.onKeyEvent: replace the wildcard-only adoption flow with the resolveControllerSlot-based slot routing so P2-P4 inputs reach the correct gamepad{N}.mem buffer. * PhysicalControllerHandler.handleInputEvent: real-device-first adoption with profile wildcard as fallback. Inverting this adopts the wildcard ExternalController (id == "*") for every physical device, collapsing all slots onto one shared instance (bllendev's PR utkarshdalal#1261 regression). Add a 'Rumble buf P%d low=... high=... mode=...' diagnostic in the rumble poller so future regressions can be triaged in logcat without rebuilding.
Contributor
There was a problem hiding this comment.
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/com/winlator/winhandler/WinHandler.java (2)
611-625:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCapture the controller after the
gamepadIdmismatch check.Line 613 can null
currentController, butenabled3was already computed astrue, so the async lambda on Line 625 can dereferencethis.currentControllerand crash the send thread.Suggested fix
- ExternalController externalController2 = this.currentController; - final boolean enabled3 = externalController2 != null || useVirtualGamepad2; - if (externalController2 != null && externalController2.getDeviceId() != gamepadId) { - this.currentController = null; - } + ExternalController externalController2 = this.currentController; + if (externalController2 != null && externalController2.getDeviceId() != gamepadId) { + this.currentController = null; + externalController2 = null; + } + final ExternalController controllerForState = externalController2; + final boolean enabled3 = controllerForState != null || useVirtualGamepad2; addAction(() -> { sendData.rewind(); sendData.put(RequestCodes.GET_GAMEPAD_STATE); this.sendData.put((byte)(enabled3 ? 1 : 0)); if (enabled3) { this.sendData.putInt(gamepadId); if (useVirtualGamepad2) { inputControlsView.getProfile().getGamepadState().writeTo(this.sendData); } else { - this.currentController.state.writeTo(this.sendData); + controllerForState.state.writeTo(this.sendData); } } sendPacket(port); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 611 - 625, The enabled3 flag and controller reference are computed before the null check that can set this.currentController to null, causing the lambda passed to addAction to dereference a cleared this.currentController; after the check that sets this.currentController = null (the block that compares externalController2.getDeviceId() to gamepadId), re-evaluate and capture the controller into a final local variable (e.g., final ExternalController capturedController = this.currentController) and compute a new enabled flag from that (or replace enabled3) and then use capturedController (or the recomputed enabled) inside the addAction lambda for calls to capturedController.state.writeTo(...) and related logic (also keep useVirtualGamepad2 and inputControlsView.getProfile().getGamepadState().writeTo(...) behavior unchanged).
458-476:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWait for the old rumble poller to exit before restarting.
stop()only flipsrunningand notifies. Ifstart()runs again before the previousrumblePollerThreadleaves its loop, Line 678 setsrunningback totrueand the old poller/file observers can survive alongside the new ones.Also applies to: 678-704
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 458 - 476, The stop() method currently only flips running to false and notifies rumbleNotifyLock, which can allow the old rumblePollerThread (and associated file observers) to continue running if start() sets running=true again; modify stop() to wait for the rumblePollerThread to terminate (e.g., call join on rumblePollerThread or otherwise block until it has exited) before returning and clearing resources, and likewise ensure start() (or the start-path around the code at lines ~678-704 that creates/starts rumblePollerThread and observers) checks that any previous rumblePollerThread has fully stopped before creating a new one; use the existing fields running, rumblePollerThread, and rumbleNotifyLock to coordinate the shutdown/wait so the old poller cannot survive alongside the new one.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java`:
- Around line 611-625: The enabled3 flag and controller reference are computed
before the null check that can set this.currentController to null, causing the
lambda passed to addAction to dereference a cleared this.currentController;
after the check that sets this.currentController = null (the block that compares
externalController2.getDeviceId() to gamepadId), re-evaluate and capture the
controller into a final local variable (e.g., final ExternalController
capturedController = this.currentController) and compute a new enabled flag from
that (or replace enabled3) and then use capturedController (or the recomputed
enabled) inside the addAction lambda for calls to
capturedController.state.writeTo(...) and related logic (also keep
useVirtualGamepad2 and
inputControlsView.getProfile().getGamepadState().writeTo(...) behavior
unchanged).
- Around line 458-476: The stop() method currently only flips running to false
and notifies rumbleNotifyLock, which can allow the old rumblePollerThread (and
associated file observers) to continue running if start() sets running=true
again; modify stop() to wait for the rumblePollerThread to terminate (e.g., call
join on rumblePollerThread or otherwise block until it has exited) before
returning and clearing resources, and likewise ensure start() (or the start-path
around the code at lines ~678-704 that creates/starts rumblePollerThread and
observers) checks that any previous rumblePollerThread has fully stopped before
creating a new one; use the existing fields running, rumblePollerThread, and
rumbleNotifyLock to coordinate the shutdown/wait so the old poller cannot
survive alongside the new one.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe9bc743-df5a-4a3c-8279-cce30a94797f
📒 Files selected for processing (9)
app/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/ui/screen/xserver/PhysicalControllerHandler.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/ContainerData.ktapp/src/main/java/com/winlator/inputcontrols/ControllerManager.javaapp/src/main/java/com/winlator/winhandler/WinHandler.javaapp/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.javaapp/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (5)
- app/src/main/res/values/strings.xml
- app/src/main/java/com/winlator/container/ContainerData.kt
- app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
- app/src/main/java/app/gamenative/utils/ContainerUtils.kt
- app/src/main/java/app/gamenative/PrefManager.kt
GET_GAMEPAD_STATE NPE: the addAction lambda captured enabled3 from a snapshot of this.currentController taken before the deviceId-mismatch check on line ~613 that nulls this.currentController. With enabled3 still true the lambda would then dereference the cleared field on `this.currentController.state.writeTo`. Same NPE class also reachable from a concurrent RELEASE_GAMEPAD on another thread. Null the local snapshot alongside the field in the mismatch branch, capture into a final local after, recompute enabled3 from the captured value, and have the lambda use the capture. If captured ends up null with no virtual gamepad, enabled3 is false and the writeTo call is skipped cleanly. stop() poller leak: stop() flipped running=false and notifyAll'd on rumbleNotifyLock but returned before the rumblePollerThread had actually exited. A subsequent start() that flipped running=true again would resurrect the old poller, and the new FileObservers would overwrite the slots without the old ones ever getting stopWatching()'d. Join the poller with a 1s timeout after the notifyAll so stop() is contractually complete on return. Guard against self-join from the poller thread. start()-side defensive check the reviewer also raised is intentionally not added: the only call pattern (one start per launch, one stop at exit, followed by PluviaApp.shutdownEnvironment) doesn't reuse the instance, and with stop() now blocking until the poller exits a properly-ordered stop()->start() is race-free. Adding a second layer would be untested code.
Contributor
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/winhandler/WinHandler.java (1)
568-576:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCapture stable controller/profile snapshots before queueing send actions.
Both async paths compute
enabledup front, then later dereferencethis.currentController/inputControlsView.getProfile()again inside the queued lambda. A disconnect, remap, orRELEASE_GAMEPADbetweenaddAction()and executor drain can throw here; since this is the single sender thread, that can stop all future replies and state pushes. Mirror theGET_GAMEPAD_STATEsnapshot fix in both places.🛠️ Suggested fix
- final boolean finalEnabled = enabled; + final ExternalController capturedController = this.currentController; + final boolean finalEnabled = capturedController != null || useVirtualGamepad; addAction(() -> { this.sendData.rewind(); this.sendData.put((byte) RequestCodes.GET_GAMEPAD); if (finalEnabled) { - this.sendData.putInt(!useVirtualGamepad ? this.currentController.getDeviceId() : profile.id); + this.sendData.putInt(!useVirtualGamepad ? capturedController.getDeviceId() : profile.id); this.sendData.put(this.dinputMapperType); - String originalName = (useVirtualGamepad ? profile.getName() : currentController.getName()); + String originalName = useVirtualGamepad ? profile.getName() : capturedController.getName();- final ControlsProfile profile = inputControlsView.getProfile(); - final boolean useVirtualGamepad = profile != null && profile.isVirtualGamepad(); - final boolean enabled = this.currentController != null || useVirtualGamepad; + final ControlsProfile profile = inputControlsView.getProfile(); + final boolean useVirtualGamepad = profile != null && profile.isVirtualGamepad(); + final ExternalController capturedController = this.currentController; + final boolean enabled = capturedController != null || useVirtualGamepad; Iterator<Integer> it = this.gamepadClients.iterator(); while (it.hasNext()) { final int port = it.next().intValue(); addAction(() -> { this.sendData.rewind(); sendData.put(RequestCodes.GET_GAMEPAD_STATE); sendData.put((byte)(enabled ? 1 : 0)); if (enabled) { - this.sendData.putInt(!useVirtualGamepad ? this.currentController.getDeviceId() : inputControlsView.getProfile().id); + this.sendData.putInt(!useVirtualGamepad ? capturedController.getDeviceId() : profile.id); if (useVirtualGamepad) { - inputControlsView.getProfile().getGamepadState().writeTo(sendData); + profile.getGamepadState().writeTo(sendData); } else { - this.currentController.state.writeTo(this.sendData); + capturedController.state.writeTo(this.sendData); } }Also applies to: 1107-1128
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 568 - 576, Before calling addAction(...) capture stable local snapshots of enabled, the controller/profile identity and any fields used inside the lambda (e.g. this.currentController.getDeviceId(), profile.id, this.dinputMapperType, originalName, useVirtualGamepad) so the queued send action uses those captured values rather than dereferencing this.currentController or inputControlsView.getProfile() later; mirror the GET_GAMEPAD_STATE snapshot approach by computing and storing deviceId, mapperType, and originalName (as bytes) into final local variables (e.g. finalDeviceId, finalMapperType, finalOriginalBytes) and then reference those inside the lambda passed to addAction().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java`:
- Around line 140-152: After updating vibrationMode or vibrationIntensity in
setVibrationMode and setVibrationIntensity, immediately reconcile any currently
active rumble: iterate players with playerPhoneAmplitudes[player] > 0 and cancel
or recompute their active slots so the handset aggregate is cleared and
re-applied to reflect the new mode/intensity (i.e., call stopVibrationForPlayer
to clear device-specific state and then reapply whatever aggregate/update logic
the code uses to start phone vibration for the new mode), and ensure any
scheduled long "device" one-shot timers are cancelled when switching away from
"device" so a running 60s vibration is stopped immediately; do this for both
setters so changes take effect instantly.
---
Outside diff comments:
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java`:
- Around line 568-576: Before calling addAction(...) capture stable local
snapshots of enabled, the controller/profile identity and any fields used inside
the lambda (e.g. this.currentController.getDeviceId(), profile.id,
this.dinputMapperType, originalName, useVirtualGamepad) so the queued send
action uses those captured values rather than dereferencing
this.currentController or inputControlsView.getProfile() later; mirror the
GET_GAMEPAD_STATE snapshot approach by computing and storing deviceId,
mapperType, and originalName (as bytes) into final local variables (e.g.
finalDeviceId, finalMapperType, finalOriginalBytes) and then reference those
inside the lambda passed to addAction().
🪄 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: 351a2171-22eb-4d8f-ac54-54dfef3ef65d
📒 Files selected for processing (1)
app/src/main/java/com/winlator/winhandler/WinHandler.java
…_GAMEPAD lambda values Vibration setting reconciliation: setVibrationMode and setVibrationIntensity used to just update the volatile field. Two visible bugs followed: switching away from "device" mode left the 60s phone one-shot (DEVICE_RUMBLE_MS) running until its timer expired, because stopVibrationForPlayer gates its phone-cancel branch on the current mode and the field had already flipped. And an intensity change in "device" mode took up to ~55s to take effect (DEVICE_RUMBLE_REFRESH_MS) because that's how often the long device one-shot is re-issued. Both setters now early-return on a no-op, otherwise call a new reconcileActiveRumble() helper after updating the field. The helper cancels the handset vibrator unconditionally (sidestepping the mode gate), calls stopVibrationForPlayer for every active slot, zeroes playerPhoneAmplitudes and lastLow/HighFreqs, and notifyAll's the poller. The next poller tick reads the current buffer state as a transition and re-fires startVibrationForPlayer under the new settings. GET_GAMEPAD lambda NPE: same class of bug as the GET_GAMEPAD_STATE one fixed in the prior commit. The addAction lambda dereferenced this.currentController and inputControlsView.getProfile() at execution time on the send thread, both of which can be cleared/reassigned by RELEASE_GAMEPAD or other handlers between queue and execution. Capture finalDeviceId, finalMapperType, and finalNameBytes into final locals before addAction so the lambda references stable snapshots. Truncation logic moved out of the lambda - bytes copy happens once at queue time. Skipped the parallel addAction at lines ~640 with the same code shape: its finalEnabled2 flag is captured from `enabled = enabled2 = false` on that branch, so the controller dereferences live inside `if (finalEnabled2)` which is always false at lambda runtime - dead code, no real NPE path.
Author
|
Hold on this. Gotta add 86_64 support. |
Author
|
Sorry, I was mistaken. x86_64 is covered! Good to go. |
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
3 participants
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.
...or as accurate as I'm smart enough to achieve.
Adds full controller vibration support that mirrors PC behavior — games trigger the physical controller's rumble motors instead of (or in addition to) the Android device vibrator.
What's included:
VibratorManager(API 31+) to drive low-frequency and high-frequency motors independently on DualShock 4, DualSense, and XInput controllers; falls back to blended single-vibrator on older APIsread()→pread(..., 0)invjoy_updaterso the polling loop always reads from offset 0 instead of advancing past the data after one iteration (this was silently breaking button/axis forwarding)BionicProgramLauncherComponentso evshim updates ship with the APK rather than requiring imagefs rebuildsKnown limitation: XInput controller vibration does not currently work over USB — this is an Android platform limitation that will be addressed in a follow-up.
Recording
https://photos.app.goo.gl/uRuX5h6xcWU9iZHf9
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
Adds PC‑accurate per‑controller vibration with per‑container mode/intensity, dual‑motor rumble, and event‑driven near‑zero latency across players. Restores and hardens multi‑controller routing, caps evshim virtual gamepads to connected controllers, redeploys
libevshim.so, enables rumble diagnostics, and applies vibration setting changes instantly; XInput over USB remains unsupported.New Features
PrefManager, surfaced in the Controller tab, applied inWinHandler.VibratorManager(ascending‑ID motor selection), with blended single‑motor fallback.Bug Fixes
libevshim.sofrom the APK on launch, guardLD_PRELOAD, enableEVSHIM_DEBUG=1.pread(..., 0)for shared memory reads.ControllerManagerfor slot‑0 in GET_GAMEPAD, capture GET_GAMEPAD lambdas, and fix GET_GAMEPAD_STATE NPE.Written for commit ea26c9a. Summary will update on new commits.
Summary by CodeRabbit