Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
API/public #24
base: HealthChart/main
Are you sure you want to change the base?
API/public #24
Changes from all commits
b68d3fd
f8c950e
124045d
cbbc335
4f5ff38
5713a64
5bf0d21
0948f21
9030f5c
4452419
5ec9afd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check failure on line 2 in Package.swift
GitHub Actions / SwiftLint / SwiftLint / SwiftLint
Check failure on line 27 in Sources/SpeziHealthCharts/MetricChart/ChartViews/DevInternalHealthChart.swift
GitHub Actions / Build and Test UI Tests / Test using xcodebuild or run fastlane
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 toDynamicProperty
(basically every SwiftUI property does to know when to prepare for the next view update). SwiftUI would then just inspect everyDynamicProperty
conforming property of the type and callupdate()
accordingly. However,Optional
does not conform toDynamicProperty
, even if itsWrapped
type does. I made a small example below. Iffoo
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 whilebody
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.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.
After discussing with @PSchmiedmayer, we had the following thoughts:
Potential workaround that addresses the fact that
Optional
does not conform toDynamicProperty
:Make
Optional
conform toDynamicProperty
when it's wrapped value conforms toDynamicProperty
. Theupdate()
function will then just call the wrapped values venation if it's notnil
, 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.
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.
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 👍
Check failure on line 11 in Sources/SpeziHealthCharts/MetricChart/ChartViews/InternalHealthChart.swift
GitHub Actions / Build and Test Swift Package / Test using xcodebuild or run fastlane