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

Conversation

Rosefield
Copy link
Contributor

@Rosefield Rosefield commented Aug 18, 2021

This commit adds initial support for saving window layout on application
close.

Done:

  • Add user setting for if tabs should be maintained.
  • Added events to track the number of open windows for the monarch, and
    then save if you are the last window closing.
  • Saves layout when the user explicitly hits the "Close Window" button.
  • If the user manually closed all of their tabs (through the tab x
    button or through closing all panes on the tab) then remove any saved
    state.
  • Saves in the ApplicationState file a list of actions the terminal can
    perform to restore its layout and the window size/position
    information.
  • This saves an action to focus the correct pane, but this won't
    actually work without Only focus the active pane once initialization is complete #10978. Note that if you have a pane zoomed, it
    does still zoom the correct pane, but when you unzoom it will have a
    different pane selected.

Todo:

Next Steps:

  • The business logic of when the save is triggered can be adjusted as
    necessary.
  • Right now I am taking the pragmatic approach and just saving the state
    as an array of objects, but only ever populate it with 1, that way
    saving multiple windows in the future could be added without breaking
    schema compatibility. Selfishly I'm hoping that handling multiple
    windows could be spun off into another pr/feature for now.
  • One possible thing that can maybe be done is that the commandline can
    be augmented with a "--saved ##" attribute that would load from the
    nth saved state if it exists. e.g. if there are 3 saved windows, on
    first load it can spawn three wt --saved {0,1,2} that would reopen the
    windows? This way there also exists a way to load a copy of a previous
    window (if it is in the saved state).
  • Is the application state something that is planned to be public/user
    editable? In theory the user could since it is just json, but I don't
    know what it buys them over just modifying their settings and
    startupActions.

Validation Steps Performed:

  • The happy path: open terminal -> set setting to true -> close terminal
    -> reopen and see tabs. Tested with powershell/cmd/wsl windows.
  • That closing all panes/tabs on their own will remove the saved
    session.
  • Open multiple windows, close windows and confirm that the last window
    closed saves its state.

The generated file stores a sequence of actions that will be executed to
restore the terminal to its saved form.

References #8324
This is also one of the items on #5000
Closes #766

- Add user setting for if tabs should be maintained (currently only supports the 1st window)
- Saves in the ApplicationState file a list of actions the terminal can perform to restore its layout.
@ghost ghost added Area-Settings Issues related to settings and customizability, for console or terminal Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Aug 18, 2021
@Rosefield
Copy link
Contributor Author

I swear the feature tests on my branches are cursed

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 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)

src/cascadia/TerminalApp/TerminalTab.cpp Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Aug 18, 2021

@check-spelling-bot Report

Unrecognized words, please review:

  • applicationstate
Previously acknowledged words that are now absent SPACEBAR Unregister xIcon yIcon
To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the git@github.com:Rosefield/terminal.git repository
on the feature/gh766-persist-tab-layout branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/638c6d029168500c5d95b33314d83bae093c8006.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/terminal/issues/comments/901492267" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u
✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

  • ... misspelled, then please correct them instead of using the command.
  • ... names, please add them to .github/actions/spelling/allow/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spelling/allow/.
  • ... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
  • ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

🗜️ If you see a bunch of garbage

If it relates to a ...

well-formed pattern

See if there's a pattern that would match it.

If not, try writing one and adding it to a patterns/{file}.txt.

Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

Note that patterns can't match multiline strings.

binary-ish string

Please add a file path to the excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

@Rosefield Rosefield changed the title Persist tab layout on window close Persist window layout on window close Aug 19, 2021
@github-actions
Copy link

github-actions bot commented Aug 19, 2021

@check-spelling-bot Report

Unrecognized words, please review:

  • firstmost
Previously acknowledged words that are now absent SPACEBAR Unregister xIcon yIcon
To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the git@github.com:Rosefield/terminal.git repository
on the feature/gh766-persist-tab-layout branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/482dcec60aa08522d4bb6b188cc05c9c80e22583.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/terminal/issues/comments/902212405" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u
✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

  • ... misspelled, then please correct them instead of using the command.
  • ... names, please add them to .github/actions/spelling/allow/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spelling/allow/.
  • ... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
  • ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

🗜️ If you see a bunch of garbage

If it relates to a ...

well-formed pattern

See if there's a pattern that would match it.

If not, try writing one and adding it to a patterns/{file}.txt.

Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

Note that patterns can't match multiline strings.

binary-ish string

Please add a file path to the excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

@github-actions
Copy link

github-actions bot commented Aug 19, 2021

