Terminal: Work with tabs rather than indices in Terminal Page

Created on 23 Nov 2020  路  8Comments  路  Source: microsoft/terminal

Description of the new feature/enhancement

The current implementation of tab management in TerminalPage uses indices as a way of communication (we remove tabs by index, we update tabs by index, we drag tabs by index, even switch to tab command stored within tab points to an index).

This creates numerous theoretical races (that materialize from time to time, at least when I am testing).
In addition, I would also expect that we will work only with a single source of truth, rather than managing both _tabs and _tabView.TabItems manually

I guess there might be a reason for the current approach of working with indices rather than tabs and for managing two collections, but I wasn't able to find it in a documentation

Proposed technical implementation details (optional)

  • Bind _tabView.TabItems property to _tabs
  • Don't work with indices, if you must work with index resolve it to the tab as soon as possible
Area-CodeHealth Help Wanted Issue-Task Product-Terminal

All 8 comments

Oh boy, does @leonMSFT have a story to share with you. He tried doing this earlier this year, but it ended up being a dead end. if I recall correctly, the way we're customizing the tabs prevented us from being able to bind them in XAML directly - something about adding the context menus, coloring, etc? I don't remember the details.

The original thread was over in #3922

@zadjii-msft - wow.. I see the threads.

I actually combined two independent questions together:

  1. Can we stop using index?
  2. Can we bind the the _tabs?
  3. @leonMSFT - can you please share the story? :blush:
  4. I understand that we need sometimes to access TabViewItem, but why does it prevent us from binding the collection (OneWay) so we do all collection level modifications (add, remove, reorder) on _tabs.

So, we have had issues binding the collection because it puts control of the TabViewItem out of our hands. We wanted to add a context menu (likely doable with binding) and customize some theme resources (likely less doable, but required because of tab color(1)), and the ListView recycling that TabView uses caused tabs to inherit colors from long-dead other tabs.

(1) we have a feature request on them to promote Background or Tint as a first-class offering, but as you might expect it is slow going 馃槃

So, we have had issues binding the collection because it puts control of the TabViewItem out of our hands. We wanted to add a context menu (likely doable with binding) and customize some theme resources (likely less doable, but required because of tab color(1)), and the ListView recycling that TabView uses caused tabs to inherit colors from long-dead other tabs.

(1) we have a feature request on them to promote Background or Tint as a first-class offering, but as you might expect it is slow going 馃槃

Yeah.. I see the PR: https://github.com/microsoft/microsoft-ui-xaml/pull/3216 (but it is idle for 2 months already).

What about the indices - I closed a bunch of tabs and dragged the remaining, found myself with 1 more tab than I expected.

I very much believe we should get off indices wherever possible, yes. :smile:

Then I will try to take a look at it 馃槉

Re: your assignment - no pressure of course :smile: We'll all be on holiday soon, so it's going to slow down around here!

Thanks again for finding these things and working to improve our code health/understandability/maintainability/features/pretty much everything!

Was this page helpful?
0 / 5 - 0 ratings