Pass full locale list with script and variant codes to framework #6557

GaryQian · 2018-10-16T18:05:02Z

To properly support locale resolution, we now pass the full set of locales defined on the user's device instead of just the first one.

Full integration is pending #6481. Currently, the script and variant codes are not passed into the Locale object yet, which will be done once Locale supports it.

GaryQian · 2018-10-16T18:40:17Z

Will update with working Android-side code.

HansMuller

Just looked over the dart changes, which seem fine. Just a few suggestions to prove that I read the code :-).

HansMuller · 2018-10-16T19:28:58Z

lib/ui/window.dart

  /// This establishes the language and formatting conventions that application
  /// should, if possible, use to render their user interface.
  ///
+  /// This is the first locale selected by the user and should be the user's


Nit: "should be the user's primary locale" begs some questions:

What's the primary locale exactly?

When would it not be the primary locale?

Including a link to a public document that covered this kind of thing would be helpful.

HansMuller · 2018-10-16T19:29:47Z

lib/ui/window.dart

+  /// This is the first locale selected by the user and should be the user's
+  /// primary locale.
+  /// 
+  /// This is equivalent to `locales[0]` and will provide an empty non-null locale


locales.first

HansMuller · 2018-10-16T19:30:20Z

lib/ui/window.dart

+    if (_locales != null && _locales.length != 0) {
+      return _locales[0];
+    }
+    return new Locale("", "");


It might be better to define a Locale.none constant and use that here.

HansMuller · 2018-10-16T19:31:32Z

lib/ui/window.dart

