Skip to content
This repository has been archived by the owner on Dec 30, 2020. It is now read-only.

macOS support #40

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

macOS support #40

wants to merge 15 commits into from

Conversation

alexandrvicente
Copy link

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.

Copy link
Contributor

@daniel-abramov daniel-abramov left a comment

Choose a reason for hiding this comment

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

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 = "*"
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably want to have some more strict version requirement for production usage.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in 6db1648

use objc_id::Id;
use crate::{Application, BoxedError, Callback, Error};

/// The generation representation of the Mac OS X application.
Copy link
Contributor

Choose a reason for hiding this comment

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

A typo in a comment.

Copy link
Author

@alexandrvicente alexandrvicente Apr 2, 2020

Choose a reason for hiding this comment

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

Fixed in 2d98b78

}

unsafe impl Send for Window {}
Copy link
Contributor

@daniel-abramov daniel-abramov Mar 30, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Removed in 3a00d0d

@alexandrvicente
Copy link
Author

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.

Thanks for mentioning those issues, I updated the PR fixing those and it passed all checks now.

@qdot
Copy link
Owner

qdot commented Apr 3, 2020

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.

Copy link
Owner

@qdot qdot left a comment

Choose a reason for hiding this comment

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

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> {
Copy link
Owner

Choose a reason for hiding this comment

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

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).

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in a5cc6bc

@ancwrd1
Copy link

ancwrd1 commented Sep 29, 2020

Hi, are there remaining blockers before this could be merged?

@qdot
Copy link
Owner

qdot commented Oct 4, 2020

The pub on wait_for_message was fixed, but I believe this still panics on windows. So that's a blocker.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants