-
-
Notifications
You must be signed in to change notification settings - Fork 758
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 handlers for lock/unlock in iOS #799
Conversation
@@ -27,11 +29,30 @@ | |||
|
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.
@mykola-mokhnach Code LGTM. Have few suggestions though,
-
Can we deprecate both
LocksIOSDevice
andLocksAndroidDevice
. HaveLocksDevice
interface instead. Currently based on this PR both android and iOS both does supports the API except this method in android doesn't accept duration. May be a server fix will make the API work like iOS. -
https://github.com/appium/java-client/blob/master/src/main/java/io/appium/java_client/android/AndroidMobileCommandHelper.java#L182 can be removed since
MobileCommand
will have it. -
isDeviceLocked
as like in this PR makes more sense to me instead ofisLocked
as in android now. So we might need to deprecate that and rename in android to make it consistent.
Your thoughts?
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.
Can we deprecate both LocksIOSDevice and LocksAndroidDevice. Have LocksDevice interface instead. Currently based on this PR both android and iOS both does supports the API except this method in android doesn't accept duration. May be a server fix will make the API work like iOS.
Done
https://github.com/appium/java-client/blob/master/src/main/java/io/appium/java_client/android/AndroidMobileCommandHelper.java#L182 can be removed since MobileCommand will have it.
Done
isDeviceLocked as like in this PR makes more sense to me instead of isLocked as in android now. So we might need to deprecate that and rename in android to make it consistent.
Done
@mykola-mokhnach It is loking good. I will try it on Android soon |
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.
I think it is necessary to replace isLocked
with isDeviceLocked
method.
Also I think that new methods should be covered with iOS tests.
As soon as we are waiting for merge of appium/appium-xcuitest-driver#595 so let this message be somethng like reminder
… into lock # Conflicts: # src/main/java/io/appium/java_client/android/AndroidMobileCommandHelper.java # src/main/java/io/appium/java_client/android/LocksAndroidDevice.java
@TikhomirovSergey I've added integration tests for iOS. The isLocked method has been marked as deprecated. |
Change list
Please do not merge it unless appium/appium-xcuitest-driver#595 is published
Types of changes