Use Windows Display Language #43341

yaakovschectman · 2023-06-29T20:09:21Z

Get the Windows Display Language for locale selection instead of the preferred languages.

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.

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

flutter-dashboard · 2023-06-29T20:09:24Z

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.

shell/platform/windows/system_utils_unittests.cc

yaakovschectman · 2023-06-29T20:13:51Z

shell/platform/windows/system_utils.cc

  return buffer;
 }

 std::vector<std::wstring> GetPreferredLanguages(


We now have no more use for GetPreferredLanguagesFromRegistry. Should we remove the function altogether?

Yes please remove this. If needed, we can always use git's history to resurrect the code.

chunhtai

LGTM.

yaakovschectman · 2023-06-30T16:14:48Z

Hmmm... it looks like the devicelab machine may actually return an empty list for GetThreadPreferredUILanguages, in which case, unit testing is going to be tricky...

loic-sharma · 2023-06-30T17:45:59Z

it looks like the devicelab machine may actually return an empty list for GetThreadPreferredUILanguages, in which case, unit testing is going to be tricky...

I'd consider using the WindowsProcTable here. Currently the WindowsProcTable is used for dynamic loading of APIs that are unavailable on older Windows versions, however, we can expand it to also be used to mock out Win32 APIs.

The WindowsProcTable would expose a virtual method that mirrors the GetThreadPreferredUILanguages Win32 API. The implementation would call the GetThreadPreferredUILanguages API directly (there's no need to dynamically load this API), and tests would override this implementation using mocks.

loic-sharma · 2023-06-30T17:52:30Z

shell/platform/windows/system_utils.cc

  ULONG buffer_size;
  ULONG count = 0;
  DWORD flags = MUI_LANGUAGE_NAME | MUI_UI_FALLBACK;
  if (!GetThreadPreferredUILanguages(flags, &count, nullptr, &buffer_size)) {


Is GetThreadPreferredUILanguages or GetPreferredUILanguages preferable? Should we use one as a fallback for the other?

Our partner team switch to GetPreferredUILanguages, but comments indicate this might have been unintended. We should make sure we align to prevent inconsistencies. See: http://cl/544490185

It looks like they are using the same method as here, there. And re: your comment on WindowsProcTable, this shouldn't be necessary. Looks like I missed a nuance of the parameters to the function that didn't make a difference locally but did on devicelab

It looks like they are using the same method as here, there

Ah you're right, I got confused by their helper's name.

And re: your comment on WindowsProcTable, this shouldn't be necessary.

The current tests depend on the machines environment. For example, the tests would fail if somehow a machine was configured such that GetThreadPreferredUILanguages returns no languages.

In my mind, the ideal test would be an integration test that mocks GetThreadPreferredUILanguages and then checks that the Windows embedder sends the expected FlutterLocale to the embedder API at startup. Through our mocks we can guarantee that the machine's language configuration can't cause unexpected test flakes.

Are we sure that:
a) This is a concern, e.g. that the environment can return no preferred languages? The test already in place always expected at least one valid language, and I do not know if a windows env can have zero preferred languages between the thread, process, user, and system. And,
b) Doing so will not obscure the purpose of this test?

Are we sure that:
This is a concern, e.g. that the environment can return no preferred languages?

This is a hypothetical scenario, yes. Another hypothetical scenario would be that GetThreadPreferredUILanguages fails and returns false. These hypothetical scenarios would cause flakes. To reduce this likelihood, our tests should be deterministic and we should control their inputs.

Doing so will not obscure the purpose of this test?

My only concern is using the real GetThreadPreferredUILanguages implementation in the test. I'm flexible on the test itself, I'd be happy with your current test too if GetThreadPreferredUILanguages was mocked out :)

shell/platform/windows/windows_proc_table.h

Co-authored-by: Loïc Sharma <[email protected]>

shell/platform/windows/windows_proc_table.h

shell/platform/windows/system_utils_unittests.cc

loic-sharma

Thanks for fixing this, excellent work!

