Application Developer Localization Override Fix #3460

michael-hawker · 2020-09-01T22:34:01Z

Fixes #3349

This fixes the GetLocalized method in order to first look in an application's resource file for the request key before falling back on the specified resource path. This will allow easier overriding of locales using this method.

Added test cases to check this, as well as cleaning up syntax and keys in the toolkit.

PR Type

What kind of change does this PR introduce?

Bugfix

Refactoring (no functional changes, no api changes)

What is the current behavior?

A developer can't specify other languages or overrides to any externalized resources within the Toolkit.

What is the new behavior?

A developer can now override a key in any langauge.

PR Checklist

Please check if your PR fulfills the following requirements:

Tested code with current supported SDKs
Pull Request has been submitted to the documentation repository instructions. Link:
Sample in sample app has been added / updated (for bug fixes / features)
- Icon has been created (if new sample) following the Thumbnail Style Guide and templates
Tests for the changes have been added (for bug fixes / features) (if applicable)
Header has been added to all new source files (run build/UpdateHeaders.bat)
Contains NO breaking changes

Other information

Left TODO:

Wait on Split Markdown.* from Controls package. #3437 to finish resource name refactor
Finish updating GetLocalized calls to use simpler syntax
Ensure Toolbar Resource Keys are Prefixed with proper "WindowsCommunityToolkit_" prefix like all others
Write test cases for x:Uid base XAML case and ensure this works as expected, as this should be the main path. We should only use GetLocalized for code-behind cases.
Optimize?
Write more test cases for GetLocalized?

TODO: - [ ] Wait on 3437 to finish resource name refactor - [ ] Finish updating GetLocalized calls to use simpler syntax - [ ] Ensure Toolbar Resource Keys are Prefixed with proper "WindowsCommunityToolkit_" prefix like all others - [ ] Optimize? - [ ] Write more test cases?

ghost · 2020-09-01T22:34:09Z

Thanks michael-hawker for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

michael-hawker · 2020-09-01T22:38:03Z

May also want to investigate adding tests here for x:Uid support within controls...?

azchohfi · 2020-09-01T23:06:46Z

Microsoft.Toolkit.Uwp/Extensions/StringExtensions.cs

-            => ResourceLoader.GetForViewIndependentUse(resourcePath).GetString(resourceKey);
+        {
+            // Try and retrieve resource at app level first.
+            var result = IndependentLoader.GetString(resourceKey);


Hummmmm, have you tried this code path on Xaml Islands? We might need to use the other overload for GetLocalized, that accepts a UIContext, and force the use of this parameter, but I'm not 100% sure. Lets test it.

@azchohfi I can just try this out over here, eh?

https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/6a05a3c264407fbd1bee43fae57197a6e75af561/UnitTests/UnitTests.XamlIslands.UWPApp/XamlIslandsTest_StringExtensions.cs#L26-L42

I'll just add a test that doesn't provide the UIContext? I'll try copying the three tests I have over there...

Yeah, create a test, just in case, and test it out on the Xaml Island test project, as the Xaml Islands tests are not automated into the CI.

@azchohfi I tested this out and our API is working fine. However, the test itself was having trouble with the ApplicationLanguages.ManifestLanguages.ToArray() API as it only returned 1 result instead of 2 like in the default UWP cases... Any ideas? FYI @marb2000

I'll check-in the updates I do have with that line commented out for now.

azchohfi · 2020-09-01T23:09:40Z

May also want to investigate adding tests here for x:Uid support within controls...?

I think this should be tracked in a different issue.

azchohfi · 2020-09-01T23:10:42Z

The FR test failed.

michael-hawker · 2020-09-02T00:08:27Z

The FR test failed.

Hmm, it worked locally, may be a timing/behavior thing...? I'll try dispatching.

vgromfeld · 2020-09-02T06:55:21Z

Seems good to me.

Also fixed a missed rename from a Debug/Release #ifdef

Microsoft.Toolkit.Uwp.SampleApp/Models/Sample.cs

UnitTests/UnitTests.XamlIslands.UWPApp/FakeTestClasses/Assert.cs

michael-hawker · 2020-09-10T20:32:14Z

I'll clean-up this PR a tiny bit now that the Markdown one is merged, I'll try and do that tomorrow morning first thing.

…n-override-fix

michael-hawker · 2020-09-16T23:34:56Z

@vgromfeld @azchohfi I've updated the bit of clean-up I wanted to do as part of this. I'm realizing we don't have a good case where we're using x:Uid in a control (we should probably be using this more than we are?), so I don't know if there's a good spot to create a unit test for that scenario yet.

@vgromfeld for the x:Uid you added, were you able to change the property in your app resources? As that may be a shortcoming in general too, this solution only fixes the GetLocalized method, but doesn't check how the x:UId lookup works. Does that look in the app resources and then the control too? That may need more testing.

FYI @RosarioPulella

vgromfeld · 2020-09-17T07:09:56Z

@michael-hawker I was able to change the x:Uid resources by adding the community toolkit resources' names in my app resources:

 <data name="WindowsCommunityToolkit_InAppNotification_LandmarkProperty.Value" xml:space="preserve">
    <value>Youpikai</value>
  </data>

This has to be done for all the application's languages.
If not done, the compiler is complaining:

3>GENERATEPROJECTPRIFILE : warning : PRI263: 0xdef01051 - No default or neutral resource given for 'Resources/WindowsCommunityToolkit_InAppNotification_LandmarkProperty/Value'. The application may throw an exception for certain user configurations when retrieving the resources.

And the runtime is throwing 😥:

Exception thrown: 'Windows.UI.Xaml.Markup.XamlParseException' in Microsoft.Media.Player.exe
Exception thrown: 'Windows.UI.Xaml.Markup.XamlParseException' in System.Private.CoreLib.dll
Exception thrown: 'Windows.UI.Xaml.Markup.XamlParseException' in System.Private.CoreLib.dll
Exception thrown: 'Windows.UI.Xaml.Markup.XamlParseException' in System.Private.CoreLib.dll
An exception of type 'Windows.UI.Xaml.Markup.XamlParseException' occurred in System.Private.CoreLib.dll but was not handled in user code

Other than this "constraint", the x:Uid override is working fine.

michael-hawker · 2020-09-17T14:44:50Z

Thanks @vgromfeld, that's perfect (good to know about the current 'constraint', but I think in this scenario it makes sense).

I think we can check-in this fix, note: I renamed the resources to be consistent and a bit shorter with the WCT_ prefix. I'll create a follow-up issue for checking other opportunities for x;UIds and creating a test for that.

michael-hawker added introduce breaking changes 💥 improvements ✨ extensions ⚡ localization 🌐 labels Sep 1, 2020

michael-hawker added this to the 7.0 milestone Sep 1, 2020

michael-hawker requested review from azchohfi, Sergio0694 and vgromfeld September 1, 2020 22:34

ghost requested a review from Kyaa-dost September 1, 2020 22:34

ghost added the bug 🐛 An unexpected issue that highlights incorrect behavior label Sep 1, 2020

michael-hawker linked an issue Sep 1, 2020 that may be closed by this pull request

[Testing] Check for overriding string resources in the controls package #3449

Closed

azchohfi reviewed Sep 1, 2020

View reviewed changes

michael-hawker added 2 commits September 8, 2020 14:06

Fix Unit Test for Localization in Release Mode

94d6e6f

Also fixed a missed rename from a Debug/Release #ifdef

Test Localization Changes on XAML Islands

5c59d87

michael-hawker marked this pull request as ready for review September 9, 2020 16:07

Merge branch 'master' into mhawker/localization-override-fix

64cb2dd

azchohfi reviewed Sep 9, 2020

View reviewed changes

Microsoft.Toolkit.Uwp.SampleApp/Models/Sample.cs Show resolved Hide resolved

azchohfi approved these changes Sep 9, 2020

View reviewed changes

UnitTests/UnitTests.XamlIslands.UWPApp/FakeTestClasses/Assert.cs Outdated Show resolved Hide resolved

Update UnitTests/UnitTests.XamlIslands.UWPApp/FakeTestClasses/Assert.cs

84b22bb

vgromfeld approved these changes Sep 10, 2020

View reviewed changes

michael-hawker added 5 commits September 10, 2020 16:45

Merge branch 'master' into mhawker/localization-override-fix

222f93c

Merge remote-tracking branch 'origin/master' into mhawker/localizatio…

568da68

…n-override-fix

Update to call Extension method for GetLocalized directly

b32b683

Align all resources to use a common 'WCT_' prefix

bf15828

Merge branch 'master' into mhawker/localization-override-fix

4ad2a12

michael-hawker mentioned this pull request Sep 17, 2020

Look for more opportunities to use x:UId #3479

Closed

michael-hawker merged commit e3c08ac into CommunityToolkit:master Sep 17, 2020

Rosuavio mentioned this pull request Sep 17, 2020

System.TypeInitializationException after #3460 #3480

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Application Developer Localization Override Fix #3460

Application Developer Localization Override Fix #3460

michael-hawker commented Sep 1, 2020 •

edited

Loading

ghost commented Sep 1, 2020

michael-hawker commented Sep 1, 2020

azchohfi Sep 1, 2020

michael-hawker Sep 8, 2020

azchohfi Sep 8, 2020

michael-hawker Sep 8, 2020

azchohfi commented Sep 1, 2020

azchohfi commented Sep 1, 2020

michael-hawker commented Sep 2, 2020

vgromfeld commented Sep 2, 2020

michael-hawker commented Sep 10, 2020

michael-hawker commented Sep 16, 2020

vgromfeld commented Sep 17, 2020

michael-hawker commented Sep 17, 2020

Application Developer Localization Override Fix #3460

Application Developer Localization Override Fix #3460

Conversation

michael-hawker commented Sep 1, 2020 • edited Loading

Fixes #3349

PR Type

What is the current behavior?

What is the new behavior?

PR Checklist

Other information

ghost commented Sep 1, 2020

michael-hawker commented Sep 1, 2020

azchohfi Sep 1, 2020

Choose a reason for hiding this comment

michael-hawker Sep 8, 2020

Choose a reason for hiding this comment

azchohfi Sep 8, 2020

Choose a reason for hiding this comment

michael-hawker Sep 8, 2020

Choose a reason for hiding this comment

azchohfi commented Sep 1, 2020

azchohfi commented Sep 1, 2020

michael-hawker commented Sep 2, 2020

vgromfeld commented Sep 2, 2020

michael-hawker commented Sep 10, 2020

michael-hawker commented Sep 16, 2020

vgromfeld commented Sep 17, 2020

michael-hawker commented Sep 17, 2020

michael-hawker commented Sep 1, 2020 •

edited

Loading