Expose AutomationOption Setter for ChildSite#15622
Expose AutomationOption Setter for ChildSite#15622vineethkuttan wants to merge 6 commits intomicrosoft:mainfrom
Conversation
2fa1a9f to
3541a3a
Compare
|
We shouldn't be adding Xaml specific code like this. |
i) will do it a seperate PR
@acoates-ms exposed the setter for ChildSite Automation |
sundaramramaswamy
left a comment
There was a problem hiding this comment.
Signed off with comments. Please handle them.
| m_visualToMountChildrenIntoHandler; | ||
| UpdateLayoutMetricsDelegate m_updateLayoutMetricsHandler; | ||
| bool m_xamlSupport{false}; | ||
| std::optional<winrt::Microsoft::UI::Content::ContentAutomationOptions> m_contentIslandChildSiteAutomationOption; |
There was a problem hiding this comment.
Why is this an optional? Is it because ContentAutomationOptions::None can be set by the 3P module and we want to differentiate that case from not being set all?
There was a problem hiding this comment.
Yes Sundaram. Since it is enum class , it will have default value which is None. So added optional to differentiate that case from not being set at all.
Or I may need to set the default value as frameworkbased in the contructor.
| if (m_builder && m_builder->ContentIslandChildSiteAutomationOption().has_value()) { | ||
| automationOption = m_builder->ContentIslandChildSiteAutomationOption().value(); | ||
| } |
There was a problem hiding this comment.
| if (m_builder && m_builder->ContentIslandChildSiteAutomationOption().has_value()) { | |
| automationOption = m_builder->ContentIslandChildSiteAutomationOption().value(); | |
| } | |
| if (m_builder->ContentIslandChildSiteAutomationOption()) { | |
| automationOption = *m_builder->ContentIslandChildSiteAutomationOption(); | |
| } |
-
Can
m_builderever benullptrwithin the lifetime of this object? If yes, then won't every usage of this member variable be starting with aif (m_builder)check? That would be unperformant, ugly and noisy, yes? -
std::optional::operator boolautomatically does whathas_value()does. Excerpt fromstd::optional:
When an object of type optional is contextually converted to bool, the conversion returns true if the object contains a value and false if it does not contain a value.
operator*returns the value within.
Even better is to use std::optional::or_value which gives the value if there's a value else defaults to whatever you pass it.
const winrt::Microsoft::UI::Content::ContentAutomationOptions automationOption = m_builder->ContentIslandChildSiteAutomationOption().value_or(winrt::Microsoft::UI::Content::ContentAutomationOptions::FrameworkBased);Shorter, cleaner and clearer to read, no? As a bonus since you initialize the variable in one statement, it can be qualified const now.
Moral: It helps to read the documentation and many things are perhaps already done for you. You just have to use them, for that you've to know them.
Description
#15611 This PR removed fragment based automation for content island that is connected using childsite.
This PR is exposes the automation option setter in builder.
Type of Change
Erase all that don't apply.
Why
This will make sure that other content island which will connected in childsite can customize their automation option

What
Screenshots
xamlws.mp4
Testing
Tested in the SampleFabricApp
Changelog
Should this change be included in the release notes: yes
Expose AutomationOption Setter for ChildSite
Microsoft Reviewers: Open in CodeFlow