…sions) (#130097) Manual roll Flutter Engine from bd2e42b203e1 to 06c936205d96 (27 revisions) Manual roll requested by [email protected] flutter/engine@bd2e42b...06c9362 2023-07-06 [email protected] Handle nested display list clips in Impeller dispatcher (flutter/engine#43442) 2023-07-06 [email protected] Add Look Up to iOS selection controls (flutter/engine#43308) 2023-07-06 [email protected] Roll Skia from 6aaef9cbc3af to 10a43e57e0a6 (1 revision) (flutter/engine#43444) 2023-07-06 [email protected] Roll Skia from 620de5ac9f6b to 6aaef9cbc3af (1 revision) (flutter/engine#43441) 2023-07-06 [email protected] Revert "[iOS][Keyboard] Wait vsync on UI thread and update viewport inset to avoid jitter." (flutter/engine#43422) 2023-07-06 [email protected] Roll ICU from a2961dc659b4 to e8c3bc9ea97d (7 revisions) (flutter/engine#43440) 2023-07-06 [email protected] Further shard clang-tidy runs (flutter/engine#43428) 2023-07-06 [email protected] Release log capture at end of test (flutter/engine#43429) 2023-07-06 [email protected] Roll Fuchsia Mac SDK from uGhka7LBG80dM_I3a... to 3D0ft09qP4-Hp_3mQ... (flutter/engine#43439) 2023-07-06 [email protected] Roll Fuchsia Linux SDK from amH2MWTworhTLdMWy... to MEXi9Xyj7u24PSx_J... (flutter/engine#43438) 2023-07-06 [email protected] Use Windows Display Language (flutter/engine#43341) 2023-07-06 [email protected] Roll Skia from b2ba6e1d8c0e to 620de5ac9f6b (2 revisions) (flutter/engine#43437) 2023-07-06 [email protected] Roll Skia from 50e866b51a64 to b2ba6e1d8c0e (2 revisions) (flutter/engine#43436) 2023-07-06 [email protected] Roll Skia from 8ed969b60e98 to 50e866b51a64 (1 revision) (flutter/engine#43435) 2023-07-06 [email protected] Fix typo in painting.dart (flutter/engine#43378) 2023-07-06 [email protected] Roll Fuchsia Mac SDK from C3Q7MJBYkiin8zw-f... to uGhka7LBG80dM_I3a... (flutter/engine#43433) 2023-07-06 [email protected] Roll Fuchsia Linux SDK from iwgWLB4KaXslnaGwK... to amH2MWTworhTLdMWy... (flutter/engine#43432) 2023-07-05 [email protected] Roll Skia from a39421eb8d7b to 8ed969b60e98 (6 revisions) (flutter/engine#43431) 2023-07-05 [email protected] [Impeller] Allocate fewer textures in dedicated memory and adjust buffer flags. (flutter/engine#43313) 2023-07-05 [email protected] Roll Skia from d6e941012b87 to a39421eb8d7b (4 revisions) (flutter/engine#43427) 2023-07-05 [email protected] Roll Skia from e0b9e047332d to d6e941012b87 (2 revisions) (flutter/engine#43424) 2023-07-05 [email protected] [iOS] Avoid crash when backdrop filter is null for PlatformViews (flutter/engine#43150) 2023-07-05 [email protected] Roll Skia from 401c397f7235 to e0b9e047332d (1 revision) (flutter/engine#43420) 2023-07-05 [email protected] [Windows] Fix `FlutterWindow::GetNativeViewAccessible` crash (flutter/engine#43368) 2023-07-05 [email protected] [iOS] Fix FlutterViewController retain cycle (flutter/engine#43379) 2023-07-05 [email protected] Roll Skia from 23052eb28808 to 401c397f7235 (1 revision) (flutter/engine#43419) 2023-07-05 [email protected] Roll Skia from d58324bf653d to 23052eb28808 (3 revisions) (flutter/engine#43418) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from iwgWLB4KaXsl to MEXi9Xyj7u24 fuchsia/sdk/core/mac-amd64 from C3Q7MJBYkiin to 3D0ft09qP4-H If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: ...

Get the Windows Display Language for locale selection instead of the preferred languages. flutter/flutter#129786 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] 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. - [x] I updated/added relevant documentation (doc comments with `///`). - [ ] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat --------- Co-authored-by: Loïc Sharma <[email protected]>

Use Windows Display Language

cd2f7dd

yaakovschectman added affects: desktop platform-windows labels Jun 29, 2023

yaakovschectman requested review from chunhtai, loic-sharma, stuartmorgan-g and thkim1011 June 29, 2023 20:09

yaakovschectman self-assigned this Jun 29, 2023

flutter-dashboard bot added the needs tests label Jun 29, 2023

Update unit test

59708af

yaakovschectman commented Jun 29, 2023

View reviewed changes

shell/platform/windows/system_utils_unittests.cc Show resolved Hide resolved

yaakovschectman commented Jun 29, 2023

View reviewed changes

Remove WindowsRegistry; now unused

ad40c6d

chunhtai approved these changes Jun 30, 2023

View reviewed changes

yaakovschectman added 3 commits June 30, 2023 12:16

Debug only

4771e8e

More debug

a0b705c

More debug

c1c5cb0

loic-sharma reviewed Jun 30, 2023

View reviewed changes

yaakovschectman added 2 commits June 30, 2023 13:54

Try init to 0

aaa69e2

Remove debug code

dc7d13a

yaakovschectman requested a review from loic-sharma June 30, 2023 20:42

yaakovschectman added 2 commits July 5, 2023 15:44

Mock out API call

f0211ef

Format

9df1103

loic-sharma reviewed Jul 5, 2023

View reviewed changes

shell/platform/windows/windows_proc_table.h Outdated Show resolved Hide resolved

Update shell/platform/windows/windows_proc_table.h

a1f01ee

Co-authored-by: Loïc Sharma <[email protected]>

loic-sharma reviewed Jul 5, 2023

View reviewed changes

shell/platform/windows/windows_proc_table.h Show resolved Hide resolved

loic-sharma reviewed Jul 5, 2023

View reviewed changes

shell/platform/windows/system_utils_unittests.cc Show resolved Hide resolved

PR comment

fc4cd50

loic-sharma approved these changes Jul 5, 2023

View reviewed changes

yaakovschectman merged commit 3e4c41c into flutter:main Jul 6, 2023

engine-flutter-autoroll mentioned this pull request Jul 6, 2023

Roll Flutter Engine from bd2e42b203e1 to c4b5dfb4b66d (24 revisions) flutter/flutter#130093

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 6, 2023

3e4c41c3e Use Windows Display Language (flutter/engine#43341)

7879e5d

engine-flutter-autoroll mentioned this pull request Jul 6, 2023

Roll Flutter Engine from bd2e42b203e1 to 06c936205d96 (27 revisions) flutter/flutter#130095

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 6, 2023

3e4c41c3e Use Windows Display Language (flutter/engine#43341)

57c01f5

engine-flutter-autoroll mentioned this pull request Jul 6, 2023

Manual roll Flutter Engine from bd2e42b203e1 to 06c936205d96 (27 revisions) flutter/flutter#130097

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 6, 2023

3e4c41c3e Use Windows Display Language (flutter/engine#43341)

139f344

Use Windows Display Language #43341

Use Windows Display Language #43341

Uh oh!

Conversation

yaakovschectman commented Jun 29, 2023

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Jun 29, 2023

Uh oh!

Uh oh!

yaakovschectman Jun 29, 2023

Choose a reason for hiding this comment

Uh oh!

loic-sharma Jun 29, 2023

Choose a reason for hiding this comment

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

yaakovschectman commented Jun 30, 2023

Uh oh!

loic-sharma commented Jun 30, 2023

Uh oh!

loic-sharma Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

yaakovschectman Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

loic-sharma Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

yaakovschectman Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

loic-sharma Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

loic-sharma left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants