Skip to content

[gen_l10n] When localizing a message, prefer placeholder definitions defined by the current locale rather than the template locale #153459

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.

Sign up for GitHub

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

Jump to bottom
Merged
auto-submit merged 22 commits into from
Nov 14, 2024
Merged

[gen_l10n] When localizing a message, prefer placeholder definitions defined by the current locale rather than the template locale #153459

auto-submit merged 22 commits into from
Nov 14, 2024

Conversation

@kzrnm
Copy link
Contributor

@kzrnm kzrnm commented Aug 14, 2024

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@kzrnm kzrnm changed the title Fix custom date [gen_l10n] Fix formatting with multiple locale Aug 14, 2024
@github-actions github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Aug 14, 2024
@andrewkolos andrewkolos self-requested a review August 15, 2024 18:50
andrewkolos
andrewkolos reviewed Sep 11, 2024
View reviewed changes
@andrewkolos

This comment was marked as resolved.

Sign in to view
@kzrnm
Copy link
Contributor Author

kzrnm commented Sep 21, 2024

@andrewkolos
I added support for cases where format is defined only in non-template and vice versa.

flutter/packages/flutter_tools/test/general.shard/generate_localizations_test.dart

Lines 2683 to 2759 in d01e119

testWithoutContext('handle number with multiple locale specifying a format only in template', () {
setupLocalizations(<String, String>{
'en': '''
{
"@@locale": "en",
"money": "Sum {number}",
"@money": {
"placeholders": {
"number": {
"type": "int",
"format": "decimalPatternDigits",
"optionalParameters": {
"decimalDigits": 3
}
}
}
}
}''',
'ja': '''
{
"@@locale": "ja",
"money": "合計 {number}",
"@money": {
"placeholders": {
"number": {
"type": "int"
}
}
}
}'''
});
expect(getGeneratedFileContent(locale: 'en'), contains('intl.NumberFormat.decimalPatternDigits('));
expect(getGeneratedFileContent(locale: 'en'), contains('decimalDigits: 3'));
expect(getGeneratedFileContent(locale: 'en'), contains(r"return 'Sum $numberString'"));
expect(getGeneratedFileContent(locale: 'ja'), isNot(contains('intl.NumberFormat')));
expect(getGeneratedFileContent(locale: 'ja'), contains(r"return '合計 $number'"));
});
testWithoutContext('handle number with multiple locale specifying a format only in non-template', () {
setupLocalizations(<String, String>{
'en': '''
{
"@@locale": "en",
"money": "Sum {number}",
"@money": {
"placeholders": {
"number": {
"type": "int"
}
}
}
}''',
'ja': '''
{
"@@locale": "ja",
"money": "合計 {number}",
"@money": {
"placeholders": {
"number": {
"type": "int",
"format": "decimalPatternDigits",
"optionalParameters": {
"decimalDigits": 3
}
}
}
}
}'''
});
expect(getGeneratedFileContent(locale: 'en'), isNot(contains('intl.NumberFormat')));
expect(getGeneratedFileContent(locale: 'en'), contains(r"return 'Sum $number'"));
expect(getGeneratedFileContent(locale: 'ja'), contains('intl.NumberFormat.decimalPatternDigits('));
expect(getGeneratedFileContent(locale: 'ja'), contains('decimalDigits: 3'));
expect(getGeneratedFileContent(locale: 'ja'), contains(r"return '合計 $numberString'"));
});

There are two changes that accompany this change.

  1. Always import package:intl/intl.dart
    It is cumbersome to determine if it is necessary to import intl for each locale, so I changed it to always import it.
  2. Compatibility Check
    I added compatibility checks that will generate an error if a method is generated that is incompatible with the template. I am not a native English speaker, so if the error message is unnatural, please suggest a better fix.

    flutter/packages/flutter_tools/lib/src/localizations/gen_l10n.dart

    Lines 131 to 142 in d01e119

    return message.templatePlaceholders.entries.map((MapEntry<String, Placeholder> e) {
    final Placeholder placeholder = e.value;
    final String? localePlaceholderType = localePlaceholders?[e.key]?.type;
    if (localePlaceholderType != null && placeholder.type != localePlaceholderType) {
    throw L10nException(
    'The placeholder, ${placeholder.name}, has its "type" resource attribute set to '
    'the "$localePlaceholderType" type in locale "$locale", but it is "${placeholder.type}" '
    'in the template placeholders. For compatibility with template placeholders, change '
    'the "type" attribute to "${placeholder.type}".');
    }
    return '${useNamedParameters ? 'required ' : ''}${placeholder.type} ${placeholder.name}';
    }).toList();

