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.

Sign up for GitHub

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

Avoid CommandBarFlyoutCommandBar clipping after dynamic command bar element change #5347

Merged
merged 9 commits into from
Jul 7, 2021
Merged

Avoid CommandBarFlyoutCommandBar clipping after dynamic command bar element change #5347

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
937948d
32506205 fix - part 1
RBrid Jul 1, 2021
05e5662
Adding comments
RBrid Jul 2, 2021
33141f8
Adding new regression test
RBrid Jul 2, 2021
ad4b84b
Applying PR feedback
RBrid Jul 3, 2021
36add3b
Made couple of comments more precise
RBrid Jul 3, 2021
1d5e39a
Merge branch 'main' into user/regisb/CommandBarFlyoutCommandBarFix2
RBrid Jul 6, 2021
0458313
Adding #ifdef _DEBUG for traces
RBrid Jul 6, 2021
2b882ce
Fixing versions <= RS3
RBrid Jul 6, 2021
20933bd
Fixing RS2/3 KeyboardAcceleratorTextOverrideProperty usage
RBrid Jul 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 126 additions & 0 deletions dev/CommandBarFlyout/CommandBarFlyout.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,26 @@
bool CommandBarFlyoutTrace::s_IsDebugOutputEnabled{ false };
bool CommandBarFlyoutTrace::s_IsVerboseDebugOutputEnabled{ false };

// List of AppBarButton dependency properties being listened to for raising the CommandBarFlyoutCommandBar::OnCommandBarElementDependencyPropertyChanged notifications.
const winrt::DependencyProperty CommandBarFlyout::s_appBarButtonDependencyProperties[s_commandBarElementDependencyPropertiesCount]
{
winrt::AppBarButton::IconProperty(),
winrt::AppBarButton::IsCompactProperty(),
RBrid marked this conversation as resolved.
Show resolved Hide resolved
winrt::AppBarButton::KeyboardAcceleratorTextOverrideProperty(),
winrt::AppBarButton::LabelProperty(),
winrt::AppBarButton::LabelPositionProperty()
};

// List of AppBarToggleButton dependency properties being listened to for raising the CommandBarFlyoutCommandBar::OnCommandBarElementDependencyPropertyChanged notifications.
const winrt::DependencyProperty CommandBarFlyout::s_appBarToggleButtonDependencyProperties[s_commandBarElementDependencyPropertiesCount]
{
winrt::AppBarToggleButton::IconProperty(),
winrt::AppBarToggleButton::IsCompactProperty(),
winrt::AppBarToggleButton::KeyboardAcceleratorTextOverrideProperty(),
winrt::AppBarToggleButton::LabelProperty(),
winrt::AppBarToggleButton::LabelPositionProperty()
};

