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
@@ -65,6 +65,7 @@
#endif

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

@@ -882,6 +883,45 @@ std::string App::GetLocale() {
return g_browser_process->GetApplicationLocale();
}

std::string App::GetLocaleCountryCode() {
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] = {0};
CFStringGetCString(value, temporaryCString, kCStringSize,
kCFStringEncodingUTF8);
region = temporaryCString;
#else
const char* locale_ptr = setlocale(LC_TIME, NULL);
if (!locale_ptr)
locale_ptr = setlocale(LC_NUMERIC, NULL);
if (locale_ptr) {
std::string locale = locale_ptr;
std::string::size_type rpos = locale.find('.');
if (rpos != std::string::npos)
locale = locale.substr(0, rpos);
rpos = locale.find('_');
if (rpos != std::string::npos && rpos + 1 < locale.size())
region = locale.substr(rpos + 1);
}
#endif
return region.size() == 2 ? region : std::string();
}

void App::OnSecondInstance(const base::CommandLine::StringVector& cmd,
const base::FilePath& cwd) {
Emit("second-instance", cmd, cwd);
@@ -1308,6 +1348,7 @@ void App::BuildPrototype(v8::Isolate* isolate,
.SetMethod("getPath", &App::GetPath)
.SetMethod("setDesktopName", &App::SetDesktopName)
.SetMethod("getLocale", &App::GetLocale)
.SetMethod("getLocaleCountryCode", &App::GetLocaleCountryCode)
#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 GetLocaleCountryCode();
void OnSecondInstance(const base::CommandLine::StringVector& cmd,
const base::FilePath& cwd);
bool HasSingleInstanceLock() const;
@@ -603,6 +603,11 @@ 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.getLocaleCountryCode()`
Returns `string` - User operating system´s locale country code in ISO3166 [here](https://www.iso.org/iso-3166-country-codes.html). The value is taken from native OS APIs.
This conversation was marked as resolved by zarubond

This comment has been minimized.

Copy link
@alexeykuzmin

alexeykuzmin Nov 8, 2018

Contributor

Please make "ISO3166" a link and remove the "here" word. The text looks weird now.

Suggested change
Returns `string` - User operating system´s locale country code in ISO3166 [here](https://www.iso.org/iso-3166-country-codes.html). The value is taken from native OS APIs.
Returns `string` - User operating system's locale two-letter [ISO 3166](https://www.iso.org/iso-3166-country-codes.html) country code. The value is taken from native OS APIs.

**Note:** When unable to detect locale country code, it returns empty string.
This conversation was marked as resolved by zarubond

This comment has been minimized.

Copy link
@alexeykuzmin

alexeykuzmin Nov 5, 2018

Contributor

Is it possible to return null?

This comment has been minimized.

Copy link
@zarubond

zarubond Nov 6, 2018

Author Contributor

There is no way how to do it from C++


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

* `path` String
@@ -122,6 +122,12 @@ describe('app module', () => {
})
})

describe('app.getLocaleCountryCode()', () => {
it('should be empty or have length of two', () => {
expect(app.getLocaleCountryCode().length).to.be.oneOf([0, 2])
This conversation was marked as resolved by zarubond

This comment has been minimized.

Copy link
@alexeykuzmin

alexeykuzmin Nov 6, 2018

Contributor

So if the function will suddenly start returning an empty string all the time, the test won't catch it?
Is there a way to predict its return value? Does it depend on a platform?

This comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound Nov 6, 2018

Member

@alexeykuzmin Linux CI machines have no locale

Maybe just skip this test on CI on linux?

This comment has been minimized.

Copy link
@alexeykuzmin

alexeykuzmin Nov 8, 2018

Contributor

@zarubond
Can you please set the expected length to 0 only for the linux CI? Like this:

let expectedLength = 2
if (isCi && process.platform === 'linux') {
  // Linux CI machines have no locale.
  expectedLength = 0
}
expect(app.getLocaleCountryCode()).to.be.a('string')
    .and.have.lengthOf(expectedLength)

This comment has been minimized.

Copy link
@zarubond

zarubond Nov 8, 2018

Author Contributor

Added the exception for Linux.

})
})

describe('app.isPackaged', () => {
it('should be false durings tests', () => {
expect(app.isPackaged).to.be.false()
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.