Skip to content
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

[Windows] Skip initial assignments in property mapper (take 3) #23987

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

MartyIX
Copy link
Contributor

@MartyIX MartyIX commented Aug 2, 2024

Description of Change

Alternative to #22108

Performance implications

image

image

image

image

image

Best time observed

image

BTW: In vanilla WinUI, the grid scenario takes ~100 ms. So MAUI is only about 50% slower than WinUI with this PR.

Issues Fixed

Contributes to #17303
Contributes to #21787

@MartyIX MartyIX requested a review from a team as a code owner August 2, 2024 18:48
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Aug 2, 2024
@MartyIX MartyIX marked this pull request as draft August 2, 2024 19:02
@@ -32,6 +32,8 @@ protected ElementHandler(IPropertyMapper mapper, CommandMapper? commandMapper =

public IElement? VirtualView { get; private protected set; }

internal bool _isPlatformViewNew;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main change.

I placed the property in ElementHandler because then it's not a public API change.

Copy link
Member

@mattleibow mattleibow Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this can be an internal property:

Suggested change
internal bool _isPlatformViewNew;
internal bool IsPlatformViewInitialized { get; }

But in a perfect world, where would this property go? Should this be on an interface or something to avoid the cast in each mapper? Pity we never added a isFirstRun property to the mappers... pity.

But maybe we just need a handler IsInitialized property to IElementHandler? Can we add that as a default interface implementation?

Copy link
Contributor Author

@MartyIX MartyIX Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this can be an internal property:

If you mean this:

private bool _isPlatformViewInitialized; // we can read & write
internal bool IsPlatformViewInitialized => _isPlatformViewInitialized; // not writable from outside

Yes. That should be possible.

But in a perfect world, where would this property go? Should this be on an interface or something to avoid the cast in each mapper? Pity we never added a isFirstRun property to the mappers... pity.

I think that in a perfect world, it would be passed through mappers. But unfortunately, I don't see a way to do it there.

But maybe we just need a handler IsInitialized property to IElementHandler? Can we add that as a default interface implementation?

If you mean this https://github.com/MartyIX/maui/tree/feature/2024-09-13-IsInitialized (MartyIX@bff5ee4), then it requires an API change and there would be many additional changes needed - see:

image

That is because .NET Standard 2.0 does not support default interface implementations (flashback).

@MartyIX MartyIX marked this pull request as ready for review September 3, 2024 19:38
@MartyIX MartyIX force-pushed the feature/2024-08-02-WinUI-IsPlatformViewNew-On-Handler-FINAL branch 2 times, most recently from 4d54f6e to f6c8ab0 Compare September 30, 2024 11:23
@MartyIX MartyIX force-pushed the feature/2024-08-02-WinUI-IsPlatformViewNew-On-Handler-FINAL branch from f6c8ab0 to d7de3ca Compare November 27, 2024 10:11
@MartyIX MartyIX force-pushed the feature/2024-08-02-WinUI-IsPlatformViewNew-On-Handler-FINAL branch from d7de3ca to 3bf5c12 Compare December 8, 2024 18:58
@PureWeen
Copy link
Member

PureWeen commented Dec 9, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@MartyIX MartyIX force-pushed the feature/2024-08-02-WinUI-IsPlatformViewNew-On-Handler-FINAL branch from 3bf5c12 to 53435ce Compare December 10, 2024 10:10
@MartyIX MartyIX force-pushed the feature/2024-08-02-WinUI-IsPlatformViewNew-On-Handler-FINAL branch from 53435ce to 2bb7162 Compare December 10, 2024 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-platform Integration with platforms community ✨ Community Contribution platform/windows 🪟
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants