Persist window layout cont. save multiple windows

[Rosefield]

Summary of the Pull Request

Continuation of #10972 to handle multiple windows, requires that to be merged first.

References

PR Checklist

• Also closes Enable Terminal to persist / restore instance settings #766
• CLA signed. If not, go over here and sign the CLA
• Tests added/passed
• Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
• 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

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

Validation Steps Performed

[Rosefield]

This PR as a whole is difficult to look at now, so discussion can wait for the main one to be merged.

Because @lhecker had some concerns

• Right now only the monarch saves the window layouts
• It wasn't a problem when the pr was initially created, but now since ApplicationState is being used for the command history I added in some basic change tracking. (1791c88) This way we only update the fields that the particular window actually changed, and specifically for this only the monarch should be updating the PersistedWindowLayouts so this should be pretty safe. I don't know the atomicity of file creates on ntfs, but another option would be to create a state.lock file when _write (or some other method of cross-process synchronization) starts and unlink it once we're done.

[Rosefield]

branch has been rebased and is now open for business

[Rosefield]

Added saving on quit, so I believe this is now feature complete.

[Rosefield]

So, I was trying to make a video to show off the feature with the new quit, but I managed to reproduce the exception on loading multiple windows that I found in #11143 that kills the monarch. I can't get this to reproduce every time, but it is somewhat frequent.

Video of it happening because I was trying to create a new video that doesn't include me killing it in task manager.

2021-09-09.11-28-43.mp4

[Rosefield]

In further testing I've also run into the issue that @leonMSFT warned about where quitting doesn't always cause all windows to quit, despite expectation. Specifically sometimes I've seen that one window might not receive the quit somehow. Its always only one window in my experience, not multiple. This also happens with what is on main, so not including any changes here.

Edit: When I've seen the crash on load, and also the failure to quit, I've noticed that the call stack is this. I'm not sure if that is a coincidence or not. Further inspection seems like the window that is left/crashed probably was/just became the monarch because it has a dozen threads in various monarch methods like _doHandleActivatePeasant.

I put a memory dump at https://rosefield.org/files/QuitAllCrash.dmp and would appreciate some help debugging :)

 	[External Code]	
>	Microsoft.Terminal.Control.dll!FontInfoBase::FontInfoBase(const std::basic_string_view<wchar_t,std::char_traits<wchar_t>> faceName, const unsigned char family, const unsigned int weight, const bool fSetDefaultRasterFont, const unsigned int codePage) Line 24	C++
 	Microsoft.Terminal.Control.dll!FontInfoBase::FontInfoBase(const FontInfoBase & fibFont) Line 40	C++
 	Microsoft.Terminal.Control.dll!FontInfo::FontInfo(const FontInfo & fiFont) Line 30	C++
 	Microsoft.Terminal.Control.dll!winrt::Microsoft::Terminal::Control::implementation::ControlCore::GetFont() Line 1011	C++
 	Microsoft.Terminal.Control.dll!winrt::Microsoft::Terminal::Control::implementation::ControlInteractivity::_getTerminalPosition(const til::point & pixelPosition) Line 598	C++
 	Microsoft.Terminal.Control.dll!winrt::Microsoft::Terminal::Control::implementation::ControlInteractivity::PointerMoved(winrt::Microsoft::Terminal::Control::MouseButtonState buttonState, const unsigned int pointerUpdateKind, const Microsoft::Terminal::Core::ControlKeyStates modifiers, const bool focused, const til::point pixelPosition, const bool pointerPressedInBounds) Line 288	C++
 	Microsoft.Terminal.Control.dll!winrt::impl::produce<winrt::Microsoft::Terminal::Control::implementation::ControlInteractivity,winrt::Microsoft::Terminal::Control::IControlInteractivity>::PointerMoved(unsigned int buttonState, unsigned int pointerUpdateKind, winrt::impl::struct_Microsoft_Terminal_Core_ControlKeyStates modifiers, bool focused, winrt::impl::struct_Microsoft_Terminal_Core_Point pixelPosition, bool pointerPressedInBounds) Line 2420	C++
 	Microsoft.Terminal.Control.dll!winrt::impl::consume_Microsoft_Terminal_Control_IControlInteractivity<winrt::Microsoft::Terminal::Control::IControlInteractivity>::PointerMoved(const winrt::Microsoft::Terminal::Control::MouseButtonState & buttonState, unsigned int pointerUpdateKind, const winrt::Microsoft::Terminal::Core::ControlKeyStates & modifiers, bool focused, const winrt::Microsoft::Terminal::Core::Point & pixelPosition, bool pointerPressedInBounds) Line 595	C++
 	Microsoft.Terminal.Control.dll!winrt::Microsoft::Terminal::Control::implementation::TermControl::_PointerMovedHandler(const winrt::Windows::Foundation::IInspectable & __formal, const winrt::Windows::UI::Xaml::Input::PointerRoutedEventArgs & args) Line 1143	C++
 	[External Code]	
 	Microsoft.Terminal.Control.dll!winrt::impl::delegate<winrt::Windows::UI::Xaml::Input::PointerEventHandler,void <lambda>(const winrt::Windows::Foundation::IInspectable &, const winrt::Windows::UI::Xaml::Input::PointerRoutedEventArgs &)>::Invoke(void * sender, void * e) Line 1461	C++
 	[External Code]	
 	WindowsTerminal.exe!wWinMain(HINSTANCE__ * __formal, HINSTANCE__ * __formal, wchar_t * __formal, int __formal) Line 201	C++
 	[External Code]	

