-
Notifications
You must be signed in to change notification settings - Fork 328
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
[MRTCore] Add Microsoft.Windows.Globalization.ApplicationLanguages class #4181
[MRTCore] Add Microsoft.Windows.Globalization.ApplicationLanguages class #4181
Conversation
/azp run |
Commenter does not have sufficient privileges for PR 4181 in repo microsoft/WindowsAppSDK |
while doubtful /azp run |
/azp run |
Commenter does not have sufficient privileges for PR 4181 in repo microsoft/WindowsAppSDK |
This is needed for PowerToys to fully enable language override |
static hstring PrimaryLanguageOverride(); | ||
|
||
private: | ||
static hstring m_language; |
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.
What happens with multiple threads?
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.
wil::srwlock could be used to protect access to this variable.
...soft.Windows.ApplicationModel.Resources/src/Microsoft.Windows.ApplicationModel.Resources.idl
Show resolved
Hide resolved
...soft.Windows.ApplicationModel.Resources/src/Microsoft.Windows.ApplicationModel.Resources.idl
Show resolved
Hide resolved
...soft.Windows.ApplicationModel.Resources/src/Microsoft.Windows.ApplicationModel.Resources.idl
Show resolved
Hide resolved
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.
I've added comments. Please address and update.
dev/MRTCore/mrt/Microsoft.Windows.ApplicationModel.Resources/src/ApplicationLanguages.cpp
Outdated
Show resolved
Hide resolved
eafa77e
to
410c575
Compare
...soft.Windows.ApplicationModel.Resources/src/Microsoft.Windows.ApplicationModel.Resources.idl
Outdated
Show resolved
Hide resolved
Thread safe PrimaryLanguageOverride for un-packaged Use W.G.PrimaryLanguageOverride for packaged
dev/MRTCore/mrt/Microsoft.Windows.ApplicationModel.Resources/src/ApplicationLanguages.cpp
Show resolved
Hide resolved
@@ -97,6 +97,10 @@ void ResourceContext::Apply() | |||
winrt::check_hresult(MrmSetQualifier(m_resourceContext, eachValue.Key().c_str(), eachValue.Value().c_str())); | |||
} | |||
} | |||
if (!ApplicationLanguages::PrimaryLanguageOverride().empty()) |
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.
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.
With W.G.PrimaryLanguageOverride set, newly created ResourceContext takes PrimaryLanguageOverride into account, i.e. does not override language QualifierValue to default language.
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.
e.g. if default language is "en-US", PrimaryLanguageOverride is "de-DE", and ResourceContext["Lanauge"]="fr-FR", what's the language that will take effect? I'd think "fr-FR"
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.
according to this logic, no, PrimaryLanguageOverride should take precedence over ResourceContext["Language"]. GetLangugageContext returns Windows::Globalization::ApplicationLanguages::Languages() which is affected if W.G.PrimaryLanguageOverride is set. This behavior remains the same with this change
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.
I think in that logic ResourceContext["Language"] still takes precedence. It would be strange to the caller that an explicit request is not honored
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.
Got it now. You're right. Fixed by latest change. Now explicit set of context Language is not overridden by PrimaryLanguageOverride.
I tested with this WinAppSDK sample. Default language is PrimaryLanguageOverride - de-DE, but fetching resources with m_overrideResourceContext passed is fetching it-IT resources:
Check if valid language tag is passed to PrimaryLanguageOverride
note: changing namespace to Microsoft.Windows.Globalization.ApplicationLanguages is in progress (as decided during API review) |
change pushed |
|
||
THROW_HR_IF_MSG(E_INVALIDARG, !isValidLanguageTag, "The parameter is incorrect"); | ||
|
||
static wil::srwlock lock; |
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.
You have to use the same lock for reading and writing. Otherwise the setter could take an exclusive lock, but another thread could still read the data.
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.
🤦 fixed
@@ -86,3 +86,31 @@ HRESULT GetDefaultPriFile(winrt::hstring& filePath) | |||
bool isPackaged = (hr != HRESULT_FROM_WIN32(APPMODEL_ERROR_NO_PACKAGE)); | |||
return GetDefaultPriFileForCurentModule(isPackaged, filePath); | |||
} | |||
|
|||
bool IsWellFormedLanguageTag(const wchar_t* languageTag) |
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.
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.
done. I don't have this header, so I reused the logic to load the DLL manually as in platform.h/.cpp. Tested. Works as expected.
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.
the header should be in SDK. The LoadLibrary code was written before it's added into SDK.
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.
according to the docs it should be there since Windows 10 Build 17763, but I don't have it on machine (OS version 10.0.22631 Build 22631), VS 22 installed with multiple SDK versions. I double-checked on another machine with the same software versions
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.
I double-checked again, the header is present in SDK v10.0.20348.0 (and later), even though docs says it should be there before
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.
Is bumping WindowsTargetPlatformVersion to 10.0.20348.0 for this specific project ok to do ?
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.
I pushed the change (including WindowsTargetPlatformVersion bump). I'll revert if it's not ok
6f37f5c
to
2d1c892
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
This has been merged as #4523 |
Changing the application language using PrimaryLanguageOverride property from
Windows.Globalization.ApplicationLanguages
namespace is not supported for WindowsAppSDK unpackaged apps, only for packaged. To support language change for un-packaged applicationsMicrosoft.Windows.Globalization.ApplicationLanguages.ApplicationLanguages
type is introduced in MRTCore.Microsoft.Windows.Globalization.ApplicationLanguages.PrimaryLanguageOverride
property is supported both for packaged and un-packaged WindowsAppSDK apps.Sample app used to verify the change: https://github.com/stefansjfw/testwasdk/blob/master/testlocalwinappsdk/App.xaml.cs#L36
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.