Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retry XGrabPointer a couple of times (up to 500ms) if it failes #1961

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Visse
Copy link

@Visse Visse commented Jun 12, 2021

If you try to grab the cursor right after you created the window you get the error:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os(OsError { line: 1266, file: "/home/visse/repos/winit/src/platform_impl/linux/x11/window.rs", error: XMisc("Cursor could not be grabbed: grab location not viewable") })', src/main.rs:11:34

Minimal repro:

use winit::{event_loop::EventLoop, window::WindowBuilder};

fn main() {
    let event_loop = EventLoop::new();

    let window = WindowBuilder::new()
        .with_inner_size(winit::dpi::LogicalSize::new(128.0, 128.0))
        .build(&event_loop)
        .unwrap();

    window.set_cursor_grab(true).unwrap();
}

Checking the sdl implementation, it looks like they are solving it by retrying it a couple of times:
https://github.com/libsdl-org/SDL/blob/e65a6583201ee1a9a3bc3064655b186fc16ef719/src/video/x11/SDL_x11window.c#L1611-L1619

This PR simply implements the same scheme and retries XGrabPointer up to 10 times with a slight delay between each, for a total delay up to 500ms.

  • Tested on all platforms changed
  • Compilation warnings were addressed
  • cargo fmt has been run on this branch
  • cargo doc builds successfully
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

This can happen if we try to grab the pointer right after we created
the window
@maroider maroider added DS - x11 C - waiting on maintainer A maintainer must review this code labels Jun 12, 2021
@Visse
Copy link
Author

Visse commented Jul 31, 2021

Is there something I can do to speed up merging? Or is something missing? I'm not too familiar with the process.

@ArturKovacs ArturKovacs self-requested a review August 5, 2021 08:10
Copy link
Contributor

@ArturKovacs ArturKovacs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome!

Depending on the application, this delay may be significant, so I recommend adding a warning about this to the documentation of the set_cursor_grab function. If you have any better idea how we can let the developers know about this, I'm open to other suggestions.

@dhardy
Copy link
Contributor

dhardy commented Aug 23, 2021

Would a viable alternative be to wait until the first cursor event is received and then request the cursor grab? It shouldn't make any difference to the user except that if the cursor isn't initially over the window it won't be captured until it is (which seems fine?). That way you're never blocking.

@parasyte
Copy link

Related: #2080

@rib
Copy link
Contributor

rib commented May 29, 2022

The SDL solution to this looks like a hack really. The X11 backend could be updated to track the 'mapped' state of the window to account for the asynchronous nature of creating X windows. The code for cursor grabs would then need to have a "pending" state that's used in case the window is not currently mapped. Whenever winit gets notified of a window being mapped then it would check for any pending grabs an then apply them, which should then succeed.

@notgull
Copy link
Member

notgull commented Nov 14, 2023

Once #3122 is merged, I can update this PR to wait for events using the new userspace event queue introduced in that PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - waiting on maintainer A maintainer must review this code DS - x11
Development

Successfully merging this pull request may close these issues.

7 participants