[WIP] query property wrappers, concepts of a HealthChart #27
Conversation
- Remove `CollectSamples`. The same functionality can be achieved using a for loop in the configuration builder. - Rework the HealthKit auth handling. We still cannot access the read auth request responses, but there's also no point in somehow trying to keep track of which type's we've already requested access to, so we now just keep track of _whether_ we've already requested access (which is the same thing). - Add an API for requesting write access - Rework the `HealthKitSampleType` to be more lean and generic (and to better support the individual types) - Add a couple more sample types
…rs until their respective sample types have been authorized
in the previous approach, it would happen that if you were observing let's say 5 different sample types using eg `CollectSample`, it would "enable" all of these in a for loop, and each would then request authorization to its respective sample type. but since they kinda all were starting their queries at about the same time, you'd get a situation where only the first one to run would be able to present the HealthKit data access request sheet, and all other ones would fail to request access.
The new approach works as follows: we try to combine all Health data we need to access into a single auth request, and we no longer automatically request access as part of e.g. starting a background CollectSample observer. instead, we give the user the responsibility to call HealthKit.askForAuthorization at some point. the (automatic) background queries will start once the permission request has completed (regardless of whether we actually were given access; we can't see that).
This is better in several ways:
- it avoids the issue of all background queries except the first failing to request access
- it allows the app control over when exactly the HealthKit access request should happen. (Eg: you probably want to embed this into an onboarding flow, instead of just having it happen randomly at some point, without the user being informed in advance what;s about to happen)
The simplest case (assuming no onboarding) would be that the app would simply have a `.task { try? await healthKit.askForAuthorization() }` somewhere in the root view. this would only result in the auth sheet being presented once, or when the access requirements change. otherwise, you can call this function without there being any side effects.
this sadly seems to be necessary to work around swiftlang/swift#78734 and swiftlang/swift#78735
There was a problem hiding this comment.
Great job with the PR and additions here @lukaskollmer!
I added some comments throughout the PR. Thank you for resolving all the remaining TODOs in this PR or we start to transform some of them into GitHub issues to be resolved later on in subsequent PRs.
We should also adjust the README (similar to other Spezi READMEs) to reflect the functionality & HealthChart examples. We should also copy the same content to the DocC setup & ensure that it also is having all the updated references and examples in place.
This will be a huge step forward for SpeziHealthKit and will bring it closer to a 1.0, thank you! 🚀
| ], | ||
| swiftSettings: [.enableUpcomingFeature("ExistentialAny")], |
| /// | ||
| /// This structure define what and how the ``HealthKit`` samples are collected. By default, all samples of the provided `HKSampleType` will be collected. The collection starts on calling ``HealthKit/triggerDataSourceCollection()`` if you configure the `deliverySetting` as ``HealthKitDeliverySetting/manual(safeAnchor:)`` or automatic once the application is launched when you configure anything else than manual, i.e. ``HealthKitDeliverySetting/anchorQuery(_:saveAnchor:)`` or ``HealthKitDeliverySetting/background(_:saveAnchor:)``. | ||
| /// This structure define what and how the ``HealthKit`` samples are collected. By default, all samples of the provided ``HealthKitSampleType`` will be collected. | ||
| /// The collection starts on calling ``HealthKit/triggerDataSourceCollection()`` if you configure the `deliverySetting` as ``HealthKitDeliverySetting/manual(saveAnchor:)`` or automatic once the application is launched when you configure anything else than manual, i.e. ``HealthKitDeliverySetting/anchorQuery(_:saveAnchor:)`` or ``HealthKitDeliverySetting/background(_:saveAnchor:)``. |
There was a problem hiding this comment.
We might want to clarify here that (automatic) collection only starts once the permissions have been requested authorization. Maybe we should add a DocC hint/tip box here to point to the function to request authorization from HealthKit?
|
|
||
| public var sampleTypes: Set<HKSampleType> { | ||
| collectSamples.sampleTypes | ||
| public var dataAccessRequirements: HealthKitDataAccessRequirements { |
|
|
||
|
|
||
|
|
||
| // TODO is this actually needed? |
There was a problem hiding this comment.
Note to address this TODO before merging the PR.
There was a problem hiding this comment.
Nice mechanism; very cool!
There was a problem hiding this comment.
Very cool mechanism, looking forward to seeing this merged & extended in future iterations. Looks nice, might be worth splitting it up into multiple files as we finalize the implementation.
There was a problem hiding this comment.
It'll definitely get split up into multiple files, though I'm not actually sure how much leeway i have on this.
There's currently some weird compilation issues around here which seem to also be somehow related to file structure (see eg swiftlang/swift/issues/78734 and swiftlang/swift/issues/78735).
That's also why I sadly had to move the HealthChart files into the SpeziHealthKit target, instead of having a separate SpeziHealthKitUI target (which i would prefer).
There was a problem hiding this comment.
Interesting, that's unfortunate to hear. Thank you for creating the issues in the Swift repo; let's see what they say about this.
Agree that it would be nice to see this moved into a separate target but we might need to wait until the issues are resolved.
| case background(HealthKitDeliveryStartSetting = .automatic, saveAnchor: Bool = true) | ||
|
|
||
| /// :nodoc: | ||
| @available(*, unavailable, renamed: "manual(saveAnchor:)") |
There was a problem hiding this comment.
Thank you for fixing this!
| /// - Note: This property wrapper is not auto-updating; if the characteristic's value is changed while a view using this property wrapper is active, | ||
| /// it will continue displaying the old value until the view gets updated for some other reason. | ||
| @propertyWrapper | ||
| public struct HealthKitCharacteristicQuery<Value>: DynamicProperty { |
There was a problem hiding this comment.
Very nice mechanism; very cool! We should add some nice examples of using this here in the inline docs as well as adjust the README & maybe add some DocC articles with examples how to use this in combination with a minimal possible view.
There was a problem hiding this comment.
Very nice, some of the TODOs can subsequently be resolved but the overall structure looks nice, a lot of convenient shorthand elements!
There was a problem hiding this comment.
Very cool mechanism; looking forward to seeing this merged 🚀
|
|
||
|
|
||
| /// The time range for which data should be fetched from the health store. | ||
| public enum HealthKitQueryTimeRange: Hashable, Sendable { |
There was a problem hiding this comment.
@PSchmiedmayer i ended up adding probably slightly too many cases here. what's your take as to which time ranges we should want to offer?
There was a problem hiding this comment.
I like the amount of choices we have here, I think they are all useful. But re-looking at this type I would suggest that we transform it into a enum-like struct to make it easier to easily extend it in the future and enable a few nicer ways to handle different elements around "last N elements". I don't think we explicit benefit from an enum here as we will never really have to iterate/switch across all cases.
| /// The time range encompassing the last `N` hours, starting at the end of the current hour. | ||
| case lastNHours(Int) | ||
| /// The time range encompassing the last `N` days, starting at the end of the current day. | ||
| /// - Note: the resulting effective time range of `lastNDays(1)` is equivalent to the one of `today`. | ||
| case lastNDays(Int) | ||
| /// The time range encompassing the last `N` weeks, starting at the end of the current day. | ||
| case lastNWeeks(Int) | ||
| /// The time range encompassing the last `N` months, starting at the end of the current day. | ||
| case lastNMonths(Int) | ||
| /// The time range encompassing the last `N` years, starting at the end of the current day. | ||
| case lastNYears(Int) |
There was a problem hiding this comment.
| /// The time range encompassing the last `N` hours, starting at the end of the current hour. | |
| case lastNHours(Int) | |
| /// The time range encompassing the last `N` days, starting at the end of the current day. | |
| /// - Note: the resulting effective time range of `lastNDays(1)` is equivalent to the one of `today`. | |
| case lastNDays(Int) | |
| /// The time range encompassing the last `N` weeks, starting at the end of the current day. | |
| case lastNWeeks(Int) | |
| /// The time range encompassing the last `N` months, starting at the end of the current day. | |
| case lastNMonths(Int) | |
| /// The time range encompassing the last `N` years, starting at the end of the current day. | |
| case lastNYears(Int) | |
| /// The time range encompassing the last `N` hours, starting at the end of the current hour. | |
| static func last(hours: Int) -> HealthKitQueryTimeRange | |
| /// The time range encompassing the last `N` days, starting at the end of the current day. | |
| /// - Note: the resulting effective time range of `lastNDays(1)` is equivalent to the one of `today`. | |
| static func last(days: Int) -> HealthKitQueryTimeRange | |
| /// The time range encompassing the last `N` weeks, starting at the end of the current day. | |
| static func last(weeks: Int) -> HealthKitQueryTimeRange | |
| /// The time range encompassing the last `N` months, starting at the end of the current day. | |
| static func last(months: Int) -> HealthKitQueryTimeRange | |
| /// The time range encompassing the last `N` years, starting at the end of the current day. | |
| static func last(years: Int) -> HealthKitQueryTimeRange |
| /// The time range encompassing the entire current week. | ||
| case currentWeek // TODO remove this? | ||
| /// The time range encompassing the entire current month. | ||
| case currentMonth // TODO remove this? | ||
| /// The time range encompassing the entire current year. | ||
| case currentYear // TODO remove this? |
There was a problem hiding this comment.
I like that we have these additional choices & think they will be helpful for people using these features.
|
|
||
|
|
||
| /// The time range for which data should be fetched from the health store. | ||
| public enum HealthKitQueryTimeRange: Hashable, Sendable { |
There was a problem hiding this comment.
I like the amount of choices we have here, I think they are all useful. But re-looking at this type I would suggest that we transform it into a enum-like struct to make it easier to easily extend it in the future and enable a few nicer ways to handle different elements around "last N elements". I don't think we explicit benefit from an enum here as we will never really have to iterate/switch across all cases.
There was a problem hiding this comment.
Interesting, that's unfortunate to hear. Thank you for creating the issues in the Swift repo; let's see what they say about this.
Agree that it would be nice to see this moved into a separate target but we might need to wait until the issues are resolved.
| @@ -9,58 +9,54 @@ | |||
| import HealthKit | |||
|
|
|||
|
|
|||
| extension HealthKitSampleType where Sample == HKQuantitySample { | |||
| /// ## Quantity Sample Types | |||
| extension SampleType { | |||
There was a problem hiding this comment.
I like the simplification into SampleType 👍
[WIP] query property wrappers, concepts of a HealthChart
♻️ Current situation & Problem
The HealthKit module currently only provides access to HealthKit data via long-running anchor queries that deliver information about new/deleted objects to the app's Standard.
It does not provide any facilities for querying for past samples, or accessing health data from SwiftUI. This PR attempts to address these issues.
Furthermore, the HealthKit module is lacking an API allowing spezi users to integrate custom HealthKit permission requests into the module's permission handling (i.e., you currently can only request HealthKit access for some specific sample type by actively defining a long-running observer for that sample type).
Furthermore, this PR attempts to implement a
HealthChartview, which can display various types of queried HealthKit data as a chart.resolves #8
requires StanfordSpezi/SpeziFoundation#19
requires StanfordBDHG/XCTestExtensions#28
⚙️ Release Notes
HealthKitQueryproperty wrapperHealthKitStatisticsQueryproperty wrapperHealthKitCharacteristicQueryproperty wrapperHealthChartviewHealthKitconfiguration API to allow users to specify sample types the system should request read and/or write access toCollectSamples. The same functionality can be achieved using aforloop creating individualCollectSampleinstances.📚 Documentation
The documentation is still WIP and is currently being reworked.
Some of the new APIs are already partly documented.
✅ Testing
There currently are no tests for the new APIs. Tests will be added once the API is finalised and the key components are implemented.
📝 Code of Conduct & Contributing Guidelines
By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: