Skip to content
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

Fix invalid Unix locale to Flutter locale (BCP-47) mapping #36512

Merged
merged 1 commit into from
Jan 19, 2023

Conversation

robert-ancell
Copy link
Contributor

Flutter locales use the BCP-47 format (https://www.rfc-editor.org/rfc/bcp/bcp47.txt), and Unix locales use a POSIX format. There is not a perfect mapping between them, see https://wiki.openoffice.org/wiki/LocaleMapping

The current implementation is not correctly setting the appropriate optional fields, so remove that.

There are likely some Unix locales that should set these fields, but these are best fixed by adding special cases for them as they are discovered.

Fixes: flutter/flutter#111341

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

@flutter-dashboard
Copy link

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 on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

These changes lgtm. Is there a way we can test this? e.g. are we reading from an environment variable we could cache, set to something, then restore?

@Hixie
Copy link
Contributor

Hixie commented Jan 17, 2023

@robert-ancell Is this still on your radar?

@robert-ancell robert-ancell force-pushed the linux-locale-mapping branch 4 times, most recently from 7f09350 to 8960174 Compare January 19, 2023 04:02
Flutter locales use the BCP-47 format
(https://www.rfc-editor.org/rfc/bcp/bcp47.txt), and Unix locales use a POSIX
format. There is not a perfect mapping between them, see
https://wiki.openoffice.org/wiki/LocaleMapping

The current implementation is not correctly setting the appropriate optional
fields, so remove that.

There are likely some Unix locales that should set these fields, but these are
best fixed by adding special cases for them as they are discovered.

Fixes: flutter/flutter#111341
@robert-ancell
Copy link
Contributor Author

@robert-ancell Is this still on your radar?

Was forgotten about, but I've added a test now.

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

LGTM stamp from a Japanese personal seal

@cbracken cbracken added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 19, 2023
@auto-submit auto-submit bot merged commit 27f6bbc into flutter:main Jan 19, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jan 20, 2023
…118852)

* 27f6bbcb3 Fix invalid Unix locale to Flutter locale (BCP-47) mapping (flutter/engine#36512)

* 7d40e77d0 Roll Skia from 69d8378097df to aedfc8695954 (4 revisions) (flutter/engine#38966)
ricardoamador pushed a commit to ricardoamador/engine that referenced this pull request Jan 25, 2023
…6512)

Flutter locales use the BCP-47 format
(https://www.rfc-editor.org/rfc/bcp/bcp47.txt), and Unix locales use a POSIX
format. There is not a perfect mapping between them, see
https://wiki.openoffice.org/wiki/LocaleMapping

The current implementation is not correctly setting the appropriate optional
fields, so remove that.

There are likely some Unix locales that should set these fields, but these are
best fixed by adding special cases for them as they are discovered.

Fixes: flutter/flutter#111341
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App needs tests platform-linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flutter engine should not use codeset as Locale.scriptCode on Linux
3 participants