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

Jump to bottom

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

Closed
zadjii-msft opened this issue Feb 25, 2021 · 10 comments
Closed

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

zadjii-msft opened this issue Feb 25, 2021 · 10 comments
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v1.8

Comments

@zadjii-msft
Copy link
Member

zadjii-msft commented Feb 25, 2021 •
edited
Loading

Build is 1.7.552.0.

  • run wt ; nt
  • mark the second pane as read only
  • open the tab switcher

You should see a lock:
image

But sometimes you don't?
image

On the debug build, I wasn't able to get them to appear, until I stepped through it once, and now they always appear. Like, no launches of the terminal ever had them, now all launches of the terminal have them

When I was first debugging through this, it looked like PaletteItemTemplateSelector::_TabItemTemplate was null. However when debugging the release build that doesn't repro this, _TabItemTemplate definitely isn't null, so that can't be it.
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Feb 25, 2021
@Don-Vito
Copy link
Contributor

Don-Vito commented Feb 25, 2021 •
edited
Loading

I hope to get back to work on Terminal - I am missing all the action

@zadjii-msft zadjii-msft added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Help Wanted We encourage anyone to jump in on these. 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 Feb 25, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Feb 25, 2021
@zadjii-msft zadjii-msft added this to the Terminal v1.8 milestone Feb 25, 2021
@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Feb 25, 2021
@zadjii-msft
Copy link
Member Author

woah today I got a new variation of this:

image

That's the tab switcher, and one of the tabs has the nested command arrow in it. That's not right 😮

@Don-Vito
Copy link
Contributor

woah today I got a new variation of this:

image

That's the tab switcher, and one of the tabs has the nested command arrow in it. That's not right 😮

Holly mother of God.. I will take a look..

@Don-Vito
Copy link
Contributor

Don-Vito commented Mar 11, 2021 •
edited
Loading

So it appears that ListView (_filteredActionViews) recycles ListViewItems. 🤦

To reproduce the problem simply:

  1. Start terminal
  2. Open command palette
  3. Open tab with two panes, and zoom one
  4. Open another tab
  5. Switch between the tabs

No zoom icon will be shown.

Still considering how to fix this.

@DHowett
Copy link
Member

DHowett commented Mar 12, 2021

HUH. I would have expected ListViewItems to be recycled only based on data template (!!!) so this is sorta scary.

@DHowett
Copy link
Member

DHowett commented Mar 12, 2021

Ah. "Container-recycling with heterogeneous collections"

ChoosingItemContainer is an event that allows you to provide an item (ListViewItem/GridViewItem) to the ListView/GridView whenever a new item is needed during start-up or recycling. You can create a container based on the type of data item the container will display (shown in the example below). ChoosingItemContainer is the higher-performing way to use different data templates for different items.

@Don-Vito
Copy link
Contributor

HUH. I would have expected ListViewItems to be recycled only based on data template (!!!) so this is sorta scary.

According to the documentation it does:

When recycling an item (ListViewItem/GridViewItem), the framework must decide whether the items that are available for use in the recycle queue (the recycle queue is a cache of items that are not currently being used to display data) have an item template that will match the one desired by the current data item. If there are no items in the recycle queue with the appropriate item template then a new item is created, and the appropriate item template is instantiated for it. If, on other hand, the recycle queue contains an item with the appropriate item template then that item is removed from the recycle queue and is used for the current data item. An item template selector works in situations where only a small number of item templates are used and there is a flat distribution throughout the collection of items that use different item templates.

@Don-Vito
Copy link
Contributor

Don-Vito commented Mar 12, 2021 •
edited
Loading

The issues seems to be somehow related to virtualization.
See: microsoft/microsoft-ui-xaml#2121.

Though looking at ItemsControl::DataTemplateSelectorRecyclingContext::IsCompatible (in /onecoreuap/windows/dxaml/xcp/dxaml/lib/ItemsControl_Partial.cpp) I cannot find an issue 😕

After debugging ModernCollectionBasePanel for two hours I still cannot understand what's going on there (each function is like 500 lines) 😄.
Currently my theory (very preliminary) is that

  • IsCompatible check is getting invoked all the time.
  • But if we don't find compatible container, we repurpose an existing container (i.e., the template is changed)
  • But UI elements are not modified.

@DHowett - I tried disabling virtualization (by overriding the stack panel with VirtualizingStackPanel in VirtualizationMode="Standard"). It solved the problem (but for some reason screwed a horizontal alignment a bit - I guess it is fixable). WDYT?

@Don-Vito Don-Vito mentioned this issue Mar 13, 2021
Merged
6 tasks
@ghost ghost added the In-PR This issue has a related PR label Mar 13, 2021
@ghost ghost removed the In-PR This issue has a related PR label Mar 25, 2021
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Needs-Tag-Fix Doesn't match tag requirements labels Mar 25, 2021
DHowett pushed a commit that referenced this issue Apr 2, 2021
@Don-Vito @DHowett
Fix wrong item template selection in CmdPal (#9487)
3ca5374 
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
Copy link

ghost commented Apr 14, 2021

🎉This issue was addressed in #9487, which has now been successfully released as Windows Terminal v1.7.1033.0.:tada:

Handy links:

@ghost
Copy link

ghost commented Apr 14, 2021

🎉This issue was addressed in #9487, which has now been successfully released as Windows Terminal Preview v1.8.1032.0.:tada:

Handy links:

zadjii-msft added a commit that referenced this issue Jun 10, 2024
@zadjii-msft
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v1.8
Development

No branches or pull requests
3 participants
@DHowett @Don-Vito @zadjii-msft