Conversation
Compiles, but example segfaults due to an NSInvalidArgumentException, haven't had time to figure this out yet.
daniel-abramov
left a comment
daniel-abramov
left a comment
There was a problem hiding this comment.
I did not check it in details (and did not test it as well), but apart from the things I've mentioned in comments it looks ok. Fixing the CI and/or PR to pass all checks would also be nice.
Cargo.toml
Outdated
| objc-foundation = "*" | ||
| objc_id = "*" | ||
| cocoa = "*" | ||
| core-foundation = "*" |
There was a problem hiding this comment.
We probably want to have some more strict version requirement for production usage.
src/api/cocoa/mod.rs
Outdated
| use objc_id::Id; | ||
| use crate::{Application, BoxedError, Callback, Error}; | ||
|
|
||
| /// The generation representation of the Mac OS X application. |
There was a problem hiding this comment.
A typo in a comment.
src/api/cocoa/mod.rs
Outdated
| } | ||
| } | ||
|
|
||
| unsafe impl Send for Window {} |
There was a problem hiding this comment.
This does not seem right. Window contains an Rc which is clearly !Send, so just declaring Window to be Send is not sound.
It seems like making Window implement Send is not the best idea, not only because it contains types which are not Send, but also due to the fact that on OS X most of the things you can do with a window must be done from the main thread. The only way to really make it Send is to use GCD inside each function the user may call on a Window after moving it into a different thread, i.e. make sure that that all actions we apply to Window are executed in a main thread.
Thanks for mentioning those issues, I updated the PR fixing those and it passed all checks now. |
|
Awesome PR! I'm a little busy this week, but I'm hoping to take a look at this and possibly get it in and roll another package version this weekend. |
qdot
left a comment
qdot
left a comment
There was a problem hiding this comment.
Needs visibility fix to compile.
With the visibility specifier fix, this will compile on all platforms.
- On linux (debian, exwm), trayicon example runs ok, exits ok.
- On macos, trayicon example runs ok, errors on exit with "illegal hardware instruction"
- On windows, trayicon example panics on start, "process didn't exit successfully"
If you can fix up the visibility, I'll try to get stacks for the mac and windows errors. RUST_BACKTRACE isn't working for some reason.
src/lib.rs
Outdated
|
|
||
| pub fn wait_for_message(&mut self) -> Result<(), Error> { | ||
| #[cfg(not(target_os = "macos"))] | ||
| fn wait_for_message(&mut self) -> Result<(), Error> { |
There was a problem hiding this comment.
You missed the visibility specifier here, meaning the example won't build on linux or windows. Just need to add pub (and I really need to get CI going for all platforms).
|
Hi, are there remaining blockers before this could be merged? |
|
The pub on wait_for_message was fixed, but I believe this still panics on windows. So that's a blocker. |
4 participants
Includes @application-developer-DA's patches and support for missing features, such as callbacks, separators and icons from files. Partially derived from https://github.com/rust-sysbar/rust-sysbar.