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

Persist window layout on window close #10972

Merged
40 commits merged into from
Sep 8, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
405c853
GH766 Persist tab layout on window close
Rosefield Aug 18, 2021
6297417
Add missing comment
Rosefield Aug 18, 2021
a265e81
Try to save pane color information
Rosefield Aug 18, 2021
48cde20
formatting
Rosefield Aug 18, 2021
4b897d0
Also persist window size and location information
Rosefield Aug 18, 2021
773cc79
Remove state if the user manually closed all of their tabs (instead o…
Rosefield Aug 18, 2021
37e5bd4
Capitalize to make the spellchecker happy?
Rosefield Aug 18, 2021
1b6dfc5
Also format. Again. Maybe Ill learn my lesson one of these days.
Rosefield Aug 18, 2021
c1722c7
Update setting name to better reflect what it actually does
Rosefield Aug 19, 2021
d6997f9
Use inheritable settings correctly for the applied color scheme
Rosefield Aug 19, 2021
8c64dd5
Also save the tabs focused pane and zoom information
Rosefield Aug 19, 2021
97bce32
Fix pane id calculation, save the correct split size
Rosefield Aug 19, 2021
ec79c93
Foiled by whitespace again.
Rosefield Aug 19, 2021
d7ce2de
hyphenation to appease the spell checker
Rosefield Aug 19, 2021
91c3d69
Update comment on Pane::BuildStartupActions
Rosefield Aug 19, 2021
ef80c66
Switch to serializing an array of window layouts so that the schema c…
Rosefield Aug 20, 2021
0a3ccd1
Add consts for json keys
Rosefield Aug 22, 2021
8d6a7ee
use consts in more places
Rosefield Aug 23, 2021
0a6d4f7
Add to schema
Rosefield Aug 23, 2021
c0f7eaa
Save the correct location of the terminal.
Rosefield Aug 23, 2021
2062eb3
Save if we are the only window open, instead of if we are the first w…
Rosefield Aug 23, 2021
464ca59
Code review changes. Also fix restoring position on startup.
Rosefield Aug 24, 2021
3b02719
even more formatting.
Rosefield Aug 24, 2021
d204f00
minor cleanup
Rosefield Aug 24, 2021
1cd14b3
Put json converters in more reasonable places, improve the WindowLayo…
Rosefield Aug 24, 2021
cfe3583
try to move the actions more forcefully
Rosefield Aug 26, 2021
072822a
Switch to using a first window behavior setting (for future expansion…
Rosefield Aug 26, 2021
b8b7d48
code review: comments, better event handlers, and exception logging.
Rosefield Aug 26, 2021
4b1bbbd
Try to more accurately detect if the user provided no commandline arg…
Rosefield Aug 26, 2021
50cf7fb
Merge remote-tracking branch 'origin/main' into feature/gh766-persist…
Rosefield Aug 31, 2021
81af065
even merges arent safe from the formatter.
Rosefield Aug 31, 2021
ef21ca6
remove old setting from the schema.
Rosefield Aug 31, 2021
ba56ded
Merge remote-tracking branch 'origin/main' into feature/gh766-persist…
Rosefield Aug 31, 2021
6aeabb1
Make the NewTerminalArgs::Equals more like the other Equals implement…
Rosefield Sep 2, 2021
7f14c7d
formatting
Rosefield Sep 2, 2021
8e22231
Add a feature flag to disable this feature in release.
Rosefield Sep 3, 2021
2f39ed9
Merge remote-tracking branch 'origin/main' into feature/gh766-persist…
Rosefield Sep 3, 2021
735de10
formatting
Rosefield Sep 3, 2021
230a1ab
Code review: Make more things const that can be const
Rosefield Sep 3, 2021
8f38e1b
Initialize properties that were uninitialized
Rosefield Sep 8, 2021
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
2 changes: 2 additions & 0 deletions src/cascadia/TerminalApp/AppActionHandlers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -463,10 +463,12 @@ namespace winrt::TerminalApp::implementation
// 1 is important to make sure that the effects of
// something like `colortool` are cleared when setting
// the scheme.
/*
Rosefield marked this conversation as resolved.
Show resolved Hide resolved
if (controlSettings.GetParent() != nullptr)
{
parentSettings = controlSettings.GetParent();
}
*/

// ApplyColorScheme(nullptr) will clear the old color scheme.
controlSettings.ApplyColorScheme(nullptr);
Expand Down
121 changes: 121 additions & 0 deletions src/cascadia/TerminalApp/Pane.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,127 @@ Pane::Pane(const GUID& profile, const TermControl& control, const bool lastFocus
});
}

// Method Description:
// - Extract the terminal settings from the current (leaf) pane's control
// to be used to create an equivalent control
// Arguments:
// - <none>
// Return Value:
// - Arguments appropriate for a SplitPane or NewTab action
NewTerminalArgs Pane::GetTerminalArgsForPane() const
{
// Leaves are the only things that have controls
assert(_IsLeaf());

NewTerminalArgs args{};
auto controlSettings = _control.Settings().as<TerminalSettings>();
Copy link
Member

Choose a reason for hiding this comment

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

Eventually (#5047) we will graduate to simply storing the NewTerminalArgs that spawned the pane, so you don't need to reverse-engineer it. That will also help with @zadjii-msft's concern about putting the scheme name into the TerminalSettings¹.

¹ TerminalSettings was meant to be "the gateway for the app to tell the control about the settings"; the worry here is that we're using it to tell another part of ourselves (the app) something it could have found out another way².

² this shows of course that we could NOT find out about this another way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will those NewTerminalArgs get modified if the terminal changes? E.g. if pane title changing is added we'd probably want whatever the user set at runtime and not what was set on creation. Regardless that is a future issue.

Copy link
Member

Choose a reason for hiding this comment

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

HMM. We honestly might want to keep it up to date, if it's going to be the backbone of "duplicate pane." After all, session restoration is "duplicate pane across time", and everything that goes into immediate duplication should go into temporal duplication!


args.Profile(controlSettings.ProfileName());
args.StartingDirectory(controlSettings.StartingDirectory());
args.TabTitle(controlSettings.StartingTitle());
args.Commandline(controlSettings.Commandline());
args.SuppressApplicationTitle(controlSettings.SuppressApplicationTitle());
if (controlSettings.TabColor() || controlSettings.StartingTabColor())
{

til::color c;
// StartingTabColor is prioritized over other colors
if (controlSettings.StartingTabColor())
Rosefield marked this conversation as resolved.
Show resolved Hide resolved
{
c = til::color(controlSettings.StartingTabColor().Value());
}
else
{
c = til::color(controlSettings.TabColor().Value());
}

args.TabColor(winrt::Windows::Foundation::IReference<winrt::Windows::UI::Color>(c));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put this here, and it saves, but it also doesn't seem like creating a new tab actually uses the setting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the problem is actually just that when you set the tab color from the command palette doesn't set it on the TermControl?

Copy link
Member

Choose a reason for hiding this comment

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

Yea, tabcolor is a tricky one. There's multiple layers to it

  • the runtime tab color
  • the color the control thinks it should be

there's more details in this doc. But basically, if you set the tab color with the picker, it'll apply to the tab itself, not to the individual pane that's focused. It's a little weird, and definitely makes restoring this element harder. We almost need a setTabColor() action

Copy link
Member

Choose a reason for hiding this comment

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

}

if (controlSettings.AppliedColorScheme())
{
auto name = controlSettings.AppliedColorScheme().Name();
args.ColorScheme(name);
}

return args;
}

// Method Description:
// - Serializes the state of this tab as a series of commands that can be
// executed to recreate it.
// - This will always result in the right-most child being the focus
// after the commands finish executing.
// Arguments:
// - <none>
// Return Value:
// - A vector of commands and the original root pane for this new tree
std::pair<std::vector<ActionAndArgs>, std::shared_ptr<Pane>> Pane::BuildStartupActions()
{
// if we are a leaf then all there is to do is defer to the parent.
if (_IsLeaf())
{
return { {}, shared_from_this() };
}

auto buildSplitPane = [&](auto newPane) {
ActionAndArgs actionAndArgs;
actionAndArgs.Action(ShortcutAction::SplitPane);
auto terminalArgs{ newPane->GetTerminalArgsForPane() };
Rosefield marked this conversation as resolved.
Show resolved Hide resolved
SplitPaneArgs args{ SplitType::Manual, _splitState, _desiredSplitPosition, terminalArgs };
actionAndArgs.Args(args);

return actionAndArgs;
};

auto buildMoveFocus = [](auto direction) {
MoveFocusArgs args{ direction };

ActionAndArgs actionAndArgs{};
actionAndArgs.Action(ShortcutAction::MoveFocus);
actionAndArgs.Args(args);

return actionAndArgs;
};

// Handle simple case of a single split (a minor optimization for clarity)
// Here we just create the second child (by splitting) and return the first
// child for the parent to deal with.
if (_firstChild->_IsLeaf() && _secondChild->_IsLeaf())
{
auto actionAndArgs = buildSplitPane(_secondChild);
return { { actionAndArgs }, _firstChild };
}

// We now need to execute the commands for each side of the tree
auto [a1, p1] = _firstChild->BuildStartupActions();
auto [a2, p2] = _secondChild->BuildStartupActions();

std::vector<ActionAndArgs> actions;
actions.reserve(a1.size() + a2.size() + 3);

// first we make our split
auto newSplit = buildSplitPane(p2);
actions.push_back(newSplit);

if (a1.size() > 0)
{
// Then move to the first child and execute any actions on the left branch
// then move back
actions.push_back(buildMoveFocus(FocusDirection::PreviousInOrder));
actions.insert(actions.end(), a1.begin(), a1.end());
actions.push_back(buildMoveFocus(FocusDirection::NextInOrder));
}

// And if there are any commands to run on the right branch do so
if (a2.size() > 0)
{
actions.insert(actions.end(), a2.begin(), a2.end());
}

return { { actions }, p1 };
}

// Method Description:
// - Update the size of this pane. Resizes each of our columns so they have the
// same relative sizes, given the newSize.
Expand Down
3 changes: 3 additions & 0 deletions src/cascadia/TerminalApp/Pane.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@ class Pane : public std::enable_shared_from_this<Pane>
void ClearActive();
void SetActive();

std::pair<std::vector<winrt::Microsoft::Terminal::Settings::Model::ActionAndArgs>, std::shared_ptr<Pane>> BuildStartupActions();
winrt::Microsoft::Terminal::Settings::Model::NewTerminalArgs GetTerminalArgsForPane() const;

void UpdateSettings(const winrt::Microsoft::Terminal::Settings::Model::TerminalSettingsCreateResult& settings,
const GUID& profile);
void ResizeContent(const winrt::Windows::Foundation::Size& newSize);
Expand Down
62 changes: 62 additions & 0 deletions src/cascadia/TerminalApp/TerminalPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,19 @@ namespace winrt::TerminalApp::implementation
if (_startupState == StartupState::NotInitialized)
{
_startupState = StartupState::InStartup;

// If the user selected to save their tab layout, we are the first
// window opened, and wt was not run with any other arguments, then
// we should use the saved settings.
if (_settings.GlobalSettings().PersistTabLayout() && _WindowId == 1 && _startupActions.Size() == 1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open to other ideas on how we should handle multiple windows. (See my thoughts in the pull description)

{
auto actions = ApplicationState::SharedInstance().PersistedTabLayout();
if (actions && actions.Size() > 0)
{
_startupActions = actions;
}
}

ProcessStartupActions(_startupActions, true);

// If we were told that the COM server needs to be started to listen for incoming
Expand Down Expand Up @@ -1146,6 +1159,50 @@ namespace winrt::TerminalApp::implementation
return nullptr;
}

// Method Description:
// - Saves the tab layout to the application state
// Arguments:
// - <none>
// Return Value:
// - <none>
void TerminalPage::PersistTabLayout()
{
std::vector<ActionAndArgs> actions;

for (auto tab : _tabs)
{
if (auto terminalTab = _GetTerminalTabImpl(tab))
{
auto tabActions = terminalTab->BuildStartupActions();
actions.insert(actions.end(), tabActions.begin(), tabActions.end());
}
else if (tab.try_as<SettingsTab>())
{
ActionAndArgs action;
action.Action(ShortcutAction::OpenSettings);
OpenSettingsArgs args{ SettingsTarget::SettingsUI };
action.Args(args);

actions.push_back(action);
}
}

// if the focused tab was not the last tab, restore that
auto idx = _GetFocusedTabIndex();
if (idx && idx != _tabs.Size() - 1)
{
ActionAndArgs action;
action.Action(ShortcutAction::SwitchToTab);
SwitchToTabArgs switchToTabArgs{ idx.value() };
action.Args(switchToTabArgs);

actions.push_back(action);
}

auto state = ApplicationState::SharedInstance();
state.PersistedTabLayout(winrt::single_threaded_vector<ActionAndArgs>(std::move(actions)));
}

// Method Description:
// - Close the terminal app. If there is more
// than one tab opened, show a warning dialog.
Expand All @@ -1165,6 +1222,11 @@ namespace winrt::TerminalApp::implementation
}
}

if (_settings.GlobalSettings().PersistTabLayout() && _WindowId == 1)
{
PersistTabLayout();
}

_RemoveAllTabs();
}

Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalApp/TerminalPage.h
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ namespace winrt::TerminalApp::implementation
bool AlwaysOnTop() const;

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

void SetInboundListener(bool isEmbedding);
static std::vector<Microsoft::Terminal::Settings::Model::ActionAndArgs> ConvertExecuteCommandlineToActions(const Microsoft::Terminal::Settings::Model::ExecuteCommandlineArgs& args);

Expand Down
25 changes: 25 additions & 0 deletions src/cascadia/TerminalApp/TerminalTab.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,31 @@ namespace winrt::TerminalApp::implementation
control.ScrollViewport(::base::ClampAdd(currentOffset, delta));
}

// Method Description:
// - Serializes the state of this tab as a series of commands that can be
// executed to recreate it.
// Arguments:
// - <none>
// Return Value:
// - A vector of commands
std::vector<ActionAndArgs> TerminalTab::BuildStartupActions() const
{
auto [args, pane] = _rootPane->BuildStartupActions();

ActionAndArgs newTabAction{};
newTabAction.Action(ShortcutAction::NewTab);
// _getNewTerminalArgs MUST be called before parsing any other options,
// as it might clear those options while finding the commandline
NewTabArgs newTabArgs{ pane->GetTerminalArgsForPane() };
newTabAction.Args(newTabArgs);

args.insert(args.begin(), newTabAction);

// TODO: persist focused pane? zoomed pane?
Rosefield marked this conversation as resolved.
Show resolved Hide resolved

return args;
}

