input/seatop_default: properly notify pointer leave #5222
Conversation
sway/input/seatop_default.c
Outdated
| if (edge & (WLR_EDGE_LEFT | WLR_EDGE_RIGHT)) { | ||
| cursor_set_image(cursor, "col-resize", NULL); | ||
| } else { | ||
| if (node && node->type == N_CONTAINER) { |
There was a problem hiding this comment.
Thanks for you PR!
Why is this block changed? It would make git-bisect's life easier if only one line was modified to add a call to wlr_seat_pointer_notify_enter.
There was a problem hiding this comment.
I guess you mean git-blame here, right?
My motivation for this was readability, but TBH cursor_do_rebase() doesn't seem to be the correct place to notify enter/leave events. So, as an improvement I'd propose to either (1) move event notification out of this function to where it's called from, or (2) rename this function to better reflect what it actually does. wdyt?
Anyways, if you still prefer just to add the missing call, I'll proceed and do it :)
Thanks
There was a problem hiding this comment.
I guess you mean git-blame here, right?
Yeah, git-blame too. When searching for the cause of a bug, it'll be harder if a commit contains unrelated refactoring.
move event notification out of this function to where it's called from
Interestingly, @Xyene suggested the same thing.
"Rebasing" here means that "update the pointer to be consistent with whatever is under it". It's used when changing the layout for instance, or after a TTY switch. If we remove enter/leave events from this function, we could rename it to something like cursor_update_image and re-use it from the tablet code.
There was a problem hiding this comment.
Yeah, git-blame too. When searching for the cause of a bug, it'll be harder if a commit contains unrelated refactoring.
Ack and done.
If we remove enter/leave events from this function, we could rename it to something like cursor_update_image and re-use it from the tablet code.
Sounds like something worth doing. Will send it in a followup patch soon.
Thanks
There was a problem hiding this comment.
👍
To summarize a discussion from IRC yesterday (since I imagine you might find it interesting):
- the cursor/seat code predates tablet and touch specs, so is tightly coupled with pointers
- this is "surprising", e.g. you'd expect functions like
cursor_motionorcursor_do_rebaseto not send pointer events, but they do- this means they're not used for tablet/touch support, instead both tablet and touch re-implement ad-hoc parts of that functionality
- since fewer people use those input methods, it's easier for the code to fall behind without anyone noticing (or reporting)
- tablet support, at least, currently has some issues1 that would likely best be fixed by decoupling the
*cursor*functions from sending pointer events, and moving those events to the call sites
The plan proposed looks something like this:
- move the pointer events out of the cursor functions
- reuse them for tablet support — this'd fix a bunch of tablet issues
- for future maintainability, add handlers for tablet and touch into seatop, so that the logic for those devices can be migrated from
cursor.c - once everything's in one place rather than two and confirmed "not broken by refactoring", it'll be easier to see what abstractions make sense
1 There are more if you just search for "tablet", but these are the ones I know the cause of so far 😉
There was a problem hiding this comment.
Hi @Xyene, thanks for the info. It sounds like a good plan for me.
Currently, clients receive wl_data_device::leave events only when the pointer enters another surface, which leads to issues, such as swaywm#5220. This happens because wlr_seat_pointer_notify_enter() is called when handling motion events only for non-NULL surfaces. Fixes swaywm#5220
nickdiego
force-pushed
the
fix/data_device_leave
branch
from
63b0f44
to
2eb59b2
Compare
Done. Thanks |
|
Thanks for the fix! |
|
This change caused wlroots to segfault on pointer leave when an xdg_shell popup is active. I've written up an analysis at swaywm/wlroots#2162, but the tl;dr is I think we should revert this until we fix the underlying issue. |
|
Let's wait a little bit, I'll revert if we don't come up with a fix in a few days. |
|
revert? |
We are not allowed to do what we did in swaywm#5222 and pass a `NULL` surface wlr_seat_pointer_notify_enter(), and it's causing crashes when an xdg-shell popup is active (see swaywm#5294 and swaywm/wlroots#2161). Instead, solve swaywm#5220 using the new wlroots API introduced in swaywm/wlroots#2217.
This is necessary for some grabs, which currently have no way of knowing when the pointer/keyboard focus has left a surface. For example, without this, a drag-and-drop grab can erroneously drop into a window that the cursor is no longer over. This is the plumbing needed to properly fix swaywm/sway#5220. The existing fix, swaywm/sway#5222, relies on every grab's `enter()` hook allowing a `NULL` surface. This is not guaranteed by the API and, in fact, is not the case for the xdg-shell popup grab and results in a crash when the cursor leaves a surface and does not immediately enter another one while a popup is open (#2161). This fix also adds an assertion to wlr_seat_pointer_notify_enter() that ensures it's never called with a `NULL` surface. This will make Sway crash much more until it fixes its usage of the API, so we should land this at the same time as a fix in Sway (which I haven't posted yet).
We are not allowed to do what we did in #5222 and pass a `NULL` surface wlr_seat_pointer_notify_enter(), and it's causing crashes when an xdg-shell popup is active (see #5294 and swaywm/wlroots#2161). Instead, solve #5220 using the new wlroots API introduced in swaywm/wlroots#2217.
Currently, clients receive wl_data_device::leave events only when the
pointer enters another surface, which leads to issues, such as #5220.
This happens because wlr_seat_pointer_notify_enter() is called when
handling motion events only for non-NULL surfaces.
Fixes #5220