Fix wrong item template selection in CmdPal #9487

Don-Vito · 2021-03-13T20:39:05Z

PR Checklist

Closes [1.7] Tab indicators in the cmdpal aren't working (sometimes?) #9288
CLA signed.
Tests added/passed
Documentation updated.
Schema updated.
I've discussed this with core contributors already.

Detailed Description of the Pull Request / Additional comments

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 ListViewItems 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

zadjii-msft

Well this is mental. But a good workaround! Thanks!

zadjii-msft · 2021-03-15T14:09:21Z

src/cascadia/TerminalApp/CommandPalette.xaml

            </DataTemplate>

            <DataTemplate x:Key="NestedItemTemplate" x:DataType="local:FilteredCommand">
+                <Grid HorizontalAlignment="Stretch" ColumnSpacing="8" AutomationProperties.Name="{x:Bind Item.Name, Mode=OneWay}" AutomationProperties.AcceleratorKey="{x:Bind Item.KeyChordText, Mode=OneWay}">


Are we losing the HelpText (More options) from x:Uid="CommandPalette_MoreOptions"

Yeah -- we gotta keep this. I'll block for the answer. 😄

I'm a bit worried we're losing a couple things by leaving ListViewItem behind. Hmm. Can the ChoosingXyz event not return a ListViewItem?

ChoosingItemContainer chooses an item container 😄 or more precisely - SelectorItem, which is a parent for *ViewItem.

I mean I played with Narrator and it felt OK.. but probably I missed stuff (my mind was erased while debugging WinUI).

What do you believe will be broken? How can I test that? I probably won't be able to make ChoosingItemContainer return something else, but if you tell me what the concern is, I might find an answer 😊

I mean I lost MoreOptions (I was not aware of it unfortunately). I guess I can try to fix this 😊 But are there additional concerns?

I will fix this by binding the automation properties in the code behind, and resubmit. Sorry.

We're getting to the things I hate about XAML. It's like... we did all this work to get away from manually setting up specific values in the codebehind or in the xaml document for specific types of items, right? It was good and clean and smart: the DataTemplate provided everything we needed for the data type. And here, we're forced to add detection to ChoosingItemContainer (sp) to set up the container correctly based on the data type. Something the data template should be able to handle. All because of a silly issue. 😄

I have expressed my distaste in the linked Xaml issue. Thanks for debugging through WinUI so much. 😀 It can be enough to drive a person mad.

Okay, one last e-mail. It does not work if we add x:Uid="CommandPalette_MoreOptions" to the Grid?

No, it doesn't. There is no AutomationPeer for the grid. I am still wondering why the text is read correctly. Most probably due to the default automation peer on the internal text block.

DHowett

as mentioned

Don-Vito · 2021-03-22T22:08:53Z

@DHowett - please re-review 😊 when you can. Currently main is quite broken without it!

DHowett · 2021-03-25T00:42:17Z

src/cascadia/TerminalApp/CommandPalette.cpp

