-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Refactor WASM input and dom-callbacks to work with multithreading #15849
Conversation
Marking as DRAFT, as I need to double/triple check if anything was broken on .NET 8 single threaded. Will do it tomorrow/after tomorrow. |
@@ -8,7 +8,7 @@ require("esbuild").build({ | |||
bundle: true, | |||
minify: true, | |||
format: "esm", | |||
target: "es2018", | |||
target: "es2019", |
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.
flatMap usage requires es2019.
At this point we can bump it a little.
You can test this PR using the following package version. |
You can test this PR using the following package version. |
You can test this PR using the following package version. |
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.
LGTM in general.
We probably need to auto-register options passed to the start method in the locator.
.SetupWithLifetime(lifetime); | ||
}); | ||
|
||
if (BrowserWindowingPlatform.IsManagedDispatcherEnabled) |
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 suspect that AvaloniaLocator.Current.GetService<BrowserPlatformOptions>()?.PreferManagedThreadDispatcher != false;
check will always succeed regardless of the options
parameter passed to StartBrowserAppAsync. Only one registered with .With
will be used
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.
We already have AvaloniaLocator.CurrentMutable.Bind<BrowserPlatformOptions>().ToConstant(options);
for that case. It's auto-registered.
// TODO: this callback should be <int, int, double>. Revert after next .NET 9 preview. | ||
Action<double, double, double> onSizeChanged); | ||
[JSExport] | ||
public static Task OnSizeChanged(int topLevelId, double width, double height, double dpr) |
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 that we are supposed to dispatch this synchronously in ST mode, maybe via Dispatcher.Invoke
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.
@@ -8,6 +10,13 @@ internal static partial class NavigationHelper | |||
[JSImport("NavigationHelper.addBackHandler", AvaloniaModule.MainModuleName)] | |||
public static partial void AddBackHandler([JSMarshalAs<JSType.Function<JSType.Boolean>>] Func<bool> backHandlerCallback); | |||
|
|||
public static Task<bool> OnBackRequested() |
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.
Probably need to use dispatcher for managed dispatcher mode.
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.
It is used down the lever here and in other services callbacks
Func<JSObject, bool> wheel); | ||
public static Task RedirectInputAsync(int topLevelId, Action<BrowserTopLevelImpl> handler) | ||
{ | ||
// TODO consider passing input events through dispatcher or something. |
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.
Probably an outdated comment, since InputHandler uses dispatcher internally when needed
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.
Yea
What does the pull request do?
Continuing #15709
Some points about WASM .NET multithreading and Avalonia after this PR:
Additionally, input handling should be generally faster now on single threaded platform too.