-
Notifications
You must be signed in to change notification settings - Fork 712
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
Implement the new installation and configuration mechanism #499
Conversation
* | ||
* **Default**: NULL | ||
*/ | ||
@property (nonatomic, copy, nullable) void (^crashNotifyCallback)(const struct KSCrashReportWriter* writer); |
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.
This is the "ugly" API for Swift. KSCrashReportWriter
is a struct that deeply integrated in KSCrash and used in callback, that makes it hard to proxy.
Visible to Swift:
open var crashNotifyCallback: ((UnsafePointer<KSCrashReportWriter>) -> Void)?
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.
Wrapping this in an ObjC class would be a really good thing to have. Swift API matters.
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.
From docs
* WARNING: Only call async-safe functions from this function! DO NOT call
* Objective-C methods!!!
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.
Right. Well, it's fine then.
I'm thinking that a wrapper with all methods being __attribute__((objc_direct))
might do the trick, but it's still ObjC.
* | ||
* **Default**: NULL | ||
*/ | ||
@property (nonatomic, copy, nullable) void (^crashNotifyCallback)(const struct KSCrashReportWriter* writer); |
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.
Wrapping this in an ObjC class would be a really good thing to have. Swift API matters.
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.
Thanks for addressing the comments. This new way of initialization makes perfect sense here. And as mentioned in another PR, let's update README later to have actual usage instructions.
As suggested by @naftaly in #491 (comment)