Convert Tab to a WinRT type #4350

leonMSFT · 2020-01-24T01:07:35Z

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

CLA signed.
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.

zadjii-msft

Overall this looks good to me. Shame that we can't have the observable vector on the implementation type directly, so we wouldn't need the 100 get_self calls, but w/e.

Hit me up on Teams if you need help with the unittests - I'll save my signoff for once you've made sure those work. Otherwise I've only got nits.

src/cascadia/inc/cppwinrt_utils.h

src/cascadia/TerminalApp/Tab.h

src/cascadia/TerminalApp/TerminalPage.cpp

…rTerminalEvents signature

DHowett-MSFT

Good work! Only a few minor bits and bobs.

src/cascadia/TerminalApp/Tab.idl

src/cascadia/TerminalApp/TerminalPage.cpp

src/cascadia/TerminalApp/TerminalPage.h

src/cascadia/inc/cppwinrt_utils.h

miniksa

GH doesn't let me read my pending comments without a lot of work, so I'm hitting Comment. I don't think I had any major qualms with this, though. I'll probably hit Approve after you react to them.

src/cascadia/TerminalApp/Tab.h

src/cascadia/TerminalApp/TerminalPage.h

src/cascadia/TerminalApp/TerminalPage.cpp

src/cascadia/TerminalApp/Tab.cpp

zadjii-msft

Looking over these comments, I guess they're just nits, so go for it

src/cascadia/inc/cppwinrt_utils.h

src/cascadia/TerminalApp/TerminalPage.h

src/cascadia/TerminalApp/TerminalPage.cpp

…rminal into dev/lelian/WinRTTab

src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp

src/cascadia/TerminalApp/TerminalPage.cpp

DHowett-MSFT · 2020-02-04T19:09:19Z

src/cascadia/TerminalApp/TerminalPage.cpp

+            for (uint32_t idx = 0; idx < _tabs.Size(); ++idx)
            {
+                auto tab{ _GetStrongTabImpl(idx) };
                tab->SetFocused(false);
            }


minor nit: weird because GetStrongTabImpl takes an index not a tab, but these are better-formed as for(auto tab:_tabs) like in the original code.

I also don't care as much. :)

I also was nitting about this, so I sort of wanted to make GetStrongTabImpl take a tab instead. Then I realized that the majority of the code passes in an index and so if it took a Tab, GetAt would be littered everywhere instead. I just prefer the index route over the tab route 🤷‍♂

You could, in theory, provide an overload. One takes a const uint32_t and one takes a ::winrt::TerminalApp::Tab 😀

🤔 Do you think an overload for this function would be overkill?
EDIT: it would not be overkill

up to you. it would clean our iterators up again, but also I don't hate this

DHowett-MSFT · 2020-02-04T19:09:51Z

src/cascadia/TerminalApp/TerminalPage.h

@@ -94,23 +95,23 @@ namespace winrt::TerminalApp::implementation
        void _HookupKeyBindings(TerminalApp::AppKeyBindings bindings) noexcept;
        void _RegisterActionCallbacks();

-        void _UpdateTitle(std::shared_ptr<Tab> tab);
-        void _UpdateTabIcon(std::shared_ptr<Tab> tab);
+        void _UpdateTitle(const Tab& tab);


why's this one const but the others aren't?

_UpdateTitle doesn't change the tab directly, it actually just calls the handlers. _UpdateTabIcon directly calls the tab's UpdateIcon function, so it couldn't be const 😢

Co-Authored-By: Carlos Zamora <carlos.zamora@microsoft.com>

DHowett-MSFT · 2020-02-04T20:54:16Z

re-queued only x86

ghost · 2020-02-04T21:50:42Z

Hello @DHowett-MSFT!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

leonMSFT added 4 commits January 22, 2020 15:39

Tab is now a WinRT type, changed its shared_ptr self calls to get_weaks

b9fa082

changed test case to use com_ptr outside the lambda to heed the warning

ae2b0d5

added a comment to the observable getset property macro

b132475

modified TerminalPage to use an ObservableVector of WinRT tabs

1afe573

zadjii-msft reviewed Jan 24, 2020

View reviewed changes

src/cascadia/inc/cppwinrt_utils.h Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/Tab.h Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved

changing macro name, using winrt_callback macro, and changing Registe…

5718127

…rTerminalEvents signature

