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-foundation and objc2-app-kit #37

Merged
merged 1 commit into from
Oct 3, 2024
Merged

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented May 15, 2024

The objc2 project is the successor to objc, and contains automatically generated bindings to the Foundation and AppKit frameworks (and many other frameworks), while ensuring that memory management rules are upheld.

This allows dark-light to reduce the amount of complex unsafe code, as well as making it easier for you to in the future change subscribe to listen for theme change notifications instead of the busy loop.

@jacksongoode
Copy link

making it easier for you to in the future change subscribe to listen for theme change notifications instead of the busy loop

How would this upgrade enable that specifically?

@madsmtm
Copy link
Contributor Author

madsmtm commented May 21, 2024

making it easier for you to in the future change subscribe to listen for theme change notifications instead of the busy loop

How would this upgrade enable that specifically?

There's two ways to listen for changes to the appearance that I know of:

  • Subscribing to the "AppleInterfaceThemeChangedNotification" notification using [NSDistributedNotificationCenter] (undocumented) example.
  • Using Key-Value Observing on -[NSApplication effectiveAppearance], as documented (in Swift) here.

objc2-foundation/objc2-app-kit provides you the tools to use either of these, by using the generated interfaces to NSDistributedNotificationCenter or to key-value observing categories.

Using these are possible without objc2, but it's a lot harder, as you also have to consider a bunch of memory management, and ensuring you get the method signatures correct.

@casperstorm
Copy link

This PR also fixes a memory leak in cargo run --example notify.
To test, run cargo run --example notify on master on macos and notice how the ram will stack up until it crashes.
No ram is leaking on this branch.

@Be-ing
Copy link
Collaborator

Be-ing commented Oct 3, 2024

I don't know enough about Objective C to review the code, but since two users report this fixes a real world issue in squidowl/halloy#598 and nobody has stepped up to review this in 6 months, I'll go ahead and merge this. Thanks for the fix.

@Be-ing Be-ing merged commit ada49f0 into frewsxcv:main Oct 3, 2024
4 checks passed
@madsmtm madsmtm deleted the objc2 branch October 3, 2024 11:55
@Be-ing
Copy link
Collaborator

Be-ing commented Oct 8, 2024

Just to be explicitly clear, can someone confirm the notify example is now working on the main branch on macOS? I want to be sure of that before the next release and I don't have a Mac to test.

@Be-ing
Copy link
Collaborator

Be-ing commented Oct 8, 2024

@madsmtm

in the future change subscribe to listen for theme change notifications instead of the busy loop.

A PR for this would be welcome!

@madsmtm
Copy link
Contributor Author

madsmtm commented Oct 8, 2024

Just to be explicitly clear, can someone confirm the notify example is now working on the main branch on macOS? I want to be sure of that before the next release and I don't have a Mac to test.

I doubt it is, this PR doesn't actually fix that.

A PR for this would be welcome!

I've written down some notes in #47, but I suspect the problem to be more fundamentally that Rust's async and Objective-C interop just do not play well together, so I doubt this is something that rust-dark-light even can fix at the moment.

@rickykresslein
Copy link

Just to be explicitly clear, can someone confirm the notify example is now working on the main branch on macOS? I want to be sure of that before the next release and I don't have a Mac to test.

I tested this on macOS and it still does not work. Also, as mentioned by @madsmtm it is extremely inefficient.

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.

5 participants