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

Version 2 #51

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Version 2 #51

wants to merge 8 commits into from

Conversation

whizsid
Copy link

@whizsid whizsid commented Jul 19, 2023

  • Moving to new User Notifications framework
  • Migrating to objc2
  • Async and multi thread support
  • Giving the control of RunLoop to user side.

@hoodie
Copy link
Collaborator

hoodie commented Jul 19, 2023

Oh wow, that is a lot of work here, thanks so much!
I'll take a bit to review, but I'll take some time this weekend.

@Pandawan
Copy link
Contributor

Haven’t tested it yet but awesome work!

I checked this rather quickly, but it seems like this would require bundling into a .app no?
I was looking into bundling the Info.plist as a single executable binary a while ago (#10) but maybe that’s just not worth it?

Also, as was discussed in that issue, we may want to provide support for both frameworks until the old one truly no longer works?

Regardless, excited for this!

@whizsid
Copy link
Author

whizsid commented Jul 20, 2023

@Pandawan We are only bundling the example app for testing purposes. However user can not use it without bundling. Because those bundle information using from 'requestAuthorization' method to display the prompt with our logo, and product name. That's why we are getting bundleProxy error.

I referred the #10 before starting this PR. I think the only missing part is sandbox entitlement. However now most of the rust application frameworks supporting their own bundling processes. Anyway we have to drop the command line application support. Because neither Xcode command line applications can send user notifications.

I opened this as a look-forward PR. Because the current implementation supporting all the features that can achieve using NSDictionary<String, String> data structure. But now we can close to the Mac OS sys calls by using icrate bindings. So we can support data structures as the UN framework requiring.

I also planned to support this PR for old API. But I faced some issues when implementing the Action buttons. Because new UN APIs requiring action buttons attached to categories. So we have to keep a category storage in our crate to support the old version. But it seems like out of scope. We should provide a simple and transparent API to user.

I like to wrap actual API calls behind a feature flag or release profile. Then we can let users to test other components without bundling.

@whizsid
Copy link
Author

whizsid commented Jul 20, 2023

Referencing objc2#168. We have to use mpsc channels until this completed. Then we can move to a oneshot channel for async calls. Because oneshot channel is moving the ownership of Sender in send function. Then our block will became a FnOnce. Actually those completion handlers only executed once from Objective-C functions.

@hoodie
Copy link
Collaborator

hoodie commented Jul 20, 2023

I agree that bundling seems to be inevitable, I think the main fix for the "notifications don't work on mac" issue is going to be documentation. Looks like we should have a working example of how to bundle (and maybe even sign) an app to make notifications work so that users know what to do.

@Pandawan
Copy link
Contributor

I’m a little worried that requiring bundling ends up being a headache to support/document (e.g. more gh issues about notifs not working because of bundling/entitlements), but if it’s the only way forward then it’ll do

@hoodie
Copy link
Collaborator

hoodie commented Jul 20, 2023

could you please use semantic commit messages in this pr

@whizsid
Copy link
Author

whizsid commented Jul 21, 2023

@hoodie Ok sure. I will do with next commits.

@whizsid
Copy link
Author

whizsid commented Jul 21, 2023

@Pandawan I agree. We can implement a higher level wrapper around low level bindings to support both NS and UN frameworks. It should act like a compatible layer. We can check the sign status in this wrapper and translate our Notifications to NS or UN. Let's try to discuss it after finalising the UN framework bindings. I think we should consider the UN as a first-class citizen. Because NS framework will retire soon.

@hoodie
Copy link
Collaborator

hoodie commented Jul 21, 2023

I agree with the UN vs NS treatment. Maybe we should stage this. because switching to UN by default would be a breaking change, and on some systems NS does not seem to require bundling/signing at all. Maybe we should make this with feature flags too. @whizsid do you know if signing is actually required?

@hoodie hoodie force-pushed the master branch 2 times, most recently from c8a971a to 0c1dbdf Compare July 23, 2023 11:12
@whizsid
Copy link
Author

whizsid commented Jul 23, 2023

@hoodie Signing is required for UN apis. I got notifications are not allowed for this application when I trying without signing. As I know NS do not require signing. Maybe we can use security framework to detect the signed status. https://developer.apple.com/documentation/security?language=objc

cp bundle/rust-logo.icns simple.app/Contents/Resources
codesign --force --sign $CERTIFICATE -o runtime --entitlements ./bundle/simple.app.xcent --timestamp\=none --generate-entitlement-der ./simple.app
touch simple.log
open simple.app --stdout ./simple.log --stderr ./simple.log
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've been playing around with cargo-bundle, do we want to use that here?

@hoodie
Copy link
Collaborator

hoodie commented Jul 23, 2023

I can already tell this is going to be a significant departure from the current crate, and maybe if it is necessary to make it work on macos that is a good thing. I'm currently not quite certain how we want to go about this migration. Do you see that there is a way that we can make this work as a a progressive release, like a 0.6 or so, or is this too breaking? At the moment it looks like you are writing a whole new crate and it really looks impressive, but at the same time you are removing the existing examples. I was wondering whether we maybe want to break this up into smaller steps before adding more Features. Maybe just replicate the existing functionality + documentation of what need to be done to have a signed/bundled app so that users of the existing lib can migrate more easily. What do you think @whizsid ?

@whizsid
Copy link
Author

whizsid commented Jul 24, 2023

@hoodie The only mismatch between those APIs are the categories and action buttons mapping. I have a plan to support it as compatible layers. As an example we can define two compatible layers as following.

  1. NS First Compatible Layer

This layer support both UN and NS frameworks while keeping the data structures same to NS framework. This layer provide a builder which map action buttons directly to the notifications. Same structure that we are already using.

UN:- the categories creating dynamically while sending the notifications. Category identifier should be a hashed value related to actions. Also we should store created category ids in-memory to de-duplicate.
NS:- Just converting the data to objc and sending

  1. UN First Compatible Layer

This layer support both UN and NS frameworks while keeping the data structures same to UN framework. This layer provide a builder which map action buttons to categories.

NS:- Once user created a category, we should store it in memory. Once user referred the category in a notification, we should fetch the referred category and bind actions to the NS notification.
UN:- Just create notifications and categories using the objc-functions.

Both compatible layers using the UN if signed, NS if not signed. Also we should mark the all types in ns_first and ns modules as deprecated. I have to re-organize the codebase by introducing un, ns, ns_first, un_first modules. Both ns_first and un_first modules should re-use the functionalities defined in the un and ns modules.

Also we can add four features to our crate.

[features]
default = ["ns-first"]
# Enabling the `un` module
un = []
# Enabling the `ns` module
ns =[]
# Enabling the `ns_first`,`ns`,`un` modules
ns-first=["un","ns"]
# Enabling the `un_first`,`ns`,`un` modules
un-first=["un","ns"]

I think we can release that as a minor version with this approach. Yes as the first step I will re-organize the codebase and try to implement the notification sending part using the both APIs.

@Pandawan
Copy link
Contributor

I may be missing something, but do we really need a dual-compatibility mode at all? Seems to me like which API to use is entirely up to the developer and whether or not they want to sign their bundle.

So you could just require the dev to specify whether or not they want to sign (perhaps with a feature) and then use the appropriate API based on that. If they select the UN one and don’t have a proper signing setup, then we can use that signature check API to give them an error message.

Thoughts?

@hoodie
Copy link
Collaborator

hoodie commented Jul 24, 2023

I mostly think of the transition. There are probably already a bunch of cases out there where apps are not signed an the notifications just work, because the NSNotificationCenter api does not require it. Some cases may be fine with the current approach (considering they know it's going to be EOL eventually). For notify-rust for instance it would be nice to be able to flick a feature flag and work either way: one without sigs, but not stable (for playing) and one "professional" version :D

@dscso
Copy link

dscso commented May 1, 2024

@whizsid what is necessary to get the UN notification running completely? Because if you want, I could help by adding my new NS notification implementation as fallback for the previously discussed feature un and ns.

I'd say that we make the Notification struct Framework agnostic and implement a un::send_notification and ns::send_notification and maybe even an provider::send_notification that chooses on runtime, weather the app is signed or not.

I don't know what would be the best way of dealing with interactions, me personally I prefer to expose the delegate into a callback function as I did here.

Maybe even for enforcing thread safety create a NotificationProvider that holds the NSUserNotificationCenter or UNUserNotificationCenter.

Then you could do stuff like provider.set_badge_count(1); or provider.register_callback(|e| {}) etc. It would also greatly facilitate dynamic detection weather the binary is signed or not.

here an example how the library could be used like:

let mut provider = NSNotificationProvider::new("terminal");
// or if you are rich and can afford signing
let mut provider = UNNotificationProvider::new().unwrap();


provider.set_callback(|id, response| {
    println!("notification activated {}: {:?}", id, response);
});

let image = Image::from_url("https://avatars.githubusercontent.com/u/6866008?v=4");

let id = Notification::new()
    .reply(true)
    .title("title")
    .subtitle("This notification will be deleted in ~5sec... Interact with it before that!")
    .image(image.as_ref())
    .delivery_date(std::time::SystemTime::now() - std::time::Duration::from_secs(100))
    .send()
    .expect("TODO: panic message");

println!("all notifications: {:?}", provider.get_all_notifications());
println!("deleting old notification(s)...");
provider.delete(id.as_str());

println!("all notifications: {:?}", provider.get_all_notifications());
for _ in 0..50 {
    provider.run_main_loop_once();
}

What are your thoughts about this?

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.

None yet

5 participants