leonMSFT marked this pull request as ready for review January 27, 2020 22:24

leonMSFT added 2 commits January 27, 2020 14:39

resolving merge conflicts

7aaed78

formatting

f608b3c

leonMSFT requested review from DHowett-MSFT, carlos-zamora and miniksa January 27, 2020 22:50

fixing build errors

516c0de

leonMSFT added Product-Terminal The new Windows Terminal. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. labels Jan 27, 2020

DHowett-MSFT suggested changes Jan 28, 2020

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jan 28, 2020

incorporating PR comments

091d953

DHowett-MSFT reviewed Jan 29, 2020

View reviewed changes

src/cascadia/inc/cppwinrt_utils.h Outdated Show resolved Hide resolved

leonMSFT added 2 commits January 28, 2020 17:15

moving this const cast magic into a macro magic

ff8ed26

FORMATTING WHY

30ea840

miniksa reviewed Jan 30, 2020

View reviewed changes

zadjii-msft approved these changes Jan 30, 2020

View reviewed changes

src/cascadia/inc/cppwinrt_utils.h Show resolved Hide resolved

src/cascadia/TerminalApp/TerminalPage.h Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved

leonMSFT added 6 commits January 31, 2020 13:42

some more PR comments inc

f66525e

Merge branch 'dev/lelian/WinRTTab' of https://github.com/microsoft/te…

26b000c

…rminal into dev/lelian/WinRTTab

merge conflicts

8239539

i should staple ctrl-k ctrl-d to my forehead

392ffc1

in the middle of some changes, it's failing though

b396451

changing function signatures to NOT take reference

0b447ba

carlos-zamora approved these changes Feb 4, 2020

View reviewed changes

src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp Outdated Show resolved Hide resolved

DHowett-MSFT reviewed Feb 4, 2020

View reviewed changes

src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved

DHowett-MSFT reviewed Feb 4, 2020

View reviewed changes

DHowett-MSFT approved these changes Feb 4, 2020

View reviewed changes

leonMSFT and others added 2 commits February 4, 2020 11:26

Update src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp

ce4c2ac

Co-Authored-By: Carlos Zamora <carlos.zamora@microsoft.com>

f o r m a t t i n g

21b82c0

overloading the function

b920356

DHowett-MSFT added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 4, 2020

ghost merged commit 99a28f9 into master Feb 4, 2020

ghost deleted the dev/lelian/WinRTTab branch February 4, 2020 21:51

leonMSFT mentioned this pull request Feb 6, 2020

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

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert Tab to a WinRT type #4350

Convert Tab to a WinRT type #4350

leonMSFT commented Jan 24, 2020 •

edited by DHowett-MSFT

Loading

zadjii-msft left a comment

DHowett-MSFT left a comment

miniksa left a comment

zadjii-msft left a comment

DHowett-MSFT Feb 4, 2020

leonMSFT Feb 4, 2020 •

edited

Loading

DHowett-MSFT Feb 4, 2020

leonMSFT Feb 4, 2020 •

edited

Loading

DHowett-MSFT Feb 4, 2020

DHowett-MSFT Feb 4, 2020

leonMSFT Feb 4, 2020 •

edited

Loading

DHowett-MSFT commented Feb 4, 2020

ghost commented Feb 4, 2020

Convert Tab to a WinRT type #4350

Convert Tab to a WinRT type #4350

Conversation

leonMSFT commented Jan 24, 2020 • edited by DHowett-MSFT Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

zadjii-msft left a comment

Choose a reason for hiding this comment

DHowett-MSFT left a comment

Choose a reason for hiding this comment

miniksa left a comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

DHowett-MSFT Feb 4, 2020

Choose a reason for hiding this comment

leonMSFT Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

DHowett-MSFT Feb 4, 2020

Choose a reason for hiding this comment

leonMSFT Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

DHowett-MSFT Feb 4, 2020

Choose a reason for hiding this comment

DHowett-MSFT Feb 4, 2020

Choose a reason for hiding this comment

leonMSFT Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

DHowett-MSFT commented Feb 4, 2020

ghost commented Feb 4, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

leonMSFT commented Jan 24, 2020 •

edited by DHowett-MSFT

Loading

leonMSFT Feb 4, 2020 •

edited

Loading

leonMSFT Feb 4, 2020 •

edited

Loading

leonMSFT Feb 4, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.