Fix wrong item template selection in CmdPal (#9487)

There seems to be a bug in WinUI (see microsoft/microsoft-ui-xaml#2121)
that results in heterogeneous `ModernCollectionBasePanel`  configured
with `DataTemplateSelector` and virtualization enabled to recycle a
container even if its `ContentTemplate` is wrong.

I considered few options of handling this:
* Disabling virtualization (by replacing item container template with
  some non-virtualizing panel (e.g., `StackPanel`,
  `VirtualizingStackPanel` with `VirtualizationMode`=`Standard`)
* Replacing `DataTemplateSelector` approach with `ChoosingItemContainer`
  event handling approach, which allows you to manage the item container
  (`ListViewItem`) allocation process.

I have chosen the last one, as it should limit the amount of
allocations, and might allow optimizations in the future.
 
The solution introduces:
* A container for `ListViewItem`s in the form of a map of sets:
  * The key of this map is a data template (e.g., `TabItemDataTemplate`)
  * The value in the set is the container
* `ChoosingItemContainer` event handler that looks for available item in
  the container or creates a new one
* `ContainerContentChanging` event handler that returns the recycled
  item to the container

Closes #9288
This commit is contained in:
Don-Vito 2021-03-25 03:11:35 +02:00 committed by GitHub
parent da1e1a693e
commit e02d9a48e3
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 256 additions and 189 deletions

View file

@ -26,6 +26,9 @@ namespace winrt::TerminalApp::implementation
{
InitializeComponent();
_itemTemplateSelector = Resources().Lookup(winrt::box_value(L"PaletteItemTemplateSelector")).try_as<PaletteItemTemplateSelector>();
_listItemTemplate = Resources().Lookup(winrt::box_value(L"ListItemTemplate")).try_as<DataTemplate>();
_filteredActions = winrt::single_threaded_observable_vector<winrt::TerminalApp::FilteredCommand>();
_nestedActionStack = winrt::single_threaded_vector<winrt::TerminalApp::FilteredCommand>();
_currentNestedCommands = winrt::single_threaded_vector<winrt::TerminalApp::FilteredCommand>();
@ -1107,4 +1110,81 @@ namespace winrt::TerminalApp::implementation
{
_switchToMode(CommandPaletteMode::TabSearchMode);
}
// Method Description:
// - This event is triggered when filteredActionView is looking for item container (ListViewItem)
// to use to present the filtered actions.
// GH#9288: unfortunately the default lookup seems to choose items with wrong data templates,
// e.g., using DataTemplate rendering actions for tab palette items.
// We handle this event by manually selecting an item from the cache.
// If no item is found we allocate a new one.
// Arguments:
// - args: the ChoosingItemContainerEventArgs allowing to get container candidate suggested by the
// system and replace it with another candidate if required.
// Return Value:
// - <none>
void CommandPalette::_choosingItemContainer(
Windows::UI::Xaml::Controls::ListViewBase const& /*sender*/,
Windows::UI::Xaml::Controls::ChoosingItemContainerEventArgs const& args)
{
const auto dataTemplate = _itemTemplateSelector.SelectTemplate(args.Item());
const auto itemContainer = args.ItemContainer();
if (itemContainer && itemContainer.ContentTemplate() == dataTemplate)
{
// If the suggested candidate is OK simply remove it from the cache
// (so we won't allocate it until it is released) and return
_listViewItemsCache[dataTemplate].erase(itemContainer);
}
else
{
// We need another candidate, let's look it up inside the cache
auto& containersByTemplate = _listViewItemsCache[dataTemplate];
if (!containersByTemplate.empty())
{
// There cache contains available items for required DataTemplate
// Let's return one of them (and remove it from the cache)
auto firstItem = containersByTemplate.begin();
args.ItemContainer(*firstItem);
containersByTemplate.erase(firstItem);
}
else
{
ElementFactoryGetArgs factoryArgs{};
const auto listViewItem = _listItemTemplate.GetElement(factoryArgs).try_as<Controls::ListViewItem>();
listViewItem.ContentTemplate(dataTemplate);
if (dataTemplate == _itemTemplateSelector.NestedItemTemplate())
{
const auto helpText = winrt::box_value(RS_(L"CommandPalette_MoreOptions/[using:Windows.UI.Xaml.Automation]AutomationProperties/HelpText"));
listViewItem.SetValue(Automation::AutomationProperties::HelpTextProperty(), helpText);
}
args.ItemContainer(listViewItem);
}
}
args.IsContainerPrepared(true);
}
// Method Description:
// - This event is triggered when the data item associate with filteredActionView list item is changing.
// We check if the item is being recycled. In this case we return it to the cache
// Arguments:
// - args: the ContainerContentChangingEventArgs describing the container change
// Return Value:
// - <none>
void CommandPalette::_containerContentChanging(
Windows::UI::Xaml::Controls::ListViewBase const& /*sender*/,
Windows::UI::Xaml::Controls::ContainerContentChangingEventArgs const& args)
{
const auto itemContainer = args.ItemContainer();
if (args.InRecycleQueue() && itemContainer && itemContainer.ContentTemplate())
{
_listViewItemsCache[itemContainer.ContentTemplate()].insert(itemContainer);
itemContainer.DataContext(nullptr);
}
else
{
itemContainer.DataContext(args.Item());
}
}
}

View file

@ -133,6 +133,12 @@ namespace winrt::TerminalApp::implementation
Windows::Foundation::Collections::IVector<winrt::TerminalApp::FilteredCommand> _commandLineHistory{ nullptr };
::TerminalApp::AppCommandlineArgs _appArgs;
void _choosingItemContainer(Windows::UI::Xaml::Controls::ListViewBase const& sender, Windows::UI::Xaml::Controls::ChoosingItemContainerEventArgs const& args);
void _containerContentChanging(Windows::UI::Xaml::Controls::ListViewBase const& sender, Windows::UI::Xaml::Controls::ContainerContentChangingEventArgs const& args);
winrt::TerminalApp::PaletteItemTemplateSelector _itemTemplateSelector{ nullptr };
std::unordered_map<Windows::UI::Xaml::DataTemplate, std::unordered_set<Windows::UI::Xaml::Controls::Primitives::SelectorItem>> _listViewItemsCache;
Windows::UI::Xaml::DataTemplate _listItemTemplate;
friend class TerminalAppLocalTests::TabTests;
};
}

View file

@ -34,221 +34,201 @@ the MIT License. See LICENSE in the project root for license information. -->
<local:EmptyStringVisibilityConverter x:Key="ParentCommandVisibilityConverter"/>
<model:IconPathConverter x:Key="IconSourceConverter"/>
<DataTemplate x:Key="GeneralItemTemplate" x:DataType="local:FilteredCommand">
<!-- This HorizontalContentAlignment="Stretch" is important
to make sure it takes the entire width of the line -->
<DataTemplate x:Key="ListItemTemplate" x:DataType="local:FilteredCommand">
<ListViewItem HorizontalContentAlignment="Stretch"
AutomationProperties.Name="{x:Bind Item.Name, Mode=OneWay}"
AutomationProperties.AcceleratorKey="{x:Bind Item.KeyChordText, Mode=OneWay}">
AutomationProperties.AcceleratorKey="{x:Bind Item.KeyChordText, Mode=OneWay}"/>
</DataTemplate>
<Grid HorizontalAlignment="Stretch" ColumnSpacing="8" >
<Grid.ColumnDefinitions>
<ColumnDefinition Width="16"/>
<!-- icon -->
<ColumnDefinition Width="Auto"/>
<!-- command label -->
<ColumnDefinition Width="*"/>
<!-- key chord -->
<ColumnDefinition Width="16"/>
<!-- gutter for scrollbar -->
</Grid.ColumnDefinitions>
<DataTemplate x:Key="GeneralItemTemplate" x:DataType="local:FilteredCommand">
<Grid HorizontalAlignment="Stretch" ColumnSpacing="8">
<Grid.ColumnDefinitions>
<ColumnDefinition Width="16"/>
<!-- icon -->
<ColumnDefinition Width="Auto"/>
<!-- command label -->
<ColumnDefinition Width="*"/>
<!-- key chord -->
<ColumnDefinition Width="16"/>
<!-- gutter for scrollbar -->
</Grid.ColumnDefinitions>
<IconSourceElement
Grid.Column="0"
Width="16"
Height="16"
IconSource="{x:Bind Item.Icon,
Mode=OneWay,
Converter={StaticResource IconSourceConverter}}"/>
<IconSourceElement
Grid.Column="0"
Width="16"
Height="16"
IconSource="{x:Bind Item.Icon,
Mode=OneWay,
Converter={StaticResource IconSourceConverter}}"/>
<local:HighlightedTextControl
Grid.Column="1"
HorizontalAlignment="Left"
Text="{x:Bind HighlightedName, Mode=OneWay}"/>
<local:HighlightedTextControl
Grid.Column="1"
HorizontalAlignment="Left"
Text="{x:Bind HighlightedName, Mode=OneWay}"/>
<!-- The block for the key chord is only visible
when there's actual text set as the label. See
CommandKeyChordVisibilityConverter for details.
We're setting the accessibility view on the
border and text block to Raw because otherwise,
Narrator will read out the key chord. Problem is,
it already did that because it was the list item's
"AcceleratorKey". It's redundant. -->
<Border
Grid.Column="2"
Visibility="{x:Bind Item.KeyChordText,
Mode=OneWay,
Converter={StaticResource CommandKeyChordVisibilityConverter}}"
Style="{ThemeResource KeyChordBorderStyle}"
Padding="2,0,2,0"
HorizontalAlignment="Right"
VerticalAlignment="Center"
AutomationProperties.AccessibilityView="Raw">
<!-- The block for the key chord is only visible
when there's actual text set as the label. See
CommandKeyChordVisibilityConverter for details.
We're setting the accessibility view on the
border and text block to Raw because otherwise,
Narrator will read out the key chord. Problem is,
it already did that because it was the list item's
"AcceleratorKey". It's redundant. -->
<Border
Grid.Column="2"
Visibility="{x:Bind Item.KeyChordText,
Mode=OneWay,
Converter={StaticResource CommandKeyChordVisibilityConverter}}"
Style="{ThemeResource KeyChordBorderStyle}"
Padding="2,0,2,0"
HorizontalAlignment="Right"
VerticalAlignment="Center"
AutomationProperties.AccessibilityView="Raw">
<TextBlock
Style="{ThemeResource KeyChordTextBlockStyle}"
FontSize="12"
Text="{x:Bind Item.KeyChordText, Mode=OneWay}"
AutomationProperties.AccessibilityView="Raw" />
</Border>
</Grid>
</ListViewItem>
<TextBlock
Style="{ThemeResource KeyChordTextBlockStyle}"
FontSize="12"
Text="{x:Bind Item.KeyChordText, Mode=OneWay}"
AutomationProperties.AccessibilityView="Raw" />
</Border>
</Grid>
</DataTemplate>
<DataTemplate x:Key="NestedItemTemplate" x:DataType="local:FilteredCommand">
<Grid HorizontalAlignment="Stretch" ColumnSpacing="8">
<Grid.ColumnDefinitions>
<ColumnDefinition Width="16"/>
<!-- icon -->
<ColumnDefinition Width="Auto"/>
<!-- command label -->
<ColumnDefinition Width="*"/>
<!-- key chord -->
<ColumnDefinition Width="16"/>
<!-- gutter for scrollbar -->
</Grid.ColumnDefinitions>
<!-- This HorizontalContentAlignment="Stretch" is important
to make sure it takes the entire width of the line -->
<ListViewItem x:Uid="CommandPalette_MoreOptions"
HorizontalContentAlignment="Stretch"
AutomationProperties.Name="{x:Bind Item.Name, Mode=OneWay}"
AutomationProperties.AcceleratorKey="{x:Bind Item.KeyChordText, Mode=OneWay}">
<IconSourceElement
Grid.Column="0"
Width="16"
Height="16"
IconSource="{x:Bind Item.Icon,
Mode=OneWay,
Converter={StaticResource IconSourceConverter}}"/>
<Grid HorizontalAlignment="Stretch" ColumnSpacing="8" >
<Grid.ColumnDefinitions>
<ColumnDefinition Width="16"/>
<!-- icon -->
<ColumnDefinition Width="Auto"/>
<!-- command label -->
<ColumnDefinition Width="*"/>
<!-- key chord -->
<ColumnDefinition Width="16"/>
<!-- gutter for scrollbar -->
</Grid.ColumnDefinitions>
<local:HighlightedTextControl
Grid.Column="1"
HorizontalAlignment="Left"
Text="{x:Bind HighlightedName, Mode=OneWay}"/>
<IconSourceElement
Grid.Column="0"
Width="16"
Height="16"
IconSource="{x:Bind Item.Icon,
Mode=OneWay,
Converter={StaticResource IconSourceConverter}}"/>
<!-- The block for the key chord is only visible
when there's actual text set as the label. See
CommandKeyChordVisibilityConverter for details.
We're setting the accessibility view on the
border and text block to Raw because otherwise,
Narrator will read out the key chord. Problem is,
it already did that because it was the list item's
"AcceleratorKey". It's redundant. -->
<Border
Grid.Column="2"
Visibility="{x:Bind Item.KeyChordText,
Mode=OneWay,
Converter={StaticResource CommandKeyChordVisibilityConverter}}"
Style="{ThemeResource KeyChordBorderStyle}"
Padding="2,0,2,0"
HorizontalAlignment="Right"
VerticalAlignment="Center"
AutomationProperties.AccessibilityView="Raw">
<local:HighlightedTextControl
Grid.Column="1"
HorizontalAlignment="Left"
Text="{x:Bind HighlightedName, Mode=OneWay}"/>
<TextBlock
Style="{ThemeResource KeyChordTextBlockStyle}"
FontSize="12"
Text="{x:Bind Item.KeyChordText, Mode=OneWay}"
AutomationProperties.AccessibilityView="Raw" />
</Border>
<!-- The block for the key chord is only visible
when there's actual text set as the label. See
CommandKeyChordVisibilityConverter for details.
We're setting the accessibility view on the
border and text block to Raw because otherwise,
Narrator will read out the key chord. Problem is,
it already did that because it was the list item's
"AcceleratorKey". It's redundant. -->
<Border
Grid.Column="2"
Visibility="{x:Bind Item.KeyChordText,
Mode=OneWay,
Converter={StaticResource CommandKeyChordVisibilityConverter}}"
Style="{ThemeResource KeyChordBorderStyle}"
Padding="2,0,2,0"
HorizontalAlignment="Right"
VerticalAlignment="Center"
AutomationProperties.AccessibilityView="Raw">
<!-- xE70E is ChevronUp. Rotated 90 degrees, it's _ChevronRight_ -->
<FontIcon
FontFamily="Segoe MDL2 Assets"
Glyph="&#xE70E;"
HorizontalAlignment="Right"
Grid.Column="2">
<TextBlock
Style="{ThemeResource KeyChordTextBlockStyle}"
FontSize="12"
Text="{x:Bind Item.KeyChordText, Mode=OneWay}"
AutomationProperties.AccessibilityView="Raw" />
</Border>
<FontIcon.RenderTransform>
<RotateTransform CenterX="0.5" CenterY="0.5" Angle="90"/>
</FontIcon.RenderTransform>
</FontIcon>
<!-- xE70E is ChevronUp. Rotated 90 degrees, it's _ChevronRight_ -->
<FontIcon
FontFamily="Segoe MDL2 Assets"
Glyph="&#xE70E;"
HorizontalAlignment="Right"
Grid.Column="2">
<FontIcon.RenderTransform>
<RotateTransform CenterX="0.5" CenterY="0.5" Angle="90"/>
</FontIcon.RenderTransform>
</FontIcon>
</Grid>
</ListViewItem>
</Grid>
</DataTemplate>
<DataTemplate x:Key="TabItemTemplate" x:DataType="local:FilteredCommand">
<Grid HorizontalAlignment="Stretch" ColumnSpacing="8" AutomationProperties.Name="{x:Bind Item.Name, Mode=OneWay}">
<Grid.ColumnDefinitions>
<ColumnDefinition Width="16"/>
<!-- icon / progress -->
<ColumnDefinition Width="Auto"/>
<!-- command label -->
<ColumnDefinition Width="*"/>
<!-- gutter for indicators -->
<ColumnDefinition Width="Auto"/>
<!-- Indicators -->
<ColumnDefinition Width="16"/>
<!-- gutter for scrollbar -->
</Grid.ColumnDefinitions>
<!-- This HorizontalContentAlignment="Stretch" is important
to make sure it takes the entire width of the line -->
<ListViewItem HorizontalContentAlignment="Stretch"
AutomationProperties.Name="{x:Bind Item.Name, Mode=OneWay}"
AutomationProperties.AcceleratorKey="{x:Bind Item.KeyChordText, Mode=OneWay}">
<mux:ProgressRing
Grid.Column="0"
IsActive="{x:Bind Item.(local:TabPaletteItem.TabStatus).IsProgressRingActive, Mode=OneWay}"
Visibility="{x:Bind Item.(local:TabPaletteItem.TabStatus).IsProgressRingActive, Mode=OneWay}"
IsIndeterminate="{x:Bind Item.(local:TabPaletteItem.TabStatus).IsProgressRingIndeterminate, Mode=OneWay}"
Value="{x:Bind Item.(local:TabPaletteItem.TabStatus).ProgressValue, Mode=OneWay}"
MinHeight="0"
MinWidth="0"
Height="15"
Width="15"/>
<Grid HorizontalAlignment="Stretch" ColumnSpacing="8" >
<Grid.ColumnDefinitions>
<ColumnDefinition Width="16"/>
<!-- icon / progress -->
<ColumnDefinition Width="Auto"/>
<!-- command label -->
<ColumnDefinition Width="*"/>
<!-- gutter for indicators -->
<ColumnDefinition Width="Auto"/>
<!-- Indicators -->
<ColumnDefinition Width="16"/>
<!-- gutter for scrollbar -->
</Grid.ColumnDefinitions>
<IconSourceElement
Grid.Column="0"
Width="16"
Height="16"
IconSource="{x:Bind Item.Icon,
Mode=OneWay,
Converter={StaticResource IconSourceConverter}}"/>
<mux:ProgressRing
Grid.Column="0"
IsActive="{x:Bind Item.(local:TabPaletteItem.TabStatus).IsProgressRingActive, Mode=OneWay}"
Visibility="{x:Bind Item.(local:TabPaletteItem.TabStatus).IsProgressRingActive, Mode=OneWay}"
IsIndeterminate="{x:Bind Item.(local:TabPaletteItem.TabStatus).IsProgressRingIndeterminate, Mode=OneWay}"
Value="{x:Bind Item.(local:TabPaletteItem.TabStatus).ProgressValue, Mode=OneWay}"
MinHeight="0"
MinWidth="0"
Height="15"
Width="15"/>
<local:HighlightedTextControl
Grid.Column="1"
HorizontalAlignment="Left"
Text="{x:Bind HighlightedName, Mode=OneWay}"/>
<IconSourceElement
Grid.Column="0"
Width="16"
Height="16"
IconSource="{x:Bind Item.Icon,
Mode=OneWay,
Converter={StaticResource IconSourceConverter}}"/>
<StackPanel
Grid.Column="2"
VerticalAlignment="Center"
HorizontalAlignment="Right"
Orientation="Horizontal">
<local:HighlightedTextControl
Grid.Column="1"
HorizontalAlignment="Left"
Text="{x:Bind HighlightedName, Mode=OneWay}"/>
<FontIcon
FontFamily="Segoe MDL2 Assets"
Visibility="{x:Bind Item.(local:TabPaletteItem.TabStatus).BellIndicator, Mode=OneWay}"
Glyph="&#xEA8F;"
FontSize="12"
Margin="0,0,8,0"/>
<StackPanel
Grid.Column="2"
VerticalAlignment="Center"
HorizontalAlignment="Right"
Orientation="Horizontal">
<FontIcon
FontFamily="Segoe MDL2 Assets"
Visibility="{x:Bind Item.(local:TabPaletteItem.TabStatus).IsPaneZoomed, Mode=OneWay}"
Glyph="&#xE8A3;"
FontSize="12"
Margin="0,0,8,0"/>
<FontIcon
FontFamily="Segoe MDL2 Assets"
Visibility="{x:Bind Item.(local:TabPaletteItem.TabStatus).BellIndicator, Mode=OneWay}"
Glyph="&#xEA8F;"
FontSize="12"
Margin="0,0,8,0"/>
<FontIcon x:Name="HeaderLockIcon"
FontFamily="Segoe MDL2 Assets"
Visibility="{x:Bind Item.(local:TabPaletteItem.TabStatus).IsReadOnlyActive, Mode=OneWay}"
Glyph="&#xE72E;"
FontSize="12"
Margin="0,0,8,0"/>
<FontIcon
FontFamily="Segoe MDL2 Assets"
Visibility="{x:Bind Item.(local:TabPaletteItem.TabStatus).IsPaneZoomed, Mode=OneWay}"
Glyph="&#xE8A3;"
FontSize="12"
Margin="0,0,8,0"/>
<FontIcon x:Name="HeaderLockIcon"
FontFamily="Segoe MDL2 Assets"
Visibility="{x:Bind Item.(local:TabPaletteItem.TabStatus).IsReadOnlyActive, Mode=OneWay}"
Glyph="&#xE72E;"
FontSize="12"
Margin="0,0,8,0"/>
</StackPanel>
</Grid>
</ListViewItem>
</StackPanel>
</Grid>
</DataTemplate>
<local:PaletteItemTemplateSelector x:Key="PaletteItemTemplateSelector"
@ -498,8 +478,9 @@ the MIT License. See LICENSE in the project root for license information. -->
AllowDrop="False"
IsItemClickEnabled="True"
ItemClick="_listItemClicked"
ItemsSource="{x:Bind FilteredActions}"
ItemTemplateSelector="{StaticResource PaletteItemTemplateSelector}">
ChoosingItemContainer="_choosingItemContainer"
ContainerContentChanging="_containerContentChanging"
ItemsSource="{x:Bind FilteredActions}">
</ListView>
</Grid>