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

Bind TerminalPage's TabView to TerminalPage::_tabs #3922

Closed
leonMSFT opened this issue Dec 11, 2019 · 3 comments
Closed

Bind TerminalPage's TabView to TerminalPage::_tabs #3922

leonMSFT opened this issue Dec 11, 2019 · 3 comments
Assignees
@leonMSFT
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
Terminal v2.0

Comments

@leonMSFT
Copy link
Contributor

leonMSFT commented Dec 11, 2019 •
edited
Loading

Currently, Tab is a regular C++ class. This gives us some annoying headaches when trying to tie Tabs to the UI. For example, as described in #2740, TerminalPage keeps track of two separate vectors of Tab that need to be kept in sync. There's one vector of TabViewItems specifically used by the TabView and another vector of <shared_ptr<Tab>>. Updating one requires an update on the other one.

This could be avoided by using an observable vector of Tab that's bound to the XAML control. However, this requires Tab to be converted to a WinRT class. This would also make future attempts to use Tab in XAML controls much easier. (such as in #1502)

EDIT: as of #4350, Tab has been converted into a WinRT type, and TerminalPage::_tabs has been converted to an IObservableVector. What's left is to bind TerminalPage's TabView to that vector.
@leonMSFT leonMSFT added Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. labels Dec 11, 2019
@leonMSFT leonMSFT self-assigned this Dec 11, 2019
@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 Dec 11, 2019
@DHowett-MSFT
Copy link
Contributor

(minor warning callout to @mkitzan as well: he requested this on a recent code review, and we're officially booking team work on it. 😄)

@DHowett-MSFT DHowett-MSFT added this to the Terminal-1912 milestone Dec 11, 2019
@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Dec 12, 2019
@DHowett-MSFT
Copy link
Contributor

I'm yanking triage and scoping it into 1912. 😄

@zadjii-msft zadjii-msft added the Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. label Dec 16, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Dec 16, 2019
@zadjii-msft zadjii-msft mentioned this issue Dec 17, 2019
Open
8 tasks
@mcpiroman mcpiroman mentioned this issue Dec 31, 2019
Closed
5 tasks
@leonMSFT leonMSFT mentioned this issue Jan 24, 2020
Merged
2 tasks
ghost pushed a commit that referenced this issue Feb 4, 2020
@leonMSFT
Convert Tab to a WinRT type (#4350)
99a28f9 
## 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.
@leonMSFT leonMSFT modified the milestones: Terminal v0.9, Terminal v0.10 Feb 5, 2020
@leonMSFT leonMSFT changed the title Convert Tab into a WinRT class Bind TerminalPage's TabView to TerminalPage::_tabs Feb 6, 2020
@leonMSFT leonMSFT changed the title Bind TerminalPage's TabView to TerminalPage::_tabs Bind TerminalPage's TabView to TerminalPage::_tabs Feb 6, 2020
@leonMSFT
Copy link
Contributor Author

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

@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Jun 22, 2020
@zadjii-msft zadjii-msft mentioned this issue Nov 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@leonMSFT leonMSFT

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.
Projects
None yet
Milestone
Terminal v2.0
Development

No branches or pull requests
5 participants
@carlos-zamora @DHowett-MSFT @zadjii-msft @cinnamon-msft @leonMSFT