-
Notifications
You must be signed in to change notification settings - Fork 568
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix macOS mouse events with multiple windows and active states. #907
Conversation
7953a17
to
4205339
Compare
I have some preliminary thoughts & questions:
for the second example you mention, this does seem like a bug; the issue as far as I can tell is that the second window isn't receiving the mouse up event, and it should be? I presume we're just dropping this somewhere? |
The second example bug on As for non-active windows drawing hot state, I did a short survey of apps on macOS Catalina.
Based on my testing it seems that although hover effects aren't used nearly as often in macOS apps as say Windows apps, they are still widely used in most apps, even if sparingly. Also the majority of them had hover effects even when the window is not active. Thus to have a macOS app that does not have hot effects when hovering would put it into the minority. I'm not sure how 3rd party apps on macOS tend to behave, but the apps I tested came with macOS Catalina and are promoted in the bottom toolbar so I assume Apple considers them well enough designed. I think the case is rather clear that hot state for non-active windows should be there. Now what I will say is that during my survey I also noticed that several apps change their design based on whether the window is active or not. To do that the app needs to know whether the window is active or not. Thus I think for druid to be powerful enough to mimic the behavior of say the Notes app on macOS, it needs to have both non-active mouse events that this PR here provides, but also an additional capability added via a different PR that allows the app to know whether the window is currently active - or I guess focus in druid's parlance. |
Okay, I think that's a good argument for keeping these concepts separated. Thanks for the survey; I'll do a review focused on the implementation when I have a few window. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some observations, happy to get this in; sorry for the wait!
} | ||
} | ||
|
||
pub trait NSTrackingArea: Sized { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would consider trying to upstream any of this if we want it. I've also personally found that these abstractions around cocoa stuff really aren't super helpful, and generally I lean towards just using msg_send!
directly everywhere. (this is fine, though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the appkit.rs
stuff I would like to get upstreamed. I've done very minimal macOS Rust programming to have an opinion on what's helpful or not, but the stuff I wrote here mimics the servo appkit crate so at least it's consistent.
owner: id, | ||
userInfo: id, | ||
) -> id { | ||
msg_send![self, initWithRect:rect options:options owner:owner userInfo:userInfo] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's possible we want an autorelease
call on this, would have to run the memory profiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The autorelease
stuff I'm least familiar with because the Apple documentation doesn't directly translate to the Rust world. Anyway for this case I did add an autorelease
call in window.rs
after calling this method. In my testing adding that autorelease
at least doesn't seem to cause any issues.
This PR replaces
acceptsMouseMovedEvents
withNSTrackingArea
. This fixes a bunch of issues with not receiving mouse events in a multiple window situation.The behavior is perhaps best testable with the
multiwin
example with the hot glow option turned on via the context menu. For example onmaster
if you overlap multiple druid windows partially and then move the mouse not-too-fast over both of them, the window that's not active might get the hot status and remain stuck with it.For another example:
Cmd
+`
to the bottom window so that it becomes the top window and is below the mouse cursor.On
master
the bottom window gets stuck in a hot state, while there's a custom fix for it in this PR.