+
+                if (dataTemplate == _itemTemplateSelector.NestedItemTemplate())
+                {
+                    const auto helpText = winrt::box_value(RS_(L"CommandPalette_MoreOptions"));


Just checking (and I will merge this before you reply! (unless you are quite fast!))

This is fine, and we never need to clear the Automation Property, because it is only ever applied to items that use the NestedItemTemplate template? And the cache stores them separately. Okay. I've talked myself into it.

src/cascadia/TerminalApp/CommandPalette.cpp

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

DHowett · 2021-03-25T00:49:15Z

src/cascadia/TerminalApp/CommandPalette.xaml

-
-                    </Grid>
-                </ListViewItem>
+                                  AutomationProperties.AcceleratorKey="{x:Bind Item.KeyChordText, Mode=OneWay}"/>


I'm surprised this binding actually works! It's on the parent ListViewItem, but that ListViewItem is never bound to the actual object in our code. Is this just something that ListView handles for us?

DHowett

I'm testing this out locally, but I expect to merge it!

DHowett · 2021-03-25T01:10:37Z

As expected, it is perfect.

DHowett · 2021-03-25T01:10:49Z

Thank you @Don-Vito. For both fixing this and putting up with my concerns ALL THE TIME

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 (cherry picked from commit e02d9a4)

ghost · 2021-04-14T17:02:18Z

🎉Windows Terminal v1.7.1033.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2021-04-14T17:05:39Z

🎉Windows Terminal Preview v1.8.1032.0 has been released which incorporates this pull request.:tada:

Handy links:

Surprise surprise! we ran into an old friend: * #9288 * #9487 * microsoft/microsoft-ui-xaml#2121 so uh, this is ded.

Don-Vito added 2 commits March 13, 2021 21:59

Fix wrong item template selection in CmdPal

699c8d6

Document new event handlers

8364f50

ghost added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Mar 13, 2021

zadjii-msft approved these changes Mar 15, 2021

View reviewed changes

DHowett requested changes Mar 15, 2021

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Mar 15, 2021

Don-Vito marked this pull request as draft March 16, 2021 00:52

DHowett mentioned this pull request Mar 16, 2021

TreeView with DataTemplateSelector not reliably using selected DataTemplate microsoft/microsoft-ui-xaml#2121

Open

Fix automation peer

ed00175

Don-Vito marked this pull request as ready for review March 21, 2021 00:12

Don-Vito requested a review from DHowett March 21, 2021 00:13

DHowett reviewed Mar 25, 2021

View reviewed changes

src/cascadia/TerminalApp/CommandPalette.cpp Outdated Show resolved Hide resolved

DHowett reviewed Mar 25, 2021

View reviewed changes

src/cascadia/TerminalApp/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved

DHowett reviewed Mar 25, 2021

View reviewed changes

Return resource key to save localization burden

7a1d3b0

DHowett approved these changes Mar 25, 2021

View reviewed changes

DHowett added the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label Mar 25, 2021

DHowett merged commit e02d9a4 into microsoft:main Mar 25, 2021

zadjii-msft mentioned this pull request Mar 29, 2021

Inexplicable crash in command palette #9648

Closed

DHowett removed the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label Apr 2, 2021

zadjii-msft added the zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. label Apr 6, 2021

DHowett removed the zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. label Apr 13, 2021

ghost mentioned this pull request Apr 14, 2021

[1.7] Tab indicators in the cmdpal aren't working (sometimes?) #9288

Closed

zadjii-msft added a commit that referenced this pull request Jun 10, 2024

Try to do the DataTemplate thing, and fail.

def4190

Surprise surprise! we ran into an old friend: * #9288 * #9487 * microsoft/microsoft-ui-xaml#2121 so uh, this is ded.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix wrong item template selection in CmdPal #9487

Fix wrong item template selection in CmdPal #9487

Don-Vito commented Mar 13, 2021 •

edited

Loading

zadjii-msft left a comment

zadjii-msft Mar 15, 2021

DHowett Mar 15, 2021

DHowett Mar 15, 2021

Don-Vito Mar 15, 2021 •

edited

Loading

Don-Vito Mar 15, 2021

Don-Vito Mar 16, 2021

DHowett Mar 16, 2021 •

edited

Loading

DHowett Mar 16, 2021

DHowett Mar 16, 2021

Don-Vito Mar 16, 2021

DHowett left a comment

Don-Vito commented Mar 22, 2021

DHowett Mar 25, 2021 •

edited

Loading

DHowett Mar 25, 2021

DHowett left a comment

DHowett commented Mar 25, 2021

DHowett commented Mar 25, 2021

ghost commented Apr 14, 2021

ghost commented Apr 14, 2021

Fix wrong item template selection in CmdPal #9487

Fix wrong item template selection in CmdPal #9487

Conversation

Don-Vito commented Mar 13, 2021 • edited Loading

PR Checklist

Detailed Description of the Pull Request / Additional comments

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Don-Vito Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

Don-Vito commented Mar 22, 2021

DHowett Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented Mar 25, 2021

DHowett commented Mar 25, 2021

ghost commented Apr 14, 2021

ghost commented Apr 14, 2021

Don-Vito commented Mar 13, 2021 •

edited

Loading

Don-Vito Mar 15, 2021 •

edited

Loading

DHowett Mar 16, 2021 •

edited

Loading

DHowett Mar 25, 2021 •

edited

Loading