Conversation
There was a problem hiding this comment.
Since this is a client, we try to be convenient :) Instead of an 'options' object, which can have any keys, and putting the key names in the comments, can we have a method that just accepts 4 arguments? Since two are optional, we can have two methods, one calling the other.
There was a problem hiding this comment.
Are appWaitPkg and appWaitAct dependent on the other? IE can you have just
one of them?
On Friday, September 5, 2014, Jonah notifications@github.com wrote:
In src/main/java/io/appium/java_client/AppiumDriver.java:
@@ -168,6 +169,17 @@ public String currentActivity() {
Response response = execute(CURRENT_ACTIVITY);
return response.getValue().toString();
}
+
- /**
- * Launches an arbitrary activity during a test. If the activity belongs to
- * another application, that application is started and the activity is opened.
- * This is an Android only method.
- * @param options 'appPackage' and 'appActivity' are required;
- * 'appWaitPackage' and 'appWaitActivity' are optional
- */
- public void startActivity(Map<String, String> options){
Since this is a client, we try to be convenient :) Instead of an 'options'
object, which can have any keys, and putting the key names in the comments,
can we have a method that just accepts 4 arguments? Since two are optional,
we can have two methods, one calling the other.—
Reply to this email directly or view it on GitHub
https://github.com/appium/java-client/pull/101/files#r17204294.
There was a problem hiding this comment.
Yes, you can have just one of them.
There was a problem hiding this comment.
In that case, we can't have overloads to handle all the cases. That's why I
went with the map. We can have:
startAct(appPkg, appAct)
startAct(appPkg, appAct, appWaitPkg OR -Act) <= can't have overloads for
both cases, won't compile
startAct(appPkg, appAct, appWaitPkg, appWaitAct)
I say we either just go for signature #3, in which case users will have to
pass null for whichever of the last 2 they don't care about, or we stick
with the hash map.
On Saturday, September 6, 2014, Jonathan Lipps notifications@github.com
wrote:
In src/main/java/io/appium/java_client/AppiumDriver.java:
@@ -168,6 +169,17 @@ public String currentActivity() {
Response response = execute(CURRENT_ACTIVITY);
return response.getValue().toString();
}
+
- /**
- * Launches an arbitrary activity during a test. If the activity belongs to
- * another application, that application is started and the activity is opened.
- * This is an Android only method.
- * @param options 'appPackage' and 'appActivity' are required;
- * 'appWaitPackage' and 'appWaitActivity' are optional
- */
- public void startActivity(Map<String, String> options){
Yes, you can have just one of them.
—
Reply to this email directly or view it on GitHub
https://github.com/appium/java-client/pull/101/files#r17211060.
047bf83 to
3aa020c
Compare
|
Is the java-client.iml change intentional? |
cf3f639 to
967c24b
Compare
|
K, hopefully it's fixed. |
There was a problem hiding this comment.
I don't think you need this? The IDE has an option called optimize imports which will resolve any problems.
There was a problem hiding this comment.
spacing on this still seems wrong
|
looks good to me. 👍 |
|
Tests don't pass, is that expected? Or am I running it wrong? |
|
They pass for me, so no, it's not expected. On Mon, Sep 8, 2014 at 7:06 PM, Jonah notifications@github.com wrote:
|
|
@Jonahss did you run reset on master? |
|
Hmmm. thought I did. Re-running |
|
I pulled @0x1mason's fork of appium and checked out the 2969 branch. Ran |
|
Are you running just that test or the whole suite? I'm running the test individually. |
|
Just the individual test. Could be a race condition. Maybe my emulator On 9/8/14, Jonah notifications@github.com wrote:
|
|
Nope, don't see it open. I added in a 20second wait and I still don't see it actually open O.o |
|
I see the problem. You need 2969 branch of appium-adb. On 9/8/14, Jonah notifications@github.com wrote:
|
|
wasn't that branch merged into master? |
|
Looks like it. Something's broken how the adb cmd line is On 9/8/14, bootstraponline notifications@github.com wrote:
|
|
Oh yeah, that is pretty undefined. Wonder why no errors were thrown. |
|
Dunno. That shouldn't happen if your appium-adb branch is at HEAD or On 9/8/14, Jonah notifications@github.com wrote:
|
|
Is a version of appium-adb with the appropriate commit included in appium's submodules/package.json? If not, I think that needed to be part of the appium PR to keep this in sync. If so, I leave the investigation to you guys :-)= |
|
@Jonahss try a fresh clone of appium into a new directory and then invoke reset.sh. |
|
@jlipps: appium-adb commit is probably wrong in appium branch 2969. On Tue, Sep 9, 2014 at 12:36 PM, bootstraponline notifications@github.com
|
|
What's holding back the merge of 2969 into appium master? That can happen prior to the java-client being updated, can't it? |
|
I guess it could, but the docs have examples from the client libraries, so On Tue, Sep 9, 2014 at 2:28 PM, Jonah notifications@github.com wrote:
|
|
Oh I see. On Tue, Sep 9, 2014 at 11:58 AM, Eric Millin notifications@github.com
|
I'm pretty sure that's the best way. It's also how we've done things so far. The clients are expected to pass against appium master when they add the new method. |
|
In this case though, I could just merge the PR because the client is On Tue, Sep 9, 2014 at 1:30 PM, bootstraponline notifications@github.com
|
|
I guess, as long as you're okay with testing against a faulty setup. |
|
@Jonahss You could also just pull the most recent version of appium-adb and run the tests. |
|
hmm. Now I get this error: |
|
I think merging into master before the clients is a good idea. Just waiting on the package.json update and conflict resolution. |
|
My bad, still hadn't gotten the right version of appium-adb. tests pass locally, merging. |
There was a problem hiding this comment.
whoops, accidental bug which escaped our code review. yay for tests.
changed to !_isNotNullOrEmpty
Related to appium/appium #2969. Added startActivity api that maps to new appium feature.