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

Navigationview: Fix init-once of the GlobalDependencyProperty #6762

Merged

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Feb 23, 2022

This commit fixes a bug in NavigationView where a GlobalDependencyProperty
was repeatedly instantiated in the constructor. Using the atomicity
of statics we ensure it's treated as a singleton instead.

Description

Additionally this commit makes a minor change to use
references during loop iteration to avoid copies

Motivation and Context

Related to #6240, which introduced this code.
Closes #6760.

@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Feb 23, 2022
@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lhecker
Copy link
Member Author

lhecker commented Feb 23, 2022

Apparently you can't hash cppwinrt types. I guess you have to wrap them with a com_ptr<> first?
-> Change reverted.

BTW I've figured out why I couldn't build this project. This is ill-formed:

explicit tracker_ref(nullptr_t)
{
static_assert(false, "tracker_ref should only be used in ReferenceRuntimeClass-derived field members and 'this' should be passed for the owner");
}

I don't know the exact language spec that makes this so, but since the assertion isn't dependent on a template parameter, the compiler can eagerly reject the code here. This will fail to compile with the v143 toolchain and later.
The solution is to simply make the expression depend on T, for instance using sizeof(T) == 0.

@lhecker lhecker force-pushed the dev/lhecker/fix-navigation-view branch from 9c2f7cc to b759b1d Compare February 23, 2022 04:27
@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters
Copy link
Contributor

@lhecker looks good to me!

@lhecker lhecker marked this pull request as ready for review February 28, 2022 20:40
@ranjeshj ranjeshj added area-NavigationView NavView control team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Mar 2, 2022
@StephenLPeters StephenLPeters merged commit 0b8152b into microsoft:main Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NavigationView NavView control team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NavigationView s_NavigationViewItemRevokersPropertySet can be set multiple times
3 participants