-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Device Orientation Manipulation #133
Conversation
3c33acc
to
e588749
Compare
e588749
to
6d27504
Compare
6d27504
to
78111e4
Compare
detox/ios/Detox/MethodInvocation.m
Outdated
@@ -80,6 +80,24 @@ + (id) getValue:(id)value withType:(id)type onError:(void (^)(NSString*))onError | |||
if (![value isKindOfClass:[NSDictionary class]]) return nil; | |||
return [MethodInvocation invoke:value onError:onError]; | |||
} | |||
if ([type isEqualToString:@"UIDeviceOrientation"]) |
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.
The main advantage of the remote invocation mechanism is that we don't need to add any other new call to the protocol
I believe we didn't make the invocation mechanism easily accessible for extension, and this is our fault.
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.
Does this mean I could have written invoke.IOS.UIDeviceOrientation('UIDeviceOrientationLandscapeLeft')
or what would the JS call need to look like in this case?
detox/src/devices/Device.js
Outdated
@@ -42,6 +43,16 @@ class Device { | |||
await this.client.sendUserNotification(params); | |||
} | |||
|
|||
async setOrientation(orientation) { |
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.
Device.js
is platform agnostic, it's a base class (will be the base class for Android devices as well). This is a simulator specific logic, hence needs to be in Simulator.js
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 see there are no unit tests here, the build will fail on lack of coverage.
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.
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.
Okay, I will move it to Simulator.js
and add some unit tests
detox/src/devices/Device.js
Outdated
@@ -42,6 +43,16 @@ class Device { | |||
await this.client.sendUserNotification(params); | |||
} | |||
|
|||
async setOrientation(orientation) { | |||
// orientation is 'landscape' (meaning left side portrait) or 'portrait' (non-reversed) | |||
const call = invoke.call( |
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.
you can invoke the EarlGrey functions directly instead
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.
So I would do
invoke.IOS.EarlGrey(
'rotateDeviceToOrientation:errorOrNil:',
invoke.IOS.UIDeviceOrientation(orientation)
);
or how would it have to look like?
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.
The ObjC code looks like this:
[EarlGrey rotateDeviceToOrientation:UIDeviceOrientationLandscapeLeft errorOrNil:nil];
JS code pass UIDeviceOrientation
which is an enum, so I guess the easiest way is to pass NSInteger with the position of the desired orientation enum.
Looking at UIDevice.h
, which holds these enums
typedef NS_ENUM(NSInteger, UIDeviceOrientation) {
UIDeviceOrientationUnknown,
UIDeviceOrientationPortrait, // Device oriented vertically, home button on the bottom
UIDeviceOrientationPortraitUpsideDown, // Device oriented vertically, home button on the top
UIDeviceOrientationLandscapeLeft, // Device oriented horizontally, home button on the right
UIDeviceOrientationLandscapeRight, // Device oriented horizontally, home button on the left
UIDeviceOrientationFaceUp, // Device oriented flat, face up
UIDeviceOrientationFaceDown // Device oriented flat, face down
} __TVOS_PROHIBITED;
For UIDeviceOrientationPortrait
pass pass invoke.IOS.NSInteger(1)
For UIDeviceOrientationLandscapeLeft
pass invoke.IOS.NSInteger(3)
I think this should work ...
Of course, for readability, we'd like to keep this enum values in JS as well.
So it would look something like this:
let invocation = invoke.IOS.EarlGrey(
'rotateDeviceToOrientation:errorOrNil:',
invoke.IOS.NSInteger(orientationEnum)
await invocationManager.execute(invocation);
);
I hope it'll work :)
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.
Hey, thanks for the insight, got it working without any native extension (yay 🎉 ). Though the call you describe doesn't work, it leads to an object like
{
type: "EarlGrey",
value: "rotateDeviceToOrientation:errorOrNil:",
id: 5
}
This leads to a "target is invalid" error, which, to me, looks a bit like a "bug", right?
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 spent try my code, it's reasonable I mistaken.
What is the working syntax then?
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.
Sure, I just wanted to state that calling
invoke.IOS.EarlGrey(
'rotateDeviceToOrientation:errorOrNil:',
invoke.IOS.NSInteger(orientationEnum)
);
would be the most readable and easiest way, I will try to make it work, the tests will tell me if I did something wrong then ;)
@@ -28,4 +28,28 @@ describe('Simulator', () => { | |||
await element(by.label('Say Hello')).tap(); | |||
await expect(element(by.label('Hello!!!'))).toBeVisible(); | |||
}); | |||
|
|||
describe('device orientation', () => { |
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.
awesome stuff!
detox/src/devices/Device.js
Outdated
throw new Error(`setOrientation failed: provided orientation ${orientation} is not part of supported orientations: ${Object.keys(orientationMapping)}`) | ||
} | ||
|
||
const call = invoke.EarlGrey.call( |
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.
take a look here
this._call = invoke.call(invoke.EarlGrey.instance, 'detox_selectElementWithMatcher:', matcher._call);
https://github.com/wix/detox/blob/master/detox/src/ios/expect.js#L229
This function calls the static methoddetox_selectElementWithMatcher
function in EarlGrey
detox/src/invoke/EarlGrey.js
Outdated
@@ -3,6 +3,18 @@ const instance = { | |||
value: 'instance' | |||
}; | |||
|
|||
function call(method, ...args) { |
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 don't think we need to add a special call
function here, we are calling EarlGrey's static methods elsewhere without the need for this code.
- take a look at https://github.com/wix/detox/blob/master/detox/src/invoke/Invoke.js#L25
It's a neat trick using Proxy objects that enables this cool syntax:
invoke.IOS.CGPoint({x: 2, y: 1}))
to actually work with no CGPoint
function ever existing
https://github.com/wix/detox/blob/master/detox/src/invoke.test.js#L19
@rotemmiz Can you give me final feedback, this PR is done from my side (at least as far as I can tell 👍 ) |
0e56230
to
8590f44
Compare
This PR adds device orientation change support to the
device
API (see #131).Last thing to do is add some description to the docs, at least from my perspective.
@talkol @rotemmiz Can you give me feedback for the implementation? Is there anything I could do better next time? The docs will follow tomorrow or the day after 👍
MethodInvocation.m
setOrientation
Simulator.js