fix: pointer constraints, xwayland positioning, and physics tick - #8
sevenluckz wants to merge 10 commits into
Conversation
|
Thanks — I went through all three commits and built the branch against wlroots 0.20. It compiles clean and merges cleanly on current 1. fwm does draw the pointer itself, but it takes the image from the client ( 2. Both the comment in One gap this leaves, which is not a regression and does not need to be in this PR — 3. The part I agree with entirely: the early But the same change now also runs focus-follows-pointer on every motion event under a lock. The cursor is frozen, yes — the windows are not. fwm's windows drift and get thrown around on their own, and when one lands under the frozen cursor, Could you keep the constraint bookkeeping and leave focus alone: if (view && !locked) server_focus_view(server, view);Same class of thing, lower stakes, up to you: Ping me when you've marked it ready and I'll merge. Nice digging — you found two places where the comments and the code had drifted apart. |
492f670 to
06b8942
Compare
|
Hey, this is all I have for the moment! I've hopefully solved the focus issue you mentioned, and I also added logic to track the cursor in relation to the window when it's locked (and instantly drop the lock if the window goes off-screen). I also moved the Finally, I added more position updates for Xwayland while its windows are moving to fix an issue I noticed where detached UI elements (like dropdown menus) were not updating their positions dynamically. |
|
Thanks for this — I built the branch against wlroots 0.20 and it compiles clean, all 7 ctest targets pass. Replacing the manual boundary drop with Three things I'd like fixed before merging, all in the constraint/physics commits: 1. The The early-out keys on Cache the computed 2. The cursor warp during glides isn't gated on LOCKED ( The condition is just Also worth a comment: 3. Breaking the lock off-screen does more than break the lock ( Besides deactivating the constraint it force-sets the Minor, not a blocker and pre-existing: the region offset is computed from One process note: the PR title reads as a cursor fix, but only |
iluaii
left a comment
There was a problem hiding this comment.
Marking this as changes-requested so the status is visible in the UI — my earlier feedback went out as plain comments, so GitHub never flagged the PR and it may well have been lost in your inbox. Nothing new here, this is just the checklist from #8 (comment):
src/view.c:168— cache the computedsx/syrather thanview->x/y, and treat aserver_world_to_screen()failure as "not cached". As written the early-out skips the reconfigure after a camera move, which is the case the coordinate fix exists for.src/server_tick.c:1157— gate the cursor warp onconstraint->type == WLR_POINTER_CONSTRAINT_V1_LOCKED; a CONFINED constraint shouldn't drag the pointer along.src/server_tick.c:1170— on breaking the lock,constraints_follow_focus(server, NULL)is enough; drop the forceddefaultxcursor and thewlr_seat_pointer_clear_focus().
No rush on my end, and no need to rebase or reshape anything. The standing offer also holds: if you'd rather land something now, I'll take d7fc7c8 (fallback cursor) on its own and we can keep the constraint work in this PR.
|
I've pushed updates to address your feedback:
Minor (CSD Geometry Insets): Updated I've also included fixes for related edge cases found during testing:
|
iluaii
left a comment
There was a problem hiding this comment.
Thanks — all three checklist items are addressed, and I rebuilt the branch against wlroots 0.20.2: it merges cleanly onto current main, compiles without warnings, and all 10 ctest targets pass.
Confirming what's now settled:
src/view.c:156— the cache keys on the computedsx/syand aserver_world_to_screen()failure invalidates it, so a camera move does reach X now.server_world_to_screentoleratesNULLout-params (src/server_output.c:483), so theNULL, NULLprobe in the tick is fine too.src/server_tick.c:1164— gated onLOCKED, with the world-vs-layout caveat written down.src/server_tick.c:1175— down toconstraints_follow_focus(server, NULL)alone.
And the CSD inset is handled via xdg_toplevel->base->current.geometry, which is what I meant.
38d8cc8 is a genuine find and the fix is the right one — the header says it outright: "Deactivate the constraint. May destroy the constraint." With lifetime == ONESHOT, wlr_pointer_constraint_v1_send_deactivated() destroys the object synchronously, handle_constraint_destroy removes the listener, and constraint_set_active then removes it a second time. Unhooking before the signal is correct.
Two things left, then I'll merge.
1. Off-screen X11 windows now get a configure every tick (src/view.c:156, src/server_tick.c:1162)
view_sync_position() runs unconditionally for every unpinned body in the physics tick, which includes every window sitting on a desktop no monitor is showing. For those server_world_to_screen() fails, so the cache is stamped with -999999, the early-out can never hit, and wlr_xwayland_surface_configure() goes out at 60 Hz per window — carrying raw world coordinates, which is the value the early-out was there to suppress in the first place. Clients that relayout on ConfigureNotify (Chrome, anything AWT) will do it 60 times a second while parked on another desktop.
Cheap fix: keep a bool last_sync_onscreen alongside the coordinates, cache whatever was actually sent in both cases, and compare all of it. That also retires the -999999 sentinel.
2. The comment under locked says the opposite of what the code does (src/server_pointer.c:145)
/* Mouse-look: the cursor does not move at all, but we still need
* to send motion events with constant coordinates. */The code deliberately skips wlr_seat_pointer_notify_enter/motion while locked, which is right — the client is getting relative motion from the relative-pointer protocol, and absolute motion under a lock is exactly what we're suppressing. The reason to fall through is hit-testing and constraints_follow_focus. Could you reword it to say that?
Neither of these needs a reshape, and no rush.
Two non-blockers for the record, no action needed here:
- If
wlr_region_confine()ever returns false the pointer is freed, but should it still be over the same surface,constraints_follow_focusre-arms the same constraint while every subsequent motion also starts outside the region — so confinement stays broken until focus actually leaves. In practice the pointer is inside the region at activation, so this only bites after aset_regionwhile the pointer is elsewhere. I'll open an issue rather than grow this PR. af1900dduplicatesa8ec5a6, which landed onmainafter you branched. Identical change, merges clean, ends up a no-op — nothing to do, I'll just drop it from the squash message.- Seven of the added lines carry trailing whitespace.
|
Heads up:
It is worth resolving by hand rather than by taking a side, because the two changes meet on the same question. if (server_drag_motion(server, lx, ly, &now)) return;
if (pointer_grab_deliver(server, lx, ly, event->time_msec)) return;
pointer_update_focus(server, lx, ly, event->time_msec);Your Also note No rush, and nothing about the review changes — the two items from my last comment still stand (the 60 Hz configure storm for off-screen X11 windows, and the comment at what is now a different line number in |
|
I've taken The constraint listener crash was real, and it was still live on Your diagnosis was exactly right: I did have to resolve the conflict rather than replay the commit, and the resolution is not the two-line swap you wrote — the function grew a cursor hint in the meantime, and unhooking the listener first turns the double-remove into a use-after-free instead of fixing it. With the listener gone, wl_list_remove(&server->constraint_destroy.link); /* unhook before the signal */
server->active_constraint = NULL;
pointer_apply_constraint_hint(server, old); /* read while old is alive */
wlr_pointer_constraint_v1_send_deactivated(old); /* old may die here */Your fix, plus the hint moved ahead of the signal for the same reason yours moved the unlink. Builds clean against wlroots 0.20, 12/12 ctest. Four of your commits are on While this PR was waiting on a rebase, I landed my own versions of work you had already written and I had already reviewed and agreed with:
I should have cherry-picked those from you on the day I reviewed them, or told you I was going to write them myself. Instead I asked you to hand-resolve a conflict against the very commits that had displaced your own, on the same day I pushed them. If that is why the branch went quiet, it was a fair reading and I don't blame you for it. For what it's worth on attribution: your commits here are authored as What is genuinely still missing from
Both are things I'd still like, and neither is invalidated by what landed. Whatever suits you:
No deadline attached to any of that. Thanks for the crash — it was a genuinely sharp find, and it would have kept biting. |
…to fix detached dropdown menus
The cache introduced for view_sync_position tracked the static 'world' coordinate of the view rather than the dynamic 'screen' coordinate. When the camera panned to a new desktop, the screen coordinates changed, but since the world coordinate remained identical, the update was skipped. This resulted in Xwayland retaining stale coordinates for the main window, causing popups (like context menus) to appear on entirely wrong physical monitors. To solve this systematically and prevent translating coordinates twice, the caching logic has been consolidated directly into view_set_size. Additionally, we explicitly treat server_world_to_screen failures as "not cached" by bypassing the early-out and invalidating the cache (setting it to -999999) when the window is parked off-screen. This ensures that the raw world position is always correctly sent as a fallback, and when the window comes back on-screen, Xwayland is guaranteed to receive the fresh screen coordinates immediately.
8d9b360 to
e491d07
Compare
|
Hey, thanks for the review. Sorry for taking a while to get back to this. I've decided to take the option to rebase and keep going. I've rebased the branch onto the latest
Regarding the git config issue with the commit author, that was indeed a fault with my config and thanks for pointing it out. It should be fixed for my previous and future commits. Let me know if everything looks good now! |
iluaii
left a comment
There was a problem hiding this comment.
Thanks for coming back to this, and for taking the rebase — it landed cleanly. The branch sits on adb5f38, builds against wlroots 0.20 with no new warnings (the two that do appear are in config_binds.c and server_config.c, neither of which you touch), and all 16 ctest targets pass. The attribution is sorted too: the API now resolves every commit here to your account, so the credit sticks this time.
One bookkeeping note before the substance. The description at the top has drifted from what the branch actually contains — the CSD geometry insets, the server_seat.c double-free, and the off-screen constraint break are all gone from the diff (the crash fix because it landed as d808223, the other two, I assume, in the rebase). Worth trimming it so the next reader isn't looking for code that isn't there.
Two things I'd like changed before I merge, one more I'd like folded in, and one I've talked myself out of.
1. The view_set_size cache swallows the answer to a ConfigureRequest (src/view.c:189)
The early-out is right for the physics tick, which is what it was asked for, and it does retire the 60 Hz storm. But view_sync_position() has a second caller with a different job. xwl_handle_request_configure calls it not to move the window but to tell the client no — the synthetic ConfigureNotify ICCCM 4.1.5 requires when a request is ignored. The cache compares exactly the fields that a refusal leaves untouched:
- The fullscreen branch (
src/view.c:564) exists solely to re-assert the geometry to a game that asked for a different size — the CS2 case in the comment right above it.view->width/heightand the position are all unchanged there by construction, so the cache hits and no configure goes out at all. The refusal never reaches the client, and thecfg_denied_w/hguard above it is now rate-limiting a call that does nothing. - The ordinary branch (
src/view.c:573) has the same hole for a client that asks only to move:ev->width/ev->heightcome back identical, our position is unchanged, cache hit, silence. Before this commit it got a configure carrying our position, which is the "no" it was waiting for.
Cheapest fix is to invalidate before answering — view->last_sync_w = -1; at the top of xwl_handle_request_configure — but I'd rather see the two jobs named apart: view_sync_position() stays cached for the tick, and the reply path gets a forced variant. The distinction is real and it will bite again otherwise.
2. The LOCKED block in pointer_grab_deliver can't be reached (src/server_pointer.c:209)
pointer_grab_deliver has one caller, process_cursor_motion:398. process_cursor_motion has two, handle_cursor_motion:478 and handle_cursor_motion_absolute:515. Both still open with
if (server->active_constraint && !lock_is_active(server)) {
if (server->active_constraint->type == WLR_POINTER_CONSTRAINT_V1_LOCKED) {
server_notify_activity(server);
return;
}which this branch leaves in place (server_pointer.c:448, :502). So getting past them with a lock held needs lock_is_active(server) to be true — and then process_cursor_motion:345 returns before line 398. No path arrives at the new block with a LOCKED constraint active.
What worries me more than the dead code is the direction it points. It returns true before the button_count check, so if that early return upstream is ever removed — which is the whole shape we agreed on in the first round — this becomes an unconditional "the pointer is spoken for" that suppresses pointer_update_focus outright, and with it constraints_follow_focus. That is the original hole, re-dug one function further down.
Either drop the hunk and the comment edit at :397 that describes it, or finish the move: take the early return out of both motion handlers and put the guard where focus actually changes, if (view && !locked) server_focus_view(...) inside pointer_update_focus. I'm happy with either — the first keeps the tree honest, the second is the design. What's there now is neither.
3. The absolute-motion path kept the old behaviour (src/server_pointer.c:508)
wlr_region_confine went into handle_cursor_motion only; handle_cursor_motion_absolute still calls constraint_allows_at and drops the event. A tablet, or the pointer a VM hands its guest, therefore still gets the stall you replaced rather than the clamp. 85a0e2d deliberately brought those two paths together for exactly this reason — please apply the same clamp there. constraint_allows_at then has no callers left and can go with it.
4. The stuck grab: taking it as written (src/server_pointer.c:736)
This is the best find in the branch and it is live on main today, independent of everything else here. A GTK client drags its own CSD titlebar → xdg_toplevel.move → handle_request_move → server_start_interactive_move sets FWM_ACTION_MOVE. The press already went out (action was NONE at the time), so button_count is 1; at the release action is no longer NONE, the release is withheld, and the count never comes back down. Since 1727912 that jams pointer_grab_deliver into returning true for every motion, and the client goes on believing its button is held.
To be precise about the damage, because it is worse than "swallows left clicks" and slightly better than permanent: focus-follows-pointer comes back for exactly one motion event each time something else clears pointer focus — the launcher, expo, the ring, the region selector all do — because ptr_surface then no longer matches focused_surface and the grab check falls through. pointer_update_focus runs once, restores the match, and the next motion jams again. wlr_seat_pointer_clear_focus doesn't touch button_count, so nothing ever unsticks it.
I went looking for a hole in button_count > 0 — it doesn't name which button, so on paper a release for a button the compositor claimed and never forwarded would reach the client as a phantom. It doesn't: I disassembled the 0.20 we build against, and wlr_seat_pointer_notify_button walks pointer_state.buttons[] on a release, and returns 0 without ever calling grab->interface->button when the button isn't there or its n_pressed doesn't reach zero. And the condition can't miss a release either — if the client holds the press, the count is at least 1. So it's correct as written, and I'd rather not add bookkeeping to say what the seat already knows.
The one thing I'd ask: the comment explains why we must send the release, but not why it's safe to send one we might not owe. A line saying the seat drops a release for a button it isn't holding would save the next reader the trip through the disassembler.
Since it stands entirely on its own, I'll take this commit ahead of the rest whenever you like — say the word and it goes onto main before the constraint work is finished.
Smaller, none of them blocking:
src/server_pointer.c:732has trailing whitespace, whichgit diff --checkcatches — a little funny given295fc36.src/server_tick.c:1853:int dx = body->x - view->x;truncates, while the scene node is placed from the exact double on the line below. Over a long glide the cursor drifts against the window by a pixel or two, which is the thing the commit sets out to prevent.- The
wlr_region_confine() == falsecase is still a stall — theelsedrops the motion, and if the pointer is genuinely outside the region every subsequent motion starts outside it too. Not a regression and not yours to fix here; that's the issue I said I'd open.
Ping me when 1, 2 and 3 are done and I'll merge.
|
Taking you up on the offer from my last review: Two small things I folded in rather than asking you for another round:
/* If we sent a press to the client (e.g. they clicked the titlebar), we MUST
* send the release so the grab completes, even if the compositor took over
* the drag in the meantime. Otherwise wlr_seat's button_count stays stuck > 0.
* Sending one we do not owe is harmless: on a release the seat walks its own
* pointer_state.buttons[] and returns without touching the client if that
* button is not among the ones it is holding. */Builds clean against wlroots 0.20, all 18 ctest targets pass. So when you next rebase, drop |
This PR addresses Xwayland coordinate sync issues and hardens the compositor's core pointer constraint logic.
1. Xwayland default cursor
Fetches the
defaultxcursor buffer and sets it viawlr_xwayland_set_cursorto prevent the legacy X11 fallback cursor.2. Xwayland Positioning/Physics Tick
Translates world coordinates to screen coordinates during
view_set_sizeto restore hit-testing on non-primary desktops.Xwayland coordinates are also continuously synced (
view_sync_position) during physics glides and drags to prevent detached popups and dropdowns. To support desktop switching, this synchronization caches the computedsx/syscreen coordinates, naturally treating coordinate translation failures as un-cached.3. Pointer Constraints & Crash Fixes
Hardens both
LOCKEDandCONFINEDpointer constraints:returnwith alockedboolean check to keep the cursor visually frozen and suppress motion events while still allowing focus/hit-testing to process.wlr_region_confineto strictly clamp the pointer.LOCKEDconstraints).constraints_follow_focus(server, NULL)without forcing the seat's pointer focus.SIGSEGVinserver_seat.ccaused by a doublewl_list_remove. The constraint listener is now unhooked before sending the deactivated signal to prevent memory corruption.