-
Notifications
You must be signed in to change notification settings - Fork 54
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
feat(platforms/macos): Basic macOS platform adapter #158
Conversation
…buttons in the winit simple example.
…nd eliminate the global platform node table
…context is dropped
…bclassing implementation. This avoids reference cycles and also possible crashes if the adapter is dropped on a non-main thread.
Forgot to re-open this as a draft. I've asked @madsmtm to review my usage of
|
I'll implement text editing support in a separate PR. |
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.
Overall, looks really good!
I feel I need to include the obligatory warning: objc2
is still unstable, so there are going to be breaking changes more often in this period. (That said, I will be available for helping you with version upgrades).
Especially the story around main thread safety is quite far from finalized - and that is reflected in your code as well, you have a lot of unsafe impl Send + Sync
that may not actually be sound.
I suspect a MainThreadSafe<T>(T)
adapter could be useful to you, might want to experiment with that?
Also, may I recommend at some point adding #![deny(unsafe_op_in_unsafe_fn)]
? It makes it much easier to narrow down which parts are actually unsafe
within an unsafe
function.
Feel free to ask me to clarify anything!
@madsmtm Thanks for a thorough review. I'll address the rest of your feedback later today. |
I took a look at the non-MacOS specific changes and they look fine to me. |
Co-authored-by: Mads Marquart <mads@marquart.dk>
…gration so far (egui via eframe) doesn't actually require this.
…t least not yet).
…t makes sense for this platform.
…minate a use of Option
@madsmtm I believe I have now addressed all of your feedback. Thanks again. |
Yeah, I think it looks good now, but you know, always difficult to say for sure!
My pleasure, do say so if you need my help with this stuff again |
If anyone wants to try this adapter with a real GUI toolkit, try this temporary egui branch. As a reminder, the big missing feature at this point is feedback during text editing. |
I want to merge this quickly so I can publish it to crates.io and use it in some of my other current work. Since @madsmtm reviewed the safety-critical code in this PR and I addressed his feedback, and @DataTriny reviewed the non-macOS-specific changes, I feel comfortable merging now. |
The really ugly part of this is the dynamic subclassing (in platforms/macos/src/subclass.rs). But since winit doesn't currently provide a direct way to use a subclass of WinitView, and I have to implement some accessibility methods on the view (starting with accessibilityChildren, but there will be others), I have to do this. And I've got other projects in the works that will require this hack, retrofitting accessibility onto Unity and a GLFW-based GUI. I'm already doing something similar on Windows, though Win32 subclassing is better documented and (I think) more widely used. I'm open to a cleaner solution in winit, but in order to show results immediately, I'm going with this hack for now.