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

Conversation

Projects
None yet
6 participants
@zarubond
Copy link
Contributor

commented Oct 9, 2018

Description of Change

On desktop operating systems the users can set different language and locale region. This allows the system to have en-CZ locale even through there is no czech region associated with english (the example results in English language with Czech data/number format/currency). However Chrome only presents region in locale based on system language.

This patch adds the API returning the OS region which can be then used for globalization. With the knowledge of region the applications would be able to show proper localization as any other native app.

Checklist
  • PR description included and stakeholders cc'd
  • relevant documentation is changed or added
  • PR title follows semantic commit guidelines
Release Notes

Notes: feat: provide user system's region with app.getLocaleCountryCode()

@zarubond zarubond requested review from as code owners Oct 9, 2018

@welcome

This comment has been minimized.

Copy link

commented Oct 9, 2018

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@zarubond zarubond force-pushed the zarubond:master branch from 1239db2 to 6aa9abe Oct 9, 2018

@zarubond zarubond changed the title [wip][feat] Add app.getUserLocale() method for locale detection [wip] feat: Add app.getUserLocale() method for locale detection Oct 9, 2018

@alexeykuzmin

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2018

@zarubond can you please rebase your branch onto the latest electron:master? A few CI fixes landed there in the last several days.

@zarubond zarubond force-pushed the zarubond:master branch 2 times, most recently from 27c16d8 to a08e050 Oct 9, 2018

@zarubond zarubond changed the title [wip] feat: Add app.getUserLocale() method for locale detection [wip] feat: Add app.getRegion() method for locale detection Oct 9, 2018

@zarubond zarubond force-pushed the zarubond:master branch from a08e050 to 031ffca Oct 9, 2018

zarubond added some commits Oct 10, 2018

@zarubond zarubond changed the title [wip] feat: Add app.getRegion() method for locale detection feat: Add app.getRegion() method for locale detection Oct 10, 2018

zarubond added some commits Oct 10, 2018

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();

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] = {}

@@ -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_

@codebytere

This comment has been minimized.

Copy link
Member

commented Oct 11, 2018

@zarubond could you please add spec(s) for this, as well as release notes?

@zarubond zarubond force-pushed the zarubond:master branch from 38e78c0 to 843379f Oct 12, 2018

@zarubond zarubond changed the title feat: Add app.getRegion() method for locale detection feat: Add app.getLocaleCountryCode() method for region detection Oct 12, 2018

@@ -1299,7 +1338,8 @@ void App::BuildPrototype(v8::Isolate* isolate,
.SetMethod("setPath", &App::SetPath)
.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

@@ -580,6 +580,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 region in ISO3166 [here](https://www.iso.org/iso-3166-country-codes.html). The value is taken from native OS APIs.

This comment has been minimized.

Copy link
@codebytere

codebytere Oct 12, 2018

Member

region => locale country code

### `app.GetLocaleCountryCode()`
Returns `String` - User operating system´s region in ISO3166 [here](https://www.iso.org/iso-3166-country-codes.html). The value is taken from native 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 ☝️

@@ -580,6 +580,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()`

This comment has been minimized.

Copy link
@HashimotoYT

HashimotoYT Oct 16, 2018

Member

app.GetLocaleCountryCode() => app.getLocaleCountryCode()

@zarubond

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

@codebytere Would it be possible to get output of "locale" command on Linux testing machine?

@MarshallOfSound

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

@zarubond This is the output of locale on the linux build machine

builduser@0495b2eff748:~$ locale
LANG=
LANGUAGE=
LC_CTYPE="POSIX"
LC_NUMERIC="POSIX"
LC_TIME="POSIX"
LC_COLLATE="POSIX"
LC_MONETARY="POSIX"
LC_MESSAGES="POSIX"
LC_PAPER="POSIX"
LC_NAME="POSIX"
LC_ADDRESS="POSIX"
LC_TELEPHONE="POSIX"
LC_MEASUREMENT="POSIX"
LC_IDENTIFICATION="POSIX"
LC_ALL=

Doesn't look too good 😆

@zarubond

This comment has been minimized.

Copy link
Contributor Author

commented Oct 22, 2018

Thanks @MarshallOfSound , so the output of the code was correct.

Show resolved Hide resolved atom/browser/api/atom_api_app.cc
Show resolved Hide resolved atom/browser/api/atom_api_app.cc Outdated
@miniak

miniak approved these changes Nov 5, 2018

Show resolved Hide resolved docs/api/app.md Outdated
Show resolved Hide resolved docs/api/app.md
@alexeykuzmin

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2018

@zarubond
None of commits in the PR are associated with your GitHub account.
Can you please change the author of the commits or add that new email to your GitHub account?

Show resolved Hide resolved spec/api-app-spec.js Outdated
@alexeykuzmin
Copy link
Contributor

left a comment

^

Show resolved Hide resolved spec/api-app-spec.js Outdated
@alexeykuzmin

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

@zarubond It should be isCI not isCi, sorry for confusion.

Show resolved Hide resolved spec/api-app-spec.js Outdated
Show resolved Hide resolved docs/api/app.md Outdated
@alexeykuzmin
Copy link
Contributor

left a comment

Looks good to me 👍

@codebytere codebytere merged commit de05ff8 into electron:master Nov 20, 2018

21 of 23 checks passed

ci/circleci: mas-testing-tests Your tests failed on CircleCI
Details
Artifact Comparison Changes Detected
Details
Absolute Zero
Semantic Pull Request ready to be squashed
Details
WIP ready for review
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: linux-arm-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-checkout Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: mas-testing Your tests passed on CircleCI!
Details
ci/circleci: osx-testing Your tests passed on CircleCI!
Details
ci/circleci: osx-testing-tests Your tests passed on CircleCI!
Details
electron-lint Build #20181109.13 succeeded
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Nov 20, 2018

Release Notes Persisted

feat: provide user system's region with app.getLocaleCountryCode()

@welcome

This comment has been minimized.

Copy link

commented Nov 20, 2018

Congrats on merging your first pull request! 🎉🎉🎉

bcpete added a commit to bcpete/electron that referenced this pull request Apr 18, 2019

feat: Add app.getLocaleCountryCode() method for region detection (ele…
…ctron#15035)

* Add method to get system´s user region

* Fix linter

* Remove auto types

* Improved detection for POSIX

* Change name, add specs, minor fixes

* Remove left overs

* Fix locale test

* Fix Linux test

* Coding style fixes

* Fix docs

* Add test excaption for Linux

* fix spelling

* Polishing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.