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

fix: Bump objc2 to 0.5.0; bring icrate 0.1.0 #323

Merged
merged 10 commits into from
Jan 3, 2024
Merged

fix: Bump objc2 to 0.5.0; bring icrate 0.1.0 #323

merged 10 commits into from
Jan 3, 2024

Conversation

DataTriny
Copy link
Member

@DataTriny DataTriny commented Dec 21, 2023

Replaces #200

The compile time of accesskit_macos increases a bit due to icrate. This shouldn't be a big issue for Rust users who will probably have it in their dependency chain already. Bindings generation will be slightly slower, but I don't think this should be a concern.

platforms/macos/src/node.rs Outdated Show resolved Hide resolved
platforms/macos/src/node.rs Outdated Show resolved Hide resolved
@DataTriny
Copy link
Member Author

icrate 0.1.0 has been released, although the path towards reducing the number of features isn't clear at the moment. I currently don't have access to my Mac, I'll come back to this next year anyway.

@DataTriny DataTriny changed the title fix: Bump objc2 to 0.5.0, bring icrate 0.1.0 fix: Bump objc2 to 0.5.0; bring icrate 0.1.0 Jan 2, 2024
@DataTriny
Copy link
Member Author

DataTriny commented Jan 2, 2024

I think we use some methods that might not need to be marked unsafe:

  • NSView::convertRect_toView,
  • NSView::isFlipped,
  • NsWindow::convertPointFromScreen,
  • NSWindow::convertRectToScreen

Otherwise this PR is ready for review.

@DataTriny DataTriny marked this pull request as ready for review January 2, 2024 21:08
@mwcampbell
Copy link
Contributor

I see several places where we use Id::cast1 in an unsafe block to cast from a specific type to an NSObject or AnyObject. I wonder if there's a safer way to do this. I'd also like clarification from @madsmtm on when to use NSObject versus AnyObject.

@DataTriny
Copy link
Member Author

The safe alternative to Id::cast in this context is Id::into_super, but then we have to call it multiple times to go up the inheritance chain.

I think the only place where we have both an NSObject and an AnyObject is in PlatformNode::parent. Since we get an AnyObject from NSView::accessibilityParent, and since I don't think it is possible to transform it into an NSObject, I don't think we have much choice here.

@mwcampbell
Copy link
Contributor

I'd rather call Id::into_super multiple times to reduce use of unsafe.

@madsmtm
Copy link
Contributor

madsmtm commented Jan 3, 2024

I think we use some methods that might not need to be marked unsafe:

I think I'm considering a way to automatically mark simple things like this as safe, but it's a difficult trade-off between usability and soundness.

I've opened madsmtm/objc2#549 for the specific ones you listed, at least.

Id::into_super, but then we have to call it multiple times to go up the inheritance chain.

Tracking the more generic way in madsmtm/objc2#518.

I agree with calling into_super multiple times for now, even though it's quite verbose.

when to use NSObject versus AnyObject.

NSObject is the root class of almost all classes, but in theory a user could use a subclass of NSProxy, or they could define their own root class. So in those cases, where we don't know anything about the object, we use AnyObject.

If you can use NSObject, then you should. But if a protocol you're implementing takes &AnyObject, or something returns Id<AnyObject>, then you must use that (and you'll have to live with the limited functionality).

@mwcampbell
Copy link
Contributor

@DataTriny I'd like to replace Id::cast with Id::into_super (making multiple calls as necessary). This PR also needs to be brought up to date with the role description support we just merged. Once those fixes are done, I'm ready to merge this, and we can release everything we have pending, including the Python bindings.

@DataTriny
Copy link
Member Author

Yup! Will do this tomorrow though.

@mwcampbell
Copy link
Contributor

@DataTriny Would you mind if I do these fixes and then merge this PR? That's one advantage of us both working in the upstream repo.

@DataTriny
Copy link
Member Author

Of course, go ahead!

@mwcampbell mwcampbell merged commit 23b3f2f into main Jan 3, 2024
5 checks passed
@mwcampbell mwcampbell deleted the icrate branch January 3, 2024 23:15
@mwcampbell mwcampbell mentioned this pull request Jan 3, 2024
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.

3 participants