// Method Description:
// - Split the focused pane in our tree of panes, and place the
// given TermControl into the newly created pane.
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalApp/TerminalTab.h
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,8 @@ namespace winrt::TerminalApp::implementation
void EnterZoom();
void ExitZoom();

std::vector<Microsoft::Terminal::Settings::Model::ActionAndArgs> BuildStartupActions() const;

int GetLeafPaneCount() const noexcept;

void TogglePaneReadOnly();
Expand Down
5 changes: 5 additions & 0 deletions src/cascadia/TerminalSettingsEditor/Launch.xaml
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,11 @@
<ToggleSwitch IsOn="{x:Bind State.Settings.GlobalSettings.StartOnUserLogin, Mode=TwoWay}" />
</local:SettingContainer>

<!-- Retain and Reload Tab Layout -->
<local:SettingContainer x:Uid="Globals_PersistTabLayout">
<ToggleSwitch IsOn="{x:Bind State.Settings.GlobalSettings.PersistTabLayout, Mode=TwoWay}" />
</local:SettingContainer>

<!-- Launch Mode -->
<local:SettingContainer x:Uid="Globals_LaunchMode">
<muxc:RadioButtons ItemTemplate="{StaticResource EnumRadioButtonTemplate}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,14 @@
<value>When enabled, this enables the launch of Windows Terminal at machine startup.</value>
<comment>A description for what the "start on user login" setting does. Presented near "Globals_StartOnUserLogin.Header".</comment>
</data>
<data name="Globals_PersistTabLayout.Header" xml:space="preserve">
<value>Persist terminal tab layout</value>
<comment>Header for a control to toggle whether the app should launch when the user's machine starts up, or not.</comment>
</data>
<data name="Globals_PersistTabLayout.HelpText" xml:space="preserve">
<value>When enabled, the tab layout of terminal windows will be saved on close and reloaded on open.</value>
<comment>A description for what the "start on user login" setting does. Presented near "Globals_PersistTabLayout.Header".</comment>
</data>
<data name="Globals_AlwaysOnTop.Header" xml:space="preserve">
<value>Always on top</value>
<comment>Header for a control to toggle if the app will always be presented on top of other windows, or is treated normally (when disabled).</comment>
Expand Down
Loading