-
Notifications
You must be signed in to change notification settings - Fork 395
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: Add the necessary primitives to be able to automate split-screen apps #209
Conversation
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.
Looks 👍
@@ -48,6 +50,7 @@ + (NSArray *)routes | |||
[[FBRoute POST:@"/wda/apps/activate"] respondWithTarget:self action:@selector(handleSessionAppActivate:)], | |||
[[FBRoute POST:@"/wda/apps/terminate"] respondWithTarget:self action:@selector(handleSessionAppTerminate:)], | |||
[[FBRoute POST:@"/wda/apps/state"] respondWithTarget:self action:@selector(handleSessionAppState:)], | |||
[[FBRoute POST:@"/wda/apps/list"] respondWithTarget:self action:@selector(handleActiveAppsList:)], |
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.
GET
?
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 would keep POST, since we might want to add some additional filtering arguments to the call in the future. GET request does not allow this though
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.
👍 make sense
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.
This should be still get and we should pass the further filtering with query params
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.
unfortunately selenium API does not really support passing of query parameters. Although, this endpoint is going to be put under mobile:
call , so I could do that
@@ -162,6 +165,11 @@ + (NSArray *)routes | |||
return FBResponseWithObject(@(state)); | |||
} | |||
|
|||
+ (id<FBResponsePayload>)handleActiveAppsList:(FBRouteRequest *)request | |||
{ | |||
return FBResponseWithObject([XCUIApplication fb_activeAppsInfo]); |
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.
WDA has an endpoint to return appinfo, bundleid, process id, name and environment variables. It has not been released yet. So, what do you think of modifying here like below and remove it?
+ (id<FBResponsePayload>)handleActiveAppsList:(FBRouteRequest *)request
{
NSMutableArray<NSDictionary<NSString *, id> *> *result = [[NSMutableArray alloc] init];
NSArray *appsInfo = [XCUIApplication fb_activeAppsInfo];
for (NSDictionary *appInfo in appsInfo) {
FBApplication *app = [FBApplication fb_activeApplicationWithDefaultBundleId:appInfo[@"bundleId"]];
NSDictionary *processArguments = @{};
if (app != nil) {
processArguments = @{
@"args": app.launchArguments,
@"env": app.launchEnvironment
};
}
[result addObject:@{
@"pid": appInfo[@"pid"] ?: @"",
@"bundleId": appInfo[@"bundleId"] ?: @"",
@"name": app.identifier ?: @"",
@"processArguments": processArguments,
}];
}
return FBResponseWithObject(result);
}
If ^ sounds good, I'll drop https://github.com/appium/appium/pull/13028/files for 1.15.0 release
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 wanted to reduce the /list
endpoint to a minimum and avoid extra calls to the accessibility if possible. Creation of FBApplication instance is already quite expensive as it touches the internal process monitoring machinery. Lets keep it at minimum for now.
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.
👍 make sense
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.
lgtm. I also locally tested
@@ -48,6 +50,7 @@ + (NSArray *)routes | |||
[[FBRoute POST:@"/wda/apps/activate"] respondWithTarget:self action:@selector(handleSessionAppActivate:)], | |||
[[FBRoute POST:@"/wda/apps/terminate"] respondWithTarget:self action:@selector(handleSessionAppTerminate:)], | |||
[[FBRoute POST:@"/wda/apps/state"] respondWithTarget:self action:@selector(handleSessionAppState:)], | |||
[[FBRoute POST:@"/wda/apps/list"] respondWithTarget:self action:@selector(handleActiveAppsList:)], |
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.
👍 make sense
@@ -162,6 +165,11 @@ + (NSArray *)routes | |||
return FBResponseWithObject(@(state)); | |||
} | |||
|
|||
+ (id<FBResponsePayload>)handleActiveAppsList:(FBRouteRequest *)request | |||
{ | |||
return FBResponseWithObject([XCUIApplication fb_activeAppsInfo]); |
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.
👍 make sense
@@ -9,18 +9,7 @@ | |||
|
|||
#import "FBSpringboardApplication.h" | |||
|
|||
#import "FBErrorBuilder.h" |
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.
👍 deletions
@@ -69,6 +69,23 @@ NS_ASSUME_NONNULL_BEGIN | |||
*/ | |||
- (BOOL)fb_waitForAppElement:(NSTimeInterval)timeout; | |||
|
|||
/** | |||
Retrievs the infomration about the applications the given accessiblity elements |
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.
Good typos :D
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.
:-P
dispatch_semaphore_signal(sem); | ||
}]; | ||
dispatch_semaphore_wait(sem, dispatch_time(DISPATCH_TIME_NOW, (int64_t)(1 * NSEC_PER_SEC))); | ||
appInfo[@"bundleId"] = bundleId ?: @""; |
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 set it to Unknown instead of empty?
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 have any strong opinion on that. Will set to "unknown"
{ | ||
NSArray<XCAccessibilityElement *> *activeApplicationElements = [FBXCAXClientProxy.sharedClient activeApplications]; | ||
XCAccessibilityElement *activeApplicationElement = [activeApplicationElements firstObject]; | ||
XCAccessibilityElement *activeApplicationElement = nil; | ||
if (activeApplicationElements.count > 1) { |
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.
Shouldn't this be now bigger than 0 instead of 1?
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.
it is fine. If it equals to 1 then the first array element is set in else
block. If the length of the array is zero then firstObject will return nil, which will lead to an exception later
WebDriverAgentLib/FBApplication.m
Outdated
NSArray<NSDictionary *> *appsInfo = [self fb_appsInfoWithAxElements:activeApplicationElements]; | ||
NSUInteger index = 0; | ||
for (NSDictionary *appInfo in appsInfo) { | ||
index++; |
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'm a bit confused here. When we are iterating over the appsInfo and we find an appInfo where bundleIds are equal, we will then get index + 1 object in the activeApplicationElements. Is this intended?
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.
good catch, we need to increment the index after the condition
The change is NOT breaking.
I've added the new setting called
defaultActiveApplication
. By default it is set toauto
and only comes to play if there is more than one active application on the screen. WDA will check if this value is set to any bundle identifier, that matches with the currently active application. It will consider this particular application as active rather than one, which is located under the default screen point. If the application with the given bundle identifier is not in the list of active apps then the default algorithm is applied. The client can list the currently active applications at any time (their PIDs and bundle ids) using the newly addedapps/list
endpoint.