Feature: mouse capture to main Window (disable/enable with mouse middle button)#1805
Feature: mouse capture to main Window (disable/enable with mouse middle button)#1805maximilien-noal wants to merge 12 commits intomasterfrom
Conversation
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
ae2e85e to
5689cc9
Compare
5689cc9 to
04a7a93
Compare
refactor: disable/enable mouse capture, inside ViewBox feature: mouse capture (crossplatform) Signed-off-by: Maximilien Noal <noal.maximilien@gmail.com>
04a7a93 to
ea96d4f
Compare
There was a problem hiding this comment.
Pull request overview
Adds an initial “mouse capture” feature to the Avalonia game window so the cursor can be constrained to the emulator display area (useful for edge-scrolling games).
Changes:
- Introduces a new
NativeMouseCapturehelper (WindowsClipCursor, macOS CoreGraphics calls, Linux X11 grab). - Moves mouse input wiring into
MainWindowand adds middle-click toggling + periodic recapture logic. - Appends a capture status hint to the main window title.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Spice86/Views/MainWindow.axaml.cs | Hooks pointer events, toggles capture via middle-click, and manages capture lifecycle on window events. |
| src/Spice86/Views/MainWindow.axaml | Adds a Name to the Viewbox hosting the display. |
| src/Spice86/ViewModels/MainWindowViewModel.cs | Adds MouseCaptureHint and includes it in the window title. |
| src/Spice86/Native/NativeMouseCapture.cs | New cross-platform native mouse capture implementation (Win/macOS/Linux). |
| src/Spice86/App.axaml.cs | Removes now-relocated pointer-event wiring from App startup. |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@maximilien-noal I've opened a new pull request, #1962, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Initial plan * fix: address all mouse capture PR review comments Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com>
| return false; | ||
| } | ||
|
|
||
| int hideResult = NativeMouseCaptureInterop.CGDisplayHideCursor(mainDisplay); |
Check notice
Code scanning / CodeQL
Calls to unmanaged code Note
Copilot Autofix
AI 1 day ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| return false; | ||
| } | ||
|
|
||
| int showResult = NativeMouseCaptureInterop.CGDisplayShowCursor(mainDisplay); |
Check notice
Code scanning / CodeQL
Calls to unmanaged code Note
Copilot Autofix
AI 1 day ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| private static bool _isCaptured; | ||
|
|
||
| public static bool EnableCapture() { | ||
| uint mainDisplay = NativeMouseCaptureInterop.CGMainDisplayID(); |
Check notice
Code scanning / CodeQL
Calls to unmanaged code Note
Copilot Autofix
AI 1 day ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| } | ||
|
|
||
| public static bool DisableCapture() { | ||
| uint mainDisplay = NativeMouseCaptureInterop.CGMainDisplayID(); |
Check notice
Code scanning / CodeQL
Calls to unmanaged code Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general terms, since we cannot reasonably replace CoreGraphics mouse APIs with fully managed alternatives, the best practical fix is to minimize and encapsulate calls to unmanaged code. For this class, that means calling CGMainDisplayID() only once, caching its result in a managed field, and reusing that cached value in both EnableCapture and DisableCapture. This reduces unmanaged interop calls and keeps the managed interface unchanged.
Concretely, in src/Spice86/Native/MacOsMouseCaptureBackend.cs:
- Add a private static
uintfield (e.g.,_mainDisplayId) initialized to 0 to cache the main display ID. - Add a private helper method (e.g.,
TryEnsureMainDisplayId(out uint mainDisplay)) that:- If
_mainDisplayIdis 0, callsNativeMouseCaptureInterop.CGMainDisplayID()once, stores it in_mainDisplayId, and returnstrueonly if it is non‑zero. - If
_mainDisplayIdis already set, just returns it without another unmanaged call.
- If
- In
EnableCapture, replace the direct call toCGMainDisplayID()with a call toTryEnsureMainDisplayIdand use the resultingmainDisplayvalue. - In
DisableCapture, similarly replace the direct call toCGMainDisplayID()with the helper, again using the cached value.
This keeps all external behavior the same (same success/failure conditions and error handling) but reduces unmanaged calls and centralizes the interop usage.
| @@ -5,10 +5,19 @@ | ||
| internal static class MacOsMouseCaptureBackend { | ||
| private const int CgErrorSuccess = 0; | ||
| private static bool _isCaptured; | ||
| private static uint _mainDisplayId; | ||
|
|
||
| private static bool TryEnsureMainDisplayId(out uint mainDisplay) { | ||
| if (_mainDisplayId == 0) { | ||
| _mainDisplayId = NativeMouseCaptureInterop.CGMainDisplayID(); | ||
| } | ||
|
|
||
| mainDisplay = _mainDisplayId; | ||
| return mainDisplay != 0; | ||
| } | ||
|
|
||
| public static bool EnableCapture() { | ||
| uint mainDisplay = NativeMouseCaptureInterop.CGMainDisplayID(); | ||
| if (mainDisplay == 0) { | ||
| if (!TryEnsureMainDisplayId(out uint mainDisplay)) { | ||
| _isCaptured = false; | ||
| return false; | ||
| } | ||
| @@ -30,8 +37,7 @@ | ||
| } | ||
|
|
||
| public static bool DisableCapture() { | ||
| uint mainDisplay = NativeMouseCaptureInterop.CGMainDisplayID(); | ||
| if (mainDisplay == 0) { | ||
| if (!TryEnsureMainDisplayId(out uint mainDisplay)) { | ||
| _isCaptured = false; | ||
| return false; | ||
| } |
| } | ||
|
|
||
| if (_display == IntPtr.Zero) { | ||
| _display = NativeMouseCaptureInterop.XOpenDisplay(IntPtr.Zero); |
Check notice
Code scanning / CodeQL
Calls to unmanaged code Note
Copilot Autofix
AI 1 day ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| } | ||
|
|
||
| if (_emptyCursor != IntPtr.Zero) { | ||
| NativeMouseCaptureInterop.XFreeCursor(_display, _emptyCursor); |
Check notice
Code scanning / CodeQL
Calls to unmanaged code Note
Copilot Autofix
AI 1 day ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| _emptyCursor = IntPtr.Zero; | ||
| } | ||
|
|
||
| NativeMouseCaptureInterop.XCloseDisplay(_display); |
Check notice
Code scanning / CodeQL
Calls to unmanaged code Note
Copilot Autofix
AI 1 day ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| } | ||
|
|
||
| private static IntPtr CreateEmptyCursor() { | ||
| IntPtr rootWindow = NativeMouseCaptureInterop.XDefaultRootWindow(_display); |
Check notice
Code scanning / CodeQL
Calls to unmanaged code Note
Copilot Autofix
AI 1 day ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| int grabResult = NativeMouseCaptureInterop.XGrabPointer( | ||
| _display, | ||
| windowHandle, | ||
| 0, | ||
| eventMask, | ||
| GrabModeAsync, | ||
| GrabModeAsync, | ||
| windowHandle, | ||
| _emptyCursor, | ||
| CurrentTime); |
Check notice
Code scanning / CodeQL
Calls to unmanaged code Note
Copilot Autofix
AI 1 day ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| CurrentTime); | ||
|
|
||
| if (grabResult != GrabSuccess) { | ||
| NativeMouseCaptureInterop.XSync(_display, 0); |
Check notice
Code scanning / CodeQL
Calls to unmanaged code Note
Copilot Autofix
AI 1 day ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
|
|
||
| _isCaptured = true; | ||
|
|
||
| NativeMouseCaptureInterop.XSync(_display, 0); |
Check notice
Code scanning / CodeQL
Calls to unmanaged code Note
Copilot Autofix
AI 1 day ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| return false; | ||
| } | ||
|
|
||
| int ungrabResult = NativeMouseCaptureInterop.XUngrabPointer(_display, CurrentTime); |
Check notice
Code scanning / CodeQL
Calls to unmanaged code Note
Copilot Autofix
AI 1 day ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| return false; | ||
| } | ||
|
|
||
| NativeMouseCaptureInterop.XSync(_display, 0); |
Check notice
Code scanning / CodeQL
Calls to unmanaged code Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
General approach: We cannot replace XSync with a pure managed alternative, but we can reduce the number of unmanaged transitions by avoiding redundant calls. That lessens overhead and slightly reduces the surface area of unmanaged interaction, aligning with the spirit of the recommendation without changing behavior.
Best concrete fix in this snippet: In DisableCapture, after a successful XUngrabPointer, we call NativeMouseCaptureInterop.XSync(_display, 0); on line 76. EnableCapture already performs a sync after attempting the grab, and Cleanup will close the display and free resources. For most X11 patterns, an explicit XSync after XUngrabPointer is not strictly required for correctness in this high‑level managed wrapper, whereas each XSync forces a roundtrip to the X server. Removing this specific XSync reduces calls to unmanaged code without altering the public API or observable behavior in typical usage (the ungrab still occurs; the X server will process it as part of its normal request handling).
Changes needed:
- File:
src/Spice86/Native/X11MouseCaptureBackend.cs - In method
DisableCapture, remove the line that callsNativeMouseCaptureInterop.XSync(_display, 0);. - No new methods, imports, or type definitions are required.
| @@ -73,8 +73,6 @@ | ||
| return false; | ||
| } | ||
|
|
||
| NativeMouseCaptureInterop.XSync(_display, 0); | ||
|
|
||
| _isCaptured = false; | ||
|
|
||
| return true; |
| NativeMouseCaptureInterop.XColor black = new NativeMouseCaptureInterop.XColor(); | ||
| IntPtr cursor = NativeMouseCaptureInterop.XCreatePixmapCursor(_display, pixmap, pixmap, ref black, ref black, 0, 0); | ||
|
|
||
| NativeMouseCaptureInterop.XFreePixmap(_display, pixmap); |
Check notice
Code scanning / CodeQL
Calls to unmanaged code Note
Copilot Autofix
AI 1 day ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
Description of Changes
Enables mouse capture to the mainWindow edges.
Rationale behind Changes
A lot of games use edge scrolling (Dune 2, Transarctica)
Dune has controls on the edges of the screen that are easier to target with this feature.
Suggested Testing Steps
Tested on Windows succesfully.
Other platforms are fully implemented, but if it doesn't work there is no issue (no exceptions) anyway.