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

fix: defaults broken in shell.openExternal() options #38038

Merged
merged 1 commit into from Apr 21, 2023

Conversation

miniak
Copy link
Contributor

@miniak miniak commented Apr 20, 2023

Description of Change

When calling shell.openExternal('https://example.com/'); it parses activate as true correctly.
However calling shell.openExternal('https://example.com/', {}); would parse activate as false.

Related to:

// Differences from the Get method in gin::Dictionary:
// 1. This is a const method;
// 2. It checks whether the key exists before reading;
// 3. It accepts arbitrary type of key.
template <typename K, typename V>
bool Get(const K& key, V* out) const {
// Check for existence before getting, otherwise this method will always
// returns true when T == v8::Local<v8::Value>.
v8::Local<v8::Context> context = isolate()->GetCurrentContext();
v8::Local<v8::Value> v8_key = gin::ConvertToV8(isolate(), key);
v8::Local<v8::Value> value;
v8::Maybe<bool> result = GetHandle()->Has(context, v8_key);
if (result.IsJust() && result.FromJust() &&
GetHandle()->Get(context, v8_key).ToLocal(&value))
return gin::ConvertFromV8(isolate(), value, out);
return false;
}

Checklist

Release Notes

Notes: Fixed broken defaults in shell.openExternal() options.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Apr 20, 2023
@miniak miniak self-assigned this Apr 20, 2023
@miniak miniak added semver/minor backwards-compatible functionality target/22-x-y PR should also be added to the "22-x-y" branch. target/23-x-y PR should also be added to the "23-x-y" branch. target/24-x-y PR should also be added to the "24-x-y" branch. target/25-x-y PR should also be added to the "25-x-y" branch. labels Apr 20, 2023
@miniak miniak added semver/patch backwards-compatible bug fixes and removed semver/minor backwards-compatible functionality labels Apr 20, 2023
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Apr 21, 2023
@jkleinsc jkleinsc merged commit 964934c into main Apr 21, 2023
24 checks passed
@jkleinsc jkleinsc deleted the miniak/fix-shell-open-external-options branch April 21, 2023 17:14
@release-clerk
Copy link

release-clerk bot commented Apr 21, 2023

Release Notes Persisted

Fixed broken defaults in shell.openExternal() options.

@trop
Copy link
Contributor

trop bot commented Apr 21, 2023

I was unable to backport this PR to "23-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot removed the target/23-x-y PR should also be added to the "23-x-y" branch. label Apr 21, 2023
@trop
Copy link
Contributor

trop bot commented Apr 21, 2023

I was unable to backport this PR to "22-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot added needs-manual-bp/23-x-y and removed target/22-x-y PR should also be added to the "22-x-y" branch. labels Apr 21, 2023
@trop
Copy link
Contributor

trop bot commented Apr 21, 2023

I have automatically backported this PR to "25-x-y", please check out #38071

@trop
Copy link
Contributor

trop bot commented Apr 21, 2023

I have automatically backported this PR to "24-x-y", please check out #38072

@trop trop bot added in-flight/24-x-y merged/25-x-y PR was merged to the "25-x-y" branch. merged/24-x-y PR was merged to the "24-x-y" branch and removed target/25-x-y PR should also be added to the "25-x-y" branch. target/24-x-y PR should also be added to the "24-x-y" branch. in-flight/25-x-y in-flight/24-x-y labels Apr 21, 2023
miniak added a commit that referenced this pull request Apr 23, 2023
Co-authored-by: Milan Burda <miburda@microsoft.com>
miniak added a commit that referenced this pull request Apr 23, 2023
Co-authored-by: Milan Burda <miburda@microsoft.com>
@trop
Copy link
Contributor

trop bot commented Apr 23, 2023

@miniak has manually backported this PR to "23-x-y", please check out #38091

@trop
Copy link
Contributor

trop bot commented Apr 23, 2023

@miniak has manually backported this PR to "22-x-y", please check out #38092

codebytere pushed a commit that referenced this pull request Apr 23, 2023
fix: defaults broken in shell.openExternal() options (#38038)

Co-authored-by: Milan Burda <miburda@microsoft.com>
@trop trop bot added merged/23-x-y PR was merged to the "23-x-y" branch. and removed in-flight/22-x-y labels Apr 23, 2023
codebytere pushed a commit that referenced this pull request Apr 24, 2023
fix: defaults broken in shell.openExternal() options (#38038)

Co-authored-by: Milan Burda <miburda@microsoft.com>
@trop trop bot added the merged/22-x-y PR was merged to the "22-x-y" branch. label Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/22-x-y PR was merged to the "22-x-y" branch. merged/23-x-y PR was merged to the "23-x-y" branch. merged/24-x-y PR was merged to the "24-x-y" branch merged/25-x-y PR was merged to the "25-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants