Skip to content

Commit

Permalink
Fix panes being dropped when tearing off tabs (#18627)
Browse files Browse the repository at this point in the history 
I don't actually know why this is happening, because it doesn't
happen with startup actions specified in the settings file.
In any case, it's fixed with more delays.

Closes #18572

## Validation Steps Performed
* Create a tab with 2 panes
* Tear it off into a new window
* New window has 1 tab with 2 panes ✅
  • Loading branch information
@lhecker
lhecker authored Feb 26, 2025
1 parent e5b972a commit e1b28e7
Show file tree
Hide file tree
Showing 5 changed files with 53 additions and 88 deletions.
2 changes: 1 addition & 1 deletion src/cascadia/LocalTests_TerminalApp/TabTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ namespace TerminalAppLocalTests
NewTabArgs args{ newTerminalArgs };
ActionAndArgs newTabAction{ ShortcutAction::NewTab, args };
// push the arg onto the front
page->_startupActions.Append(newTabAction);
page->_startupActions.push_back(std::move(newTabAction));
Log::Comment(L"Added a single newTab action");

auto app = ::winrt::Windows::UI::Xaml::Application::Current();
Expand Down
8 changes: 3 additions & 5 deletions src/cascadia/TerminalApp/AppActionHandlers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -752,13 +752,11 @@ namespace winrt::TerminalApp::implementation
{
if (const auto& realArgs = actionArgs.ActionArgs().try_as<ExecuteCommandlineArgs>())
{
auto actions = winrt::single_threaded_vector<ActionAndArgs>(
TerminalPage::ConvertExecuteCommandlineToActions(realArgs));

if (actions.Size() != 0)
auto actions = ConvertExecuteCommandlineToActions(realArgs);
if (!actions.empty())
{
actionArgs.Handled(true);
ProcessStartupActions(actions, false);
ProcessStartupActions(std::move(actions), false);
}
}
}
Expand Down
109 changes: 40 additions & 69 deletions src/cascadia/TerminalApp/TerminalPage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ namespace winrt::TerminalApp::implementation
TerminalPage::TerminalPage(TerminalApp::WindowProperties properties, const TerminalApp::ContentManager& manager) :
_tabs{ winrt::single_threaded_observable_vector<TerminalApp::TabBase>() },
_mruTabs{ winrt::single_threaded_observable_vector<TerminalApp::TabBase>() },
_startupActions{ winrt::single_threaded_vector<ActionAndArgs>() },
_manager{ manager },
_hostingHwnd{},
_WindowProperties{ std::move(properties) }
Expand Down Expand Up @@ -297,7 +296,7 @@ namespace winrt::TerminalApp::implementation
// GH#12267: Don't forget about defterm handoff here. If we're being
// created for embedding, then _yea_, we don't need to handoff to an
// elevated window.
if (!_startupActions || IsRunningElevated() || _shouldStartInboundListener || _startupActions.Size() == 0)
if (_startupActions.empty() || IsRunningElevated() || _shouldStartInboundListener)
{
// there aren't startup actions, or we're elevated. In that case, go for it.
return false;
Expand Down Expand Up @@ -375,7 +374,7 @@ namespace winrt::TerminalApp::implementation
// - <none>
void TerminalPage::HandoffToElevated(const CascadiaSettings& settings)
{
if (!_startupActions)
if (_startupActions.empty())
{
return;
}
Expand Down Expand Up @@ -489,7 +488,7 @@ namespace winrt::TerminalApp::implementation
{
_startupState = StartupState::InStartup;

ProcessStartupActions(_startupActions, true);
ProcessStartupActions(std::move(_startupActions), true);

// If we were told that the COM server needs to be started to listen for incoming
// default application connections, start it now.
Expand Down Expand Up @@ -546,80 +545,56 @@ namespace winrt::TerminalApp::implementation
// nt -d .` from inside another directory to work as expected.
// Return Value:
// - <none>
safe_void_coroutine TerminalPage::ProcessStartupActions(Windows::Foundation::Collections::IVector<ActionAndArgs> actions,
const bool initial,
const winrt::hstring cwd,
const winrt::hstring env)
safe_void_coroutine TerminalPage::ProcessStartupActions(std::vector<ActionAndArgs> actions, const bool initial, const winrt::hstring cwd, const winrt::hstring env)
{
auto weakThis{ get_weak() };

// Handle it on a subsequent pass of the UI thread.
co_await wil::resume_foreground(Dispatcher(), CoreDispatcherPriority::Normal);
const auto strong = get_strong();

// If the caller provided a CWD, "switch" to that directory, then switch
// back once we're done. This looks weird though, because we have to set
// up the scope_exit _first_. We'll release the scope_exit if we don't
// actually need it.

// back once we're done.
auto originalVirtualCwd{ _WindowProperties.VirtualWorkingDirectory() };
auto restoreCwd = wil::scope_exit([&originalVirtualCwd, this]() {
// ignore errors, we'll just power on through. We'd rather do
// something rather than fail silently if the directory doesn't
// actually exist.
_WindowProperties.VirtualWorkingDirectory(originalVirtualCwd);
});

// Literally the same thing with env vars too
auto originalVirtualEnv{ _WindowProperties.VirtualEnvVars() };
auto restoreEnv = wil::scope_exit([&originalVirtualEnv, this]() {
_WindowProperties.VirtualEnvVars(originalVirtualEnv);
auto restoreCwd = wil::scope_exit([&]() {
if (!cwd.empty())
{
// ignore errors, we'll just power on through. We'd rather do
// something rather than fail silently if the directory doesn't
// actually exist.
_WindowProperties.VirtualWorkingDirectory(originalVirtualCwd);
_WindowProperties.VirtualEnvVars(originalVirtualEnv);
}
});
_WindowProperties.VirtualWorkingDirectory(cwd);
_WindowProperties.VirtualEnvVars(env);

if (cwd.empty())
for (size_t i = 0; i < actions.size(); ++i)
{
// We didn't actually need to change the virtual CWD, so we don't
// need to restore it
restoreCwd.release();
}
else
{
_WindowProperties.VirtualWorkingDirectory(cwd);
}
if (i != 0)
{
// Each action may rely on the XAML layout of a preceding action.
// Most importantly, this is the case for the combination of NewTab + SplitPane,
// as the former appears to only have a layout size after at least 1 resume_foreground,
// while the latter relies on that information. This is also why it uses Low priority.
//
// Curiously, this does not seem to be required when using startupActions, but only when
// tearing out a tab (this currently creates a new window with injected startup actions).
// This indicates that this is really more of an architectural issue and not a fundamental one.
co_await wil::resume_foreground(Dispatcher(), CoreDispatcherPriority::Low);
}

if (env.empty())
{
restoreEnv.release();
}
else
{
_WindowProperties.VirtualEnvVars(env);
_actionDispatch->DoAction(actions[i]);
}

if (auto page{ weakThis.get() })
// GH#6586: now that we're done processing all startup commands,
// focus the active control. This will work as expected for both
// commandline invocations and for `wt` action invocations.
if (const auto& terminalTab{ _GetFocusedTabImpl() })
{
for (const auto& action : actions)
if (const auto& content{ terminalTab->GetActiveContent() })
{
if (auto page{ weakThis.get() })
{
_actionDispatch->DoAction(action);
}
else
{
co_return;
}
}

// GH#6586: now that we're done processing all startup commands,
// focus the active control. This will work as expected for both
// commandline invocations and for `wt` action invocations.
if (const auto& terminalTab{ _GetFocusedTabImpl() })
{
if (const auto& content{ terminalTab->GetActiveContent() })
{
content.Focus(FocusState::Programmatic);
}
content.Focus(FocusState::Programmatic);
}
}

if (initial)
{
_CompleteInitialization();
Expand Down Expand Up @@ -3670,13 +3645,9 @@ namespace winrt::TerminalApp::implementation
// - actions: a list of Actions to process on startup.
// Return Value:
// - <none>
void TerminalPage::SetStartupActions(std::vector<ActionAndArgs>& actions)
void TerminalPage::SetStartupActions(std::vector<ActionAndArgs> actions)
{
// The fastest way to copy all the actions out of the std::vector and
// put them into a winrt::IVector is by making a copy, then moving the
// copy into the winrt vector ctor.
auto listCopy = actions;
_startupActions = winrt::single_threaded_vector<ActionAndArgs>(std::move(listCopy));
_startupActions = std::move(actions);
}

// Routine Description:
Expand Down
6 changes: 3 additions & 3 deletions src/cascadia/TerminalApp/TerminalPage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ namespace winrt::TerminalApp::implementation
void Maximized(bool newMaximized);
void RequestSetMaximized(bool newMaximized);

void SetStartupActions(std::vector<Microsoft::Terminal::Settings::Model::ActionAndArgs>& actions);
void SetStartupActions(std::vector<Microsoft::Terminal::Settings::Model::ActionAndArgs> actions);

void SetInboundListener(bool isEmbedding);
static std::vector<Microsoft::Terminal::Settings::Model::ActionAndArgs> ConvertExecuteCommandlineToActions(const Microsoft::Terminal::Settings::Model::ExecuteCommandlineArgs& args);
Expand All @@ -146,7 +146,7 @@ namespace winrt::TerminalApp::implementation
void ActionSaveFailed(winrt::hstring message);
void ShowTerminalWorkingDirectory();

safe_void_coroutine ProcessStartupActions(Windows::Foundation::Collections::IVector<Microsoft::Terminal::Settings::Model::ActionAndArgs> actions,
safe_void_coroutine ProcessStartupActions(std::vector<Microsoft::Terminal::Settings::Model::ActionAndArgs> actions,
const bool initial,
const winrt::hstring cwd = winrt::hstring{},
const winrt::hstring env = winrt::hstring{});
Expand Down Expand Up @@ -255,7 +255,7 @@ namespace winrt::TerminalApp::implementation
winrt::Windows::UI::Xaml::Controls::Grid::LayoutUpdated_revoker _layoutUpdatedRevoker;
StartupState _startupState{ StartupState::NotInitialized };

Windows::Foundation::Collections::IVector<Microsoft::Terminal::Settings::Model::ActionAndArgs> _startupActions;
std::vector<Microsoft::Terminal::Settings::Model::ActionAndArgs> _startupActions;
bool _shouldStartInboundListener{ false };
bool _isEmbeddingInboundListener{ false };

Expand Down
16 changes: 6 additions & 10 deletions src/cascadia/TerminalApp/TerminalWindow.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1054,12 +1054,8 @@ namespace winrt::TerminalApp::implementation
{
_contentBounds = bounds;

const auto& args = _contentStringToActions(content, true);

for (const auto& action : args)
{
_initialContentArgs.push_back(action);
}
const auto args = _contentStringToActions(content, true);
_initialContentArgs = wil::to_vector(args);
}

// Method Description:
Expand All @@ -1085,7 +1081,7 @@ namespace winrt::TerminalApp::implementation
if (_appArgs->ExitCode() == 0)
{
auto& parsedArgs = _appArgs->ParsedArgs();
auto actions = winrt::single_threaded_vector<ActionAndArgs>(std::move(parsedArgs.GetStartupActions()));
auto& actions = parsedArgs.GetStartupActions();

_root->ProcessStartupActions(actions, false, _appArgs->CurrentDirectory(), _appArgs->CurrentEnvironment());

Expand Down Expand Up @@ -1200,7 +1196,7 @@ namespace winrt::TerminalApp::implementation
{
try
{
const auto& args = ActionAndArgs::Deserialize(content);
const auto args = ActionAndArgs::Deserialize(content);
if (args == nullptr ||
args.Size() == 0)
{
Expand Down Expand Up @@ -1244,9 +1240,9 @@ namespace winrt::TerminalApp::implementation

const bool replaceFirstWithNewTab = tabIndex >= _root->NumberOfTabs();

const auto& args = _contentStringToActions(content, replaceFirstWithNewTab);
auto args = _contentStringToActions(content, replaceFirstWithNewTab);

_root->AttachContent(args, tabIndex);
_root->AttachContent(std::move(args), tabIndex);
}
}
void TerminalWindow::SendContentToOther(winrt::TerminalApp::RequestReceiveContentArgs args)
Expand Down

0 comments on commit e1b28e7

Please sign in to comment.