New Simplified Localization Workflow #43934

shihaohong · 2019-10-31T22:05:47Z

Description

Builds on #41432 to simplify and improve the i18n process. This assumes that the developer has the necessary .arb files.

It can be test run on https://github.com/shihaohong/i18n_exploration.
Steps:

Be on the root directory of the i18n_exploration project.
Run dart ${FLUTTER_DIR}/dev/tools/localization/gen_l10n.dart.

In its current state, this proposal should generate the following:

app_localizations.dart:

AppLocalizations.localizationsDelegate - Helper that returns list containing the demo localizations' delegates, along with the other default delegates needed for the app (GlobalMaterialLocalizations.delegate, GlobalCupertinoLocalizations.delegate and GlobalWidgetsLocalizations.delegate)

MaterialApp(
  localizationsDelegates: AppLocalizations.localizationsDelegates,
  // ...
),

AppLocalizations.supportedLocales - Helper that returns a list of all the supported locales:

MaterialApp(
  supportedLocales: AppLocalizations.supportedLocales,
  // ...
),

messages_en.dart, messages_es.dart, messages_zh_CN.dart, and messages_zh_TW.dart, which provide the messages for their respective locales
messages_all.dart, which looks up the desired locale's translations.

Specs

Related Issues

#41437

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing ~~and new~~ tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

… and output class prefix

HansMuller

This looks good. Mostly just some suggestions about error detection.

HansMuller · 2019-11-01T23:26:07Z

dev/tools/localization/gen_l10n.dart

+  parser.addOption('dir-path', defaultsTo: path.join('lib', 'l10n'));
+  parser.addOption('input-arb-file', defaultsTo: 'app_en.arb');
+  parser.addOption('output-file-prefix', defaultsTo: 'app');
+  parser.addOption('output-class-prefix', defaultsTo: 'App');


Might be better to just make this 'output-class' and default to 'AppLocalizations'. No reason to require developers to use 'Localizations' as a class name suffix.

HansMuller · 2019-11-01T23:36:00Z

dev/tools/localization/gen_l10n.dart

+
+Future<void> main(List<String> args) async {
+  final argslib.ArgParser parser = argslib.ArgParser();
+  parser.addOption('dir-path', defaultsTo: path.join('lib', 'l10n'));


Most flutter commands options that name a directory use "-dir" as a suffix. Maybe just 'arb-dir' here.

HansMuller · 2019-11-01T23:38:00Z

dev/tools/localization/gen_l10n.dart

+  final File inputArbFile = File(path.join(l10nDirectory.path, results['input-arb-file']));
+  final File outputFile = File(path.join(l10nDirectory.path, '${results['output-file-prefix']}_localizations.dart'));
+  final String stringsClassName = '${results['output-class-prefix']}Localizations';
+


This would be a good place to check everything:

The l10nDirectory exists and supports read/write.

The inputArbFile is readable

The outputFile is writable or doesn't exist.

The stringClassName is a legit Dart class name.

I managed to add checks for the following:

If l10nDirectory exists

The stringClassName is a legit Dart class name (check for first character being uppercase alphabet and for non alphanumeric characters in the string.

I'm not quite sure how to check if a file is readable/writable. Is there an example I can reference?

It looks like you can use the FileSytemEntity stat method to check the value of the FileSystemStat's mode string.

Done! Let me know if it looks good, I'm not terribly familiar with the stat() method and file system permissions, so let me know if there's something we should change here.

HansMuller · 2019-11-01T23:39:00Z

dev/tools/localization/gen_l10n.dart

+    final String entityPath = entity.path;
+
+    if (FileSystemEntity.isFileSync(entityPath)) {
+      // TODO: what if there are multiple files with the exact same locale?


NICE. If we detect as much, we might as well abort, since it's likely to be a mistake.

I added a check here to abort if there is two arb files with the same locale

HansMuller · 2019-11-01T23:47:54Z

dev/tools/localization/gen_l10n.dart

+Future<void> main(List<String> args) async {
+  final argslib.ArgParser parser = argslib.ArgParser();
+  parser.addOption('dir-path', defaultsTo: path.join('lib', 'l10n'));
+  parser.addOption('input-arb-file', defaultsTo: 'app_en.arb');


It's a little (just a little) odd to call this the input arb file, since they're all input files. Maybe we should call this one template-arb-file?

HansMuller · 2019-11-01T23:53:50Z

dev/tools/localization/gen_l10n.dart

+      final RegExp arbFilenameRE = RegExp(r'(\w+)\.arb$');
+      if (arbFilenameRE.hasMatch(entityPath)) {
+        final File arbFile = File(entityPath);
+        final Map<String, dynamic> arbContents = json.decode(arbFile.readAsStringSync());


Presumably this can fail. If it does we should probably abort.

HansMuller · 2019-11-01T23:59:45Z

dev/tools/localization/gen_l10n.dart

+      if (arbFilenameRE.hasMatch(entityPath)) {
+        final File arbFile = File(entityPath);
+        final Map<String, dynamic> arbContents = json.decode(arbFile.readAsStringSync());
+        final RegExp arbFilenameLocaleRE = RegExp(r'^[^_]*_(\w+)\.arb$');


I'd suggest only RE matching the filename if that's needed:

String localeString = arbContents['@@locale']; if (localeString == null) { final RegExp arbFilenameLocaleRE = RegExp(r'^[^_]*_(\w+)\.arb$'); localeString = arbFilenameLocaleRE.firstMatch(entityPath)[1]; }

HansMuller · 2019-11-02T00:03:14Z

dev/tools/localization/gen_l10n.dart

+
+        if (arbFilenameLocaleRE.hasMatch(entityPath) && localeString == null)
+          localeString = arbFilenameLocaleRE.firstMatch(entityPath)[1];
+


If localeString is null at this point then we found an arb file but couldn't determine what locale it was for. That seems bad; probably abort here.

HansMuller · 2019-11-02T00:04:02Z

dev/tools/localization/gen_l10n.dart

+  parser.addOption('input-arb-file', defaultsTo: 'app_en.arb');
+  parser.addOption('output-file-prefix', defaultsTo: 'app');
+  parser.addOption('output-class-prefix', defaultsTo: 'App');
+  final argslib.ArgResults results = parser.parse(args);


Eventually we should add support for a --verbose option.

HansMuller · 2019-11-02T00:08:31Z

dev/tools/localization/gen_l10n.dart

+/// supportedLocales list. For example:
+///
+/// ```
+/// return MaterialApp(


Maybe show the import here as well (assuming that the app is defined in lib/).

HansMuller · 2019-11-04T17:01:08Z

dev/tools/localization/gen_l10n.dart

+}
+''';
+
+const String simpleMethodTemplate = '''


We could map no-parameter lookups to a getter. Would need a getterMethodTemplate .. and some other changes :-).

fluttergithubbot · 2019-11-04T18:05:28Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

HansMuller

LGTM; just needs a few small changes

HansMuller · 2019-11-04T18:25:03Z

dev/tools/localization/gen_l10n.dart

+      'Please ensure that the user has read permissions.'
+    );
+  final String outputFileStatModeString = outputFile.statSync().modeString();
+  if (outputFileStatModeString[1] == '-')


It's OK if the file doesn't exist (we're generating it for the first time). It looks like we would fail if the output file doesn't exist yet.

Right, that makes sense. I realized this a few minutes ago and removed this check

HansMuller · 2019-11-04T18:26:17Z

dev/tools/localization/gen_l10n.dart

+  final String l10nDirectoryStatModeString = l10nDirectory.statSync().modeString();
+  if (!_isDirectoryReadableAndWritable(l10nDirectoryStatModeString))
+    exitWithError(
+      "The 'arb-dir' directory, $l10nDirectory, is not readable or writable.\n"


is not readable or writable => doesn't allow reading and writing

HansMuller · 2019-11-04T18:28:38Z

dev/tools/localization/gen_l10n.dart

+  final argslib.ArgParser parser = argslib.ArgParser();
+  parser.addOption('arb-dir', defaultsTo: path.join('lib', 'l10n'));
+  parser.addOption('template-arb-file', defaultsTo: 'app_en.arb');
+  parser.addOption('output-localization-file', defaultsTo: 'app_localizations');


It might be less confusing if this defaulted to app_localizations.dart; if we didn't add the .dart suffix to the output file parameter

…#43934) *Starting with arb files in lib/l10n, the tool creates the following by default: 1. An AppLocalizations class containing: - an `AppLocalizationsDelegate` - a `supportedLocales` property that returns a list of all supported locales. - a `localizationsDelegate` property that returns a list containing this delegate along with `GlobalMaterialLocalizations.delegate`, `GlobalCupertinoLocalizations.delegate`, and `GlobalWidgetsLocalizations.delegate`. 2. One Dart `message_<locale>.dart` file for each arb file, as well as a `messages_all.dart` file that performs the locale message lookup. The tool infers the locale of each arb file from the @@Locale property or the arb file's name.

shihaohong added 2 commits October 31, 2019 10:24

Incorporate Hans' draft from #41432

ecb262c

Add localizationsDelegates and supportedLocales helper

53fd883

fluttergithubbot added c: contributor-productivity Team-specific productivity, code health, technical debt. work in progress; do not review labels Oct 31, 2019

googlebot added the cla: yes label Oct 31, 2019

shihaohong added a: internationalization Supporting other languages or locales. (aka i18n) framework flutter/packages/flutter repository. See also f: labels. labels Oct 31, 2019

shihaohong added 9 commits October 31, 2019 15:07

Add end-of-file newline

5d61b4b

Add arguments for directory path, input arb file, output file prefix,…

d73cd6c

… and output class prefix

Account for country code

3470333

Remove print statement

a2ea9ca

Prefer @@Locale over filename

2ed00d4

reorder variables

ff09156

Add iOS Info.plist instructions

d76ec3c

Remove accidental semicolon

3238787

Fix typo

66119db

HansMuller reviewed Nov 2, 2019

View reviewed changes

Incorporate code review feedback

3c43dea

HansMuller reviewed Nov 4, 2019

View reviewed changes

shihaohong added 4 commits November 4, 2019 09:09

Merge branch 'master' of github.com:flutter/flutter into l10n-s12n

73c6d50

Merge branch 'l10n-s12n' of github.com:shihaohong/flutter into l10n-s12n

771a8e3

Add additional file system checks, fix analyzer issues

4ce0b98

Create getter method template

cb767f9

shihaohong removed the work in progress; do not review label Nov 4, 2019

shihaohong marked this pull request as ready for review November 4, 2019 18:05

HansMuller approved these changes Nov 4, 2019

View reviewed changes

shihaohong added 2 commits November 4, 2019 10:49

Lint cleanup, remove output file writable check

559f9d9

Code review feedback

f82ead1

shihaohong added the waiting for tree to go green label Nov 4, 2019

shihaohong mentioned this pull request Nov 4, 2019

Proposal for a Simplified I18N Process #41437

Closed

shihaohong added 2 commits November 4, 2019 15:01

Proper escaping for translated messages

ae299dd

Add matching resource attribute checker and getter/method name validator

b9c9958

shihaohong merged commit ede637a into flutter:master Nov 5, 2019

shihaohong deleted the l10n-s12n branch November 5, 2019 02:27

HansMuller mentioned this pull request Nov 8, 2019

Simplified I18N Process for Flutter Applications #41432

Closed

shihaohong changed the title ~~L10n Simplification~~ New Simplified Localization Workflow Feb 27, 2020

github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021


		if (arbFilenameLocaleRE.hasMatch(entityPath) && localeString == null)
		localeString = arbFilenameLocaleRE.firstMatch(entityPath)[1];

New Simplified Localization Workflow #43934

New Simplified Localization Workflow #43934

Uh oh!

Conversation

shihaohong commented Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Specs

Related Issues

Checklist

Breaking Change

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!

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!

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!

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!

fluttergithubbot commented Nov 4, 2019

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!

Reviewers

Assignees

shihaohong commented Oct 31, 2019 •

edited

Loading