_tabView.Items() and _tabs should not need to be kept in sync manually #2740
Assignees
Labels
Area-CodeHealth
Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc.
Issue-Task
It's a feature request, but it doesn't really need a major design.
Needs-Tag-Fix
Doesn't match tag requirements
Product-Terminal
The new Windows Terminal.
Milestone
Comments
zadjii-msft
added
Product-Terminal
ghost
added
the
Needs-Triage
zadjii-msft
removed
the
Needs-Triage
|
yes please! If we make Tab a WinRT runtime class or something, we can probably even use it as the TabView Item. |
ghost
pushed a commit
that referenced
this issue
Feb 4, 2020
## Summary of the Pull Request This PR will make the existing `Tab` class into a WinRT type. This will allow any XAML to simply bind to the `ObservableVector` of Tabs. This PR will be followed up with a future PR to change our TabView to use the ObservableVector, which will in turn eliminate the need for maintaining two vectors of Tabs. (We currently maintain `_tabs` in `TerminalPage` and we also maintain `TabView().TabViewItems()` at the same time as described here: #2740) ## References #3922 ## PR Checklist * [x] CLA signed. * [x] Tests added/passed ## Detailed Description of the Pull Request / Additional comments I've currently only exposed a Tab's Title and IconPath to keep things simple. I foresee XAML elements that bind to Tabs to only really need these two properties for displaying. I've also converted `TerminalPage`'s `std::vector<std::shared_ptr> _tabs` into a `IObservableVector<winrt::TerminalPage::Tab> _tabs` just so that future PRs will have the ground set for binding to this vector of tabs. ## Validation Steps Performed Played around with Tabs and Panes and all sorts of combinations of keybindings for interacting with tabs and dragging and whatnot, it all seemed fine! Tab Tests also all pass.
|
Closing because we actually need access to the TabViewItem to perform some of our custom modifications like tab color and tab title boxes (and maybe future modifications), and so letting us have total control of our TabViewItems is needed, so this in fact is the way |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Right now in TerminalPage, we have two separate vectors of "tabs" that need to be kept in sync:
vector<shared_ptr<Tab>> _tabs_tabViewof all theTabViewItems.This is kinda silly, we should probably just use the
_tabView.Items()vector, and more closely associate theTabwith them.Case in point is
TerminalPage::_RemoveTabViewItem, where we need to both remove the tab from both the tabs and the tabView.Items().We could probably make tabs an observable vector or something, so the tabview is inherently tied to the tabs collection. I did a bit of similar prototyping in the command palette prototype, see #2046 for that branch.
The text was updated successfully, but these errors were encountered: