Clean up boundary between terminal app and terminal page by KaiyuWang16 · Pull Request #2208 · microsoft/terminal

KaiyuWang16 · 2019-08-02T23:22:51Z

This PR is for this project: #1878

Refere

PR Checklist

Closes Clean up the boundary between TerminalPage and TerminalApp #1878
Tests added/passed
~~Requires documentation to be updated~~
I've discussed this with core contributors already.

For now, the new TerminalPage class includes;

global error dialogs
ETW global registration
tab management
tab icons
clipboard management
ETW tab logging
key bindings

And App is responsible for:
settings
settings reload

One issue remaining for now is that I have not figured out how to move the four winrt event handlers in App down to Terminal. I have some work-arounds to attach AppHost methods to the event handlers, but I am still working on this.

Test: According to Griese, we do not have integration/unit tests for now. I have manually tested the app with following actions:

Open new tabs
close tabs
close the last tab
resize window
minimize/restore
ShowTitleBar, true/false
change settings: font size, background color, useAcrylic
Copy/Paste with mouse/keyboard
Tab switch/add/close/ using keys
About button
Split pane vertically and horizontally
Resize panes with keys
Move focus on different panes with keys
Change theme while the app is on.
Change cmd title while the app is on

src/cascadia/TerminalApp/TerminalPage.idl

src/cascadia/TerminalApp/App.cpp

src/cascadia/TerminalApp/TerminalPage.h

src/cascadia/TerminalApp/App.cpp

codendone · 2019-08-05T17:34:54Z

    std::shared_mutex _dialogLock;

Looks like this is only used in TerminalPage now, which has its own copy.

Refers to: src/cascadia/TerminalApp/App.h:56 in 26d69a5. [](commit_id = 26d69a5, deletion_comment = False)

src/cascadia/TerminalApp/TerminalPage.cpp

zadjii-msft

So far so good. I have a couple questions, an I know there's a bit more in the works so I'll come back and take another pass when that's all finished.

src/cascadia/TerminalApp/App.cpp

src/cascadia/WindowsTerminal/AppHost.cpp

src/cascadia/TerminalApp/TerminalPage.cpp

src/cascadia/TerminalApp/App.cpp

src/cascadia/TerminalApp/TerminalPage.cpp

KaiyuWang16

An alternative for ShowDialog is to let TerminalPage call back to App. We can get the App object in this way:

App app = Windows::UI::Xaml::Application::Current().as();

And we can call public methods in App. Example: app.GetLaunchDimensions(dpix)

src/cascadia/TerminalApp/App.cpp

src/cascadia/WindowsTerminal/AppHost.cpp

src/cascadia/TerminalApp/App.idl

src/cascadia/TerminalApp/TerminalPage.h

zadjii-msft

This is some preliminary feedback. I haven't gotten through all of it but there are some points here that could be addressed

src/cascadia/WindowsTerminal/AppHost.cpp

src/cascadia/WindowsTerminal/AppHost.h

src/cascadia/TerminalApp/App.h

src/cascadia/TerminalApp/App.cpp

src/cascadia/TerminalApp/TerminalPage.h

src/cascadia/TerminalApp/App.idl

src/cascadia/TerminalApp/App.h

src/cascadia/WindowsTerminal/AppHost.cpp

src/cascadia/TerminalApp/TerminalPage.idl

src/cascadia/TerminalApp/App.h

…rminalApp-and-TerminalPage

src/cascadia/TerminalApp/TerminalPage.idl

src/cascadia/TerminalApp/TerminalPage.h

outdated, need time to look again

Time to review!

DHowett-MSFT · 2019-08-30T00:35:59Z

@microsoft/windows-console-team time to review this! Thanks alL!

zadjii-msft

Looks good to me!

src/cascadia/TerminalApp/TerminalPage.h

src/cascadia/TerminalApp/App.cpp

src/cascadia/TerminalApp/TerminalPage.cpp

…rminalApp-and-TerminalPage

DHowett-MSFT

Thank you!

DHowett-MSFT · 2019-09-04T20:59:50Z

The code formatting check failed.

KaiyuWang16 · 2019-09-04T21:33:08Z

The code formatting check failed.

Format issue fixed, tested again and there is no bug. Merge.

Clean up boundary between terminal app and terminal page (microsoft#2208)

ghost · 2019-09-24T16:25:55Z

🎉Windows Terminal Preview v0.5.2661.0 has been released which incorporates this pull request.:tada:

Handy links:

KaiyuWang16 requested review from DHowett-MSFT and zadjii-msft August 2, 2019 23:48