CommandBarFlyout::CommandBarFlyout()
{
__RP_Marker_ClassById(RuntimeProfiler::ProfId_CommandBarFlyout);
Expand Down Expand Up @@ -65,6 +85,17 @@ CommandBarFlyout::CommandBarFlyout()
auto button = element.try_as<winrt::AppBarButton>();
auto toggleButton = element.try_as<winrt::AppBarToggleButton>();

UnhookCommandBarElementDependencyPropertyChanges(index);

if (button)
{
HookAppBarButtonDependencyPropertyChanges(button, index);
}
else if (toggleButton)
{
HookAppBarToggleButtonDependencyPropertyChanges(toggleButton, index);
}

if (button && !button.Flyout())
{
m_secondaryButtonClickRevokerByIndexMap[index] = button.Click(winrt::auto_revoke, closeFlyoutFunc);
Expand All @@ -91,6 +122,15 @@ CommandBarFlyout::CommandBarFlyout()
auto button = element.try_as<winrt::AppBarButton>();
auto toggleButton = element.try_as<winrt::AppBarToggleButton>();

if (button)
{
HookAppBarButtonDependencyPropertyChanges(button, index);
}
else if (toggleButton)
{
HookAppBarToggleButtonDependencyPropertyChanges(toggleButton, index);
}

if (button && !button.Flyout())
{
m_secondaryButtonClickRevokerByIndexMap[index] = button.Click(winrt::auto_revoke, closeFlyoutFunc);
Expand All @@ -103,12 +143,15 @@ CommandBarFlyout::CommandBarFlyout()
break;
}
case winrt::CollectionChange::ItemRemoved:
UnhookCommandBarElementDependencyPropertyChanges(index);

SharedHelpers::EraseIfExists(m_secondaryButtonClickRevokerByIndexMap, index);
SharedHelpers::EraseIfExists(m_secondaryToggleButtonCheckedRevokerByIndexMap, index);
SharedHelpers::EraseIfExists(m_secondaryToggleButtonUncheckedRevokerByIndexMap, index);
break;
case winrt::CollectionChange::Reset:
SetSecondaryCommandsToCloseWhenExecuted();
HookAllCommandBarElementDependencyPropertyChanges();
break;
default:
MUX_ASSERT(false);
Expand Down Expand Up @@ -250,6 +293,8 @@ CommandBarFlyout::~CommandBarFlyout()
{
m_primaryCommands.VectorChanged(m_primaryCommandsVectorChangedToken);
m_secondaryCommands.VectorChanged(m_secondaryCommandsVectorChangedToken);

UnhookAllCommandBarElementDependencyPropertyChanges();
}

winrt::IObservableVector<winrt::ICommandBarElement> CommandBarFlyout::PrimaryCommands()
Expand All @@ -270,6 +315,7 @@ winrt::Control CommandBarFlyout::CreatePresenter()
SharedHelpers::CopyVector(m_secondaryCommands, commandBar->SecondaryCommands());

SetSecondaryCommandsToCloseWhenExecuted();
HookAllCommandBarElementDependencyPropertyChanges();

winrt::FlyoutPresenter presenter;
presenter.Background(nullptr);
Expand Down Expand Up @@ -407,3 +453,83 @@ tracker_ref<winrt::FlyoutPresenter> CommandBarFlyout::GetPresenter()
{
return m_presenter;
}

void CommandBarFlyout::HookAppBarButtonDependencyPropertyChanges(winrt::AppBarButton const& appBarButton, int index)
{
for (int commandBarElementDependencyPropertyIndex = 0; commandBarElementDependencyPropertyIndex < s_commandBarElementDependencyPropertiesCount; commandBarElementDependencyPropertyIndex++)
{
m_propertyChangedRevokersByIndexMap[index][commandBarElementDependencyPropertyIndex] =
RegisterPropertyChanged(
appBarButton,
s_appBarButtonDependencyProperties[commandBarElementDependencyPropertyIndex], { this, &CommandBarFlyout::OnCommandBarElementDependencyPropertyChanged });
}
}

void CommandBarFlyout::HookAppBarToggleButtonDependencyPropertyChanges(winrt::AppBarToggleButton const& appBarToggleButton, int index)
{
for (int commandBarElementDependencyPropertyIndex = 0; commandBarElementDependencyPropertyIndex < s_commandBarElementDependencyPropertiesCount; commandBarElementDependencyPropertyIndex++)
{
m_propertyChangedRevokersByIndexMap[index][commandBarElementDependencyPropertyIndex] =
RegisterPropertyChanged(
appBarToggleButton,
s_appBarToggleButtonDependencyProperties[commandBarElementDependencyPropertyIndex], { this, &CommandBarFlyout::OnCommandBarElementDependencyPropertyChanged });
}
}

void CommandBarFlyout::HookAllCommandBarElementDependencyPropertyChanges()
{
UnhookAllCommandBarElementDependencyPropertyChanges();

for (uint32_t i = 0; i < SecondaryCommands().Size(); i++)
{
auto element = SecondaryCommands().GetAt(i);
auto button = element.try_as<winrt::AppBarButton>();
auto toggleButton = element.try_as<winrt::AppBarToggleButton>();

if (button)
{
HookAppBarButtonDependencyPropertyChanges(button, i);
}
else if (toggleButton)
{
HookAppBarToggleButtonDependencyPropertyChanges(toggleButton, i);
}
}
}

void CommandBarFlyout::UnhookCommandBarElementDependencyPropertyChanges(int index, bool eraseRevokers)
{
const auto revokers = m_propertyChangedRevokersByIndexMap.find(index);
if (revokers != m_propertyChangedRevokersByIndexMap.end())
{
for (int commandBarElementDependencyPropertyIndex = 0; commandBarElementDependencyPropertyIndex < s_commandBarElementDependencyPropertiesCount; commandBarElementDependencyPropertyIndex++)
{
m_propertyChangedRevokersByIndexMap[index][commandBarElementDependencyPropertyIndex].revoke();
}

if (eraseRevokers)
{
m_propertyChangedRevokersByIndexMap.erase(revokers);
}
}
}

void CommandBarFlyout::UnhookAllCommandBarElementDependencyPropertyChanges()
{
for (auto const& revokers : m_propertyChangedRevokersByIndexMap)
{
UnhookCommandBarElementDependencyPropertyChanges(revokers.first, false /*eraseRevokers*/);
}
m_propertyChangedRevokersByIndexMap.clear();
}

// Let the potential CommandBarFlyoutCommandBar know of the dependency property change so it can adjust its size.
void CommandBarFlyout::OnCommandBarElementDependencyPropertyChanged(winrt::DependencyObject const& dependencyObject, winrt::DependencyProperty const& dependencyProperty)
{
COMMANDBARFLYOUT_TRACE_VERBOSE(*this, TRACE_MSG_METH, METH_NAME, this);

if (auto commandBar = winrt::get_self<CommandBarFlyoutCommandBar>(m_commandBar.get()))
{
commandBar->OnCommandBarElementDependencyPropertyChanged();
}
}
12 changes: 12 additions & 0 deletions dev/CommandBarFlyout/CommandBarFlyout.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,18 @@ class CommandBarFlyout :
tracker_ref<winrt::CommandBarFlyoutCommandBar> m_commandBar{ this };

private:
static constexpr int s_commandBarElementDependencyPropertiesCount{ 5 };

static const winrt::DependencyProperty s_appBarButtonDependencyProperties[s_commandBarElementDependencyPropertiesCount];
static const winrt::DependencyProperty s_appBarToggleButtonDependencyProperties[s_commandBarElementDependencyPropertiesCount];

void SetSecondaryCommandsToCloseWhenExecuted();
void HookAppBarButtonDependencyPropertyChanges(winrt::AppBarButton const& appBarButton, int index);
void HookAppBarToggleButtonDependencyPropertyChanges(winrt::AppBarToggleButton const& appBarToggleButton, int index);
void HookAllCommandBarElementDependencyPropertyChanges();
void UnhookCommandBarElementDependencyPropertyChanges(int index, bool eraseRevokers = true);
void UnhookAllCommandBarElementDependencyPropertyChanges();
void OnCommandBarElementDependencyPropertyChanged(winrt::DependencyObject const& dependencyObject, winrt::DependencyProperty const& dependencyProperty);

bool m_alwaysExpanded;

Expand All @@ -50,6 +61,7 @@ class CommandBarFlyout :
std::map<int, winrt::ButtonBase::Click_revoker> m_secondaryButtonClickRevokerByIndexMap;
std::map<int, winrt::ToggleButton::Checked_revoker> m_secondaryToggleButtonCheckedRevokerByIndexMap;
std::map<int, winrt::ToggleButton::Unchecked_revoker> m_secondaryToggleButtonUncheckedRevokerByIndexMap;
std::map<int, PropertyChanged_revoker[s_commandBarElementDependencyPropertiesCount]> m_propertyChangedRevokersByIndexMap;

tracker_ref<winrt::FlyoutPresenter> m_presenter{ this };

Expand Down
Loading