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

feat: consider NotificationShade and StatusBar as lock screen for newer android versions #564

Merged
merged 5 commits into from
Mar 18, 2021

Conversation

KazuCocoa
Copy link
Member

@KazuCocoa KazuCocoa commented Mar 17, 2021

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 or StatusBar after keycode 62. But current our logic does not consider such case as locked screen. So, our lock 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 and StatusBar as part of isShowingLockscreen.

(at least, an Android 7.1 real device had mShowingLockscreen. Haven't checked with android 8 and 9.)

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);
Copy link
Contributor

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?

Copy link
Contributor

@mykola-mokhnach mykola-mokhnach Mar 18, 2021

Choose a reason for hiding this comment

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

for example .+\sNotificationShade\b

Copy link
Member Author

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.

Copy link
Contributor

@mykola-mokhnach mykola-mokhnach Mar 18, 2021

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

@mykola-mokhnach
Copy link
Contributor

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

@KazuCocoa
Copy link
Member Author

Yea, i tried some stuff like power. But for example adb shell dumpsys power | grep 'mHolding' was both true in both lock/unlcok in devices when the device's screen was one. ( I checked this behavior with a few nexus devices and a samsung device)
NotificationShade and StatusBar are the most reasonable way to consider possible lock/unlock (at least after power on button)...

@mykola-mokhnach
Copy link
Contributor

mykola-mokhnach commented Mar 18, 2021

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:

  1. adb shell dumpsys nfc | echo FAIL
  2. if FAIL is printed then fall back to the current method
  3. Otherwise check if the output contains mScreenState=\w+UNLOCKED

@KazuCocoa
Copy link
Member Author

KazuCocoa commented Mar 18, 2021

The nfc command returned outputs but it did not have mScreenState in my checked devices before..

Oh, I just found Screen State: ON_UNLOCKED. No mScreenState tho.

adb shell dumpsys nfc | grep 'Screen State'
Screen State: ON_UNLOCKED

It was ON_LOCKED or OFF_LOCKED when that was locked.
No unlocked word in the nfc output, so potentially we can check only ON_UNLOCKED or _UNLOCKED match.

Let me check more devices

@mykola-mokhnach
Copy link
Contributor

@mykola-mokhnach
Copy link
Contributor

@KazuCocoa
Copy link
Member Author

KazuCocoa commented Mar 18, 2021

Thanks, I dug in dumpsys focusing on keyguard. Then, it seems the below condition is the most reliable.
mIsShowing=false && mInputRestricted=false

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.

  • unlocked
    KeyguardServiceDelegate
      ....
      KeyguardStateMonitor
        mIsShowing=false
        mSimSecure=false
        mInputRestricted=false
        mCurrentUserId=0
        ...
  • locked
    KeyguardServiceDelegate
      ....
      KeyguardStateMonitor
        mIsShowing=true
        mSimSecure=false
        mInputRestricted=true
        mCurrentUserId=0
        ...

lib/helpers.js Outdated Show resolved Hide resolved
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));
Copy link
Contributor

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

@KazuCocoa KazuCocoa merged commit 5835ce8 into appium:master Mar 18, 2021
@KazuCocoa KazuCocoa deleted the lock-status-for-new-versions branch March 18, 2021 17:06
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.

None yet

2 participants