Skip to content
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

Merged
merged 47 commits into from Nov 23, 2022
Merged

Conversation

mwcampbell
Copy link
Contributor

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.

…bclassing implementation. This avoids reference cycles and also possible crashes if the adapter is dropped on a non-main thread.
@mwcampbell mwcampbell marked this pull request as draft November 19, 2022 15:43
@mwcampbell
Copy link
Contributor Author

Forgot to re-open this as a draft. I've asked @madsmtm to review my usage of objc2, but I'm not quite ready to merge the current state of the platform adapter. Things I want to do before I call this PR complete:

  • Support lazy tree initialization as I do in the Windows adapter
  • Implement hit testing
  • Support the increment and decrement actions for sliders and steppers

@mwcampbell mwcampbell marked this pull request as ready for review November 20, 2022 21:10
@mwcampbell
Copy link
Contributor Author

I'll implement text editing support in a separate PR.

Copy link
Contributor

@madsmtm madsmtm left a 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!

platforms/macos/src/appkit/mod.rs Show resolved Hide resolved
platforms/macos/src/context.rs Outdated Show resolved Hide resolved
platforms/macos/src/node.rs Outdated Show resolved Hide resolved
platforms/macos/src/node.rs Show resolved Hide resolved
platforms/macos/src/context.rs Outdated Show resolved Hide resolved
platforms/macos/src/subclass.rs Outdated Show resolved Hide resolved
platforms/macos/src/subclass.rs Show resolved Hide resolved
platforms/macos/src/subclass.rs Outdated Show resolved Hide resolved
platforms/macos/src/subclass.rs Outdated Show resolved Hide resolved
platforms/macos/src/subclass.rs Outdated Show resolved Hide resolved
@mwcampbell
Copy link
Contributor Author

@madsmtm Thanks for a thorough review. I'll address the rest of your feedback later today.

@DataTriny
Copy link
Member

I took a look at the non-MacOS specific changes and they look fine to me.

platforms/macos/src/subclass.rs Outdated Show resolved Hide resolved
platforms/macos/src/subclass.rs Outdated Show resolved Hide resolved
@mwcampbell
Copy link
Contributor Author

@madsmtm I believe I have now addressed all of your feedback. Thanks again.

@madsmtm
Copy link
Contributor

madsmtm commented Nov 23, 2022

I believe I have now addressed all of your feedback

Yeah, I think it looks good now, but you know, always difficult to say for sure!

Thanks again

My pleasure, do say so if you need my help with this stuff again

@mwcampbell
Copy link
Contributor Author

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.

@mwcampbell
Copy link
Contributor Author

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.

@mwcampbell mwcampbell merged commit a06725e into main Nov 23, 2022
@mwcampbell mwcampbell deleted the macos-basics branch November 23, 2022 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants