-
Notifications
You must be signed in to change notification settings - Fork 391
Allow ApplicationLanguages.PrimaryLanguageOverride to be unset #5726
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
base: main
Are you sure you want to change the base?
Changes from all commits
d12d29b
7881142
076d803
ad46f6d
fd9e16d
1352029
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
using System; | ||
using Microsoft.VisualStudio.TestTools.UnitTesting; | ||
using Microsoft.Windows.ApplicationModel.Resources; | ||
using Microsoft.Windows.Globalization; | ||
#else | ||
using System; | ||
using System.ComponentModel; | ||
|
@@ -18,6 +19,7 @@ | |
using WEX.TestExecution; | ||
using WEX.TestExecution.Markup; | ||
using Microsoft.Windows.ApplicationModel.Resources; | ||
using Microsoft.Windows.Globalization; | ||
#endif | ||
|
||
namespace CommonTestCode | ||
|
@@ -585,4 +587,23 @@ public static void NoResourceFileWithContextTest() | |
Verify.IsNull(resourceCandidate); | ||
} | ||
} | ||
|
||
public class ApplicationLanguagesTest | ||
{ | ||
public static void PrimaryLanguageOverrideAcceptsEmptyStringTest() | ||
{ | ||
ApplicationLanguages.PrimaryLanguageOverride = "fr-FR"; | ||
ApplicationLanguages.PrimaryLanguageOverride = ""; | ||
|
||
Verify.AreEqual(ApplicationLanguages.PrimaryLanguageOverride, ""); | ||
} | ||
|
||
public static void PrimaryLanguageOverrideAcceptsNullStringTest() | ||
{ | ||
ApplicationLanguages.PrimaryLanguageOverride = "fr-FR"; | ||
ApplicationLanguages.PrimaryLanguageOverride = null; | ||
|
||
Verify.AreEqual(ApplicationLanguages.PrimaryLanguageOverride, ""); // C# projection of null HSTRING is empty string | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, I moved the test code over |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,7 +41,7 @@ namespace winrt::Microsoft::Windows::Globalization::implementation | |
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, it doesn't seem possible for externals to build anything from the Windows App SDK due to missing permissions for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In C++/WinRT it is impossible to compare a Maybe the OS code is not written in C++WinRT and is therefore able to compare with In C#, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OS implementation doesn't use C++/WinRT. I assume it works because eventually nullptr is passed to it. |
||
|
||
THROW_HR_IF_MSG(E_INVALIDARG, !isValidLanguageTag, "The parameter is incorrect"); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider adding a verification that winrt::Windows::Globalization::ApplicationLanguages::PrimaryLanguageOverride is reset for packaged process, as that's what matters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I've added a test