Improve Shooter Mode runtime controls and settings#1641
Improve Shooter Mode runtime controls and settings#1641Nightwalker743 wants to merge 12 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds shooter-mode config storage and parsing, a new shooter settings dialog, QuickMenu and XServer wiring, InputControlsView shooter handling, localized strings, and tests for ChangesShooter Mode Configuration Feature
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
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/widget/InputControlsView.java (1)
1240-1261: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRelease all shooter pointers on ACTION_CANCEL.
ACTION_CANCELis not a single-pointer release. Using onlyactionIndexcan leave another active joystick/look/pending pointer held, including sprint or gamepad axis state.Possible fix
case MotionEvent.ACTION_UP: case MotionEvent.ACTION_POINTER_UP: + // existing single-pointer release logic + ... + break; case MotionEvent.ACTION_CANCEL: - // Shooter mode intercept if (shooterModeActive || containerShooterModeRuntime) { - if (pointerId == pendingButtonLookPointerId) { - releasePendingButtonLook(); - handled = true; - } - if (pointerId == joystickPointerId) { - releaseShooterJoystick(); - handled = true; - } - if (pointerId == rightJoystickPointerId) { - releaseRightJoystick(); - handled = true; - } - if (pointerId == lookPointerId) { - releaseShooterLook(); - handled = true; - } + releasePendingButtonLook(); + releaseShooterJoystick(); + releaseRightJoystick(); + releaseShooterLook(); + releaseShooterSprint(); + handled = true; } - for (ControlElement element : profile.getElements()) if (element.handleTouchUp(pointerId)) handled = true; + for (int i = 0; i < event.getPointerCount(); i++) { + int cancelledPointerId = event.getPointerId(i); + for (ControlElement element : profile.getElements()) { + if (element.handleTouchUp(cancelledPointerId)) handled = true; + } + } if (!handled) touchpadView.onTouchEvent(event); break;🤖 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/widget/InputControlsView.java` around lines 1240 - 1261, ACTION_CANCEL in InputControlsView should clear every active shooter input, not just the pointer from actionIndex. Update the MotionEvent.ACTION_CANCEL handling in the shooter-mode branch to release all tracked shooter pointers/state (pendingButtonLookPointerId, joystickPointerId, rightJoystickPointerId, lookPointerId, and any related sprint/gamepad axis state) rather than only the matching pointerId. Use the existing releasePendingButtonLook, releaseShooterJoystick, releaseRightJoystick, and releaseShooterLook helpers, and make sure ACTION_CANCEL fully resets shooter-mode interaction state.
🧹 Nitpick comments (2)
app/src/sharedTest/java/app/gamenative/ui/component/dialog/ContainerConfigDialogTestData.kt (1)
133-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGenerate this fixture with
ShooterModeConfig.toJson().This hard-coded JSON duplicates the runtime payload shape. If the model’s keys/default handling change, these tests can stay green while the real saved config drifts. Prefer a shared fixture built from
ShooterModeConfig(...)so the test follows the production serializer.Suggested refactor
+import app.gamenative.data.ShooterModeConfig + +private val SAMPLE_SHOOTER_CONFIG = + ShooterModeConfig( + buttonLookThroughEnabled = false, + movementZoneSplit = 0.6f, + ).toJson() + private fun ContainerData.commonOverrides(): ContainerData { @@ - shooterConfig = "{\"buttonLookThroughEnabled\":false,\"movementZoneSplit\":0.6}", + shooterConfig = SAMPLE_SHOOTER_CONFIG,🤖 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/sharedTest/java/app/gamenative/ui/component/dialog/ContainerConfigDialogTestData.kt` at line 133, The hard-coded shooterConfig JSON in ContainerConfigDialogTestData should be replaced with a fixture generated from ShooterModeConfig.toJson() so the test stays aligned with the production serializer. Update the shared test data to build the config from ShooterModeConfig(...) and serialize it via toJson(), rather than embedding the JSON string directly, so changes to keys or default handling are reflected automatically.app/src/main/java/app/gamenative/data/ShooterModeConfig.kt (1)
147-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct tests for the config compatibility layer.
fromJson()now owns the important behavior here: invalid-JSON fallback, legacy joystick-key fallback, clamping, and sprint-binding normalization. The added tests in this PR only verify raw string persistence, so regressions in the actual config contract would slip through. Please add a small model test matrix around this method.🤖 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/app/gamenative/data/ShooterModeConfig.kt` around lines 147 - 268, Add direct tests for ShooterModeConfig.fromJson() to cover the compatibility layer behavior, not just raw persistence. Include cases for invalid JSON falling back to a default ShooterModeConfig, legacy joystick key fallback via the legacyJoystickSize and legacyJoystickBehavior handling, clamping of out-of-range values, and sprint binding normalization through normalizeSprintBinding. Use the existing fromJson() entry point and assert the resulting config fields so regressions in the config contract are caught.
🤖 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/app/gamenative/ui/component/dialog/SettingsDialogBlocks.kt`:
- Around line 127-138: The local text state in SettingsDialogBlocks.kt is not
being resynced when the parent clamps or rejects an update, so
NoExtractOutlinedTextField can keep showing stale input. Update the state
handling around the remembered text and the onValueChange path so the displayed
text always follows the current value parameter (including clamped parent
values), and locate the fix in the text field block that uses remember(value)
and NoExtractOutlinedTextField.
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 1202-1209: When handling QuickMenuAction.SHOOTER_MODE in
XServerScreen, enabling shooter mode should also make the on-screen input
controls visible, not just update InputControlsView if it already exists. Update
the Shooter Mode toggle path to explicitly show/restore the input controls
surface before calling setContainerShooterMode and setShooterModeConfig, so a
previously hidden view from hideInputControls() becomes usable again. Keep the
change localized to the Shooter Mode branch and use the existing
PluviaApp.inputControlsView and isShooterModeActive flow.
In `@app/src/main/java/com/winlator/widget/InputControlsView.java`:
- Around line 436-441: The release paths in InputControlsView can update
shooter/gamepad state outside onTouchEvent, so the guest may keep seeing a held
stick or sprint until the next touch commit. Update setShooterModeConfig and the
other Shooter Mode disable/reconfigure release paths to call
commitGamepadState() immediately after releasing joystick, right joystick, or
sprint state, using the existing helpers like releaseShooterJoystick,
releaseRightJoystick, releaseShooterLook, releasePendingButtonLook, and
releaseShooterSprint as the anchor points.
---
Outside diff comments:
In `@app/src/main/java/com/winlator/widget/InputControlsView.java`:
- Around line 1240-1261: ACTION_CANCEL in InputControlsView should clear every
active shooter input, not just the pointer from actionIndex. Update the
MotionEvent.ACTION_CANCEL handling in the shooter-mode branch to release all
tracked shooter pointers/state (pendingButtonLookPointerId, joystickPointerId,
rightJoystickPointerId, lookPointerId, and any related sprint/gamepad axis
state) rather than only the matching pointerId. Use the existing
releasePendingButtonLook, releaseShooterJoystick, releaseRightJoystick, and
releaseShooterLook helpers, and make sure ACTION_CANCEL fully resets
shooter-mode interaction state.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/data/ShooterModeConfig.kt`:
- Around line 147-268: Add direct tests for ShooterModeConfig.fromJson() to
cover the compatibility layer behavior, not just raw persistence. Include cases
for invalid JSON falling back to a default ShooterModeConfig, legacy joystick
key fallback via the legacyJoystickSize and legacyJoystickBehavior handling,
clamping of out-of-range values, and sprint binding normalization through
normalizeSprintBinding. Use the existing fromJson() entry point and assert the
resulting config fields so regressions in the config contract are caught.
In
`@app/src/sharedTest/java/app/gamenative/ui/component/dialog/ContainerConfigDialogTestData.kt`:
- Line 133: The hard-coded shooterConfig JSON in ContainerConfigDialogTestData
should be replaced with a fixture generated from ShooterModeConfig.toJson() so
the test stays aligned with the production serializer. Update the shared test
data to build the config from ShooterModeConfig(...) and serialize it via
toJson(), rather than embedding the JSON string directly, so changes to keys or
default handling are reflected automatically.
🪄 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: 5ebf234d-5738-4fa0-8abb-1d00744076ed
📒 Files selected for processing (28)
app/src/main/java/app/gamenative/data/ShooterModeConfig.ktapp/src/main/java/app/gamenative/ui/component/QuickMenu.ktapp/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.ktapp/src/main/java/app/gamenative/ui/component/dialog/SettingsDialogBlocks.ktapp/src/main/java/app/gamenative/ui/component/dialog/ShooterModeSettingsDialog.ktapp/src/main/java/app/gamenative/ui/component/dialog/TouchGestureSettingsDialog.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/ContainerUtils.ktapp/src/main/java/com/winlator/container/Container.javaapp/src/main/java/com/winlator/container/ContainerData.ktapp/src/main/java/com/winlator/widget/InputControlsView.javaapp/src/main/res/values-da/strings.xmlapp/src/main/res/values-de/strings.xmlapp/src/main/res/values-es/strings.xmlapp/src/main/res/values-fr/strings.xmlapp/src/main/res/values-it/strings.xmlapp/src/main/res/values-ja/strings.xmlapp/src/main/res/values-ko/strings.xmlapp/src/main/res/values-pl/strings.xmlapp/src/main/res/values-pt-rBR/strings.xmlapp/src/main/res/values-ro/strings.xmlapp/src/main/res/values-ru/strings.xmlapp/src/main/res/values-uk/strings.xmlapp/src/main/res/values-zh-rCN/strings.xmlapp/src/main/res/values-zh-rTW/strings.xmlapp/src/main/res/values/strings.xmlapp/src/sharedTest/java/app/gamenative/ui/component/dialog/ContainerConfigDialogTestData.ktapp/src/test/java/app/gamenative/ui/component/dialog/ContainerConfigDialogContainerUpdateTest.kt
💤 Files with no reviewable changes (1)
- app/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.kt
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
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/widget/InputControlsView.java (1)
1017-1022: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLimit button look-through to fire buttons.
startPendingButtonLookPointer(...)receives afireElement, andisFireButton(...)exists, but Line 1020 currently enables drag look-through for every button. Dragging from non-fire buttons can unexpectedly convert that touch into movement/look while keeping the original action held.Proposed fix
- if (element.getType() == ControlElement.Type.BUTTON) { + if (isFireButton(element)) { startPendingButtonLookPointer(pointerId, x, y, element); }🤖 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/widget/InputControlsView.java` around lines 1017 - 1022, The touch handling in InputControlsView currently starts look-through for every ControlElement.Type.BUTTON via startPendingButtonLookPointer(...), which can wrongly turn non-fire buttons into movement/look input while still holding the original action. Update the BUTTON branch in the touch-down flow to gate this behavior with isFireButton(...) so only fire buttons call startPendingButtonLookPointer(pointerId, x, y, element), leaving other buttons handled normally.
🤖 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/widget/InputControlsView.java`:
- Around line 411-415: The shooter-mode shutdown path in setShooterModeActive
currently clears shooterModeActive before calling releaseAllShooterInputs(),
which breaks releaseShooterJoystick()’s ability to resolve legacy SHOOTER_MODE
movement bindings. Update the shutdown order so inputs are released first and
the standalone Shooter Mode state is cleared afterward, then keep the
commitGamepadState() call after both steps to ensure the correct bindings are
released for wasd/arrow_keys.
---
Outside diff comments:
In `@app/src/main/java/com/winlator/widget/InputControlsView.java`:
- Around line 1017-1022: The touch handling in InputControlsView currently
starts look-through for every ControlElement.Type.BUTTON via
startPendingButtonLookPointer(...), which can wrongly turn non-fire buttons into
movement/look input while still holding the original action. Update the BUTTON
branch in the touch-down flow to gate this behavior with isFireButton(...) so
only fire buttons call startPendingButtonLookPointer(pointerId, x, y, element),
leaving other buttons handled normally.
🪄 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: 740ca2bf-f560-4941-9ebf-f19135b430f4
📒 Files selected for processing (5)
app/src/main/java/app/gamenative/ui/component/dialog/SettingsDialogBlocks.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/com/winlator/widget/InputControlsView.javaapp/src/sharedTest/java/app/gamenative/ui/component/dialog/ContainerConfigDialogTestData.ktapp/src/test/java/app/gamenative/data/ShooterModeConfigTest.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/sharedTest/java/app/gamenative/ui/component/dialog/ContainerConfigDialogTestData.kt
- app/src/main/java/app/gamenative/ui/component/dialog/SettingsDialogBlocks.kt
- app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
There was a problem hiding this comment.
2 issues found across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/res/values/strings.xml`:
- Around line 1661-1675: Standardize the Bionic wrapper name across the game
launch tips by making the entries in strings.xml use the same canonical label
for the wrapper option. Update the affected string resources referenced by
game_launch_tip_4 and game_launch_tip_18 so they both match the correct wrapper
name used elsewhere in the app, ensuring the user-facing copy is consistent and
searchable.
🪄 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: 324b0746-4c1d-4edf-b262-258223434e53
📒 Files selected for processing (15)
app/src/main/res/values-da/strings.xmlapp/src/main/res/values-de/strings.xmlapp/src/main/res/values-es/strings.xmlapp/src/main/res/values-fr/strings.xmlapp/src/main/res/values-it/strings.xmlapp/src/main/res/values-ja/strings.xmlapp/src/main/res/values-ko/strings.xmlapp/src/main/res/values-pl/strings.xmlapp/src/main/res/values-pt-rBR/strings.xmlapp/src/main/res/values-ro/strings.xmlapp/src/main/res/values-ru/strings.xmlapp/src/main/res/values-uk/strings.xmlapp/src/main/res/values-zh-rCN/strings.xmlapp/src/main/res/values-zh-rTW/strings.xmlapp/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (13)
- app/src/main/res/values-zh-rCN/strings.xml
- app/src/main/res/values-uk/strings.xml
- app/src/main/res/values-fr/strings.xml
- app/src/main/res/values-da/strings.xml
- app/src/main/res/values-it/strings.xml
- app/src/main/res/values-ja/strings.xml
- app/src/main/res/values-pl/strings.xml
- app/src/main/res/values-de/strings.xml
- app/src/main/res/values-es/strings.xml
- app/src/main/res/values-ro/strings.xml
- app/src/main/res/values-pt-rBR/strings.xml
- app/src/main/res/values-ru/strings.xml
- app/src/main/res/values-ko/strings.xml
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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/res/values/strings.xml`:
- Around line 1661-1675: Standardize the Bionic wrapper name across the game
launch tips by making the entries in strings.xml use the same canonical label
for the wrapper option. Update the affected string resources referenced by
game_launch_tip_4 and game_launch_tip_18 so they both match the correct wrapper
name used elsewhere in the app, ensuring the user-facing copy is consistent and
searchable.
🪄 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: 324b0746-4c1d-4edf-b262-258223434e53
📒 Files selected for processing (15)
app/src/main/res/values-da/strings.xmlapp/src/main/res/values-de/strings.xmlapp/src/main/res/values-es/strings.xmlapp/src/main/res/values-fr/strings.xmlapp/src/main/res/values-it/strings.xmlapp/src/main/res/values-ja/strings.xmlapp/src/main/res/values-ko/strings.xmlapp/src/main/res/values-pl/strings.xmlapp/src/main/res/values-pt-rBR/strings.xmlapp/src/main/res/values-ro/strings.xmlapp/src/main/res/values-ru/strings.xmlapp/src/main/res/values-uk/strings.xmlapp/src/main/res/values-zh-rCN/strings.xmlapp/src/main/res/values-zh-rTW/strings.xmlapp/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (13)
- app/src/main/res/values-zh-rCN/strings.xml
- app/src/main/res/values-uk/strings.xml
- app/src/main/res/values-fr/strings.xml
- app/src/main/res/values-da/strings.xml
- app/src/main/res/values-it/strings.xml
- app/src/main/res/values-ja/strings.xml
- app/src/main/res/values-pl/strings.xml
- app/src/main/res/values-de/strings.xml
- app/src/main/res/values-es/strings.xml
- app/src/main/res/values-ro/strings.xml
- app/src/main/res/values-pt-rBR/strings.xml
- app/src/main/res/values-ru/strings.xml
- app/src/main/res/values-ko/strings.xml
🛑 Comments failed to post (1)
app/src/main/res/values/strings.xml (1)
1661-1675: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Standardize the Bionic wrapper name in these tips.
Line 1661 says
leegao-wrapper, while Line 1675 sayswrapper-leegao. One of those labels is wrong, so users following the tips may not find the right option. Please pick the canonical name and update the translated copy to match.🤖 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/res/values/strings.xml` around lines 1661 - 1675, Standardize the Bionic wrapper name across the game launch tips by making the entries in strings.xml use the same canonical label for the wrapper option. Update the affected string resources referenced by game_launch_tip_4 and game_launch_tip_18 so they both match the correct wrapper name used elsewhere in the app, ensuring the user-facing copy is consistent and searchable.
There was a problem hiding this comment.
1 issue found across 29 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Description
Improves Shooter Mode by moving the enable/disable toggle into the in-game quick menu, then adding a settings gear so users can customize it per game.
Main changes:
Had multiple requests for various features in this PR:
https://discord.com/channels/1378308569287622737/1520633504050974851
https://discord.com/channels/1378308569287622737/1519807205459886100
https://discord.com/channels/1378308569287622737/1516506078907469865
https://discord.com/channels/1378308569287622737/1522919897120903229
Recording
https://drive.google.com/file/d/1e8wDOqyhvrXltZ-U3yhpmYz_Q3BBx_Lk/view?usp=sharing
Type of Change
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.