API/public #24
API/public #24
Conversation
Still need to work on binding initializer and reducing view modifiers.
There was a problem hiding this comment.
Thank you for working on this @nriedman!
@Supereg I had the chance to sync with @nriedman on this in our last meeting and added some first suggestions. Would love to get your input in the API surface and how we should structure this before we take over elements from ENGAGE and put them in here.
A special focus would be on configuration and an easy way to also load date from data sources outside of HealthKit if necessary.
There was a problem hiding this comment.
Hi @nriedman,
I’ve shared a few thoughts and suggestions throughout—hope they’re helpful! 🎉 I'm happy to dive deeper into any of them if you’d like to discuss.
I’m excited to see how things shape up, especially as you move forward with the Chart implementation. 🚀 I think seeing some of the use cases in action will really help shape some of the current interfaces. Just let me know when you’d like another round of feedback!
|
|
||
| public struct HealthChart: View { | ||
| @State private var privateRange: ChartRange | ||
| private var privateRangeBinding: Binding<ChartRange>? |
There was a problem hiding this comment.
I'm not sure if this works in the end, as this will not be used for view re-render by SwiftUI. Not sure if it is needed though. Just a little note.
There was a problem hiding this comment.
Hi @Supereg! Would you mind explaining a little more about this? When I tested the UI with this setup, I saw that when I change the date range in the lowest view in the hierarchy, this binding propagated that change up to the top level as expected, and vice versa when I change the range at the highest level. That is, the views seem to re-render correctly when I change the state? Let me know if I'm misunderstanding what you mean!
There was a problem hiding this comment.
tldr: There is a slight technical different, though, it is not 100% clear if it makes any difference. I think it shouldn't impact your development work right now as the interfaces might not be fully there where they will be. Chances are it might be refactored anyways. And if it works it works.
Long answer:
You are right, it seems that from a view-update perspective everything works fine. Either it's just because the parent view refreshes or the Bindings transaction actually changes from a Equatable perspective.
What I head in the back of my head: Binding conforms to DynamicProperty (basically every SwiftUI property does to know when to prepare for the next view update). SwiftUI would then just inspect every DynamicProperty conforming property of the type and call update() accordingly. However, Optional does not conform to DynamicProperty, even if its Wrapped type does. I made a small example below. If foo is declared as optional, update() is never called, if you declare it as non-optional it will be called.
There is two questions here: a) does Binding contain any important logic in its update() method and how would it influence behavior (e.g., properties like @Environment or @State make sure to have a consistent view on the data while body is getting called) and b) does SwiftUI workaround this internally and has custom handling for optional types of e.g. Binding. So honestly not sure if it actually makes a difference at all.
struct Foo: DynamicProperty {
func update() {
print("UPDATE")
}
}
struct MyView: View {
var a: Foo? = Foo()
var body: some View {
Text("Hello World")
}
}There was a problem hiding this comment.
After discussing with @PSchmiedmayer, we had the following thoughts:
Potential workaround that addresses the fact that Optional does not conform to DynamicProperty:
Make Optional conform to DynamicProperty when it's wrapped value conforms to DynamicProperty. The update() function will then just call the wrapped values venation if it's not nil, and do nothing otherwise.
Note: Apple/SwiftUI may already have native support to work around this, as we are not the first to encounter this issue.
...SpeziHealthCharts/MetricChart/DataProvider/HealthKitDataProvider/HealthKitDataProvider.swift
Outdated
Show resolved
Hide resolved
| public init( | ||
| _ type: HKQuantityType, | ||
| in initialRange: ChartRange = .month, | ||
| provider: any DataProvider = HealthKitDataProvider() |
There was a problem hiding this comment.
What was the reason to have the concept of the DataProvider be part of the public interface here? Would it make sense to just to just pass a collection of (HK) samples to the chart to be as reusable as possible. For example, if I just have a few recordings form my Bluetooth device that I turned into HKQuantitySamples, I can just pass the collection to the Chart.
I think the concept of the HealthKitDataProvider API is great, especially to make it easier to query samples from the HealthKitStore. But can these two APIs be separate? Or have a chart view that works with a DataProvider be a separate component that sits on top of a simple chart implementation that just takes a collection of samples?
Let me know what your thoughts are here 👍
| extension HealthChart { | ||
| public func disable(interactions newValue: HealthChartInteractions) -> some View { |
There was a problem hiding this comment.
I think having an extension on View would provide the most flexibility. Though, to not collide with the existing disabled(_:) modifier we might want to find a more unique name like healthChartInteractions(disabled:) maybe? Some of the accessibility modifiers have a similar naming scheme, where the external parameter name express the semantic effect of the modifier.
| /// Default implementation fetches `HealthKitDataProvider` fetches data from a HealthKit `HealthStore`. | ||
| public protocol DataProvider: Sendable { | ||
| // TODO: Pass `ChartRange` instead of interval, query all health data at once aggregated by `DateRange` granularity. | ||
| func fetchData(for measurementType: HKQuantityType, in interval: ChartRange) async throws -> [HKQuantitySample] |
There was a problem hiding this comment.
Out of interest, what would be other DataProvider implementations except for a HealthKit-based one?
| import Foundation | ||
|
|
||
|
|
||
| public struct HealthChartStyle: Sendable { |
There was a problem hiding this comment.
Eager to see how the health chart style turn out. What's the general idea, just having some parameters that control how the chart is rendered?
Sources/SpeziHealthCharts/MetricChart/Models/HealthChartInteractions.swift
Show resolved
Hide resolved
|
Thank you for all the input @Supereg! I think we might want to stick with HKSample types for now as the majority of the package here deals with them (and their challenges within Swift + SwiftUI). I am wondering if small and targeted extensions could help here. Or alternatively a wrapper type that could still allow access to the underlying types. I think for now we could stick with HKSample types, similar to how we enable their usage in SpeziDevices and other elements. It would be amazing to see if Apple adopts some more modern types here in the future. |
This reverts commit 5713a64.
Health Chart Public Interface
♻️ Current situation & Problem
This PR defines the public API for the Health Chart module.
⚙️ Release Notes
MeasurementType, aDateRange, and aDataProvider(defaults toHealthKitDataProvider)HealthChart.disable(interactions:)modifier.HealthChartvia a.style(HealthChartStyle)modifier.📚 Documentation
See inline documentation.
✅ Testing
NA
📝 Code of Conduct & Contributing Guidelines
By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: