Allow ApplicationLanguages.PrimaryLanguageOverride to be unset#5726
Allow ApplicationLanguages.PrimaryLanguageOverride to be unset#5726whiskhub wants to merge 6 commits intomicrosoft:mainfrom
Conversation
|
@microsoft-github-policy-service agree |
|
Hi @RDMacLachlan @codendone could this Pull Request be looked at for a future release? Thank you! |
| void ApplicationLanguages::PrimaryLanguageOverride(hstring const& language) | ||
| { | ||
| bool isValidLanguageTag = IsWellFormedTag(language.c_str()); | ||
| bool isValidLanguageTag = language.empty() || IsWellFormedTag(language.c_str()); |
There was a problem hiding this comment.
my impression is that winrt::Windows::Globalization::ApplicationLanguages::PrimaryLanguageOverride(language) requires nullptr to reset PrimaryLanguageOverride. Have you verified that the test reached line 53?
There was a problem hiding this comment.
In WinRT, an empty HSTRING is equivalent to a nullptr (https://devblogs.microsoft.com/oldnewthing/20160615-00/?p=93675).
So, windows::Globalization::ApplicationLanguages::PrimaryLanguageOverride does accept both nullptr/null and empty string.
There was a problem hiding this comment.
Also, it doesn't seem possible for externals to build anything from the Windows App SDK due to missing permissions for the ProjectReunion internal NuGet source. So I was only able to test in my own app.
There was a problem hiding this comment.
I checked the implementation of winrt::Windows::Globalization::ApplicationLanguages::PrimaryLanguageOverride(language) in OS, and it will reset the override when null is passed in, and fail if empty string is passed in.
There was a problem hiding this comment.
In C++/WinRT it is impossible to compare a winrt::hstring to nullptr - the equality operator is explicitly deleted: bool operator==(hstring const& left, std::nullptr_t) = delete;
This is exactly because an empty HSTRING is always represented by a null pointer internally. Please see the blog post by Raymond Chen: https://devblogs.microsoft.com/oldnewthing/20220706-00/?p=106836
Maybe the OS code is not written in C++WinRT and is therefore able to compare with nullptr, but code with C++/WinRT cannot do that.
In C#, WG.ApplicationLanguages.PrimaryLanguageOverride = null; and WG.ApplicationLanguages.PrimaryLanguageOverride = ""; will also both work. You can try in a test app.
There was a problem hiding this comment.
OS implementation doesn't use C++/WinRT. I assume it works because eventually nullptr is passed to it.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| ApplicationLanguages.PrimaryLanguageOverride = "fr-FR"; | ||
| ApplicationLanguages.PrimaryLanguageOverride = ""; | ||
|
|
||
| Verify.AreEqual(ApplicationLanguages.PrimaryLanguageOverride, ""); |
There was a problem hiding this comment.
consider adding a verification that winrt::Windows::Globalization::ApplicationLanguages::PrimaryLanguageOverride is reset for packaged process, as that's what matters.
| ApplicationLanguages.PrimaryLanguageOverride = null; | ||
| Verify.AreEqual(Windows.Globalization.ApplicationLanguages.PrimaryLanguageOverride, ""); // C# projection of null HSTRING is empty string | ||
| } | ||
| } |
There was a problem hiding this comment.
The tests are added under CommonTestCode. But they don't work for unpackaged test. Ideally they need to be moved to unittests.cs, or add a packaged check.
There was a problem hiding this comment.
True, I moved the test code over
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
The build failed: |
|
/azp run |
|
Commenter does not have sufficient privileges for PR 5726 in repo microsoft/WindowsAppSDK |
|
Thank you @huichen123, I hope it's fixed with the latest commit. It’s unfortunately not easy for 3rd parties to contribute, even for basic MRs like this one. I cannot compile it locally, nor can I trigger builds or see the build results. |
|
new failure: |
|
It seems like CsWinRT doesn't generate a projection for Sorry, without being able to debug this locally, I cannot fix it. |
2 participants
Fixes #5335
A microsoft employee must use /azp run to validate using the pipelines below.
WARNING:
Comments made by azure-pipelines bot maybe inaccurate.
Please see pipeline link to verify that the build is being ran.
For status checks on the main branch, please use TransportPackage-Foundation-PR
(https://microsoft.visualstudio.com/ProjectReunion/_build?definitionId=81063&_a=summary)
and run the build against your PR branch with the default parameters.