macOS: add dynamic accepts_first_mouse callback#4488
Conversation
9e4e2f6 to
91b3567
Compare
madsmtm
left a comment
There was a problem hiding this comment.
Thanks for the PR, and sorry for being slow to get back to it, the continuos rebases helped ;)
| }) | ||
| .flatten() | ||
| }) | ||
| .unwrap_or(self.ivars().accepts_first_mouse) |
There was a problem hiding this comment.
When is acceptsFirstMouse: called by AppKit? If it's only done when responding to events from the OS, and not in e.g. methods like Window::set_title() or whatever, maybe it'd make sense to warn if we had a fn accepts_first_mouse, but couldn't call it?
There was a problem hiding this comment.
`acceptsFirstMouse:` is only called by AppKit in response to mouse-down events on an inactive window — not from programmatic calls like `set_title()`. Added a warning in 7ea93ef for the case where the handler can't be called synchronously (re-entrancy); it logs before falling back to the static value.
| let handler = EventHandler::new(); | ||
| handler.set(Box::new(DummyApp), || { | ||
| handler.handle(|_app| { | ||
| // Re-entrant handle must still panic after the refactoring. |
There was a problem hiding this comment.
Nit: Makes no sense to talk about "after the refactoring" in a code comment
| /// here so that the application can make per-click decisions, e.g. accept first mouse for | ||
| /// low-risk actions (selection, scrolling) but reject it for buttons or destructive actions. | ||
| /// | ||
| /// The default implementation returns `true`. |
There was a problem hiding this comment.
Hmm, but that'd mean that by returning Some(self) in macos_handler, you overwrite WindowAttributesMacOS.accepts_first_mouse?
There was a problem hiding this comment.
Good point — I've clarified the docs in 6cf8480 to make the precedence explicit: when macos_handler() returns Some, the dynamic callback takes precedence over the static WindowAttributesMacOS::with_accepts_first_mouse setting. The static value is only used as a fallback during re-entrancy.
I've also added guidance in 2cb9dce on when to use one vs the other: use the static setting when a per-window value is sufficient, use the dynamic callback when you need to decide based on click position.
There was a problem hiding this comment.
IMHO this is confusing to have both. I'm wondering if it would not be better here to remove WindowAttributesMacOS::with_accepts_first_mouse entirely.
| /// | ||
| /// [`acceptsFirstMouse:`]: https://developer.apple.com/documentation/appkit/nsview/acceptsfirstmouse(_:) | ||
| #[doc(alias = "acceptsFirstMouse:")] | ||
| fn accepts_first_mouse( |
There was a problem hiding this comment.
Could you add usage to the app example?
Or maybe just document when, as a user, you'd want to implement this vs. WindowAttributesMacOS.accepts_first_mouse.
There was a problem hiding this comment.
Done in 2cb9dce — added accepts_first_mouse to the application example and documented when you'd use the static setting vs the dynamic callback.
| /// It is important that we keep the `RefMut` borrowed during the callback, so that `in_use` | ||
| /// can properly detect that the handler is still in use. If the handler unwinds, the `RefMut` | ||
| /// will ensure that the handler is no longer borrowed. | ||
| pub fn handle_with_result<R>( |
There was a problem hiding this comment.
I think it's the right approach to the problem, but boy do I not like the fallback behavior when we're re-entrant, I wish we could do something different - but I don't see a good solution, AppKit is hella re-entrant and that's hard to avoid. Spitballing, maybe making ApplicationHandlerExtMacOS take &self, and do fn macos_handler(&self) -> Option<Box<dyn macos::ApplicationHandlerExtMacOS>> instead? That would push the re-entrancy to the user.
Anyhow, not something we need to fix in this PR, such changes can be made later when we better understand the requirements.
madsmtm
left a comment
There was a problem hiding this comment.
Actually, thinking about it a bit more, maybe the better approach would be to allow querying whether an event was the first mouse event?
E.g. something like:
match event {
WindowEvent::PointerButton {
button: ButtonSource::Mouse(MouseButton::Left),
is_macos_first_after_focus_bikeshed, // bool
position,
..
} => {
if is_macos_first_after_focus_bikeshed && self.thing_at_pos_is_button(position) {
return; // Don't try to click buttons for first
}
// Select item / click button / whatever you'd normally do
}
_ => ...,
}Whether we can actually implement that is a different story, but I think it'd be more consistent with how Winit otherwise works.
82da762 to
9b0b42b
Compare
|
Thanks for the review! I've addressed the inline comments — see replies above. Regarding the alternative approach (a
That said, if this is something you'd prefer to explore further or if it's a blocker for the PR, happy to discuss. The event-based approach could also work as a future addition alongside the callback — they aren't mutually exclusive. |
|
Sorry to nag, but is there anything more I can supply to move this forward? Happy to rework this if you have other ideas for how to fix it or if you prefer the event approach described above. |
ogoffart
left a comment
There was a problem hiding this comment.
I made a review. But please note that i am not familiar with macOS at all and i don't know really how this is supposed to work.
I have the feeling is_macos_first_after_focus might still be nicer.
Regarding your arguments:
Native mapping
On the other hand winit is an abstraction and doesn't need to map exactly to each platform API (otherwise we'd use the platform API directly)
Reliable detection is tricky
Suppression semantics
That's the question whether it is possible to implement or not.
Regardless, how do you think people will use this? For example, in Slint we'd also probably map this to a is_first_after_forcus or something like that. Or do you have other idea in mind?
| /// here so that the application can make per-click decisions, e.g. accept first mouse for | ||
| /// low-risk actions (selection, scrolling) but reject it for buttons or destructive actions. | ||
| /// | ||
| /// The default implementation returns `true`. |
There was a problem hiding this comment.
IMHO this is confusing to have both. I'm wondering if it would not be better here to remove WindowAttributesMacOS::with_accepts_first_mouse entirely.
|
|
||
| /// Called when the user clicks on an inactive window to determine whether the click should | ||
| /// also be processed as a normal mouse event. | ||
| /// |
There was a problem hiding this comment.
Is it the equivalent of a press, or a release event?
When returning true, do we get normal press end release event as well?
955851a to
d073235
Compare
|
Reworked this as the event-flag approach you both suggested. Force-pushed a single squashed commit ( What changed:
@ogoffart, to answer your specific questions:
Both — the activating left press and its matching release are tagged with
Done. Verified locally: @madsmtm, this also obviates the re-entrancy fallback you flagged — there's nothing to fall back to anymore. |
d073235 to
be05dc0
Compare
Apps need per-click decisions to follow the macOS convention of accepting first mouse for low-risk actions (selection, scrolling) but rejecting it for buttons and destructive actions. Motivated by slint-ui/slint#10451. Always return `true` from `acceptsFirstMouse:`, and tag the resulting `PointerButton` events with `is_macos_activation_click: true` (on both the activating left press and its matching release) so the app can short-circuit the whole gesture with a single check: WindowEvent::PointerButton { is_macos_activation_click: true, .. } => return, Replaces an earlier callback-based design (`accepts_first_mouse` on `ApplicationHandlerExtMacOS`) that mapped more closely to AppKit but didn't fit winit's event-driven model and required re-entrancy handling. Also removes `WindowAttributesMacOS::with_accepts_first_mouse`, which is now redundant — apps that want to reject activation clicks check the flag on the per-event instead.
be05dc0 to
d0ddda0
Compare
Summary
ApplicationHandlerExtMacOS::accepts_first_mousecallback that receives the window ID and click position (PhysicalPosition<f64>), enabling per-click decisions about whether to accept first mouse on inactive windows.EventHandler::handle_with_resultfor synchronous, re-entrant-safe handler dispatch that returnsOption<R>. Refactor existinghandleon top of it to eliminate duplication.accepts_first_mousevalue fromWindowAttributesis used as a fallback.Motivated by slint-ui/slint#10451 — applications need per-click decisions to follow the macOS convention of accepting first mouse for low-risk actions (selection, scrolling) but rejecting it for buttons and destructive actions.
Implementation notes
acceptsFirstMouse:must returnboolsynchronously, so the handler is called viahandle_with_resultwhich returnsNoneon re-entrancy instead of panicking.NSEventparameter isOption<&NSEvent>per Apple's API contract (can be nil); nil falls back to the static default.convertPoint_fromView+ scale factor), withisFlippedproviding top-left origin.Test plan
cargo build -p winit-core/winit-appkit/winit— compilescargo test -p winit-core— passescargo test -p winit-common --features event-handler— 5 new tests pass:handle_with_resultreturns value (normal path)handle_with_resultreturnsNonewhen handler not sethandle_with_resultreturnsNoneon re-entrancy viahandlehandle_with_resultreturnsNoneon re-entrancy via itselfhandlestill panics on re-entrancy (regression test)cargo clippy -p winit-appkit— no warningscargo +nightly fmt -- --check— clean