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

Add permissions requests to wear OS sensors and add network sensors #2956

Merged
merged 8 commits into from
Oct 19, 2022

Conversation

dshokouhi
Copy link
Member

@dshokouhi dshokouhi commented Oct 7, 2022

Summary

Adds permissions requests to Wear OS when a user enables a sensor, also adds Network Sensors as a start.

WTH: https://community.home-assistant.io/t/wth-dont-we-have-more-sensor-in-wear-os/469354

Screenshots

image

Link to pull request in Documentation repository

Documentation: home-assistant/companion.home-assistant#834

Any other notes

This was a "quick and dirty" approach to just get some more sensors added as permissions is holding up adding some of the sensors. Some of this code may be stale but wanted to get some extra eyes on this.

@jpelgrom
Copy link
Member

jpelgrom commented Oct 7, 2022

Foreground/background location permissions also need to be handeld separately on Wear OS. Using the Wear OS 3 emulator (API 30), after tapping on WiFi Connection it is instantly denied with logcat showing:

2022-10-07 21:28:34.559 12392-12392/com.google.android.permissioncontroller E/GrantPermissionsActivity: Apps targeting 30 must have foreground permission before requesting background and must request background on its own.

@dshokouhi
Copy link
Member Author

Foreground/background location permissions also need to be handeld separately on Wear OS. Using the Wear OS 3 emulator (API 30), after tapping on WiFi Connection it is instantly denied with logcat showing:

2022-10-07 21:28:34.559 12392-12392/com.google.android.permissioncontroller E/GrantPermissionsActivity: Apps targeting 30 must have foreground permission before requesting background and must request background on its own.

Thanks for that! I submitted a quick fix for that but its not pretty as of the moment.

@jpelgrom
Copy link
Member

jpelgrom commented Oct 7, 2022

Still not working - while it will ask for while in use location permission, after granting it I still cannot toggle the sensor on. You need to request Manifest.permission.ACCESS_BACKGROUND_LOCATION from the permission result, because when tapping the toggle it will ask for everything except Manifest.permission.ACCESS_BACKGROUND_LOCATION.

Also not a problem yet, but checked should only be set to true if all permissions for the sensor are granted, not looping through the request results one by one?

It's getting very close to having too much logic in the UI layer.

@dshokouhi
Copy link
Member Author

Still not working - while it will ask for while in use location permission, after granting it I still cannot toggle the sensor on. You need to request Manifest.permission.ACCESS_BACKGROUND_LOCATION from the permission result, because when tapping the toggle it will ask for everything except Manifest.permission.ACCESS_BACKGROUND_LOCATION.

Also not a problem yet, but checked should only be set to true if all permissions for the sensor are granted, not looping through the request results one by one?

Thanks hopefully its all a bit better after the latest commits :)

It's getting very close to having too much logic in the UI layer.

I agree, its a bit difficult to do the permissions properly without refactoring things a bit I think. I was hoping to perform the checks here on the page where we enable sensors as it seemed to be the most practical.

@dshokouhi
Copy link
Member Author

For anyone who wants to test this issue, please be aware of this issue which may impact the initial state of the sensor home-assistant/core#79837

you will need to trigger a second update

Copy link
Member

@jpelgrom jpelgrom left a comment

Choose a reason for hiding this comment

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

Enabling the network sensors now works! (The state remains Unknown but that is probably due to the emulator, not permissions.)

It's getting very close to having too much logic in the UI layer.

I agree, its a bit difficult to do the permissions properly without refactoring things a bit I think.

So why not refactor things a bit, instead of the current code? 😉

@dshokouhi
Copy link
Member Author

(The state remains Unknown but that is probably due to the emulator, not permissions.)

yea to get the state to change and properly update is not easy in teh emulator, on my watch I had BT connected then turned wifi off and it finally sent the first update. Definitely seems to be a HA core issue based on what I had filed.

So why not refactor things a bit, instead of the current code? 😉

I don't think at this point there is too much logic here, and in terms of permissions this might be about it as most of the special permissions on the phone side are not there on Wear OS like Usage Stats.

Copy link
Member

@jpelgrom jpelgrom left a comment

Choose a reason for hiding this comment

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

Great work, hopefully this will make it easier to share more sensors soon!

@JBassett JBassett merged commit 7c792c4 into home-assistant:master Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants