[material_ui, cupertino_ui] Localizations#12119
Conversation
* Make progress indicators accessible
* add missing entry to material_mr.arb
Updating Flutter fork
…nd to make room for cupertino (#29644)
…e class with date time formatting (#29767)
…n the instructions (#30527)
…t of gen_localizations.dart (#29822)
Updating flutter fork
…nd generate one for cupertino english and french (#29824)
|
@elliette Good catch, when I tested this I didn't use GlobalCupertinoLocalizations. I think the best way to solve this is the following: import 'package:material_ui/material_ui.dart';
// No flutter_localizations needed.
...
localizationsDelegates: const <LocalizationsDelegate<dynamic>>[
...GlobalMaterialLocalizations.delegates,
// No GlobalWidgetsLocalizations needed, because it's included in the above line.
],I'm open to other recommendations but I'll update the description up top with this for now. |
I've added this to my checklist doc and my comms doc.
Yes, I've added this to my checklist doc as well. |
QuncCccccc
left a comment
There was a problem hiding this comment.
I did another round of review and verified that we are able to add new strings to both MaterialLocalizations and CupertinoLocalizations, after fixing the comments below:) This is the test PR I created only for verification justinmc#2
There was a problem hiding this comment.
Since we update the script localization, we should also update the first paragraph from
// This file has been automatically generated. Please do not edit it manually.
// To regenerate the file, use:
// dart packages/material_ui/script/l10n/bin/gen_localizations.dart --overwrite
to
// This file has been automatically generated. Please do not edit it manually.
// To regenerate the file, use:
// dart script/l10n/bin/gen_localizations.dart --overwrite
Sorry, the file shows Diff is too big to render, so I cannot directly suggest changes:).
|
|
||
| // This file has been automatically generated. Please do not edit it manually. | ||
| // To regenerate the file, use: | ||
| // dart packages/material_ui/script/l10n/bin/gen_localizations.dart --overwrite |
There was a problem hiding this comment.
| // dart packages/material_ui/script/l10n/bin/gen_localizations.dart --overwrite | |
| // dart script/l10n/bin/gen_localizations.dart --overwrite |
| // | ||
| // These classes are constructed by the [getCupertinoTranslation] method at the | ||
| // bottom of this file, and used by the [_GlobalCupertinoLocalizationsDelegate.load] | ||
| // method defined in `cupertino_ui/lib/src/global_cupertino_localizations.dart`. |
There was a problem hiding this comment.
| // method defined in `cupertino_ui/lib/src/global_cupertino_localizations.dart`. | |
| // method defined in `packages/cupertino_ui/lib/src/global_cupertino_localizations.dart`. |
When we run gen_localizations.dart, the header will be updated to above.
There was a problem hiding this comment.
Indeed it worked 👍
| generateConstructorForCountrySubClass ??= generateConstructor; | ||
| final output = StringBuffer(); | ||
| output.writeln( | ||
| generateHeader('dart packages/material_ui/script/l10n/bin/gen_localizations.dart --overwrite'), |
There was a problem hiding this comment.
| generateHeader('dart packages/material_ui/script/l10n/bin/gen_localizations.dart --overwrite'), | |
| generateHeader('dart script/l10n/bin/gen_localizations.dart --overwrite'), |
| The `.arb` files in this directory contain localized values (primarily strings) | ||
| used by the Material library. The `generated_material_localizations.dart` file | ||
| combines all of the localizations into a single Map that is linked with the rest | ||
| of flutter_localizations package. |
There was a problem hiding this comment.
| of flutter_localizations package. | |
| of material_ui package. |
| The `.arb` files in this directory contain localized values (primarily strings) | ||
| used by the Cupertino library. The `generated_cupertino_localizations.dart` file | ||
| combines all of the localizations into a single Map that is linked with the rest | ||
| of flutter_localizations package. |
There was a problem hiding this comment.
| of flutter_localizations package. | |
| of cupertino_ui package. |
| 2. Update the .arb files. Modify the out-of-date English strings in | ||
| `lib/src/l10n/cupertino_en.arb`. | ||
|
|
||
| You also need to re-generate `lib/src/l10n/generated_material_localizations.dart` by running: |
There was a problem hiding this comment.
| You also need to re-generate `lib/src/l10n/generated_material_localizations.dart` by running: | |
| You also need to re-generate `lib/src/l10n/generated_cupertino_localizations.dart` by running: |
|
@QuncCccccc Thanks again for catching all of these that I missed! And thanks for testing adding a new string. I tried the same thing locally and got similarly good results.
Good call, I've updated the generation script to reflect this in d9f5dd6 and then regenerated those files in ff4a5ba. Bonus verification that the generation script works 👍 . |
chunhtai
left a comment
There was a problem hiding this comment.
Code LGTM, left some comment on readme, don't want to block the merge so feel free to ignore them or address them as follow up
|
|
||
| You also need to re-generate `lib/src/l10n/generated_cupertino_localizations.dart` by running: | ||
| ``` | ||
| dart script/l10n/bin/gen_localizations.dart --overwrite |
There was a problem hiding this comment.
I think we should mention where should one run the script since this will affect which pubspec.yaml will be use
There was a problem hiding this comment.
Done. The script also enforces that it's run from the repo root.
| You can see what that script would generate by running: | ||
|
|
||
| ```dart | ||
| dart script/l10n/bin/gen_localizations.dart |
| Actually update the generated files with: | ||
|
|
||
| ```dart | ||
| dart script/l10n/bin/gen_localizations.dart --overwrite |
| The `CupertinoLocalizations` class implementation uses these to lookup localized | ||
| resource values. | ||
|
|
||
| The gen_localizations script must be run by hand after .arb files have |
There was a problem hiding this comment.
this should probably be in the readme of the script directory
There was a problem hiding this comment.
I've added this there along with brief sections about each script.
| Finally you need to re-generate | ||
| lib/src/l10n/generated_material_localizations.dart by running: | ||
| ``` | ||
| dart script/l10n/bin/gen_localizations.dart --overwrite |
| This directory contains scripts for generating Dart localizations. Currently it | ||
| is only used by material_ui and cupertino_ui. | ||
|
|
||
| Originally, this code was located in flutter/flutter at |
There was a problem hiding this comment.
Should we mention this at all in this readme? This is good context during this transition, but not something necessary to know in order to use or work on this script
There was a problem hiding this comment.
Good call, I've reworded and moved this section.
|
|
||
| For instructions on how to use these scripts to generate localizations for | ||
| material_ui and cupertino_ui, see | ||
| [packages/material_ui/lib/src/l10n/README.md](https://github.com/flutter/packages/blob/main/packages/material_ui/lib/src/l10n/README.md) |
There was a problem hiding this comment.
I wonder if this should be another way around to put the instruction here and let both readme link to this doc
There was a problem hiding this comment.
This one I'll defer until after this PR while we're still feeling this out. I agree there's a lot of duplication here and order that could be improved. flutter/flutter#189450
|
As soon as CI passes here, I will squash my commits to prepare the PR to land. |
| // Run this program from the root of the git repository. | ||
| // | ||
| // ``` | ||
| // dart dev/tools/localization/bin/gen_missing_localizations.dart |
There was a problem hiding this comment.
This path is no longer correct.
There was a problem hiding this comment.
It's fine if you want to leave this, but we have a repo-level ignore for .dart_tool already.
There was a problem hiding this comment.
Thanks I'll remove it, it was generated by dart create.
| name: l10n | ||
| description: Generates localizations for material_ui and cupertino_ui. | ||
| version: 1.0.0 | ||
| # repository: https://github.com/flutter/packages/tree/main/script/l10n |
There was a problem hiding this comment.
You should be able to omit this is this is just local tooling.
Also, presumably you want publish_to: none in this file.
| path: ^1.9.0 | ||
|
|
||
| dev_dependencies: | ||
| lints: ^6.0.0 |
There was a problem hiding this comment.
Is this actually used?
There was a problem hiding this comment.
Another one from dart create, I'll remove it.
| intl: any | ||
| intl: ^0.20.2 | ||
| # This is not unpinned currently. | ||
| # See https://github.com/flutter/flutter/issues/185017 |
There was a problem hiding this comment.
FWIW, this doesn't really clarify what the plan is for this package; that issue just discusses it in the context of flutter/flutter. I would assume that this would just become a standard unpinned ^ carat dependency in the context of a published package.
There was a problem hiding this comment.
I've started a conversation in the issue about what to do about this, thanks for flagging.
| return found; | ||
| }); | ||
| if (shouldWrite) { | ||
| // ignore: avoid_print |
There was a problem hiding this comment.
Optional, but I would remove these and just do an ignore for the whole file; that's how we generally handle things in bin or tool directories here.
flutter/flutter's flutter_localization package that relates to material_ui and cupertino_ui. Moves the code to the right place, makes sure it operates on the correct directories and works correctly and passes CI, and rewords docs and READMEs to reflect the changes. * Creates a *_ui/lib/src/l10n directory. * Moves dart classes into material_ui and cupertino_ui. * Moves the arb files into material_ui and cupertino_ui. * Moves the *_ui l10n tests and makes sure they pass. * Moves the localization scripts to script/l10n and updates them to work in their new location. * Moves READMEs and rewords them, and creates a new README for the scripts. * Updates the license headers to match flutter/packages. * Removed some tests that shouldn't come in until flutter/flutter#188473 is ported to flutter/packages. * Skips some tests that failed on web that were previously not run on the web at all.
d8a1442 to
43a4d61
Compare
|
I think I'll need to push some buttons in the admin settings to land this with @stuartmorgan-g like we did for the main decoupling PR. I am standing by, let me know when you would like to do so. :) |
|
Go for it, thanks! |
This PR attempts to solve the flutter_localizations problem for material_ui and cupertino_ui. The crux of the problem is that apps that import flutter_localizations were getting a version of GlobalMaterialLocalizations that uses flutter/flutter's MaterialLocalizations instead of the one in material_ui (and the same thing for Cupertino). This PR moves these classes into material_ui and cupertino_ui, while keeping other localization code inside flutter_localizations.
How this PR was created
Similar to #11888, this PR copies history from flutter/flutter. All commits are preserved as-is, plus one final commit from me that is a squash of all of my work on this PR. This PR should be landed as a merge without squash.
Before
As described in flutter/flutter#188757, a typical localized app like the following would encounter runtime type errors:
After
Open Questions
I put the l10n folder in side of <material_ui or cupertino_ui>/lib/l10n because that's what the docs said. Should it be in src/?I put the generation scripts inside of packages/material_ui/tools/l10n. Is there a better spot? Is it ok that cupertino_ui has to use a script inside of material_ui? The reason why I didn't split the scripts in two is that there would be large amounts of code duplication.I have moved it to the root of the repo at /script/l10nTODOs
Update l10n README files.Resources
Fixes flutter/flutter#188757