-
-
Notifications
You must be signed in to change notification settings - Fork 168
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
feat: consider NotificationShade and StatusBar as lock screen for newer android versions #564
feat: consider NotificationShade and StatusBar as lock screen for newer android versions #564
Conversation
…er android versions
lib/helpers.js
Outdated
* @param {string} dumpsys - The output of dumpsys window command. | ||
* @return {boolean} True if lock screen is showing. | ||
*/ | ||
function isShowingLockscreen (dumpsys) { | ||
return /(mShowingLockscreen=true|mDreamingLockscreen=true)/gi.test(dumpsys); | ||
return /(mShowingLockscreen=true|mDreamingLockscreen=true|mCurrentFocus=.+NotificationShade|mCurrentFocus=.+StatusBar)/gi.test(dumpsys); |
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.
should we be more restrictive about .+NotificationShade
and .+StatusBar
regexps to avoid false positives?
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.
for example .+\sNotificationShade\b
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.
ah, could be to tune the regex. Let me try.
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.
also would it be possible to split this long line? Maybe we could simply move the regexp itself into constants, which should also speed up the API call
I was also checking some finding regarding screen locked state. We could also test if solutions from https://stackoverflow.com/questions/35275828/is-there-a-way-to-check-if-android-device-screen-is-locked-via-adb work for us |
Yea, i tried some stuff like |
Yes, I saw some comments about the output there is not very reliable. What if we check the NFC state first and fall back to this method if it is disabled? Something like:
|
The nfc command returned outputs but it did not have Oh, I just found
It was Let me check more devices |
Btw, this is how the check is implemented in Android itself: https://android.googlesource.com/platform/packages/apps/Nfc/+/refs/heads/master/src/com/android/nfc/ScreenStateHelper.java#32 |
Also, how Google checks if the device screen is ready in internal tests: https://android.googlesource.com/platform/cts/+/ab9e203328a3edc5c40c5887bfc5bc8e9ba92769/tests/app/src/android/app/cts/ActivityManagerProcessStateTest.java#159 |
Thanks, I dug in dumpsys focusing on keyguard. Then, it seems the below condition is the most reliable. They existed since Android OS 6. I haven't checked on OS 5 tho. Non smart phone like Android TV did not have it. So, the primary is current way -> fallback to this keyguard is the most reliable so far, I think.
|
lib/helpers.js
Outdated
// mIsShowing and mInputRestricted are true in lock condition. | ||
return dumpsys.includes('mIsShowing=false', 'mInputRestricted=false'); | ||
return _.some(['mShowingLockscreen=true', 'mDreamingLockscreen=true'], (x) => dumpsys.includes(x)) | ||
|| _.every(['mIsShowing=false', 'mInputRestricted=false'], (x) => dumpsys.includes(x)); |
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.
Please add an indentation here to enhance the readability. The comment also makes sense
I think we should relax locked screen condition logic for newer Android devices.
https://github.com/appium/appium-adb/blob/master/lib/tools/adb-commands.js#L615-L632
Newer Android devices (i tested a few real devices), lock screen is simply
NotificationShade
orStatusBar
after keycode 62. But current our logic does not consider such case as locked screen. So, ourlock
never be succeeded.When these app is focused, Appium can unlock them with https://github.com/appium/appium-android-driver/blob/9d120ba76df18ada8c5793af3d42f9b1aa4937cb/lib/android-helpers.js#L648-L679 , so I think we can consider
NotificationShade
andStatusBar
as part ofisShowingLockscreen
.(at least, an Android 7.1 real device had
mShowingLockscreen
. Haven't checked with android 8 and 9.)