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

Use objc2 and its framework crates #148

Merged
merged 1 commit into from
Apr 27, 2024
Merged

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Apr 23, 2024

objc2 is a replacement for objc/objc_id that contains a bunch of safety improvements, including msg_send_id! which automatically upholds memory management rules (Id::from_ptr/Id::from_retained_ptr is no longer necessary).

Additionally, we use the framework crates objc2-foundation and objc2-app-kit, which provide for example the NSPasteboard type, which has the methods that arboard needs already defined, and with the correct types, ensuring that passing e.g. Id<NSArray> and thus accidentally giving away ownership over the array won't happen again.

These crates are automatically generated, ensuring that if you need some obscure API in the future, it's very likely to be there already.

Feel free to ask if there's anything about the new code that's unclear!

`objc2` is a replacement for `objc`/`objc_id` that contains a bunch of safety improvements, including `msg_send_id!` which automatically upholds memory management rules (`Id::from_ptr`/`Id::from_retained_ptr` is no longer necessary).

Additionally, we use the framework crates `objc2-foundation` and `objc2-app-kit`, which provide for example the `NSPasteboard` type, which has the methods that arboard needs already defined, and with the correct types, ensuring that passing e.g. `Id<NSArray>` and thus accidentally giving away ownership over the array won't happen again.

These crates are automatically generated, ensuring that if you need some obscure API in the future, it's very likely to be there already.
src/platform/osx.rs Show resolved Hide resolved
Comment on lines +94 to +95
unsafe impl Send for Clipboard {}
unsafe impl Sync for Clipboard {}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are... Probably not entirely true, NSPasteboard is likely only safe to use from the main thread, as that's where other code in an application would be using them from - at least, the thread-safety of NSPasteboard isn't documented anywhere.

The impls are added to keep backwards compatibility.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW we have been using NSPasteboard off the main thread (inside Tokio workers and std::thread::spawn) at 1Password for several years now and haven't seen a single crash with a clipboard-related stacktrace. This of course isn't a guarantee but at minimum it hasn't been problematic.

My intuition says this would be somewhat thread-safe because its just an IPC/XPC wrapper to talk to the pboard daemon, and XPC uses queues underneath.

src/platform/osx.rs Show resolved Hide resolved
Copy link
Collaborator

@complexspaces complexspaces left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR. Its great to see that objc2 is making progress from when I last looked at it. I have some small comments, but otherwise this looks good.

src/platform/osx.rs Show resolved Hide resolved
Copy link
Collaborator

@complexspaces complexspaces left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you again for these changes!

@complexspaces complexspaces merged commit 0bff1e0 into 1Password:master Apr 27, 2024
11 checks passed
@madsmtm madsmtm deleted the objc2 branch April 27, 2024 21:14
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.

2 participants