Skip to content

Conversation

@zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Mar 26, 2021

I think we can all agree that TerminalPage.cpp is an unruly beast of a
file. It's got everything. It does everything. It can sometimes be a bit
hard to work with, because of simply how big it is. This PR tries to
alleviate this by making TerminalPage.cpp just a little smaller. It
does so by moving pretty much everything related to tab management into
its own file, TabManagement.cpp. These methods that have moved are all
the same as they were before, and they're still members of
TerminalPage. But now they're all in one place.

I tried to move all the references to _tabs in TerminalPage.cpp, but
there's still a few that I left behind. Mostly because I felt that
moving those would be too gnarly a code change for an otherwise simple
cut&paste PR.

There are a few new methods I introduced:

  • _TabDragStarted and _TabDragCompleted: These were lambdas before,
    promoted to full methods.
  • _DismissTabContextMenus: Remove all the right-click context menus
    from the tabs
  • _FocusCurrentTab: This one's a bit trickier, we were actually doing
    this in a few different places, so I tried consolidating.
  • _HasMultipleTabs: This doesn't need explaining.
  • _RemoveAllTabs: Really, just encapsulation for the sake of removing
    a _tabs from TerminalPage.cpp
  • _ResizeTabContent: Really, just encapsulation for the sake of
    removing a _tabs from TerminalPage.cpp

In the future, some enterprising young soul could try promoting that
file to its own class, and hiding _tabs (and _mruTabs) inside it.
Probably would need to take a reference to TerminalPage's _tabView and
_newTabButton. I'm not doing that right now, because I already hate
the idea of the ...

920 additions and 847 deletions.

... I'm making you look at already.

Other thoughts

Some of the calls might be a little arbitrary - _OpenNewTab and
_CreateNewTabFromSettings probably should stay in TerminalPage? Or
at least elements of those might need to get split up better. Similarly
TerminalPage::_OpenSettingsUI stayed in TerminalPage.cpp, but it
does a lot of the same work as _CreateNewTabFromSettings. I'm not
saying this is the definitive places for these methods - it's code we're
working with, not stone ☺️

@zadjii-msft zadjii-msft added Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. labels Mar 26, 2021
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeeeeaaaaah. We should probably break up TerminalPage more haha.

Curious: does breaking it up help out with glomming & tab tear off at all? Maybe there's ways to shoe-horn some of that work in that effort?

If not, I say we file a CodeHealth task to figure out how to clean up TerminalPage a bit. Just to break up the responsibilities better.

@zadjii-msft
Copy link
Member Author

Curious: does breaking it up help out with glomming & tab tear off at all? Maybe there's ways to shoe-horn some of that work in that effort?

maybe? We're way too far out to know for sure. Better encapsulation is always good though.

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Mar 29, 2021
@ghost ghost requested review from DHowett, PankajBhojwani, leonMSFT and miniksa March 29, 2021 11:14
{
// Since _RemoveTab is asynchronous, create a snapshot of the tabs we want to remove
std::vector<winrt::TerminalApp::TabBase> tabsToRemove;
std::copy(begin(_tabs), end(_tabs), std::back_inserter(tabsToRemove));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clever

@DHowett
Copy link
Member

DHowett commented Mar 29, 2021

Conflict resolution req'd.

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 29, 2021
@ghost
Copy link

ghost commented Mar 29, 2021

Hello @zadjii-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.

@ghost ghost merged commit ba543c0 into main Mar 29, 2021
@ghost ghost deleted the dev/migrie/a-little-less-terminalpage branch March 29, 2021 19:53
This pull request 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. AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants