Make ClickGrab's motion focus better when the surface has moved#2091
Make ClickGrab's motion focus better when the surface has moved#2091kelnos wants to merge 3 commits into
Conversation
If the focus target has moved since the grab has started, the focus parameter in the start data will be out of date, and motion events will be sent relative to the old/original position of the surface. We ran into this with xfce4-panel's move code. It's a layer-shell surface, and when the user grabs the move handle and drags, it moves itself by watching pointer motion events and calculating the diff from the previous position, and then updating its margins. With the motion events delivered with respect to the start data's focus position, the panel move goes wild, because it is (unknowingly) recomputing its margins based on the cumulative difference in the pointer's location, rather than the diff. This isn't perfect, however: if the user makes a fast drag that causes the pointer to leave the surface entirely, the `focus` param will be None, and it will have to fall back to the value in start data. I'm not sure there's a good way to fix that without changing the interface of PointerGrab or SeatHandler::PointerFocus.
This is related to the previous commit, and refines it slightly. When the pointer is over the start focus, continually update the current location. If the pointer leaves the start focus, fall back to using the last-known location of the start focus.
This is related to the previous two commits, and is the "perfect" solution. The downside is it requires the compositor to implement a new (optional) hook in SeatHandler to provide the current location of the focus target. This completely fixes the issue at hand.
Ouch. This sounds like quite the hack for something that really should have a move request (something layer surface really was never designed to provide). Does this behave correctly in other compositors? I would really like to avoid adding api, that assumes anything in global coordinate space. (At the very least we need to update the wording to mean the cursor coordinate space, but ideally I would love to figure out a way to avoid commit 3 in it's current form all together.) |
Yeah, I don't love it. We've always offered the ability (on X11) for users to move panels around by grabbing them, and there wasn't really another way to implement this. I thought about an extension protocol on top of layer-shell, but as you say, how the surface works isn't really designed for that in the first place. And it would be difficult to implement for us client-side as we're using gtk3-layer-shell, and I suspect an extension protocol that messes with margins and stuff from the compositor side would break the library's internal state.
Yes, it works fine in labwc and wayfire at least.
Understood, I'm not really happy with the third commit either. My fallback plan was going to be to just copy/paste ClickGrab into my compositor, edit it, and manually install my custom grab on click, since of course I can fix this myself in my own grab code. But I do believe this represents a real issue, regardless of our hacky move code/process. If ClickGrab is active and then the surface (not just layer-surface, any surface) initially under it moves (for any reason, even if not user-initiated), then motion events during the grab will be incorrect. This is maybe a very rare corner case, so maybe it isn't worth fixing. labwc and wayfire don't have this problem because AFAICT wlroots doesn't have an implicit grab there, and the compositors themselves implement them, and have the information needed to not have this problem. Anyway, I'm open to whatever you'd like to do here, even if that turns out to be nothing :) |
Fair enough.
Yeah thinking of solutions to this mess is the difficult part I think. I think the sanest option to fix this in smithay would be to expose more of the DefaultGrab and ClickGrab and allow downstream to set it's own. This could be as simple as having a But maybe something like a generic + trait and have smithay provide a |
2 participants
Description
If the focus target has moved since the grab has started, the focus
parameter in the start data will be out of date, and motion events will
be sent relative to the old/original position of the surface.
We ran into this with xfce4-panel's move code. It's a layer-shell
surface, and when the user grabs the move handle and drags, it moves
itself by watching pointer motion events and calculating the diff from
the previous position, and then updating its margins.
With the motion events delivered with respect to the start data's focus
position, the panel move goes wild, because it is (unknowingly)
recomputing its margins based on the cumulative difference in the
pointer's location, rather than the diff.
I've broken this into three commits, because the first may be less objectionable than the second, which may be less objectionable than the third, even though the third is the only one that truly fixes the problem.
The first commit prefers the coordinates in the
focusparam to the function over that instart_data.focus. This helps, but if the user moves the pointer fast enough that it's outside the focus target for any length of time, it has to go back to the start data, which again causes problems.The second commit stores the last-known focus (that matches the target of the start focus) in
ClickGrab, and when thefocusparam to the function isNone, it falls back to the stored location. While this is better, honestly it still doesn't behave very well in my testing; it's still very easy to get the panel to start moving wildly around the screen, completely divorced from the pointer's position (though it is a bit easier to carefully get the pointer back on top of the panel to fix the situation).The third commit is the real fix: I've added an optional hook to
SeatHandlerto allowClickGrabto ask the compositor the current location of thePointerFocus. I'm not quite sure about the API here, if that's a reasonable place to put it, but this does completely fix the issue for me, at least.Anyhow, happy to make any changes, and/or squash things down to whatever solution y'all feel is the correct one. I looked at labwc and wayfire, and this is how they both work in the face of moving surfaces during an implicit pointer grab.
Checklist