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

add app.setLocale() #11469

Merged
merged 11 commits into from Feb 8, 2018

Conversation

Projects
None yet
6 participants
@codebytere
Member

codebytere commented Dec 19, 2017

Closes #5649.

Adds app.setLocale(locale).

/cc @deepak1556

@codebytere codebytere requested review from electron/docs as code owners Dec 19, 2017

@codebytere codebytere changed the title from [WIP] add setLocale to add app.setLocale() Jan 13, 2018

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Jan 13, 2018

LGTM, quick Q. Is this going to blow up if you throw a completely nonsense string at it?

@deepak1556

LGTM, possible to add some tests ?

@deepak1556

This comment has been minimized.

Member

deepak1556 commented Jan 13, 2018

Is this going to blow up if you throw a completely nonsense string at it?

Won't crash, but will be returning empty strings. We can verify if the given locale is loaded by checking the result of ui::ResourceBundle::ReloadLocaleResources and if its empty, fallback to default locale. @codebytere thoughts ?

@codebytere

This comment has been minimized.

Member

codebytere commented Jan 13, 2018

@deepak1556 i'd agree with that; it's probably the most logical behavior from a user's perspective too. i'll add tests this morning :)

@ckerr

"Are we doing the thing right?" comments inline.

"Are we doing the right thing?" ... Is there a use case for changing the locale in the middle of a session, or is it enough to specify at startup what locale to use? If the latter, it might be simpler to pass a nonempty "preferred locale" string in our calls to l10n_util::GetApplicationLocale()

} else {
rb.ReloadLocaleResources(fallback_locale);
}
return;

This comment has been minimized.

@ckerr

ckerr Jan 18, 2018

Member

You don't ever need a return; at the end of a void function

@@ -850,6 +852,25 @@ void App::SetDesktopName(const std::string& desktop_name) {
#endif
}
void App::SetLocale(std::string locale) {
std::string fallback_locale = g_browser_process->GetApplicationLocale();

This comment has been minimized.

@ckerr

ckerr Jan 18, 2018

Member

const std::string

void App::SetLocale(std::string locale) {
std::string fallback_locale = g_browser_process->GetApplicationLocale();
base::ThreadRestrictions::ScopedAllowIO allow_io;

This comment has been minimized.

@ckerr

ckerr Jan 18, 2018

Member

Is this needed for setting locale, or is it a temporary for the HELLO I AM?

If the latter, suggest moving it to right before the printf() and leading with a blank line to emphasize that the whole paragraph is for the WIP and should be removed as a block before merging

This comment has been minimized.

@codebytere

codebytere Jan 18, 2018

Member

it's needed; this thread disallows io so it will fail without it

base::ThreadRestrictions::ScopedAllowIO allow_io;
ResourceBundle& rb = ResourceBundle::GetSharedInstance();
std::string loaded_locale = rb.ReloadLocaleResources(locale);

This comment has been minimized.

@ckerr

ckerr Jan 18, 2018

Member

const std::string

std::string loaded_locale = rb.ReloadLocaleResources(locale);
printf("HELLO I AM %s\n", loaded_locale.c_str());
if (loaded_locale != "") {

This comment has been minimized.

@ckerr

ckerr Jan 18, 2018

Member

This works but is non-idiomatic. You're testing to see if the string is empty, so a more direct way of expressing that: if (loaded_locale.empty())

brightray::BrowserClient::SetApplicationLocale(locale);
g_browser_process->SetApplicationLocale(locale);
brightray::BrowserClient::SetLocaleOverridden();

This comment has been minimized.

@ckerr

ckerr Jan 18, 2018

Member

Is there a use case for calling one of [SetApplicationLocale(), SetLocaleOverridden()] without calling the other?

If they are always called as a pair, maybe move g_locale_is_set = true; into SetApplicationLocale() and skip the new API

This comment has been minimized.

@ckerr

ckerr Feb 8, 2018

Member

I meant to delete this comment before finishing the review, I found the answer to the question myself and revised this comment below in the part beginning "I know I suggested this in BrowserClient..."

@@ -184,6 +184,7 @@ class App : public AtomBrowserClient::Delegate,
const base::FilePath& path);
void SetDesktopName(const std::string& desktop_name);
void SetLocale(std::string locale);

This comment has been minimized.

@ckerr

ckerr Jan 18, 2018

Member

void SetLocale(const std::string& locale);

@@ -21,6 +21,8 @@ class BrowserClient : public content::ContentBrowserClient {
public:
static BrowserClient* Get();
static void SetApplicationLocale(const std::string& locale);
static void SetLocaleOverridden();
static bool IsLocaleOverridden();

This comment has been minimized.

@ckerr

ckerr Jan 18, 2018

Member

I know I suggested this in BrowserClient, but after reading the discussion and the code I think I made the wrong suggestion 😅 --

Locale first gets set automatically in BrowserMainParts::PreCreateThreads, and I think what we really want to know here is "did the user override that by calling app.setLocale()?"

As such, I'd remove these two functions and move the bool into App::SetLocale(), making the flag a field in App.

Because if you look at BrowerClient in isolation and ask yourself "what is this flag for?" you can't tell -- there's really no relation between that flag and the rest of the BrowserClient code

@@ -112,6 +112,18 @@ describe('app module', () => {
})
})
describe('app.setLocale()', () => {
it('shouldnt change locale if an invalid locale is passed', () => {

This comment has been minimized.

@ckerr

ckerr Jan 18, 2018

Member

should not

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Feb 6, 2018

@codebytere I think the locale probably has to be set before the ready event to make web pages have the right locale.

codebytere added some commits Feb 6, 2018

@codebytere

This comment has been minimized.

Member

codebytere commented Feb 8, 2018

This is passing on everything except linux owing to the fact that chromium only respects GTK, parsing from LANGUAGE, LC_ALL, LC_MESSAGES and LANG env vars. To make it work fully cross-platform, we'd need to patch chromium here. What do you all think is the best way forward?

/cc @deepak1556 @zcbenz @ckerr

@ckerr

This comment has been minimized.

Member

ckerr commented Feb 8, 2018

Looks like g_get_charset_name()'s helper guess_category_value() tries a g_getenv() on each of those strings to figure out a key to use for its cache.

So you should be able to use a g_setenv(LANGUAGE, foo, true() to change the return value of subsequent calls to g_get_charset_names()

glib source: https://github.com/GNOME/glib/blob/master/glib/gcharset.c

@ckerr

This comment has been minimized.

Member

ckerr commented Feb 8, 2018

Yep, that did it. Tests passing on my Linux x64 box now. Patch is on its way.

@ckerr

A couple of suggestions inline, but we're in the home stretch

brightray::BrowserClient::SetApplicationLocale(locale);
g_browser_process->SetApplicationLocale(locale);
brightray::BrowserClient::SetLocaleOverridden();

This comment has been minimized.

@ckerr

ckerr Feb 8, 2018

Member

I meant to delete this comment before finishing the review, I found the answer to the question myself and revised this comment below in the part beginning "I know I suggested this in BrowserClient..."

fake_browser_process_->SetApplicationLocale(
l10n_util::GetApplicationLocale(""));
return brightray::BrowserMainParts::PreCreateThreads();
brightray::BrowserClient::Get()->GetApplicationLocale());

This comment has been minimized.

@ckerr

ckerr Feb 8, 2018

Member

I think we should skip this if PreCreateThreads() returns an error, e.g.

const int errorCode = brightray::BrowserMainParts::PreCreateThreads();
if (!errorCode) {
  fake_browser_process_->SetApplicationLocale(
    brightray::BrowserClient::Get()->GetApplicationLocale());
}
return errorCode;
if (cmd_line->HasSwitch(switches::kLang)) {
base::FilePath locale_file_path;
const std::string locale = cmd_line->GetSwitchValueASCII(switches::kLang);
locale_file_path =

This comment has been minimized.

@ckerr

ckerr Feb 8, 2018

Member

(minor) any reason for not declaring locale_file_path at the point of use?

const std::string locale = cmd_line->GetSwitchValueASCII(switches::kLang);
const base::FilePath locale_file_path =
  ui::ResourceBundle::GetSharedInstance().GetLocaleFilePath(locale, true);


ui::ResourceBundle::InitSharedInstanceWithLocale(
locale, nullptr, ui::ResourceBundle::DO_NOT_LOAD_COMMON_RESOURCES);
ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance();
bundle.ReloadLocaleResources(locale);

This comment has been minimized.

@ckerr

ckerr Feb 8, 2018

Member

After creating a shared instance, this code loads the locale, then unloads it, then reloads it.

Better:

  const bool initialized = ui::ResourceBundle::HasSharedInstance();
  if (!initialized)
    ui::ResourceBundle::InitSharedInstanceWithLocale(
        locale, nullptr, ui::ResourceBundle::DO_NOT_LOAD_COMMON_RESOURCES);
  ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance();
  if (initialized)
    bundle.ReloadLocaleResources(locale);

This comment has been minimized.

@deepak1556

deepak1556 Feb 8, 2018

Member

Would it be better to cleanup the shared instance and reinitialize with the new locale if there is already a live instance rather than just ReloadLocaleResources.

const bool initialized = ui::ResourceBundle::HasSharedInstance();
if (initialized)
  ui::ResourceBundle::CleanupSharedInstance();
ui::ResourceBundle::InitSharedInstanceWithLocale(
    locale, nullptr, ui::ResourceBundle::DO_NOT_LOAD_COMMON_RESOURCES);

This comment has been minimized.

@ckerr

ckerr Feb 8, 2018

Member

What is the trade-off here -- what is the benefit in creating a new shared interest?

Not agreeing or disagreeing, just trying to understand the choice

This comment has been minimized.

@deepak1556

deepak1556 Feb 8, 2018

Member

InitSharedInstanceWithLocale seems like a single point of initialization for resources, if chromium adds additional logic in the future we wouldn't have to worry about missing them while ReloadLocaleResources only reloads strings.

  // Changes the locale for an already-initialized ResourceBundle, returning the
  // name of the newly-loaded locale.  Future calls to get strings will return
  // the strings for this new locale.  This has no effect on existing or future
  // image resources.  |locale_resources_data_| is protected by a lock for the
  // duration of the swap, as GetLocalizedString() may be concurrently invoked
  // on another thread.
locale, nullptr, ui::ResourceBundle::DO_NOT_LOAD_COMMON_RESOURCES);
ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance();
bundle.ReloadLocaleResources(locale);
// Load other resource files.
#if defined(OS_MACOSX)
LoadCommonResources();

This comment has been minimized.

@ckerr

ckerr Feb 8, 2018

Member

I don't understand where LoadCommontResources() gets called on non-OS_MACOSX systems

Not new to this PR, but looks like this could be a problem on other plaforms e.g. Windows

edit: ignore this, since this is a separate problem it should be handled separately. I'll file a new issue for it

assert.equal(output, 'en-US')
done()
})
})

This comment has been minimized.

@ckerr

ckerr Feb 8, 2018

Member

these only differ in input and expected result... would it be possible to extract a test_lang(done,lang,expected) {...} method here? Then the tests could read

describe('app.setLocale()', () => {
  it('should set the locale', (done) => test_lang(done, 'fr', 'fr'))
  it('should not set an invalid locale', (done) => test_lang(done, 'asdfkl', 'en-US'))
})
@ckerr

This comment has been minimized.

Member

ckerr commented Feb 8, 2018

continuous-integration/jenkins/pr-head failed on

not ok 13 app module app.exit(exitCode) exits gracefully on macos

surely that's a false negative? 🤔

@ckerr

ckerr approved these changes Feb 8, 2018

🎉

@ckerr ckerr merged commit ca34978 into master Feb 8, 2018

8 checks passed

ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@ckerr ckerr deleted the add-set-locale branch Feb 8, 2018

@@ -530,6 +530,12 @@ Returns `String` - The current application locale. Possible return values are do
**Note:** On Windows you have to call it after the `ready` events gets emitted.
### `app.setLocale(locale)`

This comment has been minimized.

@deepak1556

deepak1556 Feb 8, 2018

Member

This method is not available any longer.

@@ -112,6 +112,25 @@ describe('app module', () => {
})
})
describe('app.setLocale()', () => {

This comment has been minimized.

@deepak1556

deepak1556 Feb 8, 2018

Member

The PR fixes the use of --lang flag, maybe the test should be moved to chromium-spec.js and describe the flag.

sethlu added a commit to sethlu/electron that referenced this pull request May 3, 2018

add app.setLocale() (electron#11469)
* infrastructure for setLocale via klang

* add documentation for setLocale

* add test for setLocale

* fix spec

* add spec and update docs

* fix carriage feeds on windows

* SetLocale() sets LC_ALL on Linux

* in SetLocale() on Linux, use g_setenv()

* fix tyop: '#ifdef OSX_POSIX'

* make the linter happy

* improvements from review
@wuyue92tree

This comment has been minimized.

wuyue92tree commented May 12, 2018

I got a error when call app.setLocale('zh-CN') base on electron 2.0.0

TypeError: __WEBPACK_IMPORTED_MODULE_0_electron__.app.setLocale is not a function

what's wrong with it? Thanks!

@codebytere

This comment has been minimized.

Member

codebytere commented May 13, 2018

@wuyue92tree documentation is incorrect, i'll handle this in the upcoming week!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment