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

[health] Fixed request permission to Health Connect for Android #927

Closed

Conversation

iamLuCat
Copy link

@iamLuCat iamLuCat commented Mar 27, 2024

Issue: #893

Fix the issue of requesting permission when the Health Connect application hasn't been installed

Video demo

media_20240327_140219_2556024780277294492.mp4

@bardram
Copy link
Contributor

bardram commented Mar 29, 2024

Hi @iamLuCat - I really like this PR, but I've been updating the Health plugin to version 10.0.0 (published), and this PR would need to go into this version. I can't quite see exactly what changes this PR adds and it would be best if you could update this PR to be compatible and mergeable with the current master branch.

@bardram bardram self-assigned this Mar 30, 2024
@ciriousjoker
Copy link
Contributor

ciriousjoker commented Apr 2, 2024

@bardram

Differences:

  • Ready to merge, no conflicts
  • [health] Add getHealthConnectSdkStatus #941 has more flexibility because it exposes the status directly instead of condensing the 3 possible states into a single bool.
  • Returning only a single bool has the massive downside of requiring DeviceApps.isAppInstalled(), which I see as a much more fragile way of checking for Health Connect availability than asking the Health Connect sdk directly. Also this requires a separate package (DeviceApps), which isn't really necessary
  • I like the choice in the example code of using a different Flutter package to open the Play Store with the relevant package url, but I saw this too late and by that point I already integrated created the second PR

@iamLuCat
Copy link
Author

iamLuCat commented Apr 3, 2024

@bardram I will update the version today. Thank you for your response.

@iamLuCat iamLuCat force-pushed the health-dev-permission-android branch from 7d3978e to 67e73e2 Compare April 3, 2024 06:42
@bardram
Copy link
Contributor

bardram commented Apr 3, 2024

@bardram I will update the version today. Thank you for your response.

@iamLuCat - please see the comment from @ciriousjoker above. As far as I can see, this PR can be replaced by #941 and #943. So - there is no need to do anything here. Right?

@iamLuCat
Copy link
Author

iamLuCat commented Apr 3, 2024

@iamLuCat - please see the comment from @ciriousjoker above. As far as I can see, this PR can be replaced by #941 and #943. So - there is no need to do anything here. Right?

@bardram That's correct. I want to make a slight adjustment on my branch. @ciriousjoker's PR is better.

@bardram
Copy link
Contributor

bardram commented Apr 3, 2024

This has now been released in 10.2.0

@bardram bardram closed this Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants