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

zarubond · 2018-10-09T07:27:31Z

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

welcome · 2018-10-09T07:27:34Z

💖 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.

alexeykuzmin · 2018-10-09T09:01:10Z

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

codebytere · 2018-10-11T17:12:04Z

atom/browser/api/atom_api_app.cc

+      region = locale.substr(rpos + 1, 2);
+  }
+#endif
+  if (region.size() == 2)


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

codebytere · 2018-10-11T17:17:00Z

atom/browser/api/atom_api_app.cc

+      static_cast<CFTypeRef>(CFLocaleGetValue(locale, kCFLocaleCountryCode)));
+  const CFIndex kCStringSize = 128;
+  char temporaryCString[kCStringSize];
+  bzero(temporaryCString, kCStringSize);


bzero() is legacy; prefer memset()

or just char temporaryCString[kCStringSize] = {}

codebytere · 2018-10-11T17:18:56Z

docs/api/app.md

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


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

codebytere · 2018-10-11T17:21:17Z

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

codebytere · 2018-10-12T16:50:37Z

atom/browser/api/atom_api_app.cc

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


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

codebytere · 2018-10-12T16:51:36Z

docs/api/app.md

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


region => locale country code

codebytere · 2018-10-12T16:51:52Z

docs/api/app.md

+### `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.


same as above ☝️

vhashimotoo · 2018-10-16T08:42:59Z

docs/api/app.md

@@ -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()`


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

codebytere · 2018-10-17T05:14:04Z

@zarubond spec is failing on
AssertionError: expected 'US' to be one of [ 0, 2 ]
you just need to correct this and we should be good to go!

zarubond · 2018-10-17T15:10:51Z

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

MarshallOfSound · 2018-10-19T00:36:53Z

@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 · 2018-10-22T14:53:16Z

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

atom/browser/api/atom_api_app.cc

docs/api/app.md

alexeykuzmin · 2018-11-05T23:18:05Z

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

spec/api-app-spec.js

alexeykuzmin

^

spec/api-app-spec.js

alexeykuzmin · 2018-11-08T15:33:18Z

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

spec/api-app-spec.js

docs/api/app.md

alexeykuzmin

Looks good to me 👍

release-clerk · 2018-11-20T20:33:26Z

Release Notes Persisted

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

welcome · 2018-11-20T20:33:27Z

Congrats on merging your first pull request! 🎉🎉🎉

…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

zarubond requested review from a team October 9, 2018 07:27

zarubond force-pushed the master branch from 1239db2 to 6aa9abe Compare October 9, 2018 07:40

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

zarubond force-pushed the master branch 2 times, most recently from 27c16d8 to a08e050 Compare October 9, 2018 16:10

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

Add method to get system´s user region

031ffca

zarubond force-pushed the master branch from a08e050 to 031ffca Compare October 9, 2018 16:21

zarubond added 2 commits October 10, 2018 09:15

Merge remote-tracking branch 'upstream/master'

b68133b

Fix linter

72cff16

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 2 commits October 10, 2018 14:23

Remove auto types

6db9f19

Improved detection for POSIX

7e0eeda

codebytere reviewed Oct 11, 2018

View reviewed changes

iAmWillShepherd mentioned this pull request Oct 12, 2018

Format the ahead/behind count in a locale-aware manner desktop/desktop#1245

Open

Change name, add specs, minor fixes

843379f

zarubond force-pushed the master branch from 38e78c0 to 843379f Compare October 12, 2018 16:34

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

codebytere reviewed Oct 12, 2018

View reviewed changes

Remove left overs

75b5ae5

vhashimotoo reviewed Oct 16, 2018

View reviewed changes

codebytere approved these changes Oct 17, 2018

View reviewed changes

Fix locale test

95441a0

Fix Linux test

5244367

MarshallOfSound added the semver/minor backwards-compatible functionality label Oct 22, 2018

zarubond added 2 commits October 23, 2018 09:34

Merge remote-tracking branch 'upstream/master'

a6d099d

Merge remote-tracking branch 'upstream/master'

dce962e

miniak requested changes Nov 4, 2018

View reviewed changes

atom/browser/api/atom_api_app.cc Show resolved Hide resolved

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

zarubond added 2 commits November 5, 2018 15:43

Merge remote-tracking branch 'upstream/master'

bf1a43b

Coding style fixes

e8921ee

miniak approved these changes Nov 5, 2018

View reviewed changes

alexeykuzmin suggested changes Nov 5, 2018

View reviewed changes

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

docs/api/app.md Show resolved Hide resolved

Fix docs

5357685

alexeykuzmin reviewed Nov 6, 2018

View reviewed changes

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

alexeykuzmin suggested changes Nov 8, 2018

View reviewed changes

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

Add test excaption for Linux

914ad31

fix spelling

bdc3561

alexeykuzmin reviewed Nov 8, 2018

View reviewed changes

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

alexeykuzmin reviewed Nov 8, 2018

View reviewed changes

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

Polishing

0ba6802

alexeykuzmin approved these changes Nov 9, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master'

79c0e06

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zarubond commented Oct 9, 2018 •

edited by miniak

welcome bot commented Oct 9, 2018

alexeykuzmin commented Oct 9, 2018

codebytere Oct 11, 2018 •

edited

codebytere Oct 11, 2018

miniak Nov 4, 2018

codebytere Oct 11, 2018

codebytere commented Oct 11, 2018 •

edited

codebytere Oct 12, 2018

codebytere Oct 12, 2018

codebytere Oct 12, 2018

vhashimotoo Oct 16, 2018

codebytere commented Oct 17, 2018

zarubond commented Oct 17, 2018

MarshallOfSound commented Oct 19, 2018 •

edited

zarubond commented Oct 22, 2018

alexeykuzmin commented Nov 5, 2018

alexeykuzmin left a comment

alexeykuzmin commented Nov 8, 2018

alexeykuzmin left a comment

release-clerk bot commented Nov 20, 2018

welcome bot commented Nov 20, 2018

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

		@@ -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()`

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

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

Conversation

zarubond commented Oct 9, 2018 • edited by miniak

Description of Change

Checklist

Release Notes

welcome bot commented Oct 9, 2018

alexeykuzmin commented Oct 9, 2018

codebytere Oct 11, 2018 • edited

Choose a reason for hiding this comment

codebytere Oct 11, 2018

Choose a reason for hiding this comment

miniak Nov 4, 2018

Choose a reason for hiding this comment

codebytere Oct 11, 2018

Choose a reason for hiding this comment

codebytere commented Oct 11, 2018 • edited

codebytere Oct 12, 2018

Choose a reason for hiding this comment

codebytere Oct 12, 2018

Choose a reason for hiding this comment

codebytere Oct 12, 2018

Choose a reason for hiding this comment

vhashimotoo Oct 16, 2018

Choose a reason for hiding this comment

codebytere commented Oct 17, 2018

zarubond commented Oct 17, 2018

MarshallOfSound commented Oct 19, 2018 • edited

zarubond commented Oct 22, 2018

alexeykuzmin commented Nov 5, 2018

alexeykuzmin left a comment

Choose a reason for hiding this comment

alexeykuzmin commented Nov 8, 2018

alexeykuzmin left a comment

Choose a reason for hiding this comment

release-clerk bot commented Nov 20, 2018

welcome bot commented Nov 20, 2018

zarubond commented Oct 9, 2018 •

edited by miniak

codebytere Oct 11, 2018 •

edited

codebytere commented Oct 11, 2018 •

edited

MarshallOfSound commented Oct 19, 2018 •

edited