-
Notifications
You must be signed in to change notification settings - Fork 1k
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
chore(cli): Autodetect Android Studio path on Windows #1633
Conversation
Excuse me, It would not be better to check first if the current platform is Windows before executing the retrieval of the path and
|
I guess this could work too. In that case The current code always sets Current version:
|
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.
Proposed two changes.
Other than that works fine and looks good.
cli/src/config.ts
Outdated
@@ -193,7 +194,17 @@ export class Config implements CliConfig { | |||
} | |||
|
|||
private initWindowsConfig() { | |||
this.windows.androidStudioPath = this.app.windowsAndroidStudioPath && this.app.windowsAndroidStudioPath; | |||
if (this.app.windowsAndroidStudioPath) { | |||
if (!existsSync(this.app.windowsAndroidStudioPath) && this.cli.os === OS.Windows) { |
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 you put the this.cli.os === OS.Windows
first? in that case it won't try to see if the file exists in non windows. Or even as @Genarito proposed as it doesn't matter if it's not set in other platforms different from windows.
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.
An alternative is to switch the comparison inside the if
If the first operand is false, JavaScript will short-circuit and not evaluate the second condition.
if (this.cli.os === OS.Windows && !existsSync(this.app.windowsAndroidStudioPath)) {
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.
Yeah, that's what I meant when I said first, in the same if, but it's also ok to just do nothing if it's not windows as Genarito suggested.
cli/src/config.ts
Outdated
this.windows.androidStudioPath = this.app.windowsAndroidStudioPath && this.app.windowsAndroidStudioPath; | ||
if (this.app.windowsAndroidStudioPath) { | ||
if (!existsSync(this.app.windowsAndroidStudioPath) && this.cli.os === OS.Windows) { | ||
const buffer = execSync('REG QUERY "HKEY_LOCAL_MACHINE\\SOFTWARE\\Android Studio" /v Path'); |
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 you put all this in a try catch?
If Android Studio is not installed it shows an ugly message because it doesn't find the registry.
In the catch you can set the path to empty string so it can be used later in the open command to not open if it's empty. I'll do that in a separate PR once this is merged.
Add commits from upstream
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.
nice, thanks!
Auto Detect Android Studio path on Windows by reading a Windows registry entry.
First checks if path in
windowsAndroidStudioPath
exist.If not it tries to read the Windows registry entry
HKEY_LOCAL_MACHINE\\SOFTWARE\\Android Studio
Issues #560 and #230
Based on getASPath.bat
https://github.com/apache/cordova-android/blob/master/bin/templates/cordova/lib/getASPath.bat