Edit Edit: Possibly a com deadlock? Looks like there are a bunch of threads stuck on MSVCP140D.dll _Mtxlock waiting on a thread that is at combase.dll MTAThreadWaitForCall. I don't know if that is an artifact of attaching the debugger to the hung process or if the process was hung because of it.

Edit Edit Edit: All of the monarch as written basically assumes to be single-threaded, but that is definitely not true. E.g. I managed to get this exception too on main by just opening a bunch of windows.

[ghost]

Hello @zadjii-msft!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

• I'll only merge this pull request if it's approved by @zadjii-msft and @lhecker

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

[Rosefield]

Otherwise, I'm looking forward to selfhosting this. In the time I've been selfhosting #10972, EVERY time I've gotten an update, I've accidentally had another window open that got persisted instead of my main work window -.-

Wait, any window at all is getting persisted with just 10972 on a force close of the terminal? Interesting. I would've expected nothing to get saved. At least that is what happened when I killed the process locally in my testing.

[Rosefield]

Oh, has the project taken an explicit dependency on powershell 7 now? I notice that I can't compile after the merge because I don't have pwsh.exe in my path

[Rosefield]

Oh, that came out of the json::value refactor, didn't it.

[zadjii-msft]

Wait, any window at all is getting persisted with just 10972 on a force close of the terminal? Interesting. I would've expected nothing to get saved. At least that is what happened when I killed the process locally in my testing.

It's possible that it's got leftover state from some previous window closing and then the force-killed one isn't getting persisted. I'm not really sure, this is of course happening at like 1am 😛

[zadjii-msft]

Oh, has the project taken an explicit dependency on powershell 7 now? I notice that I can't compile after the merge because I don't have pwsh.exe in my path

Yep, that's right - we may want to update the docs for that...

[lhecker]

Oh, has the project taken an explicit dependency on powershell 7 now? I notice that I can't compile after the merge because I don't have pwsh.exe in my path

I'm sorry. That was me! I needed pwsh.exe because the old powershell.exe doesn't support comments in JSON. I'm using PowerShell to "minify" our JSON files before bundling them into our binaries.

The reason I didn't just write a regex is because some of the patterns (like //) can occur within strings. Regex is basically lacking the contextual awareness necessary for JSON.

[Rosefield]

I remembered reading that comment in the other pr, but didn't realize it had been merged yet so was thrown off guard. I figured it out once I noticed the failing command was on running against defaults.json

[Rosefield]

Probably took longer that you would have, and I might have blown up my state file a couple of times with the wrong file attributes, but I think it is working correctly now.

[github-actions]

@check-spelling-bot Report

Unrecognized words, please review:

• LOCKFILE

Previously acknowledged words that are now absent

carlos dpg sid SPACEBAR Unregister xIcon yIcon zamora

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 [email protected]:Rosefield/terminal.git repository
on the feature/gh766-cont-save-multiple-windows 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/0f122ca290125951c21d7c830d73a10337bab6b7.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/926213584" > "$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]

Just to confirm the only outstanding issue to be resolved is the file synchronization? I tried rereading all of the resolved/hidden comments but I want to make sure I'm not missing anything.

[lhecker]

@Rosefield We've talked within our team about this today and we'll probably move the ApplicationState singleton into the monarch. That way only a single process reads/writes the state.json file at a time and holds a consistent state for all peasants. This will be an excellent follow-up PR for a later time. 🙂

[zadjii-msft]

NICENICENICE

I'm gonna merge this so we can selfhost it for our internal bug bash tomorrow. I'm sure Dustin will have his share of thoughts when he gets back from vacation, but let's fix the nits in post.

[Rosefield]

I look forward to no longer babysitting this pr, but instead compulsively checking the issues list to see if I broke anything :)

[ghost]

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

Handy links:

• Release Notes
• Store Download