+  /// This is equivalent to `locales[0]` and will provide an empty non-null locale
+  /// if the [locales] list has not been set or is empty.
+  Locale get locale {
+    if (_locales != null && _locales.length != 0) {


_locales.isNotEmpty

HansMuller · 2018-10-16T19:32:07Z

lib/ui/window.dart

+  /// should, if possible, use to render their user interface.
+  ///
+  /// The list is ordered in order of priority, with lower-indexed locales being
+  /// preferred over higher-indexed ones. The zeroth element is the default [locale].


zeroth - first

Earlier, locales[0] was referred to as the "primary" locale.

HansMuller · 2018-10-16T19:35:38Z

lib/ui/window.dart

+  /// if the [locales] list has not been set or is empty.
+  Locale get locale {
+    if (_locales != null && _locales.length != 0) {
+      return _locales[0];


_locales.first

lib/ui/window.dart

GaryQian · 2018-10-17T00:30:11Z

This is part of the fix for flutter/flutter#22642

flutter/engine@04c860f...8cc49ca git log 04c860f..8cc49ca --no-merges --oneline 8cc49ca Ensure that the isolate shutdown callback occurs in an isolate scope. (flutter/engine#6572) 04a1ffa Roll src/third_party/skia cb65ce7f77c9..3a0c66da5fc2 (10 commits) (flutter/engine#6571) 35340ce Pass full locale list with script and variant codes to framework (flutter/engine#6557) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary.

And update goldens. git log 04c860f..8cc49ca --no-merges --oneline 8cc49ca Ensure that the isolate shutdown callback occurs in an isolate scope. (flutter/engine#6572) 04a1ffa Roll src/third_party/skia cb65ce7f77c9..3a0c66da5fc2 (10 commits) (flutter/engine#6571) 35340ce Pass full locale list with script and variant codes to framework (flutter/engine#6557)

flutter/engine@04c860f...705738f git log 04c860f..705738f --no-merges --oneline 705738f Roll src/third_party/skia 3a0c66da5fc2..928db927f7fc (5 commits) (flutter/engine#6573) 8cc49ca Ensure that the isolate shutdown callback occurs in an isolate scope. (flutter/engine#6572) 04a1ffa Roll src/third_party/skia cb65ce7f77c9..3a0c66da5fc2 (10 commits) (flutter/engine#6571) 35340ce Pass full locale list with script and variant codes to framework (flutter/engine#6557) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary.

flutter/engine@04c860f...979de07 git log 04c860f..979de07 --no-merges --oneline 979de07 Add a CountDownLatch to fml with tests. (flutter/engine#6574) 705738f Roll src/third_party/skia 3a0c66da5fc2..928db927f7fc (5 commits) (flutter/engine#6573) 8cc49ca Ensure that the isolate shutdown callback occurs in an isolate scope. (flutter/engine#6572) 04a1ffa Roll src/third_party/skia cb65ce7f77c9..3a0c66da5fc2 (10 commits) (flutter/engine#6571) 35340ce Pass full locale list with script and variant codes to framework (flutter/engine#6557) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary.

flutter/engine@04c860f...607c05c git log 04c860f..607c05c --no-merges --oneline 607c05c Run all supported host unit tests on Cirrus. (flutter/engine#6575) 979de07 Add a CountDownLatch to fml with tests. (flutter/engine#6574) 705738f Roll src/third_party/skia 3a0c66da5fc2..928db927f7fc (5 commits) (flutter/engine#6573) 8cc49ca Ensure that the isolate shutdown callback occurs in an isolate scope. (flutter/engine#6572) 04a1ffa Roll src/third_party/skia cb65ce7f77c9..3a0c66da5fc2 (10 commits) (flutter/engine#6571) 35340ce Pass full locale list with script and variant codes to framework (flutter/engine#6557) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary.

And update goldens. git log 04c860f..8cc49ca --no-merges --oneline 8cc49ca Ensure that the isolate shutdown callback occurs in an isolate scope. (flutter/engine#6572) 04a1ffa Roll src/third_party/skia cb65ce7f77c9..3a0c66da5fc2 (10 commits) (flutter/engine#6571) 35340ce Pass full locale list with script and variant codes to framework (flutter/engine#6557)

Hixie · 2018-10-18T17:47:26Z

lib/ui/window.dart

  const Locale(this._languageCode, [ this._countryCode ]) : assert(_languageCode != null);

+  /// Empty locale constant. This is an invalid locale.
+  static const Locale none = const Locale('', '');


why not null?

or just assert that it's never empty -- can we ever have no locale at all?

There seems to be a large string of calls that do not expect the returned locale to ever be null. I can look into changing it to do null checking though, since that is probably a more robust design in the long run.

This mostly applies to when the locale is accessed before the platform has had a chance to pass it over on startup. The empty locale would just result in the resolution to use the default (first) locale.

miDeb · 2018-10-18T18:35:49Z

It seems as setLocales(https://github.com/GaryQian/engine/blob/982dc4f3f3fbd69ccaed1862e6cc20ab6cf21e39/shell/platform/android/io/flutter/view/FlutterView.java#L348) lets my app crash:

java.lang.NoSuchMethodError: java.util.Locale.getScript
        at io.flutter.view.FlutterView.setLocales(FlutterView.java:348)
        at io.flutter.view.FlutterView.<init>(FlutterView.java:175)
        at io.flutter.app.FlutterActivityDelegate.onCreate(FlutterActivityDelegate.java:157)
        at io.flutter.app.FlutterActivity.onCreate(FlutterActivity.java:85)
        at com.example.flutterlpcc.MainActivity.onCreate(MainActivity.java:10)
        at android.app.Activity.performCreate(Activity.java:5442)
        at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1094)
        at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2393)
        at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2493)
        at android.app.ActivityThread.access$800(ActivityThread.java:166)
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1283)
        at android.os.Handler.dispatchMessage(Handler.java:102)
        at android.os.Looper.loop(Looper.java:136)
        at android.app.ActivityThread.main(ActivityThread.java:5584)
        at java.lang.reflect.Method.invokeNative(Native Method)
        at java.lang.reflect.Method.invoke(Method.java:515)
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1268)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1084)
        at dalvik.system.NativeStart.main(Native Method)

My device is API 19 and Locale.getScript was introduced in API 21, so it will probably be because of this.

GaryQian · 2018-10-18T18:43:57Z

Ahh good catch, my bad, I'll fix that asap.

tvolkert · 2018-10-19T14:44:38Z

This looks like it might have triggered flutter/flutter#23241 (see original fix in #4495)

/cc @yjbanov @zanderso

GaryQian added 4 commits October 11, 2018 17:19

Locale Passing

6683dbf

Merge remote-tracking branch 'upstream/master' into locale

ded705a

Merge branch 'master' of github.com:flutter/engine into locale

d00a356

Pass full locale list and script and variant codes to framework

d6a1ab7

GaryQian requested review from HansMuller, cbracken and chinmaygarde October 16, 2018 18:05

googlebot added the cla: yes label Oct 16, 2018

HansMuller reviewed Oct 16, 2018

View reviewed changes

Working Android locale list passing and fallback

82b892f

GaryQian force-pushed the locale branch from 2243ed2 to 82b892f Compare October 16, 2018 22:23

Merge branch 'master' of github.com:flutter/engine into locale

af15f05

GaryQian force-pushed the locale branch from c97bc78 to 419c9c2 Compare October 17, 2018 00:25

Fix tests

a5390e4

GaryQian force-pushed the locale branch from 419c9c2 to a5390e4 Compare October 17, 2018 00:28

Merge branch 'master' of github.com:flutter/engine into locale

982dc4f

GaryQian requested a review from jason-simmons October 17, 2018 17:18

jason-simmons approved these changes Oct 17, 2018

View reviewed changes

GaryQian merged commit 35340ce into flutter:master Oct 17, 2018

engine-flutter-autoroll mentioned this pull request Oct 17, 2018

Roll engine 04c860fa1dba..8cc49cac3b4a (3 commits) flutter/flutter#23222

Closed

liyuqian mentioned this pull request Oct 17, 2018

Roll engine 04c860fa1dba..8cc49cac3b4a (3 commits) flutter/flutter#23223

Merged

engine-flutter-autoroll mentioned this pull request Oct 18, 2018

Roll engine 04c860fa1dba..705738fd78ff (4 commits) flutter/flutter#23227

Closed

engine-flutter-autoroll mentioned this pull request Oct 18, 2018

Roll engine 04c860fa1dba..979de076b335 (5 commits) flutter/flutter#23229

Closed

engine-flutter-autoroll mentioned this pull request Oct 18, 2018

Roll engine 04c860fa1dba..607c05c93d18 (6 commits) flutter/flutter#23230

Closed

Hixie reviewed Oct 18, 2018

View reviewed changes

tvolkert mentioned this pull request Oct 19, 2018

io.Platform.localeName is '_' on iOS simulator flutter/flutter#23241

Closed

Pass full locale list with script and variant codes to framework #6557

Pass full locale list with script and variant codes to framework #6557

Uh oh!

Conversation

GaryQian commented Oct 16, 2018

Uh oh!

GaryQian commented Oct 16, 2018

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GaryQian commented Oct 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miDeb commented Oct 18, 2018

Uh oh!

GaryQian commented Oct 18, 2018

Uh oh!

tvolkert commented Oct 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants