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

Fix duplication issue for unfocused tabs #13964

Merged
merged 2 commits into from
Oct 11, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/cascadia/LocalTests_TerminalApp/TabTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -503,7 +503,7 @@ namespace TerminalAppLocalTests

Log::Comment(NoThrowString().Format(L"Duplicate the first pane"));
result = RunOnUIThread([&page]() {
page->_SplitPane(SplitDirection::Automatic, 0.5f, page->_MakePane(nullptr, true, nullptr));
page->_SplitPane(SplitDirection::Automatic, 0.5f, page->_MakePane(nullptr, page->_GetFocusedTab(), nullptr));

VERIFY_ARE_EQUAL(1u, page->_tabs.Size());
auto tab = page->_GetTerminalTabImpl(page->_tabs.GetAt(0));
Expand All @@ -521,7 +521,7 @@ namespace TerminalAppLocalTests

Log::Comment(NoThrowString().Format(L"Duplicate the pane, and don't crash"));
result = RunOnUIThread([&page]() {
page->_SplitPane(SplitDirection::Automatic, 0.5f, page->_MakePane(nullptr, true, nullptr));
page->_SplitPane(SplitDirection::Automatic, 0.5f, page->_MakePane(nullptr, page->_GetFocusedTab(), nullptr));

VERIFY_ARE_EQUAL(1u, page->_tabs.Size());
auto tab = page->_GetTerminalTabImpl(page->_tabs.GetAt(0));
Expand Down Expand Up @@ -843,7 +843,7 @@ namespace TerminalAppLocalTests
// | 1 | 2 |
// | | |
// -------------------
page->_SplitPane(SplitDirection::Right, 0.5f, page->_MakePane(nullptr, true, nullptr));
page->_SplitPane(SplitDirection::Right, 0.5f, page->_MakePane(nullptr, page->_GetFocusedTab(), nullptr));
secondId = tab->_activePane->Id().value();
});
Sleep(250);
Expand All @@ -861,7 +861,7 @@ namespace TerminalAppLocalTests
// | 3 | |
// | | |
// -------------------
page->_SplitPane(SplitDirection::Down, 0.5f, page->_MakePane(nullptr, true, nullptr));
page->_SplitPane(SplitDirection::Down, 0.5f, page->_MakePane(nullptr, page->_GetFocusedTab(), nullptr));
auto tab = page->_GetTerminalTabImpl(page->_tabs.GetAt(0));
// Split again to make the 3rd tab
thirdId = tab->_activePane->Id().value();
Expand All @@ -881,7 +881,7 @@ namespace TerminalAppLocalTests
// | 3 | 4 |
// | | |
// -------------------
page->_SplitPane(SplitDirection::Down, 0.5f, page->_MakePane(nullptr, true, nullptr));
page->_SplitPane(SplitDirection::Down, 0.5f, page->_MakePane(nullptr, page->_GetFocusedTab(), nullptr));
auto tab = page->_GetTerminalTabImpl(page->_tabs.GetAt(0));
fourthId = tab->_activePane->Id().value();
});
Expand Down
3 changes: 2 additions & 1 deletion src/cascadia/TerminalApp/AppActionHandlers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -235,10 +235,11 @@ namespace winrt::TerminalApp::implementation
}
}

const auto& duplicateFromTab{ realArgs.SplitMode() == SplitType::Duplicate ? _GetFocusedTab() : nullptr };
_SplitPane(realArgs.SplitDirection(),
// This is safe, we're already filtering so the value is (0, 1)
::base::saturated_cast<float>(realArgs.SplitSize()),
_MakePane(realArgs.TerminalArgs(), realArgs.SplitMode() == SplitType::Duplicate));
_MakePane(realArgs.TerminalArgs(), duplicateFromTab));
args.Handled(true);
}
}
Expand Down
6 changes: 3 additions & 3 deletions src/cascadia/TerminalApp/TabManagement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ namespace winrt::TerminalApp::implementation
//
// This call to _MakePane won't return nullptr, we already checked that
// case above with the _maybeElevate call.
_CreateNewTabFromPane(_MakePane(newTerminalArgs, false, existingConnection));
_CreateNewTabFromPane(_MakePane(newTerminalArgs, nullptr, existingConnection));
return S_OK;
}
CATCH_RETURN();
Expand Down Expand Up @@ -337,7 +337,7 @@ namespace winrt::TerminalApp::implementation
// In the future, it may be preferable to just duplicate the
// current control's live settings (which will include changes
// made through VT).
_CreateNewTabFromPane(_MakePane(nullptr, true, nullptr));
_CreateNewTabFromPane(_MakePane(nullptr, tab, nullptr));

const auto runtimeTabText{ tab.GetTabText() };
if (!runtimeTabText.empty())
Expand All @@ -360,7 +360,7 @@ namespace winrt::TerminalApp::implementation
try
{
_SetFocusedTab(tab);
_SplitPane(tab, SplitDirection::Automatic, 0.5f, _MakePane(nullptr, true));
_SplitPane(tab, SplitDirection::Automatic, 0.5f, _MakePane(nullptr, tab));
}
CATCH_LOG();
}
Expand Down
32 changes: 14 additions & 18 deletions src/cascadia/TerminalApp/TerminalPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2484,38 +2484,34 @@ namespace winrt::TerminalApp::implementation
// - newTerminalArgs: an object that may contain a blob of parameters to
// control which profile is created and with possible other
// configurations. See CascadiaSettings::BuildSettings for more details.
// - duplicate: a boolean to indicate whether the pane we create should be
// a duplicate of the currently focused pane
// - sourceTab: an optional tab reference that indicates that the created
// pane should be a duplicate of the tab's focused pane
// - existingConnection: optionally receives a connection from the outside
// world instead of attempting to create one
// Return Value:
// - If the newTerminalArgs required us to open the pane as a new elevated
// connection, then we'll return nullptr. Otherwise, we'll return a new
// Pane for this connection.
std::shared_ptr<Pane> TerminalPage::_MakePane(const NewTerminalArgs& newTerminalArgs,
const bool duplicate,
const winrt::TerminalApp::TabBase& sourceTab,
TerminalConnection::ITerminalConnection existingConnection)
{
TerminalSettingsCreateResult controlSettings{ nullptr };
Profile profile{ nullptr };

if (duplicate)
if (const auto& terminalTab{ _GetTerminalTabImpl(sourceTab) })
{
const auto focusedTab{ _GetFocusedTabImpl() };
if (focusedTab)
profile = terminalTab->GetFocusedProfile();
if (profile)
{
profile = focusedTab->GetFocusedProfile();
if (profile)
// TODO GH#5047 If we cache the NewTerminalArgs, we no longer need to do this.
profile = GetClosestProfileForDuplicationOfProfile(profile);
controlSettings = TerminalSettings::CreateWithProfile(_settings, profile, *_bindings);
const auto workingDirectory = terminalTab->GetActiveTerminalControl().WorkingDirectory();
const auto validWorkingDirectory = !workingDirectory.empty();
if (validWorkingDirectory)
{
// TODO GH#5047 If we cache the NewTerminalArgs, we no longer need to do this.
profile = GetClosestProfileForDuplicationOfProfile(profile);
controlSettings = TerminalSettings::CreateWithProfile(_settings, profile, *_bindings);
const auto workingDirectory = focusedTab->GetActiveTerminalControl().WorkingDirectory();
const auto validWorkingDirectory = !workingDirectory.empty();
if (validWorkingDirectory)
{
controlSettings.DefaultSettings().StartingDirectory(workingDirectory);
}
controlSettings.DefaultSettings().StartingDirectory(workingDirectory);
}
}
}
Expand Down Expand Up @@ -3292,7 +3288,7 @@ namespace winrt::TerminalApp::implementation
// elevated version of the Terminal with that profile... that's a
// recipe for disaster. We won't ever open up a tab in this window.
newTerminalArgs.Elevate(false);
const auto newPane = _MakePane(newTerminalArgs, false, connection);
const auto newPane = _MakePane(newTerminalArgs, nullptr, connection);
newPane->WalkTree([](auto pane) {
pane->FinalizeConfigurationGivenDefault();
});
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/TerminalPage.h
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ namespace winrt::TerminalApp::implementation
const winrt::Microsoft::Terminal::TerminalConnection::ITerminalConnection& connection);

std::shared_ptr<Pane> _MakePane(const Microsoft::Terminal::Settings::Model::NewTerminalArgs& newTerminalArgs = nullptr,
const bool duplicate = false,
const winrt::TerminalApp::TabBase& sourceTab = nullptr,
winrt::Microsoft::Terminal::TerminalConnection::ITerminalConnection existingConnection = nullptr);

void _RefreshUIForSettingsReload();
Expand Down