wayland/pointer_constraints: call remove_constraint when pointer leave surface

[skygrango]

Description

related: #2027 (comment)

The implementation ensures that remove_constraint() is invoked when the pointer leaves the surface, allowing the last saved cursor position hint to be applied.

This both improves the expected behavior of Lifetime::Persistent and ensures that pointer focus changes are handled immediately.

Checklist

• I agree to smithay's Developer Certificate of Origin.

[skygrango]

@YaLTeR Does this meet your needs ?

[skygrango]

Introducing the PointerConstraintsHandler trait significantly increases the scope of the changes. Perhaps we should move remove_constraint() out of PointerConstraintsHandler

[YaLTeR]

I would imagine that this code should go inside deactivate() itself to avoid putting it into every caller?

Or I guess data is inaccessible from there? Hmm, not sure how to structure this

[skygrango]

I've updated the signature of deactivate(), which I believe addresses your suggestion.

However, the downside is that PointerConstraintsHandler has now become a trait dependency of PointerTarget.

[YaLTeR]

That looks like the change that I had in mind, yeah.

It also doesn't seem like the repeated remove_constraint() call from destroyed() is a problem, because the constraint is required to be unique per surface, so the surface must destroy the previous constraint before making a new one, so it shouldn't be possible for a repeated remove_constraint() call to operate on a cursor position hint from a different constraint. Is my reasoning correct here?

[skygrango]

For Lifetime::Oneshot, explicitly calling deactivate() does result in one extra invocation of remove_constraint(). However, I don't think this has any practical impact. The compositor should ensure that only a single surface's cursor position hint is stored at any given time(per seat), and that the hint is consumed after it has been been applied. Consequently, the second invocation becomes a no-op because there is no cursor position hint remaining to apply.

[YaLTeR]

Yes, and I am thinking through whether it's possible to end up with the incorrect stored hint through overlapping pointer constraint objects, and so far I don't see any way.

[skygrango]

I moved remove_constraint() to SeatHandler, which significantly reduces the amount of trait boilerplate and maintenance.

Since remove_constraint() may be called while with_pointer_constraint() is active, the PointerConstraint is passed to the compositor so it can inspect the RegionAttributes without re-entering with_pointer_constraint().

[Drakulix]

Urgh, I am not sure I like the move here. So if you don't expose the constrains-protocol, you still have to implement this function? That makes no sense.

[Drakulix]

So the issue is this call, right? (This would be so easy, if we had specialization.)

I would honestly rather have the PointerTarget impl for WlSurface require the PointerContraintsHandler, than remove_constraint to be part of the SeatHandler, if nobody has a better idea.

[skygrango]

how do you think about

/// Callback that returns a reference to the pointer constraints handler, if implemented.
fn pointer_constraints_handler(&mut self) -> Option<&mut dyn PointerConstraintsHandler<Self>> {
    None
}

for SeatHandler

[Drakulix]

No please. The SeatHandler should not expose anything related to pointer constraints. If our WlSurface implementation of PointerTarget needs it, just add a requirement for the PointerConstraintsHandler.

[skygrango]

@Drakulix Following your suggestion, I added PointerConstraintsHandler to PointerTarget. Could you please let me know if this is good enough? I'll clean up the Git history later.

[Drakulix]

LGTM

I'll clean up the Git history later.

I can simply squash the commits on merge, if you want.