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 app.getLocaleCountryCode() method for region detection #15035

Merged
merged 18 commits into from Nov 20, 2018
@@ -60,6 +60,7 @@
#endif

#if defined(OS_MACOSX)
#include <CoreFoundation/CoreFoundation.h>
#include "atom/browser/ui/cocoa/atom_bundle_mover.h"
#endif

@@ -874,6 +875,50 @@ std::string App::GetLocale() {
return g_browser_process->GetApplicationLocale();
}

std::string App::GetRegion() {
std::string region;
#if defined(OS_WIN)
WCHAR locale_name[LOCALE_NAME_MAX_LENGTH] = {0};

if (GetLocaleInfoEx(LOCALE_NAME_USER_DEFAULT, LOCALE_SISO3166CTRYNAME,
(LPWSTR)&locale_name,
sizeof(locale_name) / sizeof(WCHAR)) ||
GetLocaleInfoEx(LOCALE_NAME_SYSTEM_DEFAULT, LOCALE_SISO3166CTRYNAME,
(LPWSTR)&locale_name,
sizeof(locale_name) / sizeof(WCHAR)))
base::WideToUTF8(locale_name, wcslen(locale_name), &region);
This conversation was marked as resolved by zarubond

This comment has been minimized.

Copy link
@miniak

miniak Nov 4, 2018

Contributor

even though not mandatory, please put the body in {} block, it's a bit confusing now considering the condition is 6 lines of code

#elif defined(OS_MACOSX)
CFLocaleRef locale = CFLocaleCopyCurrent();
CFStringRef value = CFStringRef(
static_cast<CFTypeRef>(CFLocaleGetValue(locale, kCFLocaleCountryCode)));
const CFIndex kCStringSize = 128;
char temporaryCString[kCStringSize];
bzero(temporaryCString, kCStringSize);

This comment has been minimized.

Copy link
@codebytere

codebytere Oct 11, 2018

Member

bzero() is legacy; prefer memset()

This comment has been minimized.

Copy link
@miniak

miniak Nov 4, 2018

Contributor

or just char temporaryCString[kCStringSize] = {}

CFStringGetCString(value, temporaryCString, kCStringSize,
kCFStringEncodingUTF8);
region = temporaryCString;
#else
const char* locale_ptr = setlocale(LC_TIME, NULL);
if (locale_ptr == NULL)
This conversation was marked as resolved by zarubond

This comment has been minimized.

Copy link
@miniak

miniak Nov 4, 2018

Contributor

if (!locale_ptr)

locale_ptr = setlocale(LC_NUMERIC, NULL);

if (locale_ptr) {
std::string locale = locale_ptr;
std::string::size_type rpos = locale.rfind('.');
if (rpos != std::string::npos)
locale = locale.substr(0, rpos);

rpos = locale.rfind('_');
if (rpos != std::string::npos && rpos + 1 < locale.size())
region = locale.substr(rpos + 1, 2);
}
#endif
if (region.size() == 2)

This comment has been minimized.

Copy link
@codebytere

codebytere Oct 11, 2018

Member

nit: this is probably cleaner as a ternary:
return region.size() == 2 ? region : std::string();

return region;

return std::string();
}

void App::OnSecondInstance(const base::CommandLine::StringVector& cmd,
const base::FilePath& cwd) {
Emit("second-instance", cmd, cwd);
@@ -1300,6 +1345,7 @@ void App::BuildPrototype(v8::Isolate* isolate,
.SetMethod("getPath", &App::GetPath)
.SetMethod("setDesktopName", &App::SetDesktopName)
.SetMethod("getLocale", &App::GetLocale)
.SetMethod("getRegion", &App::GetRegion)

This comment has been minimized.

Copy link
@codebytere

codebytere Oct 12, 2018

Member

getRegion should be removed now i believe since you changed the name

#if defined(USE_NSS_CERTS)
.SetMethod("importCertificate", &App::ImportCertificate)
#endif
@@ -182,6 +182,7 @@ class App : public AtomBrowserClient::Delegate,

void SetDesktopName(const std::string& desktop_name);
std::string GetLocale();
std::string GetRegion();
void OnSecondInstance(const base::CommandLine::StringVector& cmd,
const base::FilePath& cwd);
bool HasSingleInstanceLock() const;
@@ -580,6 +580,10 @@ To set the locale, you'll want to use a command line switch at app startup, whic

**Note:** On Windows you have to call it after the `ready` events gets emitted.

### `app.getRegion()` _macOS_ _Linux_ _Windows_

This comment has been minimized.

Copy link
@codebytere

codebytere Oct 11, 2018

Member

For functions that work across platforms, no need to specify all of _macOS_ _Linux_ _Windows_

Returns `String` - User operating system region in ISO3166 [here](https://www.iso.org/iso-3166-country-codes.html). The value is taken from OS apis.
**Note:** When unable to detect region, it returns empty string.

This comment has been minimized.

Copy link
@codebytere

codebytere Oct 12, 2018

Member

same as above ☝️


### `app.addRecentDocument(path)` _macOS_ _Windows_

* `path` String
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.