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

Wayland: Fix panic when calling set_cursor_grab from a different thread than evlp's one. #1206

Merged
merged 1 commit into from
Oct 16, 2019

Conversation

kchibisov
Copy link
Member

@kchibisov kchibisov commented Oct 4, 2019

CursorManagers` grab_pointer can only be called from a thread on which pointer_constraints_proxy event queue is located, so calling it directly from a Window doesn't work well, in case you've sent your window to another thread, so we need to pass cursor grab updates to the event loop and call this function from there.

The code is basically a copy of @andersrein approach for handling a cursor grab thing.

Fixes regression introdced in 5ced36e

Some explanation why things were changed in #1204.

However @andersrein approach is not a good one for set_cursor_icon and set_cursor_visible, since you can face cursor icon update delays when pressing and holding Shift for e.g., the cursor icon will be changed with a bit of a delay according to my testing. I didn't find it at the first place while testing alacritty, since this case was quite odd and I completely forgot about it. So cursor_grab was auto converted to follow set_cursor_icon.

  • Tested on all platforms changed
  • Compilation warnings were addressed
  • cargo fmt has been run on this branch
  • 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

@@ -441,6 +441,8 @@ impl WindowStore {
bool,
bool,
bool,
Option<bool>,
&wl_surface::WlSurface,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any particular reason we don't just pass a reference to the entire internal window instead of many bit and pieces of it?

Copy link
Member Author

Choose a reason for hiding this comment

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

They wont be the same and we can't really pass it, since we want to mutate existing one and send something different (replace, take). make_wid doesn't make sense in event loop proxy either. We can try to do it ofc, but I'd like to leave it as it is.

@vberger probably have some explanation on why things are written this way though.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be quite honest I don't remember exactly the reason I set it like that originally, most likely I didn't find a better way. And then things grew on top of it without changing the pattern.

There may certainly be a better way to structure this, but I agree this would be for an other PR. In general I suspect there is room for some refactoring / reorganization of winit's wayland backend, and I also suspect this would better be done in link with Smithay/client-toolkit#77.

self.current_cursor = cursor;

self.set_cursor_icon_impl(cursor);
if self.cursor_visible {
Copy link
Member Author

Choose a reason for hiding this comment

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

Follow X11 behavior on cursor icon updates when it's in invisible state.

Copy link
Contributor

@andersrein andersrein left a comment

Choose a reason for hiding this comment

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

I think it looks good

…ad than evlp's one.

This commit also start following X11 behavior on cursor icon update when
cursor is invisible.

Fixes regression introdced in 5ced36e
@Osspial Osspial merged commit 765225d into rust-windowing:master Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants