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.

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

Unify pane splitting and new tab creation #11021

Closed
DHowett opened this issue Aug 23, 2021 · 4 comments · Fixed by #11305
Closed

Unify pane splitting and new tab creation #11021

DHowett opened this issue Aug 23, 2021 · 4 comments · Fixed by #11305
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@DHowett
Copy link
Member

DHowett commented Aug 23, 2021

In my eyes, spawning a new tab is a special case of splitting a nonexistent window into one pane.

Right now, we have a lot of divergence between opening a tab (1) and opening a pane (2); in both cases we spawn a new control, but the way it gets propagated into the tree is vastly different. The same applies for duplicating an active pane; the code in pane splitting suggests that duplicating a pane keeps the working directory but a tab does not (!) (see 3) EDIT: No, we accounted for it. Double the code, double the places to look 😉

1

auto connection = existingConnection ? existingConnection : _CreateConnectionFromSettings(profile, settings.DefaultSettings());
// If we had an `existingConnection`, then this is an inbound handoff from somewhere else.
// We need to tell it about our size information so it can match the dimensions of what
// we are about to present.
if (existingConnection)
{
connection.Resize(settings.DefaultSettings().InitialRows(), settings.DefaultSettings().InitialCols());
}
TerminalConnection::ITerminalConnection debugConnection{ nullptr };
if (_settings.GlobalSettings().DebugFeaturesEnabled())
{
const CoreWindow window = CoreWindow::GetForCurrentThread();
const auto rAltState = window.GetKeyState(VirtualKey::RightMenu);
const auto lAltState = window.GetKeyState(VirtualKey::LeftMenu);
const bool bothAltsPressed = WI_IsFlagSet(lAltState, CoreVirtualKeyStates::Down) &&
WI_IsFlagSet(rAltState, CoreVirtualKeyStates::Down);
if (bothAltsPressed)
{
std::tie(connection, debugConnection) = OpenDebugTapConnection(connection);
}
}
// Give term control a child of the settings so that any overrides go in the child
// This way, when we do a settings reload we just update the parent and the overrides remain
auto term = _InitControl(settings, connection);
auto newTabImpl = winrt::make_self<TerminalTab>(profile, term);
_RegisterTerminalEvents(term);
_InitializeTab(newTabImpl);

2

const auto controlConnection = _CreateConnectionFromSettings(profile, controlSettings.DefaultSettings());
const float contentWidth = ::base::saturated_cast<float>(_tabContent.ActualWidth());
const float contentHeight = ::base::saturated_cast<float>(_tabContent.ActualHeight());
const winrt::Windows::Foundation::Size availableSpace{ contentWidth, contentHeight };
auto realSplitType = splitType;
if (realSplitType == SplitState::Automatic)
{
realSplitType = tab.PreCalculateAutoSplit(availableSpace);
}
const auto canSplit = tab.PreCalculateCanSplit(realSplitType, splitSize, availableSpace);
if (!canSplit)
{
return;
}
auto newControl = _InitControl(controlSettings, controlConnection);
// Hookup our event handlers to the new terminal
_RegisterTerminalEvents(newControl);
_UnZoomIfNeeded();
tab.SplitPane(realSplitType, splitSize, profile, newControl);

Collapsed note 3

3

if (splitMode == SplitType::Duplicate)
{
profile = tab.GetFocusedProfile();
if (profile)
{
controlSettings = TerminalSettings::CreateWithProfile(_settings, profile, *_bindings);
const auto workingDirectory = tab.GetActiveTerminalControl().WorkingDirectory();
const auto validWorkingDirectory = !workingDirectory.empty();
if (validWorkingDirectory)
{
controlSettings.DefaultSettings().StartingDirectory(workingDirectory);
}

@DHowett DHowett added Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. labels Aug 23, 2021
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Aug 23, 2021
@DHowett
Copy link
Member Author

DHowett commented Aug 23, 2021

/cc @PankajBhojwani you did some of the work here to make control initialization uniform, but this is the "farther step" i was talking about. You don't need to do this; it's just an FYI. 😄

@PankajBhojwani
Copy link
Contributor

Ah I like this

@zadjii-msft
Copy link
Member

I've meant to do this for so long. The TabManagement.cpp PR really highlighted how bad this one is

@zadjii-msft zadjii-msft added Help Wanted We encourage anyone to jump in on these. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Aug 23, 2021
@zadjii-msft zadjii-msft added this to the Terminal v2.0 milestone Aug 23, 2021
@zadjii-msft zadjii-msft added the Priority-3 A description (P3) label Aug 23, 2021
@Rosefield
Copy link
Contributor

Rosefield commented Aug 24, 2021

The CreateNewTabFromPane function that I added as part of move pane to tab could probably be used for this. If you look at _SplitPane, you create the profile/control, then in one branch you do the PreCalculate* and then tab.SplitPane and on the other you just CreateNewTabFromPane. That should get you most of the way there, I think. Internally Pane::Split is effectively just auto pane = std::make_shared<Pane>; _Split(pane) so that could just be modified to take a pane directly instead of passing the control/profile down.

@ghost ghost added the In-PR This issue has a related PR label Sep 22, 2021
@ghost ghost closed this as completed in #11305 Nov 4, 2021
ghost pushed a commit that referenced this issue Nov 4, 2021
Implements `_MakePane` in `TerminalPage`, which creates a pane that then can be used to pass into another pane to split or to create a new tab with. Places where we split pane or create a new tab now use `_MakePane`. 

## PR Checklist
* [x] Closes #11021
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] I work here

## Validation Steps Performed
Stands up to manual testing with multiple new pane/new tab commands as well as startup actions
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Nov 4, 2021
This issue 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. Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants