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

query property wrappers, HealthChart, other refactoring #27

Open
wants to merge 92 commits into
base: main
Choose a base branch
from

Conversation

lukaskollmer
Copy link
Member

@lukaskollmer lukaskollmer commented Jan 12, 2025

query property wrappers, HealthChart, other refactoring

♻️ 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 HealthChart view, which can display various types of queried HealthKit data as a chart.

resolves #8
requires StanfordSpezi/SpeziFoundation#19
requires StanfordBDHG/XCTestExtensions#28

⚙️ Release Notes

  • Added HealthKitQuery property wrapper
  • Added HealthKitStatisticsQuery property wrapper
  • Added HealthKitCharacteristicQuery property wrapper
  • Added HealthChart view
  • Extended HealthKit configuration API to allow users to specify sample types the system should request read and/or write access to
  • Removed CollectSamples. The same functionality can be achieved using a for loop creating individual CollectSample instances.

📚 Documentation

All added new and changed existing APIs are documented. The DocC structure was reworked and some key aspects of SpeziHealthKit now have dedicated article/extension pages.

✅ Testing

The new and changed APIs are tested using both "normal" unit tests and UI tests. Existing tests were kept and adapted where possible

📝 Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

- 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.
@PSchmiedmayer PSchmiedmayer added the enhancement New feature or request label Jan 19, 2025
Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

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")],
Copy link
Member

Choose a reason for hiding this comment

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

🚀

///
/// 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:)``.
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

I ended up changing this back to the old behaviour: data collection for sample types for which access has already been requested starts directly as part of the HealthKit module initialization. (Apps are still strongly encouraged to call askForAuthorization() on every launch, so that changes to the access requirements are properly handled with.

Sources/SpeziHealthKit/CollectSample/CollectSample.swift Outdated Show resolved Hide resolved
Sources/SpeziHealthKit/Sample Types/SampleType.swift Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Very cool mechanism; looking forward to seeing this merged 🚀

Sources/SpeziHealthKit/Queries/HealthKitQuery.swift Outdated Show resolved Hide resolved
Sources/SpeziHealthKit/Queries/HealthKitQuery.swift Outdated Show resolved Hide resolved
Sources/SpeziHealthKit/Queries/HealthKitQuery.swift Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

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 {
Copy link
Member

Choose a reason for hiding this comment

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

I like the simplification into SampleType 👍

@PSchmiedmayer PSchmiedmayer linked an issue Jan 20, 2025 that may be closed by this pull request
1 task
even though the Health data store isn't available on macOS, HealthKit as a framework still is.
Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Amazing job, thank you! 🚀

- default to automatic query starting
- remove the manual option
- remove the saveAnchor option
- store the anchor using SpeziLocalStorage, as opposed to in the UserDefaults
@jdisho
Copy link

jdisho commented Jan 25, 2025

😂😂
Screenshot 2025-01-25 at 18 53 44

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Complete Concurrency Checking Improve HealthKit Query Reusability
3 participants