Adding Localization To Foundation #5495
Conversation
…zation-may-prototype
…zation-may-prototype
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -14,7 +14,10 @@ | |||
| <MicrosoftProjectReunionInteractiveExperiencesTransportPackagePackageVersion>$([System.Text.RegularExpressions.Regex]::Match($(VersionDetailsXml), 'Name="Microsoft.ProjectReunion.InteractiveExperiences.TransportPackage"\s+Version="(.*?)"').Groups[1].Value)</MicrosoftProjectReunionInteractiveExperiencesTransportPackagePackageVersion> | |||
| <MicrosoftWinAppSDKEngCommonPackageVersion>$([System.Text.RegularExpressions.Regex]::Match($(VersionDetailsXml), 'Name="Microsoft.WinAppSDK.EngCommon"\s+Version="(.*?)"').Groups[1].Value)</MicrosoftWinAppSDKEngCommonPackageVersion> | |||
| <MicrosoftWindowsAppSDKAppLicensingInternalTransportPackagePackageVersion>$([System.Text.RegularExpressions.Regex]::Match($(VersionDetailsXml), 'Name="Microsoft.WindowsAppSDK.AppLicensingInternal.TransportPackage"\s+Version="(.*?)"').Groups[1].Value)</MicrosoftWindowsAppSDKAppLicensingInternalTransportPackagePackageVersion> | |||
| <MicrosoftWindowsAppSDKBasePackageVersion>$([System.Text.RegularExpressions.Regex]::Match($(VersionDetailsXml), 'Name="Microsoft.WindowsAppSDK.Base"\s+Version="(.*?)"').Groups[1].Value)</MicrosoftWindowsAppSDKBasePackageVersion> | |||
There was a problem hiding this comment.
While MSIXPackageVersion below is used in appruntime, the BasePackgeVersion and InteractiveExperiencePackageVersion seems auto generated by DevCheck.cmd -SyncDependencies by following notes in Version.Details.xml. Not sure if we should remove those
There was a problem hiding this comment.
This filer is entirely generated by DevCheck. Don't edit this file directly.
This is derived from Microsoft.WindowsAppSDK.Base in Version.Details.xml -- which present in main but not in Version.Dependencies.props. That means someone previously edited Version.Details.xml and DIDN'T run DevCheck -SyncDependencies to propogate the change. Thank you for correcting this previous gap.
I've got a change which adds this and other crosschecks, but hasn't been incorporated into the build yet. Once added it will make these oversights impossible
| void UpdateAllTextLocalization(PickerCommon::PickerParameters& parameter) | ||
| { | ||
| winrt::Microsoft::Windows::ApplicationModel::Resources::ResourceManager manager = winrt::Microsoft::Windows::ApplicationModel::Resources::ResourceManager(priPath); | ||
| auto text = manager.MainResourceMap().GetValue(resourceName).ValueAsString(); |
There was a problem hiding this comment.
[Question] Should we use try get value related API to allow fall back text or fail early to make it easier to find that pri file is not found (which indicates WinAppSDK runtime may not be properly packaged into consumer app)
Current implementation is using simple fail early way.
| @@ -21,7 +21,7 @@ | |||
| <WindowsTargetPlatformMinVersion>10.0.17134.0</WindowsTargetPlatformMinVersion> | |||
| <!-- Need to add this otherwise the ADO build won't copy mrm.dll to output directory, and eventually the file will miss in ManagedTest.build.appxrecipe and make | |||
| the tests fail to run in ADO pipeline --> | |||
| <BuildingInsideVisualStudio>true</BuildingInsideVisualStudio> | |||
| <!-- <BuildingInsideVisualStudio>true</BuildingInsideVisualStudio> --> | |||
There was a problem hiding this comment.
[Note & Question]
In current pipeline test it seems that no test is failing after this change.
If we don't comment this out, this AdHoc BuilidingInsideVisualStudio config will case pipeline errors,
by binlog it seems that adding this will prevent pipeline to build its dependencies,
thus mrm.lib will not build.
Question: is there concern on this modification?
There was a problem hiding this comment.
If there is no need, the above comment should be removed together.
There was a problem hiding this comment.
There's subtle differences between building inside VS vs outside. Do you know why it was added in the first place?
@Scottj1s any comment?
There was a problem hiding this comment.
P.S. +1 if this line is obsolete so too the comment above it -- keep or delete both
| @@ -47,6 +47,10 @@ | |||
| <Uri>https://dev.azure.com/microsoft/LiftedIXP/_git/DCPP</Uri> | |||
| <Sha>b430accfb06428e6d5f4a4306f6acd07c84fc7d6</Sha> | |||
| </Dependency> | |||
| <Dependency Name="Microsoft.Windows.SDK.BuildTools.MSIX" Version="1.8.0-main.20250602.0"> | |||
There was a problem hiding this comment.
[Question] Would maestro automatically start to pushing future version update to this package?
Although it might be safe to merge first and monitor maestro behavior.
Hong-Xiang
changed the title
Hong-Xiang
requested review from
yeelam-gordon,
Copilot,
DinahK-2SO and
codendone
There was a problem hiding this comment.
Pull Request Overview
This PR adds multi-language localization support for the "All Files" label in storage pickers by introducing new resource files and integrating a new localization API. Key changes include:
- Adding new .resw resource files for multiple languages.
- Implementing the PickerLocalization API to update picker parameters with localized text.
- Updating project and build scripts to include the new PRI resource file and associated localization files.
Reviewed Changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Strings/*/StoragePickers.resw | New resource files added for various locales with localized string for "All Files". |
| PickerLocalization.h / .cpp | New API implemented to update picker parameters with localized "All Files" text. |
| PickerCommon.{h,cpp} | Default "All Files" text updated and then replaced by localized text. |
| StoragePickers.vcxitems, BuildAll.ps1, CopyFilesToStagingDir.ps1, WindowsAppRuntime.sln | Updated build configuration to include PRI and localization files. |
| const winrt::hstring resourceName = L"Microsoft.WindowsAppRuntime/StoragePickers/All Files"; | ||
| void UpdateAllTextLocalization(PickerCommon::PickerParameters& parameter) | ||
| { | ||
| winrt::Microsoft::Windows::ApplicationModel::Resources::ResourceManager manager = winrt::Microsoft::Windows::ApplicationModel::Resources::ResourceManager(priPath); |
There was a problem hiding this comment.
Consider caching or reusing the ResourceManager instance to avoid the performance overhead of creating a new instance every time UpdateAllTextLocalization is called.
| winrt::Microsoft::Windows::ApplicationModel::Resources::ResourceManager manager = winrt::Microsoft::Windows::ApplicationModel::Resources::ResourceManager(priPath); | |
| static winrt::Microsoft::Windows::ApplicationModel::Resources::ResourceManager manager(priPath); |
Copilot uses AI. Check for mistakes.
| winrt::Microsoft::Windows::ApplicationModel::Resources::ResourceManager manager = winrt::Microsoft::Windows::ApplicationModel::Resources::ResourceManager(priPath); | ||
| auto text = manager.MainResourceMap().GetValue(resourceName).ValueAsString(); | ||
| parameter.AllFilesText = text; | ||
| } |
There was a problem hiding this comment.
This code is highly coupling on all the localization logic with PickerParameters.
Two comments and see if it make sense:
a. We usually define localization "Key" as constant, and having helper method to load the localization resource at the place they should consume.
e.g. the code PickerCommon.cpp, it try to add FilterData "AllFilesText", it should be like:
GetLocalizedResource(AllFilesTextKey)
b. What you wrote on how loading resources now are like loading resource from an file, instead of EMBEDDED resource as part of the Microsoft.WindowsAppRuntime?
Should we always load with our dll so that we don't need to worry it? And probably the resourceName may be simpler in case.
| @@ -21,7 +21,7 @@ | |||
| <WindowsTargetPlatformMinVersion>10.0.17134.0</WindowsTargetPlatformMinVersion> | |||
| <!-- Need to add this otherwise the ADO build won't copy mrm.dll to output directory, and eventually the file will miss in ManagedTest.build.appxrecipe and make | |||
| the tests fail to run in ADO pipeline --> | |||
| <BuildingInsideVisualStudio>true</BuildingInsideVisualStudio> | |||
| <!-- <BuildingInsideVisualStudio>true</BuildingInsideVisualStudio> --> | |||
There was a problem hiding this comment.
If there is no need, the above comment should be removed together.
| @@ -75,6 +76,7 @@ namespace winrt::Microsoft::Windows::Storage::Pickers::implementation | |||
| auto logTelemetry{ StoragePickersTelemetry::FileOpenPickerPickSingleFile::Start(m_telemetryHelper) }; | |||
|
|
|||
| PickerCommon::PickerParameters parameters{}; | |||
| PickerCommon::UpdateAllTextLocalization(parameters); | |||
|
|
|||
| CaptureParameters(parameters); | |||
There was a problem hiding this comment.
Please see my comment below. I think loading localized resource should be happening at CaptureParameters
| @@ -155,6 +155,18 @@ | |||
| <ActivatableClass ActivatableClassId="Microsoft.Windows.Storage.Pickers.FolderPicker" ThreadingModel="both" /> | |||
| </InProcessServer> | |||
| </Extension> | |||
|
|
|||
| <Extension Category="windows.activatableClass.inProcessServer"> | |||
| <InProcessServer> | |||
There was a problem hiding this comment.
MRT defines 8 runtimeclasses. Why only list 4 here?
dev\MRTCore\mrt\Microsoft.Windows.ApplicationModel.Resources\src\Microsoft.Windows.ApplicationModel.Resources.idl
RECOMMEND: Add all MRT runtimeclasses. Missing: ResourceNotFoundEventArgs, ResourceMap, ResourceContext, ApplicationLanguages
There was a problem hiding this comment.
P.S. Please sort the lines alphabetically e.g. ...KnownResourceQualifierName shouldn't appear after ...ResourceCandidate
| <InProcessServer> | ||
| <Path>Microsoft.Windows.ApplicationModel.Resources.dll</Path> | ||
| <ActivatableClass ActivatableClassId="Microsoft.Windows.ApplicationModel.Resources.ResourceManager" ThreadingModel="both" /> | ||
| <ActivatableClass ActivatableClassId="Microsoft.Windows.ApplicationModel.Resources.IResourceManagerFactory" ThreadingModel="both" /> |
There was a problem hiding this comment.
What is IResourceManagerFactory? I don't see it listed in any file under dev\MRTCore
| @@ -14,7 +14,10 @@ | |||
| <MicrosoftProjectReunionInteractiveExperiencesTransportPackagePackageVersion>$([System.Text.RegularExpressions.Regex]::Match($(VersionDetailsXml), 'Name="Microsoft.ProjectReunion.InteractiveExperiences.TransportPackage"\s+Version="(.*?)"').Groups[1].Value)</MicrosoftProjectReunionInteractiveExperiencesTransportPackagePackageVersion> | |||
| <MicrosoftWinAppSDKEngCommonPackageVersion>$([System.Text.RegularExpressions.Regex]::Match($(VersionDetailsXml), 'Name="Microsoft.WinAppSDK.EngCommon"\s+Version="(.*?)"').Groups[1].Value)</MicrosoftWinAppSDKEngCommonPackageVersion> | |||
| <MicrosoftWindowsAppSDKAppLicensingInternalTransportPackagePackageVersion>$([System.Text.RegularExpressions.Regex]::Match($(VersionDetailsXml), 'Name="Microsoft.WindowsAppSDK.AppLicensingInternal.TransportPackage"\s+Version="(.*?)"').Groups[1].Value)</MicrosoftWindowsAppSDKAppLicensingInternalTransportPackagePackageVersion> | |||
| <MicrosoftWindowsAppSDKBasePackageVersion>$([System.Text.RegularExpressions.Regex]::Match($(VersionDetailsXml), 'Name="Microsoft.WindowsAppSDK.Base"\s+Version="(.*?)"').Groups[1].Value)</MicrosoftWindowsAppSDKBasePackageVersion> | |||
| <MicrosoftWindowsAppSDKInteractiveExperiencesPackageVersion>$([System.Text.RegularExpressions.Regex]::Match($(VersionDetailsXml), 'Name="Microsoft.WindowsAppSDK.InteractiveExperiences"\s+Version="(.*?)"').Groups[1].Value)</MicrosoftWindowsAppSDKInteractiveExperiencesPackageVersion> | |||
There was a problem hiding this comment.
Is your branch stale? I see this line already exists in main
| <MicrosoftWindowsCsWinRTPackageVersion>$([System.Text.RegularExpressions.Regex]::Match($(VersionDetailsXml), 'Name="Microsoft.Windows.CsWinRT"\s+Version="(.*?)"').Groups[1].Value)</MicrosoftWindowsCsWinRTPackageVersion> | ||
| <MicrosoftWindowsSDKBuildToolsMSIXPackageVersion>$([System.Text.RegularExpressions.Regex]::Match($(VersionDetailsXml), 'Name="Microsoft.Windows.SDK.BuildTools.MSIX"\s+Version="(.*?)"').Groups[1].Value)</MicrosoftWindowsSDKBuildToolsMSIXPackageVersion> |
There was a problem hiding this comment.
How'd this get here? I don't see it in Version.Dependencies.xml
This is a generated file. Never edit it directly -- edit Version.Dependencies.xml or Version.Details.xml and run DevCheck -SyncDependencies to propogate your updates (here and elsewhere)
See the huge comment at the top of eng\Version.Dependendencies.xml or eng\Version.Details.xml for more details
| @@ -14,7 +14,10 @@ | |||
| <MicrosoftProjectReunionInteractiveExperiencesTransportPackagePackageVersion>$([System.Text.RegularExpressions.Regex]::Match($(VersionDetailsXml), 'Name="Microsoft.ProjectReunion.InteractiveExperiences.TransportPackage"\s+Version="(.*?)"').Groups[1].Value)</MicrosoftProjectReunionInteractiveExperiencesTransportPackagePackageVersion> | |||
| <MicrosoftWinAppSDKEngCommonPackageVersion>$([System.Text.RegularExpressions.Regex]::Match($(VersionDetailsXml), 'Name="Microsoft.WinAppSDK.EngCommon"\s+Version="(.*?)"').Groups[1].Value)</MicrosoftWinAppSDKEngCommonPackageVersion> | |||
| <MicrosoftWindowsAppSDKAppLicensingInternalTransportPackagePackageVersion>$([System.Text.RegularExpressions.Regex]::Match($(VersionDetailsXml), 'Name="Microsoft.WindowsAppSDK.AppLicensingInternal.TransportPackage"\s+Version="(.*?)"').Groups[1].Value)</MicrosoftWindowsAppSDKAppLicensingInternalTransportPackagePackageVersion> | |||
| <MicrosoftWindowsAppSDKBasePackageVersion>$([System.Text.RegularExpressions.Regex]::Match($(VersionDetailsXml), 'Name="Microsoft.WindowsAppSDK.Base"\s+Version="(.*?)"').Groups[1].Value)</MicrosoftWindowsAppSDKBasePackageVersion> | |||
There was a problem hiding this comment.
This filer is entirely generated by DevCheck. Don't edit this file directly.
This is derived from Microsoft.WindowsAppSDK.Base in Version.Details.xml -- which present in main but not in Version.Dependencies.props. That means someone previously edited Version.Details.xml and DIDN'T run DevCheck -SyncDependencies to propogate the change. Thank you for correcting this previous gap.
I've got a change which adds this and other crosschecks, but hasn't been incorporated into the build yet. Once added it will make these oversights impossible
| @@ -21,7 +21,7 @@ | |||
| <WindowsTargetPlatformMinVersion>10.0.17134.0</WindowsTargetPlatformMinVersion> | |||
| <!-- Need to add this otherwise the ADO build won't copy mrm.dll to output directory, and eventually the file will miss in ManagedTest.build.appxrecipe and make | |||
| the tests fail to run in ADO pipeline --> | |||
| <BuildingInsideVisualStudio>true</BuildingInsideVisualStudio> | |||
| <!-- <BuildingInsideVisualStudio>true</BuildingInsideVisualStudio> --> | |||
There was a problem hiding this comment.
There's subtle differences between building inside VS vs outside. Do you know why it was added in the first place?
@Scottj1s any comment?
| @@ -21,7 +21,7 @@ | |||
| <WindowsTargetPlatformMinVersion>10.0.17134.0</WindowsTargetPlatformMinVersion> | |||
| <!-- Need to add this otherwise the ADO build won't copy mrm.dll to output directory, and eventually the file will miss in ManagedTest.build.appxrecipe and make | |||
| the tests fail to run in ADO pipeline --> | |||
| <BuildingInsideVisualStudio>true</BuildingInsideVisualStudio> | |||
| <!-- <BuildingInsideVisualStudio>true</BuildingInsideVisualStudio> --> | |||
There was a problem hiding this comment.
P.S. +1 if this line is obsolete so too the comment above it -- keep or delete both
| #include <iostream> | ||
|
|
||
| namespace PickerCommon { | ||
| const winrt::hstring priPath = L"Microsoft.WindowsAppRuntime.pri"; |
There was a problem hiding this comment.
WinAppSDK has multiple .pri files e.g. Microsoft.UI.pri and Microsoft.UI.Xaml.Controls.pri.
Given these resources are specifically for the Pickers this should be a picker-specific filename e.g. Microsoft.Windows.Storage.Pickers.pri
| void UpdateAllTextLocalization(PickerCommon::PickerParameters& parameter) | ||
| { | ||
| winrt::Microsoft::Windows::ApplicationModel::Resources::ResourceManager manager = winrt::Microsoft::Windows::ApplicationModel::Resources::ResourceManager(priPath); | ||
| auto text = manager.MainResourceMap().GetValue(resourceName).ValueAsString(); |
|
|
||
| namespace PickerCommon { | ||
| const winrt::hstring priPath = L"Microsoft.WindowsAppRuntime.pri"; | ||
| const winrt::hstring resourceName = L"Microsoft.WindowsAppRuntime/StoragePickers/All Files"; |
There was a problem hiding this comment.
Use a Picker-specific name e.g. Microsoft.Windows.Storage.Pickers
see other comment
This PR introduces basic localization support to the WindowsAppRuntime DLL within the Foundation layer, specifically adding localization for the "All Files" label in storage pickers.
Although the runtime DLL only requires MRTCore's WinMD for compilation, we are currently referencing MRTCore projects directly within the WindowsAppRuntime project. This approach simplifies integration, at a cost of unnecessary MRTCore build during the pipeline execution. Fortunately, the additional build time is negligible in the context of a full pipeline run.
Manual testing has been done using a referenced component package. However, unit tests for the new localization method are currently not executable due to the absence of a proper MRTCore reference. The tests rely on a mocked package rather than the actual Foundation result package. We plan to address this by refactoring the unit tests to use the component package, enabling proper validation of the localization APIs.