-
Notifications
You must be signed in to change notification settings - Fork 33
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
Rewrite to use objc2
#172
Rewrite to use objc2
#172
Conversation
.map(|menu| menu.retain()); | ||
ns_status_item.setMenu(menu.as_deref()); | ||
if let Some(menu) = &menu { | ||
let () = msg_send![menu, setDelegate: &**ns_status_item]; |
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'm not sure what the original code intended here, but this is wrong, NSStatusItem
doesn't implement NSMenuDelegate
(at least not publicly).
I have kept the implementation as-is, but this should probably be looked into after this 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.
Hmm, it was set to tray_target
. Maybe @lucasfernog has some ideas?
This ensures that memory management rules are upheld, as well as greatly improving type-safety. API-wise, this adds a new error case `NotMainThread`, which is triggered when a TrayIcon is created on a thread that is not the main thread.
The "covector" CI step is failing, it seems to be lacking certain permissions? |
yeah that workflow can be ignored, it doesn't like forks (we are working on it) |
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.
Looks good! It's a great example to follow. Just a question for you from my side.
Btw, I'm also doing the migration on wry. It's kind of chaos now, and the functionality is also broken 🤣
I saw, it's nice! When you're ready, I'd be happy to help out, or review a PR, or something! |
LGTM! |
Ah yeah, have done that now (not sure it's correct though). (Would've been nice to note in the |
This ensures that memory management rules are upheld, as well as greatly improving type-safety.
API-wise, this adds a new error case
NotMainThread
, which is triggered when a TrayIcon is created on a thread that is not the main thread.There is probably a bug lurking in here somewhere, I'm not too familiar with the exact details of the
NSStatusItem
API.Notably, there's also still a lot of
if Some(...)
checks that I'm not sure are completely necessary, though this PR shouldn't make the situation any worse on that front.Related: The migration to
objc2
inwry
: tauri-apps/wry#1239