@check-spelling-bot Report

Unrecognized words, please review:

  • firstmost
Previously acknowledged words that are now absent SPACEBAR Unregister xIcon yIcon
To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the git@github.com:Rosefield/terminal.git repository
on the feature/gh766-persist-tab-layout branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/482dcec60aa08522d4bb6b188cc05c9c80e22583.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/terminal/issues/comments/902214057" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u
✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

  • ... misspelled, then please correct them instead of using the command.
  • ... names, please add them to .github/actions/spelling/allow/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spelling/allow/.
  • ... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
  • ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

🗜️ If you see a bunch of garbage

If it relates to a ...

well-formed pattern

See if there's a pattern that would match it.

If not, try writing one and adding it to a patterns/{file}.txt.

Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

Note that patterns can't match multiline strings.

binary-ish string

Please add a file path to the excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

@DHowett
Copy link
Member

DHowett commented Sep 3, 2021

(Even asking if we want it behind a feature flag! You're on fire!)

…-tab-layout

 Conflicts:
	src/cascadia/Remoting/WindowManager.h
	src/cascadia/Remoting/WindowManager.idl
@Rosefield
Copy link
Contributor Author

I guess one problem with touching 41 different files is that you get merge conflicts all the time 🙃.

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.

Just a few comments. None blocking.

Excited to see this land! Thanks so much for doing this 😊

src/cascadia/TerminalApp/AppLogic.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/AppLogic.cpp Outdated Show resolved Hide resolved
other.Profile() == _Profile &&
other.SuppressApplicationTitle() == _SuppressApplicationTitle &&
other.ColorScheme() == _ColorScheme;
auto otherAsUs = other.try_as<NewTerminalArgs>();
Copy link
Member

Choose a reason for hiding this comment

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

oh wow. This is a really good catch. Thanks!

src/cascadia/Remoting/WindowManager.cpp Show resolved Hide resolved
src/cascadia/TerminalApp/Pane.cpp Outdated Show resolved Hide resolved
@Rosefield
Copy link
Contributor Author

I would like to get this in soon if there are no other comments, that way I can start addressing comments / bug fixes on the other branch.

@zadjii-msft
Copy link
Member

@DHowett is taking one last look at this, I think his review is in progress as we speak 😉

@DHowett
Copy link
Member

DHowett commented Sep 8, 2021

Moved image from body of commit:

Example
output

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

I am cool with all of this. It's super freakin' neat. Couple of nits, but barring the initialization one they can likely be addressed in post. Well, well done.

<description>Whether to allow the user to enable persisted window layout saving and loading</description>
<id>766</id>
<stage>AlwaysEnabled</stage>
<!-- This feature will not ship to Stable until it is complete. -->
Copy link
Member

Choose a reason for hiding this comment

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

I so much appreciate you doing this with a feature flag. 😄

@@ -131,6 +131,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
INHERITABLE_SETTING(Model::TerminalSettings, IFontAxesMap, FontAxes);
INHERITABLE_SETTING(Model::TerminalSettings, IFontFeatureMap, FontFeatures);

INHERITABLE_SETTING(Model::TerminalSettings, Model::ColorScheme, AppliedColorScheme);
Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Thanks.

Comment on lines +609 to +612
// The size is saved as a non-scaled real pixel size,
// so we need to scale it appropriately.
proposedSize.Height = proposedSize.Height * scale;
proposedSize.Width = proposedSize.Width * scale;
Copy link
Member

Choose a reason for hiding this comment

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

This may cause an issue when you restore your window on the wrong DPI screen; worth worrying about?

Copy link
Contributor Author

@Rosefield Rosefield Sep 8, 2021

Choose a reason for hiding this comment

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

When we save we save the "actual" pixel size instead of the visual size(i.e. without any scaling). So if you save on a screen at 150% scaling, and reopen on a screen at 100% scaling, then it will appear smaller (assuming both screens have the same dpi). Whereas if you save on a 150% screen and open on a 150% screen it will appear the same size because we scale it on load.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me! Thanks

if (const auto args = action.Args().try_as<NewTabArgs>())
{
NewTerminalArgs defaultArgs{};
return args.TerminalArgs() == nullptr || args.TerminalArgs().Equals(defaultArgs);
Copy link
Member

Choose a reason for hiding this comment

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

technically wt nt will trigger this, but I don't know if i care 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in #11083 I think this is partially ameliorated in that when we open multiple windows we actually check if any arguments at all were provided, and if so we make a new window instead of putting the saved layout in the current window.


auto dpi = GetDpiForWindow(_hostingHwnd.value());
RECT nonClientArea{};
LOG_IF_WIN32_BOOL_FALSE(AdjustWindowRectExForDpi(&nonClientArea, windowStyle, false, 0, dpi));
Copy link
Member

Choose a reason for hiding this comment

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

technically TerminalPage shouldn't know too much about its hwnd, but if we ever change our hosting model we'll just change this 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is fixed in #11083 so that all of the position logic is handled in AppHost

src/cascadia/TerminalApp/TerminalPage.h Outdated Show resolved Hide resolved
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!

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 8, 2021
@ghost
Copy link

ghost commented Sep 8, 2021

Hello @DHowett!

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.

@DHowett
Copy link
Member

DHowett commented Sep 8, 2021

Automerge: LET'S DO THIS

@ghost ghost merged commit 13e9546 into microsoft:main Sep 8, 2021
ghost pushed a commit that referenced this pull request Sep 14, 2021
Fix infinite loop when trying to summon a window after close.

In #10972 code was added to try to clean up state manually when a window
was closed instead of waiting for it to be detected as a dead peasant.
Unfortunately I didn't know any better and missed cleaning up
`_mruPeasants` as well. The result is there  would be an infinite loop
in `_getMostRecentPeasant` since `_getPeasant` will only clean up ids if
it finds a peasant, not if it doesn't find anything. This is the minimal
change to get this working, but it might be a good idea to make
`_getPeasant` be more thorough about cleanup.

## Validation Steps Performed
Tested that before the change we infinitely loop, and after the change
we summon correctly.

Closes #11215
Rosefield added a commit to Rosefield/terminal-1 that referenced this pull request Sep 22, 2021
ghost pushed a commit that referenced this pull request Sep 27, 2021
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
Continuation of #10972 to handle multiple windows, requires that to be merged first. 

<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> 
## References

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Also closes #766
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [x] Schema updated.
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments
Rough changelog:
Normally saving is triggered to occur every 30s, or sooner if a window is created/closed. The existing behavior of saving on last close is maintained to bypass that throttling. The automatic saving allows for crash recovery. Additionally all window layouts will be saved upon taking the `quit` action.

For loading we will check if we are the first window, that there are any saved layouts, and if the setting is enabled, and then depending on if we were given command line args or startup actions.

- create a new window for each saved layout, or
- take the first layout for our self and then a new window for each other layout.

This also saves the layout when the quit action is taken.

Misc changes
- A -s,--saved argument was added to the command line to facilitate opening all of the windows with the right settings. This also means that while a terminal session is running you can do wt -s idx to open a copy of window idx. There isn't a stable ordering of which idx each window gets saved as (it is whatever the iteration order of _peasants is), so it is just a cute hack for now.
- All position calculation has been moved up to AppHost this does mean we need to awkwardly pass around positions in a couple of unexpected places, but no solution was perfect.
- Renamed "Open tabs from a previous session" to "Open windows from a previous session". (not reflected in video below)
- Now save runtime tab color and window names
- Only enabled for non-elevated windows
- Add some change tracking to ApplicationState

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
![output](https://user-images.githubusercontent.com/6185249/131163473-d649d204-a589-41ad-b9d9-c4c0528cb684.gif)
@ghost
Copy link

ghost commented Oct 20, 2021

🎉Windows Terminal Preview v1.12.2922.0 has been released which incorporates this pull request.:tada:

Handy links:

@Poopooracoocoo
Copy link

I got my party poppers out too early there. Terminal STILL doesn't persist its window size and position! Does this PR not do what I thought it does?

@fgodoy
Copy link

fgodoy commented Oct 20, 2021

Greet news! ;)) Tks!

@zadjii-msft
Copy link
Member

If you're coming here in the 1.12 release wondering why this doesn't work, please make sure to enable the setting:
image
It also won't work for elevated windows at the moment.

@kwkaiwang
Copy link

tested 1.12 in three monitors environment, still failed to restore the position in the 2nd or 3rd monitor with maximized window mode.

Windows Terminal window got restored in the primary monitor (3840x2160) instead, with similar 2nd monitor size (1920x1080).

@zadjii-msft
Copy link
Member

@kwkaiwang can you file a new issue? Make sure to include state.json, which lives next to settings.json

@kwkaiwang
Copy link

@kwkaiwang can you file a new issue? Make sure to include state.json, which lives next to settings.json

Check this: #11639

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-Settings Issues related to settings and customizability, for console or terminal Area-UserInterface Issues pertaining to the user interface of the Console or Terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable Terminal to persist / restore instance settings
9 participants