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.

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

Work with tabs rather than indices in Terminal Page #8374

Closed
Don-Vito opened this issue Nov 23, 2020 · 8 comments
Closed

Work with tabs rather than indices in Terminal Page #8374

Don-Vito opened this issue Nov 23, 2020 · 8 comments
Assignees
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@Don-Vito
Copy link
Contributor

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
@Don-Vito Don-Vito added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Nov 23, 2020
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Nov 23, 2020
@zadjii-msft
Copy link
Member

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

@Don-Vito
Copy link
Contributor Author

@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?
  • @leonMSFT - can you please share the story? 😊
  • 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.

@DHowett
Copy link
Member

DHowett commented Nov 23, 2020

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 😄

@Don-Vito
Copy link
Contributor Author

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: microsoft/microsoft-ui-xaml#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.

@DHowett
Copy link
Member

DHowett commented Nov 23, 2020

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

@Don-Vito
Copy link
Contributor Author

Then I will try to take a look at it 😊

@DHowett DHowett added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Nov 24, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 24, 2020
@DHowett DHowett added this to the Terminal Backlog milestone Nov 24, 2020
@DHowett
Copy link
Member

DHowett commented Nov 24, 2020

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

@DHowett
Copy link
Member

DHowett commented Nov 24, 2020

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

@ghost ghost added the In-PR This issue has a related PR label Dec 17, 2020
@ghost ghost removed the In-PR This issue has a related PR label Dec 25, 2020
@ghost ghost added the In-PR This issue has a related PR label Apr 22, 2021
@ghost ghost closed this as completed in 51920d9 Apr 23, 2021
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Apr 23, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. 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
Development

Successfully merging a pull request may close this issue.

3 participants