-
Notifications
You must be signed in to change notification settings - Fork 630
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 2 different display sensors around various screen states #4634
Add 2 different display sensors around various screen states #4634
Conversation
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.
Haven't tested this yet, but how do the values hold up over time when they are updated in the background?
Is it a good idea to ask the users of the original request to test whether it solves their issue?
common/src/main/java/io/homeassistant/companion/android/common/sensors/DisplaySensorManager.kt
Outdated
Show resolved
Hide resolved
common/src/main/java/io/homeassistant/companion/android/common/sensors/DisplaySensorManager.kt
Outdated
Show resolved
Hide resolved
common/src/main/java/io/homeassistant/companion/android/common/sensors/DisplaySensorManager.kt
Outdated
Show resolved
Hide resolved
common/src/main/java/io/homeassistant/companion/android/common/sensors/DisplaySensorManager.kt
Outdated
Show resolved
Hide resolved
Placing in draft while working out some more specifics in the PR, this also needs testing on other devices as mentioned and also by end users who requested the sensor(s) |
in my extensive testing they work well but like other sensors require the app to run in the background to get timely updates this still needs testing especially from a foldable device |
Testing on a foldable works as expected! All states update as we expect. Marking as ready for review, have not heard back from end user but I dont think it should hold this up. |
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.
Positively surprised by how well the face down sensor is working (even if it did get stuck once)!
common/src/main/java/io/homeassistant/companion/android/common/sensors/DisplaySensorManager.kt
Show resolved
Hide resolved
common/src/main/java/io/homeassistant/companion/android/common/sensors/DisplaySensorManager.kt
Show resolved
Hide resolved
common/src/main/java/io/homeassistant/companion/android/common/sensors/DisplaySensorManager.kt
Show resolved
Hide resolved
9f9ca16
to
a57a2df
Compare
added possible states to rotation sensor based on user feedback to help make it more clear |
I have updated the face down sensor logic ad name, I am honestly not sure what to name or even if it should belong under the display sensors. I originally had it under display sensors because the state is relative to the screen. I am not sure if "position" is an appropriate sensor name but I do see value in having various states like up or down to determine the display position. I opted to add attributes as well in case we find additional states to use. In that case the current name makes no sense. I am also ok with removing for now but the coolness factor of the data is holding me back 🤓 |
i am going to remove teh face up/down sensor for now as it needs more thought about how to best handle it and the different states that may be useful |
Summary
Fixes: #2440 by adding 2 different sensors to capture the various states.
2 different sensors because the states can all be independent of each other.
Originally I tried to be fancy with reverse portrait states however I realized the angles are dependent on the device in question and theres too much for account for when you consider all the different types of screens like on a watch, car etc...
The intent that is added only fires while the screen is on and if the screen has to redraw teh UI so battery impact is very minimal here.
Screenshots
Link to pull request in Documentation repository
Documentation: home-assistant/companion.home-assistant#1102
Any other notes