@andrewkolos andrewkolos self-requested a review October 8, 2024 20:25
andrewkolos
andrewkolos reviewed Oct 11, 2024
View reviewed changes
packages/flutter_tools/lib/src/localizations/gen_l10n.dart Outdated
return message.templatePlaceholders.entries.map((MapEntry<String, Placeholder> e) {
final Placeholder placeholder = e.value;
final String? localePlaceholderType = localePlaceholders?[e.key]?.type;
if (localePlaceholderType != null && placeholder.type != localePlaceholderType) {
Copy link
Contributor

@andrewkolos andrewkolos Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we always check that types match regardless of whether localePlaceholderType is null?

Consider this example:

// lib/l10n/app_en.arb
{
  "@@locale": "en",
  "springBegins": "Spring begins on {springStartDate}",
  "@springBegins": {
    "description": "The first day of spring",
    "placeholders": {
      "springStartDate": {
        "type": "DateTime",
        "format": "MMMd"
      }
    }
  }
}

// lib/l10n/app_ja.arb

{
  "@@locale": "es",
  "springBegins": "春が始まるのは{springStartDate}",
  "@springBegins": {
    "placeholders": {
      "springStartDate": {
        "format": "MMMMd"
      }
    }
  }
}

In the ja locale, the localized text looks like 春が始まるのは2024-10-10 10:44:48.635339, but if I add "type": "DateTime" to the ja placeholder, the text looks like 春が始まるのは10月10日.

Copy link
Contributor Author

@kzrnm kzrnm Oct 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to make the type explicit in this pull request. I think it would be better to loosen the type constraint in another pull request if necessary.

Copy link
Contributor

@andrewkolos andrewkolos Oct 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, i'm saying that I want the type check to be strict, so I always want the type in the template ARB and the other ARBs to always match.

Suggested change
if (localePlaceholderType != null && placeholder.type != localePlaceholderType) {
if (placeholder.type != localePlaceholderType) {

packages/flutter_tools/lib/src/localizations/gen_l10n_templates.dart Outdated

const String classFileTemplate = '''
@(header)@(requiresIntlImport)import '@(fileName)';
@(header)import 'package:intl/intl.dart' as intl;
Copy link
Contributor

@andrewkolos andrewkolos Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are always going to import package:intl (which seems fine1), then we'll need to add // ignore: unused_import before this line. Otherwise, the Dart compiler will report an unused_import error.

Footnotes

  1. package:intl is a dependency of package:flutter_localizations and a documented requirement of this feature, so I think it should always be present. ↩

Copy link
Contributor Author

@kzrnm kzrnm Oct 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand. I have committed the fix.

@bkonyi
Copy link
Contributor

bkonyi commented Oct 17, 2024

@kzrnm, FYI it looks like some checks are failing now.

@andrewkolos andrewkolos self-requested a review October 22, 2024 00:10
@andrewkolos andrewkolos changed the title [gen_l10n] Fix formatting with multiple locale [gen_l10n] When localizing a message, prefer placeholder definitions defined by the current locale rather than the template locale Oct 23, 2024
andrewkolos
andrewkolos approved these changes Oct 23, 2024
View reviewed changes
Copy link
Contributor

@andrewkolos andrewkolos left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I will find a second reviewer soon.

(I also just restarted the Google testing check)

andrewkolos
andrewkolos reviewed Oct 23, 2024
View reviewed changes
packages/flutter_tools/lib/src/localizations/gen_l10n.dart
final Placeholder placeholder = e.value;
final Placeholder? localePlaceholder = localePlaceholders?[e.key];
if (localePlaceholder != null && placeholder.type != localePlaceholder.type) {
throw L10nException(
Copy link
Contributor

@andrewkolos andrewkolos Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for second reviewer:

This change could1 break peoples' builds when they upgrade. Consider

// app_en.arb
{
  "@@locale": "en",
  "springBegins": "Spring begins on {springStartDate}",
  "@springBegins": {
    "description": "The first day of spring",
    "placeholders": {
      "springStartDate": {
        "type": "DateTime",
        "format": "MMMMd"
      }
    }
}

and

// app_ja.arb
{
  "@@locale": "ja",
  "springBegins": "春が始まるのは{springStartDate}",
  "@springBegins": {
    "description": "The first day of spring",
    "placeholders": {
      "springStartDate": {
        "type": "int", // Type change.
        "format": "compactCurrency"
      }
    }
  },

Then, trying to regenerate localization code will result in a failure:

Generating synthetic localizations package failed with 1 error:

Error: The placeholder, springStartDate, has its "type" resource attribute set to the "int" type in locale "ja", but it is "DateTime" in the template placeholders. For
compatibility with template placeholders, change the "type" attribute to "DateTime".

I think this failing the build makes sense here, and I believe there's no pre-existing behavior for users to be dependent on here (the springStartDate placeholder in the ja locale would have been silently ignored, hence this PR). I'm content with this landing without a breaking change announcement.

Footnotes

  1. And therefore will 🙃 ↩

Copy link
Contributor

@bkonyi bkonyi Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's reasonable.

@andrewkolos andrewkolos requested a review from bkonyi October 23, 2024 21:36
bkonyi
bkonyi approved these changes Oct 24, 2024
View reviewed changes
Copy link
Contributor

@bkonyi bkonyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the contribution. I'll land this when the checks pass.

packages/flutter_tools/lib/src/localizations/gen_l10n.dart
final Placeholder placeholder = e.value;
final Placeholder? localePlaceholder = localePlaceholders?[e.key];
if (localePlaceholder != null && placeholder.type != localePlaceholder.type) {
throw L10nException(
Copy link
Contributor

@bkonyi bkonyi Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's reasonable.

@bkonyi bkonyi added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 14, 2024
@auto-submit auto-submit bot merged commit dca37ad into flutter:master Nov 14, 2024
145 checks passed
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 14, 2024
@kzrnm kzrnm deleted the fixCustomDate branch November 15, 2024 00:38
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 15, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2024
@engine-flutter-autoroll
dca37ad [gen_l10n] When localizing a message, prefer placeholder defi…
0c8a05a 
…nitions defined by the current locale rather than the template locale (flutter/flutter#153459)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 15, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2024
@engine-flutter-autoroll
dca37ad [gen_l10n] When localizing a message, prefer placeholder defi…
d4d01b5 
…nitions defined by the current locale rather than the template locale (flutter/flutter#153459)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 15, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2024
@engine-flutter-autoroll
dca37ad [gen_l10n] When localizing a message, prefer placeholder defi…
3b13b90 
…nitions defined by the current locale rather than the template locale (flutter/flutter#153459)
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Nov 15, 2024
@engine-flutter-autoroll
Roll Flutter from ed553d1 to 0e2d55e (40 revisions) (#8106)
319fe77 
Roll Flutter from ed553d1 to 0e2d55e (40 revisions)

flutter/flutter@ed553d1...0e2d55e

2024-11-15 [email protected] Roll Packages from b9ac917 to b164be3 (4 revisions) (flutter/flutter#158986)
2024-11-15 [email protected] Label PRs with gradle and Android paths "platform-android" (flutter/flutter#158970)
2024-11-15 [email protected] Fix code asset copying logic in native asset code (flutter/flutter#158984)
2024-11-15 [email protected] Fix duplicate work in native assets release builds (flutter/flutter#158980)
2024-11-15 [email protected] Stop generate both `.kts` and non-`.kts` gradle files for a test project. (flutter/flutter#158965)
2024-11-15 [email protected] Further skip `native_assets_test`(s) that runs `flutter build apk`. (flutter/flutter#158966)
2024-11-15 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 4.2.1 to 4.2.2 (flutter/flutter#157473)
2024-11-15 [email protected] Try running historically flaky tests first to make `flutter build apk` health tests time out more often? (flutter/flutter#158967)
2024-11-15 [email protected] [ tool ] Don't throw StateError when DDS fails to start (flutter/flutter#158744)
2024-11-14 [email protected] Create merge_queue.md (flutter/flutter#158959)
2024-11-14 [email protected] Marks Mac_x64 hot_mode_dev_cycle_macos_target__benchmark to be flaky (flutter/flutter#158569)
2024-11-14 [email protected] Marks Mac_arm64 mac_desktop_impeller to be unflaky (flutter/flutter#158564)
2024-11-14 [email protected] [gen_l10n] When localizing a message, prefer placeholder definitions defined by the current locale rather than the template locale (flutter/flutter#153459)
2024-11-14 [email protected] feat: Include web 1.x.x in plugin template (flutter/flutter#156947)
2024-11-14 [email protected] hide members where possible (flutter/flutter#158492)
2024-11-14 [email protected] Move platform-specific log-reading implementation details from `ResidentRunner`/`FlutterDevice` to `DeviceLogReader` implementations (flutter/flutter#156181)
2024-11-14 [email protected] Updated document to clarify Clip Behaviour (flutter/flutter#157719)
2024-11-14 [email protected] Enable --verbose for android_plugin_skip_unsupported_test tests (flutter/flutter#158933)
2024-11-14 [email protected] Roll Packages from 26e123a to b9ac917 (5 revisions) (flutter/flutter#158938)
2024-11-14 [email protected] Add `dev_dependency` attribute to plugins in `.flutter-plugins-dependencies` (flutter/flutter#158009)
2024-11-14 [email protected] No longer pass `--verbose` to implicit `pub` calls when `flutter --verbose` is set. (flutter/flutter#158898)
2024-11-14 [email protected] Update triage flow chart for SVG packages (flutter/flutter#158670)
2024-11-14 [email protected] Add one MenuAnchor alignment test (flutter/flutter#158915)
2024-11-14 [email protected] Reland Add test for dynamic_content_color.0.dart (flutter/flutter#158547)
2024-11-14 [email protected] Roll Flutter Engine from e97b148e796d to 619804c0fbb7 (1 revision) (flutter/flutter#158905)
2024-11-14 [email protected] Roll Flutter Engine from 82940a9aa714 to e97b148e796d (1 revision) (flutter/flutter#158901)
2024-11-14 [email protected] Move explicit package dependencies to a feature flag (flutter/flutter#158016)
2024-11-14 [email protected] Try with `bringup: true` debugging why `flutter build apk` often times out. (flutter/flutter#158895)
2024-11-14 [email protected] Add constraint options to `SearchAnchor` suggestions builder (flutter/flutter#148856)
2024-11-14 [email protected] Adjusts the Hindi TimeOfDayFormat to display in a LTR orientation in localizations. (flutter/flutter#157998)
2024-11-14 [email protected] Fix update order of SliverAppBar (flutter/flutter#158159)
2024-11-13 [email protected] #154792 - CupertinoActionSheetAction cursor doesn't change to clickable on desktop (flutter/flutter#158470)
2024-11-13 [email protected] Adds a skip message for analyzer (flutter/flutter#158890)
2024-11-13 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Marks Mac_arm64_ios hot_mode_dev_cycle_ios__benchmark to be flaky (#158242)" (flutter/flutter#158891)
2024-11-13 [email protected] [SwiftPM] Move the logic for SwiftPM enablement to the platform project (flutter/flutter#158213)
2024-11-13 [email protected] Temporarily skip flutter build apk for native_assets tests. (flutter/flutter#158880)
2024-11-13 [email protected] docs: include Human Interface haptic information in HapticFeedback (flutter/flutter#158587)
2024-11-13 [email protected] Roll Flutter Engine from db3e5af2ca22 to 82940a9aa714 (2 revisions) (flutter/flutter#158799)
2024-11-13 [email protected] Stream the output of `flutter build` for debugging. (flutter/flutter#158757)
2024-11-13 [email protected] "Fix failing checks" wiki page for new contributors (flutter/flutter#154629)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.
...
@darshankawar darshankawar mentioned this pull request Jan 28, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 12, 2025
@engine-flutter-autoroll
dca37ad [gen_l10n] When localizing a message, prefer placeholder defi…
19f48f8 
…nitions defined by the current locale rather than the template locale (flutter/flutter#153459)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
@engine-flutter-autoroll
dca37ad [gen_l10n] When localizing a message, prefer placeholder defi…
2bd5e79 
…nitions defined by the current locale rather than the template locale (flutter/flutter#153459)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
@engine-flutter-autoroll
dca37ad [gen_l10n] When localizing a message, prefer placeholder defi…
8e7bde1 
…nitions defined by the current locale rather than the template locale (flutter/flutter#153459)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 6, 2025
@engine-flutter-autoroll
dca37ad [gen_l10n] When localizing a message, prefer placeholder defi…
8a12c08 
…nitions defined by the current locale rather than the template locale (flutter/flutter#153459)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025
@engine-flutter-autoroll
dca37ad [gen_l10n] When localizing a message, prefer placeholder defi…
d2a606d 
…nitions defined by the current locale rather than the template locale (flutter/flutter#153459)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andrewkolos andrewkolos andrewkolos approved these changes

@bkonyi bkonyi bkonyi approved these changes

Assignees

No one assigned

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

date formats and number formats always use template arb Custom date formats always use english locale

3 participants

@kzrnm @andrewkolos @bkonyi