fix: Read PrimaryLanguageOverride from an app-specific key #12321
Conversation
github-actions
bot
added
area/skia ✏️
|
|
||
| private static string GetAppSpecificSettingKey() | ||
| { | ||
| return $"PrimaryLanguageOverrideSettingKey.{Package.EntryAssembly.GetName().Name}"; |
There was a problem hiding this comment.
If someone tried to access ApplicationLanguages too early, before an Application instance is created. This is going to blow up.
@MartinZikmund Thoughts? Is it a valid use case to access ApplicationLanguages before Application instance is created? Not sure how to best approach this.
There was a problem hiding this comment.
Technically it should never happen - I imagine the earliest usage in the App constructor, which is after Current was already set in the Application base constructor. I would suggest throwing a clear exception here (something like you are trying to use ApplicationLanguages too early in tyhe application lifecycle).
There was a problem hiding this comment.
@MartinZikmund One could attempt to access it before this line, for example:
I'll throw an exception.
There was a problem hiding this comment.
Actually, based on my tests, it is possible to use even the Package.Current API in WinUI before the app starts:
static void Main(string[] args)
{
XamlCheckProcessRequirements();
global::WinRT.ComWrappersSupport.InitializeComWrappers();
var name = Package.Current.Id.Name;
global::Microsoft.UI.Xaml.Application.Start((p) => {
var context = new global::Microsoft.UI.Dispatching.DispatcherQueueSynchronizationContext(global::Microsoft.UI.Dispatching.DispatcherQueue.GetForCurrentThread());
global::System.Threading.SynchronizationContext.SetSynchronizationContext(context);
new App();
});
}So I would vote for making the SetEntryAssembly part of the host initialization sequence somehow. Let's discuss later
There was a problem hiding this comment.
Failed: VerifyMultipleEraCalendar() [System.TypeInitializationException]
System.TypeInitializationException: TypeInitialization_Type, Windows.UI.Xaml.Controls.CalendarView
---> System.TypeInitializationException: TypeInitialization_Type, Windows.Globalization.ApplicationLanguages
---> System.InvalidOperationException: ApplicationLanguages is being accessed too early before an instance of Application was created.
at Windows.Globalization.ApplicationLanguages.GetAppSpecificSettingKey()
at Windows.Globalization.ApplicationLanguages..cctor()
Exception_EndOfInnerExceptionStack
at Windows.Globalization.Calendar..ctor()
at Windows.UI.Xaml.Controls.CalendarView.GetOrCreateGregorianCalendar()
at Windows.UI.Xaml.Controls.CalendarView.GetDefaultMaxDate()
at Windows.UI.Xaml.Controls.CalendarView..cctor()
Exception_EndOfInnerExceptionStack
at Private.Infrastructure.CalendarHelper.CalendarViewHelper.<GetCalendarView>b__1_0()
at Private.Infrastructure.TestServices.RunOnUIThread(Action action)
at Private.Infrastructure.CalendarHelper.CalendarViewHelper.GetCalendarView()
at Windows.UI.Xaml.Tests.Enterprise.CalendarViewIntegrationTests.VerifyMultipleEraCalendar()
at Private.Infrastructure.CalendarHelper.CalendarViewHelper.<GetCalendarView>b__1_0()
at Private.Infrastructure.TestServices.RunOnUIThread(Action action)
at Private.Infrastructure.CalendarHelper.CalendarViewHelper.GetCalendarView()
at Windows.UI.Xaml.Tests.Enterprise.CalendarViewIntegrationTests.VerifyMultipleEraCalendar()
at Private.Infrastructure.CalendarHelper.CalendarViewHelper.<GetCalendarView>b__1_0()
at Private.Infrastructure.TestServices.RunOnUIThread(Action action)
at Private.Infrastructure.CalendarHelper.CalendarViewHelper.GetCalendarView()
at Windows.UI.Xaml.Tests.Enterprise.CalendarViewIntegrationTests.VerifyMultipleEraCalendar()
at Uno.UI.Samples.Tests.UnitTestsControl.<>c__DisplayClass63_1.<<ExecuteTestsForInstance>g__InvokeTestMethod|2>d.MoveNext()
😕
There was a problem hiding this comment.
I wasn't able to figure out what is happening here. It is Wasm-specific
Youssef1313
force-pushed
the
prim-lang-override
branch
from
272c90a to
bac92a5
Compare
Youssef1313
force-pushed
the
prim-lang-override
branch
from
bac92a5 to
f7de493
Compare
|
I see that the lifecycle changes you are hitting here are the same ones I am seeing in my PR, so I will postpone my PR until this one is merged and also included in breaking changes branch, as there will be conflicts. |
| throw new InvalidOperationException("ApplicationLanguages is being accessed too early before an instance of Application was created."); | ||
| } | ||
|
|
||
| return $"PrimaryLanguageOverrideSettingKey.{entryAssembly.GetName().Name}"; |
There was a problem hiding this comment.
I don't believe this should be the fix. The origin of the issue is likely because we're not storing app settings in an app identity based location, but rather a common location that is incorrectly shared.
There was a problem hiding this comment.
Right but how do we fix this in 4.x without it being a breaking change (ie what @MartinZikmund is working on).
My additional comment would be that this fix would limit overlap of wasm apps when debugging - currently if you use the same port (the template defaults to a fixed post to allow ui tests to "just work") you'll see PrimaryLanguageOverride from other apps you debugged with the same port. Adding the app name would limit this overlap
There was a problem hiding this comment.
Those are two different issues then. If this fix is for wasm, we're talking about a debug issue (which I don't believe needs fixing), not a production issue. The issue for Skia is different, where settings are placed at the wrong location.
In general linking the settings to the assembly name will cause issues, as if an app is logically deployed at the same location, but somehow changes its internal structure, the settings will be lost.
There was a problem hiding this comment.
It's a bit of an edge case but what happens if you have two different wasm apps on the same host eg https://myserver.com/app1 and https://myserver.com/app2 will they see the same, or different, settings?
There was a problem hiding this comment.
That's indeed interesting. Per the documentation: Web storage is per origin (per domain and protocol). All pages, from one origin, can store and access the same data.
I don't think that we should be discriminating based on the assembly name, though. We're currently not using the app manifest for wasm, but it feels like we should start doing so, so we can pick up the app identity and use it as a based for storing settings.
This is likely something we should be explaining in the storage docs as well, as files from one app may interfere in another app from the same origin.
/cc @MartinZikmund
There was a problem hiding this comment.
@jeromelaban yeah, that's definitely something to adjust for 5x then (as a breaking change)
|
Closing as the linked issue got closed |
Pull request was closed
GitHub Issue (If applicable): closes #12320
PR Type
What kind of change does this PR introduce?
What is the current behavior?
What is the new behavior?
PR Checklist
Please check if your PR fulfills the following requirements:
Screenshots Compare Test Runresults.Other information
Internal Issue (If applicable):