-
Notifications
You must be signed in to change notification settings - Fork 4
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
Basic LocalNotifications functionality #2
base: main
Are you sure you want to change the base?
Conversation
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.
Thank you for this!
I added some comments, feel free to argue against me. And if you don't have the time to fix it it's not a problem as I can take a look later
- returns no value is instantly expected. onComplete read the variable `notificationAccepted` | ||
*/ | ||
@Callable | ||
func schedule(_ uid:String, _ title:String, _ body:String, _ seconds:Int, _ onComplete:Callable) { |
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.
I don't think we should use underscores for the parameters in @callable functions, it makes no difference for godot and if called from within swift it's better to have either proper names or just use the variable name, I generally try to replicate what the API calls them if available
- parameter body: text body of the local notification. | ||
- parameter seconds: display notification in how many seconds time. | ||
- returns no value is instantly expected. onComplete read the variable `notificationAccepted` | ||
*/ |
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.
Minor but it's always better to use // or /// comments. Because if you are searching through a project you immediately see that a line starts with // whereas that is not the case with /** */ as that comment can start and end anywhere around the line you search for
Most editors have a shortcut to comment/uncomment blocks of code, in VSCodium it's cmd/ctrl + k + c and cmd/ctrl + k + u to comment and uncomment respectively
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.
I was just copying the style of a page explaining how to create swift documentation style comments. I found another page from apple on this, so I'll update to to follow https://www.swift.org/documentation/docc/writing-symbol-documentation-in-your-source-files
notificationAccepted = true | ||
onComplete.callDeferred() | ||
} catch { | ||
GD.print("Error scheduling: \(error.localizedDescription)") |
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.
I would probably use GD.pushError() to handle errors, unless the error is sometimes expected, in which case I would use GD.pushWarning() instead. Even though it doesn't matter as much if you can't test it in the editor
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.
I was using GD.print() to cause information to display in the Xcode console, to allow debugging/testing inside Xcode. I'll remove most of the print statements, except for true errors that we would would want to see in the Xcode console.
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.
GD.pushError also show up in the Xcode console with a big "USER ERROR" in front, I'm not sure about GD.pushWarning but I assume it's similar so I would keep those around for when needed
} catch { | ||
GD.print("Error scheduling: \(error.localizedDescription)") | ||
notificationAccepted = false | ||
onComplete.callDeferred() |
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.
Using callDeferred is nice and simplifies things on the Godot side, but it might be better to return an error value if something fails so the user can react to the problem. The standard Godot way is to return 0 as an ok and >0 for errors.
You should be able to use onComplete.bind() / bindv() to add parameters even though callDeferred doesn't take any.
I fell back to using callv() because I didn't get the .bind() functions to work, it might be a bug in SwiftGodot or I did something wrong, will investigate on my end as using callDeferred is probably preferred
account daylight savings. | ||
*/ | ||
@Callable | ||
func scheduleTomorrow(_ uid:String, _ title:String, _ body:String, _ hour:Int, _ onComplete:Callable) { |
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.
Could this be made more general to schedule X days into the future instead of always just tomorrow?
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.
oops, i didn’t mean to submit this. that was meant to just be a helper for me. but maybe it’s a good idea to keep it and generalize it.
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.
Since it looks like the scheduling takes seconds in order to schedule, it would be nice to remove the use of the Date class so the extension doesn't depend on Foundation anymore
All good comments, i’ll have a look at this tomorrow. My main reason for wanting to use call deferred and force fetching of results (instead of returning a value directly), is to avoid “Mysterious thread issues.” In the past I had some cases of occasional iOS crashes in my apps that only occurred while running on the phone. It was super difficult to debug because it only occurred on phone, so i never got to the bottom of it. Using deferred solved this and i don’t know why. (this was using godot rust though, not godot swift) This call deferred solution forces the main tread to read a value from the already completed node, whereas a normal call results in your thread trying to call a node that could be being used by the main thread. |
Yeah, ideally we should use callDeferred() with .bind() or bindv() which is supposed to work but currently doesn't, so I'm using an intermediate file in GDScript that takes the information returned from the swift classes, convert it into typed GDScript and then goes through call_deferred in order to bring it to the main thread. |
Good news about callDeferred, we can now use it with parameters migueldeicaza/SwiftGodot#491 he even gave us a variable amount of arguments directly on the call so One gotcha that remains though is that to receive the callback you always need to have the same amount of arguments, so you might have to send Currently in the process of cleaning up the callbacks in the other scripts |
This code allows creating on device local notifications (Notifications that do not need a remote server of any type). I have completed basic testing and it works for my personal use case. Callbacks are made using
call_deferred
which I suspect might help avoid multi threading issues, but I am not sure.Please feel free to merge it into your main branch. This code is released to the public domain on the condition that people who use or share the code maintain a license that makes it clear this code is not guaranteed/warranted to work. (MIT or comparable)