-
-
Notifications
You must be signed in to change notification settings - Fork 677
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
Settings activity app lock #2926
Settings activity app lock #2926
Conversation
app/src/main/java/io/homeassistant/companion/android/webview/WebViewActivity.kt
Show resolved
Hide resolved
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.
getSessionExpireMillis()
in the integration repository is no longer used from outside the integration repository so it should switch to a private fun
instead.
I see a lot of log statements, are all of those still useful outside development / when the app is released? For example in SettingsActivity.onResume
I see it logging if the app lock is enabled, but this will also become obvious immediately after as the app is locked (or not) (and is also logged by the function you're calling).
For logging, try using Kotlin string templates to make the code cleaner, so "someValue: " + variable
should be "someValue: $variable"
instead (Android Studio offers a quick fix for this).
app/src/main/java/io/homeassistant/companion/android/settings/SettingsActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/io/homeassistant/companion/android/settings/SettingsActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/io/homeassistant/companion/android/webview/WebViewActivity.kt
Show resolved
Hide resolved
...io/homeassistant/companion/android/common/data/integration/impl/IntegrationRepositoryImpl.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/io/homeassistant/companion/android/settings/SettingsActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/io/homeassistant/companion/android/settings/SettingsFragment.kt
Show resolved
Hide resolved
app/src/main/java/io/homeassistant/companion/android/settings/SettingsFragment.kt
Outdated
Show resolved
Hide resolved
Testing on an older device (though not that old, ~3y flagship level) with commit c6dae45 I'm getting an authentication prompt when opening settings. Is that supposed to happen? Logcat output
|
Based on the logcat output I cannot say the exact scenario in which you got the authentication prompt. The device I use and run my tests on is a Galaxy S10 running Android 12 (also not that new anymore). Based on the testing done on this device the following behavior was validated: But I would not rule out that the 200ms grace period could be a bit to short for older devices?.. |
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.
Tested the latest version and now it works correctly with the increased grace period. Hopefully the 1000ms works even on old and slow devices
app/src/main/java/io/homeassistant/companion/android/settings/SettingsActivity.kt
Outdated
Show resolved
Hide resolved
c829495
to
312a535
Compare
7c9b2b0
to
58dc1c4
Compare
Handle enabling app lock setting authentication in SettingsActivity to avoid callback clash.
58dc1c4
to
c1505af
Compare
As this PR is approved, can it be merged for the next beta? |
right now we are holding off on merges as we are stuck in review with the play store for 2022.10.1 release |
Got it, thanks for clarifying. That also explains why I could only update to 2022.10.0 after I saw the 2022.10.1 release on github. |
@RoboMagus Sorry but there is now a merge conflict. Once resolved we can merge! |
Summary
This PR fixes issue #2877.
The following changes are included:
WebViewActivity
and unified such that theSettingsActivity
can use it as well.SettingsActivity
when app lock is active.SettingsFragment
toSettingsActivity
, as it was impossible to have 2 different authentication initiators and callback methods in the same activity.Screenshots
Link to pull request in Documentation repository
Documentation: home-assistant/companion.home-assistant#
Any other notes