Skip to content

Conversation

@Scottj1s
Copy link
Member

@Scottj1s Scottj1s commented May 17, 2022

fixes #1140

This change adds a helper, ComponentConnectorT, to resolve the breaking change introduced with #1140. Note that this is manual opt-in for the developer, and not an automatic fix, as that would require coordinated work with the Xaml Compiler. I don't think it's worth the effort to do that, for several reasons:

When deriving from a base class with markup, IComponentConnector::Connect has never been reliable outside construction, such as when called back from FindName. If a base class has a property with both the same type (e.g., TextBlock) and connectionId as a derived class, then Connect would erroneously call the derived property setter. The same risk applies to DisconnectUnloadedObject. This was the opposite of the non-virtual dispatch that InitializeComponent was previously relying on. At steady state (outside construction), virtual dispatch would be functional and the derived Connect would always be called. However, unlike with calls from within InitializeComponent, where _dispatch_base can be used to correctly dispatch callbacks, there is no way to know how to do this with Xaml runtime calls for FindName (x:Load and template apply scenarios). The risk exists in both directions - a base property can erroneously override a derived property and vice versa.

For that reason, the Xaml team does not recommend bases with markup, although there is nothing to prevent that or inform the developer of the risks (see related discussion). To preserve the existing (risky) behavior outside construction, _dispatch_base defaults to false to dispatch to the derived class.

@Scottj1s Scottj1s requested a review from kennykerr May 17, 2022 21:45
@sylveon
Copy link
Contributor

sylveon commented May 17, 2022

Seems good to me - although there should probably be some unit testing for it. I suppose this works if we have more than 2 classes in the hierarchy with XAML markup? E.g. BaseClass -> DerivedClass -> FurtherDerivedClass. I don't actually do that but it's probably something that others might do.

@Scottj1s
Copy link
Member Author

Seems good to me - although there should probably be some unit testing for it. I suppose this works if we have more than 2 classes in the hierarchy with XAML markup? E.g. BaseClass -> DerivedClass -> FurtherDerivedClass. I don't actually do that but it's probably something that others might do.

Yes, will work on adding unit tests when time permits. And yes, I tested your scenario of 3 classes in the hierarchy to ensure it was fully general.

kennykerr
kennykerr previously approved these changes May 24, 2022
Copy link
Collaborator

@kennykerr kennykerr left a comment

Choose a reason for hiding this comment

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

Looks reasonable. I don't claim to understand all the Xaml fanciness going on here, perhaps @oldnewthing can take a peek.

@Scottj1s Scottj1s requested a review from oldnewthing May 24, 2022 16:06
@Scottj1s
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Scottj1s Scottj1s merged commit dcea885 into master May 24, 2022
@Scottj1s Scottj1s deleted the scottj1s/component_connector branch May 24, 2022 21:58
@mikrause79
Copy link

Did this cause:
#1155

@sylveon
Copy link
Contributor

sylveon commented Jun 6, 2022

Yes, that appears to be it. It seems we'd need to either have a different string for Microsoft.UI.Xaml, or have the current one be able to detect if IComponentConnector2 actually exists in the current namespace. I think the former is better, since WinUI 3 is bound to diverge more over time while system XAML is essentially frozen at this point.

Ideally this is a thing that WinUI 3 would ship by itself, but the code makes use of some impl stuff, and I think creating a dependency on cppwinrt's implementation details within WinUI 3 to not be a great idea (especially since future endeavors like C++20 modules support will make the impl namespace entirely invisible to consumers).

@Scottj1s what approach is the best here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

InitializeComponent is not called for base classes.

5 participants