-
Notifications
You must be signed in to change notification settings - Fork 678
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
Fix issue with TabPaneFooter Width/MinWidth not being respected #3531
Fix issue with TabPaneFooter Width/MinWidth not being respected #3531
Conversation
dev/TabView/TabView.cpp
Outdated
} | ||
if (const auto newFooter = args.NewValue().try_as<winrt::FrameworkElement>()) | ||
{ | ||
m_footerMinWidthProperyChangedToken = newFooter.RegisterPropertyChangedCallback(winrt::FrameworkElement::MinWidthProperty(), { this,&TabView::OnComponentSizeChanged }); |
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.
can these be done using a revoker ?
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 don't think that's possible. We need to listen to more or less generic dependency property changes, there is no dedicated (Min)WidthPropertyChanged event, so we need to use that route. Unfortunately, RegisterPropertyChangedCallback only works through tokens.
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.
@chingucoding please use the cpp winrt helper RegisterPropertyChanged
microsoft-ui-xaml/dev/inc/CppWinRTHelpers.h
Line 280 in cc0d193
inline PropertyChanged_revoker RegisterPropertyChanged(winrt::DependencyObject const& object, winrt::DependencyProperty const& dp, winrt::DependencyPropertyChangedCallback const& callback) |
In reply to: 528202327 [](ancestors = 528202327)
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.
Sure, wish I knew that existed earlier. Is fixed now.
I also noticed that a lot of the code uses the token pattern to do this, should we updated all occurrences to the helper instead of a token?
dev/TabView/TabView.cpp
Outdated
} | ||
if (const auto newFooter = args.NewValue().try_as<winrt::FrameworkElement>()) | ||
{ | ||
m_footerMinWidthProperyChangedToken = newFooter.RegisterPropertyChangedCallback(winrt::FrameworkElement::MinWidthProperty(), { this,&TabView::OnComponentSizeChanged }); |
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.
OnComponentSizeChanged [](start = 150, length = 22)
nit: Component sounds very generic. perhaps OnFooterSizeChanged ?
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.
Good idea, changed the name to OnFooterSizeChanged.
dev/TabView/TabView.cpp
Outdated
UnhookEventsAndClearFields(); | ||
if (const auto footer = TabStripFooter().try_as<winrt::FrameworkElement>()) | ||
{ | ||
footer.UnregisterPropertyChangedCallback(winrt::FrameworkElement::MinWidthProperty(), m_footerMinWidthProperyChangedToken); |
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.
m_footerMinWidthProperyChangedToken [](start = 94, length = 35)
Do we need to check to see if this token is not 0?
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 don't think it can be 0 if we actually get in there since that would require us to have a footer, yet not having registered to said footer. Anytime the footer changes, we update the tokens so we would need to get here with footer changes being unnoticed by the TabView.
3b346e3
to
7e73dc5
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Description
The layout of the TabView didn't take into account changes of the Width/MinWidth property of the TabPaneFooter. This PR changes that and listens to changes made to those two properties.
Motivation and Context
Closes #1615 , not sure if would close issue #3280
How Has This Been Tested?
Add new API test, also checked in the tear out sample of the XCG for future testing of that scenario (e.g. new titlebar api for TabView)
Screenshots (if appropriate):