Platform support, Application property wrapper and notifications support #98
Conversation
Supereg
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #98 +/- ##
==========================================
- Coverage 91.34% 84.78% -6.55%
==========================================
Files 34 40 +6
Lines 704 959 +255
==========================================
+ Hits 643 813 +170
- Misses 61 146 +85
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Thank you for all the improvements and additions @Supereg; I added some comments here and there but it all looks great and can be merged after all of them are resolved 🚀
I would suggest to tag a minor release after one other small documentation PR that I will hopefully merge later tonight or tomorrow 🚀
Sources/Spezi/Capabilities/Notifications/NotificationHandler.swift
Outdated
Show resolved
Hide resolved
Sources/Spezi/Spezi.docc/Module/Interactions with Application.md
Outdated
Show resolved
Hide resolved
|
Thanks for the review @PSchmiedmayer. Will try to add some minimal unit testing where possible and sensible to validate functionality. Should make the codecov checks go green ✅ |
|
Thank you @Supereg; sounds like a great plan! |
|
@PSchmiedmayer Added some tests to cover the new notification registration actions and test most of the new delegate functionality. We can't test the UserNotification delegate functionality as those types cannot be constructed by us and the NotificationCenter itself is not available in the testing environment. |
|
Sounds great; I merged the PR @Supereg 👍 |
# Fix registerRemoteNotifications action would never return ## ♻️ Current situation & Problem #98 introduced the `registerRemoteNotifications` action. However, the knowledge source that stores the continuation was never persisted in the store. Therefore, the continuation was never resumed and allocated resources forever. This was fixed with this PR. We originally discussed supporting cancellation of the registration processed, however I decided against as it wasn't clear to properly handle cancellation (e.g., automatically call the unregister action? How to handle the delegate call that might arrive later. Especially when another call to the action might incur in the mean time). This PR changes the behavior when concurrently calling the registerRemoteNotifications action. Now, the second caller will wait for the first call to complete instead of throwing a concurrence access error. This helps to share this resources with, e.g., multiple modules trying to retrieve an device token. **To emphasize: `registerRemoteNotifications` works on simulator devices!** ## ⚙️ Release Notes * Fixed an issue where the `registerRemoteNotifications` actions would never return. ## 📚 Documentation Slightly updated docs to provide more guidance around requesting authorization for notifications. ## ✅ Testing Added a unit test that verifies that the action returns. ## 📝 Code of Conduct & Contributing Guidelines By submitting creating this pull request, you agree to follow our [Code of Conduct](https://github.com/StanfordSpezi/.github/blob/main/CODE_OF_CONDUCT.md) and [Contributing Guidelines](https://github.com/StanfordSpezi/.github/blob/main/CONTRIBUTING.md): - [x] I agree to follow the [Code of Conduct](https://github.com/StanfordSpezi/.github/blob/main/CODE_OF_CONDUCT.md) and [Contributing Guidelines](https://github.com/StanfordSpezi/.github/blob/main/CONTRIBUTING.md). --------- Co-authored-by: Paul Schmiedmayer <[email protected]>
Platform support, Application property wrapper and notifications support
♻️ Current situation & Problem
Currently, the Core Spezi infrastructure is only developed for iOS. However, we are aiming to support Spezi on all Apple platforms. This step required to reengineer some Spezi infrastructure that was heavily tailored towards the iOS platform (e.g., the
LifecycleHandler). This PR deprecates theLifecycleHandlerin favor of using SwiftUI native approaches like theScenePhaseenvironment property or platform-specific publisher-based solutions likewillEnterForegroundNotification.Further we introduce the new
@Applicationproperty wrapper forModulesand theStandardto access application-specific properties likelaunchOptions.Based on the new
@Applicationinfrastructure we resolve issues like #80 to provide Modules with Module-specific Logger instances.To further validate our
@Applicationapproach, we added infrastructure around (Remote Push) Notifications. You can use@Applicationto trigger application actions likeregisterRemoteNotificationsorunregisterRemoteNotificationsand additionally adopt protocols likeNotificationTokenHandlerandNotificationHandlerto react to notification actions, control how notifications are delivered in foreground or fetch additional content for background notifications.⚙️ Release Notes
LifecycleHandlerprotocol.@Applicationproperty wrapper to access application properties and actions in a a platform-agnostic way.NotificationTokenHandlerandNotificationHandlerto easily support notification handling within your apps.📚 Documentation
There are two new articles
Interactions with ApplicationandUser Notificationsthat provide an overview for those new infrastructure elements.✅ Testing
TBA
📝 Code of Conduct & Contributing Guidelines
By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: