[Feature] Long-Press Start to Toggle Mouse-Mode - #272
Conversation
📝 WalkthroughWalkthroughThis PR implements Menu button long-press detection with toast notification feedback and mouse mode toggling. The gamepad combo state machine gains a MenuLongPressWaiting state with injection timing logic; StreamPage adds a toast notification UI with animation; ProcessInput integrates the long-press trigger to toggle mouse mode and display transient feedback. ChangesMenu Long-Press Gamepad Combo with Toast
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: 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 |
|
@andygrundman I THINK I might have found an improvement for combo-handling while creating this (ok, claude found the bug, I fixed it). With this, I am able to spam 'select' or 'start' as quickly as I want, without interfering with the combos. Still testing, but thought you might be interested. |
mpotrykus
changed the title
mpotrykus
changed the title
|
This gets a big fat no from me, I hate long press start. Using the menu is much faster and more elegant. I also think that overloading hold-start is more risky than only taking select+start. I'm adding the same select+start menu to moonlight-qt in my Mac branch, it will have an ImGui similar popup menu because it's so handy. |
I would argue the opposite. The rest, I'll leave to preference. That being said. You should at least review the input fixes to improve your current select+start handling. |
|
Is there something wrong with the current 50ms select+start handling? I can't tell in this diff because it's mostly about the start button. I also don't understand what this comment means at all: "Sustain any injected taps for 100ms so the host gets reliable frame coverage" |
|
The fix does two things:
With the Select+Start, there was the issue where it was blocking those keys until the poll length was filled. That's how we ended up on 50ms, because it was short enough to work as a combo, but not enough to hinder the actual use of the button. But this does still add a 50ms blocker on those buttons. Not a big deal, but it's there. So far from testing, this fix completely removes the block from combo keys and allows you to spam them as quickly as you want (why you would want to spam select or start is beyond me, but to each their own). So no, the current handling is fine. This is just an improvement to make it non-blocking. |
|
You have to block the buttons in order to use them in a combo, don't you? What does non-blocking mean in this context? I don't want to allow a start press to get sent to the host until I'm sure there is no associated select button press. You're right there is no reason to spam these 2 buttons which is why they're a good choice. In qt I've implemented this using ImGui's button handling, maybe there is something to be learned from the way it works there. There is one big difference: in ImGui this check is made inline on the render thread which runs at various framerates. Possibly as low as 16fps or at max refresh rate in display-locked mode. On Xbox, the input thread runs independently and always at 500fps. This is why it's so difficult to get your 2 buttons to hit on the same "frame". I need to use the qt version a lot more to see how it feels when done the ImGui way. It doesn't do anything to hold buttons but the lower framerate may make it easier to hit both together. I also added Ctrl-Alt-Shift-Space to bring up the quick menu and as with any of the 4-button combos this is likely to send the first keys to the host. No one has tried this yet so I don't know how it will feel to people. |
|
You do need to wait, but you do not need to block. This new handling will:
So all input is now sent "as expected", but the listening is still there to achieve a combo. If the combo is achieved, no buttons are sent, except the combo. Like we want. |
|
Been running games with heavy reliance on start/select with no blocking issues. I'm going to set this PR as active, but up to you all if you actually want the feature. Let me know if you want me to separate the combo-input fix to another PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
State/GamepadState.h (1)
357-368: 💤 Low valueOptional: include the new combo fields in
DumpState.
DumpStatestill only logscomboState,viewPressed,menuPressed,startTime. Given the new long-press / injection-window logic is timing-sensitive and tricky to reason about (per the PR thread), addingmenuLongPressFired,menuInjectionStartQpc, andviewInjectionStartQpcto the log line will pay off when a user reports a misfire.♻️ Suggested addition
moonlight_xbox_dx::Utils::Logf( - "GamepadState[localId: %d, hostId: %d] buttons: %s %s axes: %d %d, %d %d, triggers: %d %d, combo{ state: %d, viewPressed: %d, menuPressed: %d, startTime: %d }\n", + "GamepadState[localId: %d, hostId: %d] buttons: %s %s axes: %d %d, %d %d, triggers: %d %d, combo{ state: %d, viewPressed: %d, menuPressed: %d, startTime: %lld, menuLongPressFired: %d, menuInj: %lld, viewInj: %lld }\n", localId, hostId, buttons, isGuideButtonDown.load() ? "Guide" : "", ltX, ltY, rtX, rtY, lTrig, rTrig, - combo.comboState, combo.viewPressed, combo.menuPressed, combo.startTime); + combo.comboState, combo.viewPressed, combo.menuPressed, + (long long)combo.startTime, combo.menuLongPressFired, + (long long)combo.menuInjectionStartQpc, (long long)combo.viewInjectionStartQpc); }🤖 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 `@State/GamepadState.h` around lines 357 - 368, DumpState currently logs only combo.comboState, combo.viewPressed, combo.menuPressed, and combo.startTime; update the moonlight_xbox_dx::Utils::Logf call inside DumpState to also include combo.menuLongPressFired, combo.menuInjectionStartQpc, and combo.viewInjectionStartQpc by adding corresponding format specifiers to the format string and passing those fields as additional arguments after combo.startTime so the new long-press/injection-window state is visible when dumping the GamepadState.
🤖 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 `@Pages/StreamPage.xaml`:
- Around line 115-126: ToastView is positioned with a fixed Margin="0,56" and
VerticalAlignment="Bottom" so it can be clipped by TV overscan; update the
layout to honor State.ScreenMargin by either moving ToastView inside the
SwapChainPanel or applying Margin="{x:Bind State.ScreenMargin}" to ToastView
(and then keep any extra 56px offset inside an inner element if you still want a
small buffer). Refer to the ToastView element, the SwapChainPanel sibling, and
State.ScreenMargin when making the change.
---
Nitpick comments:
In `@State/GamepadState.h`:
- Around line 357-368: DumpState currently logs only combo.comboState,
combo.viewPressed, combo.menuPressed, and combo.startTime; update the
moonlight_xbox_dx::Utils::Logf call inside DumpState to also include
combo.menuLongPressFired, combo.menuInjectionStartQpc, and
combo.viewInjectionStartQpc by adding corresponding format specifiers to the
format string and passing those fields as additional arguments after
combo.startTime so the new long-press/injection-window state is visible when
dumping the GamepadState.
🪄 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: b05ba62e-007b-4933-826e-59f886212df9
📒 Files selected for processing (5)
Pages/StreamPage.xamlPages/StreamPage.xaml.cppPages/StreamPage.xaml.hState/GamepadState.hStreaming/moonlight_xbox_dxMain.cpp
| <Border x:Name="ToastView" | ||
| HorizontalAlignment="Center" | ||
| VerticalAlignment="Bottom" | ||
| Margin="0,56" | ||
| Padding="20,10,20,10" | ||
| CornerRadius="6" | ||
| Visibility="Collapsed"> | ||
| <Border.Background> | ||
| <SolidColorBrush Color="DimGray" /> | ||
| </Border.Background> | ||
| <TextBlock x:Name="ToastText" Foreground="White" FontSize="12" /> | ||
| </Border> |
There was a problem hiding this comment.
Toast may be clipped by TV overscan — ToastView doesn't honor State.ScreenMargin.
ToastView is a sibling of SwapChainPanel inside the outer Grid, but only SwapChainPanel applies Margin="{x:Bind State.ScreenMargin}". With VerticalAlignment="Bottom" and Margin="0,56", the toast sits 56 px from the raw window edge. On Xbox displays with overscan that's roughly 54 px per side at 1080p and ~108 px at 4K — so on 4K TVs with default overscan the toast is likely clipped.
Either move ToastView inside SwapChainPanel or wrap the contents of the outer Grid with the screen margin.
♻️ Suggested fix (apply title-safe margin to ToastView)
- <Border x:Name="ToastView"
- HorizontalAlignment="Center"
- VerticalAlignment="Bottom"
- Margin="0,56"
- Padding="20,10,20,10"
- CornerRadius="6"
- Visibility="Collapsed">
+ <Border x:Name="ToastView"
+ HorizontalAlignment="Center"
+ VerticalAlignment="Bottom"
+ Margin="{x:Bind State.ScreenMargin}"
+ Padding="20,10,20,10"
+ CornerRadius="6"
+ Visibility="Collapsed">
<Border.Background>
<SolidColorBrush Color="DimGray" />
</Border.Background>
<TextBlock x:Name="ToastText" Foreground="White" FontSize="12" />
</Border>(If you still want a small offset above the safe-area edge, wrap the toast text in another inner element with the 56 px bottom padding.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Border x:Name="ToastView" | |
| HorizontalAlignment="Center" | |
| VerticalAlignment="Bottom" | |
| Margin="0,56" | |
| Padding="20,10,20,10" | |
| CornerRadius="6" | |
| Visibility="Collapsed"> | |
| <Border.Background> | |
| <SolidColorBrush Color="DimGray" /> | |
| </Border.Background> | |
| <TextBlock x:Name="ToastText" Foreground="White" FontSize="12" /> | |
| </Border> | |
| <Border x:Name="ToastView" | |
| HorizontalAlignment="Center" | |
| VerticalAlignment="Bottom" | |
| Margin="{x:Bind State.ScreenMargin}" | |
| Padding="20,10,20,10" | |
| CornerRadius="6" | |
| Visibility="Collapsed"> | |
| <Border.Background> | |
| <SolidColorBrush Color="DimGray" /> | |
| </Border.Background> | |
| <TextBlock x:Name="ToastText" Foreground="White" FontSize="12" /> | |
| </Border> |
🤖 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 `@Pages/StreamPage.xaml` around lines 115 - 126, ToastView is positioned with a
fixed Margin="0,56" and VerticalAlignment="Bottom" so it can be clipped by TV
overscan; update the layout to honor State.ScreenMargin by either moving
ToastView inside the SwapChainPanel or applying Margin="{x:Bind
State.ScreenMargin}" to ToastView (and then keep any extra 56px offset inside an
inner element if you still want a small buffer). Refer to the ToastView element,
the SwapChainPanel sibling, and State.ScreenMargin when making the change.
2 participants
Saw a mention on #185 about how the xbox version does not have the "long press start to toggle mouse mode" and realized I also miss that.
This PR adds:
Summary by CodeRabbit