Add Health Connect Syncing - #109
Conversation
IamDg
left a comment
There was a problem hiding this comment.
Hi,
Thank you for your awesome contribution!
I'm kinda surprise by your work which is mostly correct and well implemented. Just correct/answer some points of the review and it should be ready to merge.
Regarding health connect in general, I looked up some info and documentation and you are right: a migration to timestamp is necessary to actually make integration possible. Since this is not a priority, I would be glad if you were willing to work on this also because you did a good job so far.
Let me know what do you think!
| healthConnectState.hasPermissions | ||
|
|
||
| when { | ||
| !healthConnectState.isAvailable -> Unit |
| val isHealthConnectEnabled = healthConnectState.isEnabled && | ||
| healthConnectState.hasPermissions | ||
|
|
||
| when { |
There was a problem hiding this comment.
Would be possible to move this logic in view model?
| } No newline at end of file | ||
| } | ||
|
|
||
| data class HealthConnectState( |
There was a problem hiding this comment.
This should be moved in a separated file. Also, maybe would be better an ENUM instead?
| val healthConnectState = _healthConnectState.asStateFlow() | ||
|
|
||
| init { | ||
| viewModelScope.launch { |
There was a problem hiding this comment.
Maybe I'm wrong but it looks like that a combine() can be used to merge _healthConnectState flow with userPreferences.healthConnectEnabled flow in a single one in order to avoid broken state or spaghetti updates.
|
Thanks, that’s really nice to hear. I’ve already implemented the changes, but I thought it would also be useful to make the settings configurable by scopes, such as read and write permissions for body metrics, as well as write permissions for exercises. That way, everyone could adjust the permissions however they prefer. For this, it would be really nice to have a dropdown in the settings: when the main toggle is enabled, the dropdown could expand and show the different read and write toggles separately. Do you have any thoughts or ideas on that? |
Since the long term plan is to implement health connect fully, I think it would be better to create a dedicated section in settings (e.g. called Health connect) which would contain all toggles. |
IamDg
left a comment
There was a problem hiding this comment.
Great improvements here.
Try to keep classes separated from each other and in suitable location.
Also make the logic as simple and clear as possible (right now it's fine)
Yeah, that makes sense. I just thought it might be better to hide the individual toggles as long as Health Connect is not activated, so the screen stays clean. Should each sync option be its own card, or could we combine them into one card to make it more space-efficient? The options shown could be: Body weight (read / write) Fat mass (read / write) Lean mass (read / write) Workouts (write)(I would not read workouts, since that does not really make sense and would create too many edge cases.) Would you simplify this into fewer options, or would you show all of them? |
I think a card with write/read for each type, as you proposed, should do the job |
@IamDg I think the option with a single card containing all sync settings underneath looks much better and is more space-efficient. However, it is also different from the rest of the settings page, which makes the design slightly less consistent. In both options, the card would remain hidden as long as Health Connect is disabled. Many other apps also include a “Manage permissions in Health Connect” button, which opens the Health Connect app so users can review and manage all permissions there. We could add something similar. |
IamDg
left a comment
There was a problem hiding this comment.
Hi,
Thank you for your contribution!
I've left some comments to address. Now back to your question:
I think the option with a single card containing all sync settings underneath looks much better and is more space-efficient. However, it is also different from the rest of the settings page, which makes the design slightly less consistent.
In both options, the card would remain hidden as long as Health Connect is disabled.
Many other apps also include a “Manage permissions in Health Connect” button, which opens the Health Connect app so users can review and manage all permissions there. We could add something similar.
I'd opt for second option since as you said it's more consistent. Regarding the settings button, I think it would be a great addition. Furthermore, the official health connect icon looks better than the generic one
| .distinctUntilChanged() | ||
| .stateIn( | ||
| scope = viewModelScope, | ||
| started = SharingStarted.Eagerly, |
There was a problem hiding this comment.
is there a reasong why it's eagerly instead of subscribed? If not, I'd suggest the following
| started = SharingStarted.Eagerly, | |
| started = SharingStarted.WhileSubscribed(5_000), |
| onHealthConnectClick = { | ||
| when (viewModel.onHealthConnectClick()) { | ||
| HealthConnectClickAction.REQUEST_PERMISSIONS -> { | ||
| healthConnectPermissionLauncher.launch(viewModel.healthConnectPermissions) | ||
| } | ||
| HealthConnectClickAction.NONE -> Unit | ||
| } | ||
| }, |
There was a problem hiding this comment.
According to official documentation, before launching the permission request, it should be explained the rationale. I think an dialog or modal bottom sheet (even use the LibreFit's ones) should do the job
| ) | ||
|
|
||
| init { | ||
| refreshHealthConnectState() |
There was a problem hiding this comment.
These should be called after, for instance, app comes back from background to foreground. For this reason it should be considered adding an OnLifecycleEvent observer in SettingsScreen to call viewModel.refreshHealthConnectState() when the app comes back to the foreground.
|
|
||
| fun updateHealthConnectPermissions(grantedPermissions: Set<String>) { | ||
| viewModelScope.launch { | ||
| val hasPermissions = grantedPermissions.containsAll(healthConnectPermissions) |
There was a problem hiding this comment.
By using containsAll it means all permissions are mandatory? Maybe it's better checking one by one?
|
LibreFit currently stores the value entered as “Lean Mass” in the UI as This distinction matters for the Health Connect integration because Health Connect provides lean body mass in kilograms, not muscle mass as a percentage. I can address this as part of the current PR, or handle it separately if you would prefer to keep this one focused. |
This is a good point and thank you for asking clarification. The intention is to store muscle mass. The latter could be calculated by multiplying body mass and muscle mass percentage, right? |
|
I would update the UI accordingly later to display muscle mass instead of lean mass. I checked again, and Health Connect only supports lean mass, not muscle mass in any form. So the most practical option is probably to remove it for now. I was already thinking about making body fat and muscle mass optional in the settings, since many users do not have a scale that supports them, which would also simplify the UI. But that can be addressed later. For now, we can only sync workouts, weight, and body fat percentage with Health Connect. |
|
Older workouts do not have timestamps for individual sets. Should we estimate these timestamps when exporting them to Health Connect, or should we only sync new workouts that have real timestamps? |
|
For the Health Connect workout export, I currently map LibreFit exercises to Health Connect types by checking keywords in their displayed names. For example, an exercise containing After testing this against the 912 existing exercises, I found that this approach creates incorrect mappings:
There are a few possible approaches:
I think storing a Health Connect type directly on every exercise would be the safest approach and i would let codex do the first run over all current exercises. Which solution would you prefer for this PR? I'm sorry for the many questions i realy wanna get this right because its such a big implementation thank you for you patient. |
I would sync only workouts with timestamps.
Go with second approach since it's the long term solution. Just be sure to follow official android documentation and minimize error rate of agent by using thinking, planning, skills or anything else. |
d1715e3 to
5bea01d
Compare
|
I updated the PR and merged the current main branch. Health Connect is now implemented for weight, body fat and workouts. Weight and body fat can be read and written, while completed workouts are write-only. I changed the settings to one card per data type with separate Read and Write switches. Enabling Health Connect for the first time now requests all supported permissions together. Data imported from Health Connect is not written back to it. The sync is limited to 30 days and imported body values are rounded to two decimal places. Old workouts without real set timestamps are skipped, so only new workouts with complete timestamps are exported. I also replaced the exercise name guessing with an explicit Health Connect exercise type for all 912 built-in exercises. Custom exercises use a safe fallback. The mappings and the complete exercise dataset are validated by tests. Because main already uses database version 4, the Health Connect database change is now one migration from version 4 to 5. All 89 unit tests pass and the debug APK builds successfully. Please let me know if you would prefer anything to be structured differently. |
|
i would test the APK myself for a few training sessions to see if everything is fine and report back after. |
|
Update lint_baseline so CI pass and I can download debug apk |


What changed
This PR adds Health Connect synchronization for:
Details
Testing
Issue: #104
Health Connect permissions
Privacy settings
Settings toggle