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: Add an option to control intent wait for a started URI #389

Merged
merged 5 commits into from
May 2, 2020

Conversation

shibupanda
Copy link
Contributor

@shibupanda shibupanda commented Apr 27, 2020

Added condition to handle waitForLaunch capability to call concerned startUrl function.
Which will fix below issue.
appium/appium#14180
It's depends on below PR
appium/appium-adb#520

return await this.adb.startUri(url, pkg);
const {
url,
package: pkg
Copy link
Contributor

Choose a reason for hiding this comment

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

Please destruct waitForAppLaunch explicitly from opts and add a doctring to this method

Copy link
Contributor Author

@shibupanda shibupanda Apr 28, 2020

Choose a reason for hiding this comment

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

Is it something like below?
commands.mobileDeepLink = async function (opts = {}) {
const {
url,
package: pkg,
waitForLaunch: wait,
} = opts;
return await this.adb.startUri(url, pkg, wait);
}; //This I checked didn't work because startUri expect object

So below may work
return await this.adb.startUri(url, pkg, { wait });

Copy link
Contributor

Choose a reason for hiding this comment

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

something like

const {
  url,
  package,
  waitForLaunch,
} = opts;
return await this.adb.startUri(url, package, {waitForLaunch});

Copy link
Contributor Author

@shibupanda shibupanda Apr 28, 2020

Choose a reason for hiding this comment

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

Yes this also worked for me. Let me do it.
package is a reserved word so I will use the same "package: pkg" in this case.
And doc string is not there in any commands you want me to add that too?

Copy link
Contributor

@mykola-mokhnach mykola-mokhnach Apr 28, 2020

Choose a reason for hiding this comment

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

And doc string is not there in any commands you want me to add that too?

yes, I know. But we want to make it better, don't we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very true @mykola-mokhnach.

@mykola-mokhnach mykola-mokhnach changed the title fix: Encountered internal error running command: Error: Error attempting to start URI feat: Add an option to control intent wait for a started URI Apr 28, 2020
@mykola-mokhnach
Copy link
Contributor

@shibupanda Do you have time to add the docstring for the mobileDeepLink method?

@shibupanda
Copy link
Contributor Author

shibupanda commented Apr 30, 2020

@mykola-mokhnach Sorry for the delay I stuck some where so will add it in this weekend or I can create a different pull request for it sure.

@mykola-mokhnach
Copy link
Contributor

np, lets do it on the weekend

@@ -234,7 +234,18 @@ commands.mobileViewportScreenshot = async function () {
commands.setUrl = async function (url) {
await this.adb.startUri(url, this.opts.appPackage);
};
/**
* @typedef {object} DeepLinkOpts
* @property {?string} url - The name of URL to start.
Copy link
Contributor

Choose a reason for hiding this comment

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

the type would be {!string} since this property is required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! Yes.

/**
* @typedef {object} DeepLinkOpts
* @property {?string} url - The name of URL to start.
* @property {?string} package - The name of the package to start the URI with.
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@shibupanda
Copy link
Contributor Author

This needs to be update here I guess.

@mykola-mokhnach mykola-mokhnach merged commit 9ac4478 into appium:master May 2, 2020
@shibupanda
Copy link
Contributor Author

This needs to be update here I guess.

Shall I proposed for the changes?

@mykola-mokhnach
Copy link
Contributor

simply create a PR

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

3 participants