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

megathread: Window State Persistence #9800

Open
10 of 16 tasks
zadjii-msft opened this issue Apr 13, 2021 · 12 comments
Open
10 of 16 tasks

megathread: Window State Persistence #9800

zadjii-msft opened this issue Apr 13, 2021 · 12 comments
Labels
Area-Remoting Communication layer between windows. Often for windowing behavior, quake mode, etc. Issue-Scenario Product-Terminal The new Windows Terminal.
Milestone

Comments

@zadjii-msft
Copy link
Member

zadjii-msft commented Apr 13, 2021

[Original thread: #766] [Spec: {none}] [Initial PR: #10972] [Multiple windows PR: #11083]

This thread is being used to track all the component work for restoring the window state. Additionally related threads:

Settings UI Mockup (programmer art warning)

mockup

2.0 Bugs

these are all presuming that #10972 merges basically as is

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Apr 13, 2021
@zadjii-msft zadjii-msft added ⛺ Reserved For future use and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Apr 13, 2021
@zadjii-msft zadjii-msft changed the title <reserved> megathread: Window State Persistence Sep 1, 2021
@zadjii-msft zadjii-msft added Area-Remoting Communication layer between windows. Often for windowing behavior, quake mode, etc. Issue-Scenario Product-Terminal The new Windows Terminal. and removed ⛺ Reserved For future use labels Sep 1, 2021
@zadjii-msft zadjii-msft added this to the Terminal Backlog milestone Sep 1, 2021
@Rosefield
Copy link
Contributor

Rosefield commented Sep 1, 2021

Re

How do we persist something like wt -- cmd.exe? That won't have a profile, only a NewTerminalArgs, that we don't cache at runtime

I can answer that concretely, just fine (the TerminalSettings on a control has all of these properties)

{
	...
	"tabLayout" : 
	[
		{
			"action" : "newTab",
			"colorScheme" : "Solarized Dark",
			"commandline" : "cmd.exe",
			"profile" : "Default",
			"startingDirectory" : null,
			"suppressApplicationTitle" : false,
			"tabTitle" : "cmd.exe"
		}
	]
}

@Rosefield
Copy link
Contributor

Rosefield commented Sep 3, 2021

Re

We should wrap this feature up in a Preview-only feature flag (no external community member should have to deal with that)

This is done on 10972 now

Opening an elevated window will automatically re-open all the unelevated window state (as elevated!). This is because there are separate monarchs for elevated and unelevated windows.

I disabled the feature entirely for elevated processes on 11083 in 8ffd314

Window names need to be persisted as well.

Was added to 11083 in a93d3b1
With some minor modification it should be pretty safe to cherry-pick these if necessary.

How exactly does this work with the _quake window? Should that participate in the session restore? Probably, yea.

With just 10972 this is a problem since that is explicitly checking for the existence of only one window remaining. With 11083 the _quake window will be saved just fine, but might lead to confusing users if they closed all of their visible windows and thinking that it saved the last layout. If the quit action is added that just becomes a point of user education that quit is the proper way to close all of their windows and save them. There might be some issues with saving the position/size of the quake window, but there is already special logic to handle _quake so it might just turn out OK magically.

Same idea with a defterm connection - We won't be able to restore defterm panes at all, because we have no idea what profile they were. Right?

Hypothetically this should be the same as the commandline version above in that it saves fine, but as the "Default" profile.

@zadjii-msft
Copy link
Member Author

wowow I half-assed a list of showerthoughts of things that might go wrong with this (that could be fixed in post), and the man just goes and fixes/addresses all of them. That's great, thanks!

@Rosefield
Copy link
Contributor

Rosefield commented Sep 20, 2021

Re

Closing the last window with ClosePane will clear out any persisted layout.
Similarly with closing the last tab by clicking the x on the tab itself. Doesn't seem to save the window layout in that case.

That was deliberate behavior because I took those actions to mean "I don't want to save these tab(s)". If you close all of your tabs in your browser and then re-open the browser it wouldn't have saved anything either. This is yet again a problem that is ameliorated by quit.

@zadjii-msft
Copy link
Member Author

Okay, thanks for clearing that up. That makes sense, it was a little unexpected by that's fine so long as we've got it all documented.

@zadjii-msft
Copy link
Member Author

Another bug (sorry for the very non minimal repro)

        {
            "hidden":false,
            "name" : "Ubuntu",
            "background" : "#2C001E",
            "tabColor": "#2C001E",
            "commandline" : "wsl.exe",
            "colorScheme" : "Builtin Tango Dark",
        }
        

If you open a tab with that profile, then close the window and have it reopen, the background that's set in the profile (#2C001E) will not be visible, it'll be the one from he scheme

@Rosefield
Copy link
Contributor

Added to #11083 (until that is merged I'll be making bug fixes there)

ghost pushed a commit that referenced this issue Oct 4, 2021
…11374)

<!-- 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
Try to save the working directory if we know what it is (just copied what was done in duplicating a pane). I overlooked this in my original implementation that always used the settings StartingDirectory.

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

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [ ] Closes #xxx
* [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
* [ ] 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

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
Tried setting the working directory using the OSC 9;9 escape and confirmed that the directory saves correctly.
@ykoehler
Copy link

In the same idea, I would like to be able to "obtain" the wt command to execute to open up my tab exactly the same way (with same panes and panes size, etc). From there I could paste that into a shortcut icon and create a different layout for each icon to open with. So if somehow we could dump a wt command output that I re-run and get a window with the same tab/pane and size that would be appreciated, in this case, I do not care about running commands in those, but I get you would have to support it.

@zadjii-msft
Copy link
Member Author

I'm stashing this here because it was weird, but not because I think we need to do anything about it.

I restarted after an update and found two of these entries in my state.json:

			"tabLayout" : 
			[
				{
					"action" : "newTab",
					"commandline" : "\"C:\\Windows\\System32\\cmd.exe\" /q /c rmdir /s /q \"C:\\Users\\migrie\\AppData\\Local\\Microsoft\\OneDrive\\21.234.1111.0001\"",
					"profile" : "cmd",
					"startingDirectory" : "C:\\Users\\migrie",
					"suppressApplicationTitle" : false,
					"tabTitle" : "C:\\Windows\\System32\\cmd.exe"
				}
			]

(The other was for 21.226.1101.0001, but otherwise idendical)

Both of them opened like
image

Which is quite curious. Presumably these are defterm windows that got captured by the Terminal, but also the Terminal must have been closed at just the right time that these were open.

@zadjii-msft zadjii-msft removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 23, 2021
ghost pushed a commit that referenced this issue Dec 1, 2021
## Summary of the Pull Request

Currently, the TermControl and ControlCore recieve a settings object that implements `IControlSettings`. They use for this for both reading the settings they should use, and also storing some runtime overrides to those settings (namely, `Opacity`). The object they recieve currently is a `T.S.M.TerminalSettings` object, as well as another `TerminalSettings` object if the user wants to have an `unfocusedAppearance`. All these are all hosted in the same process, so everything is fine and dandy. 

With the upcoming move to having the Terminal split into multiple processes, this will no longer work. If the `ControlCore` in the Content Process is given a pointer to a `TerminalSettings` in a certain Window Process, and that control is subsequently moved to another window, then there's no guarantee that the original `TerminalSettings` object continues to exist. In this scenario, when window 1 is closed, now the Core is unable to read any settings, because the process that owned that object no longer exists. 

The solution to this issue is to have the `ControlCore`'s own their own copy of the settings they were created with. that way, they can be confident those settings will always exist. Enter `ControlSettings`, a dumb struct for just storing all the contents of the Settings. I used x-macros for this, so that we don't need to copy-paste into this file every time we add a setting. 

Changing this has all sorts of other fallout effects:
* Previewing a scheme/anything is a tad bit more annoying. Before, we could just sneak the previewed scheme into a `TerminalSettings` that lived between the settings we created the control with, and the settings they were actually using, and it would _just work_. Even explaining that here, it sounds like magic, because it was. However, now, the TermControl can't use a layered `TerminalSettings` for the settings anymore. Now we need to actually read out the current color table, and set the whole scheme when we change it. So now there's also a `Microsoft.Terminal.Core.Scheme` _struct_ for holding that data. 
  - Why a `struct`? Because that will go across the process boundary as a blob, rather than as a pointer to an object in the other process. That way we can transit the whole struct from window to core safely. 
* A TermControl doesn't have a `IControlSettings` at all anymore - it initalizes itself via the settings in the `Core`. This will be useful for tear-out, when we need to have the `TermControl` initialize itself from just a `ControlCore`, without being able to rebuild the settings from scratch.
* The `TabTests` that were written under the assumption that the Control had a layered `TerminalSettings` obviously broke, as they were designed to. They've been modified to reflect the new reality.
* When we initialize the Control, we give it the settings and the `UnfocusedAppearance` all at once. If we don't give it an `unfocusedAppearance`, it will just use the focused appearance as the unfocused appearance.
* The Control no longer can _write_ settings to the `ControlSettings`. We don't want to be storing things in there. Pretty much everything we set in the control, we store somewhere other than in the settings object itself. However, `opacity` and `useAcrylic`, we need to store in a handy new `RUNTIME_SETTING` property. We can write those runtime overrides to those properties.  
* We no longer store the color scheme for a pane in the persisted state. I'm tracking that in #9800. I don't think it's too hard to add back, but I wanted this in front of eyes sooner than later.

## References

* #1256
* #5000
* #9794 has the scheme previewing in it.
* #9818 is WAY more possible now.

## PR Checklist
* [x] Surprisingly there wasn't ever a card or issue for this one. This was only ever a bullet point in #5000. 
* A bunch of these issues were fixed along the way, though I never intended to fix them:
  * [x] Closes #11571
  * [x] Closes #11586
  * [x] Closes #7219
  * [x] Closes #11067
  * [x] I think #11623 actually ended up resolving this one, but I'm double tapping on it here: Closes #5703
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

Along the way I tried to clean up code where possible, but not too agressively. 

I didn't end up converting the various `MockTerminalSettings` classes used in tests to the x macros quite yet. I wanted to merge this with #11416 in `main` before I went too crazy.

## Validation Steps Performed

* [x] Scheme previewing works
* [x] Adjusting the font size works
* [x] focused/unfocused appearances still work
* [x] mouse-wheeling opacity still works
* [x] acrylic & cleartype still does the right thing
* [x] saving the settings still works
* [x] going wild on sliding the opacity slider in the settings doesn't crash the terminal
* [x] toggling retro effects with a keybinding still works
* [x] toggling retro effects with the command palette works
* [x] The matrix of (`useAcrylic(true,false)`)x(`opacity(50,100)`)x(`antialiasingMode(cleartype, grayscale)`) works as expected. Slightly changed, falls back to grayscale more often, but looks more right.
ghost pushed a commit that referenced this issue Jan 10, 2022
This commit adds additional information to the persisted window layouts.
- Runtime tab titles
- Focus, Maximized, and Fullscreen modes.

Also,
- Adds actions for Set{Focus,FullScreen} that take a boolean
  to work in addition to the current Toggle{Focus, Fullscreen} actions.
- Adds SetMaximized that takes a boolean.
  This adds the capability to maximize (resp restore) a window using
  standard terminal actions.
  This also involves hooking up a good amount of state tracking between
  the terminal page and the window to see when maximize state has changed
  so that it can be persisted.
- These actions are not added to the default settings, but they could be.
  The intention is that they could assist with automation (and was originally)
  how I planned on persisting the state instead of augmenting the LaunchMode.
- The fullscreen/maximized saving isn't perfect because we don't have a
  way to save the non-maximized/fullscreen size, so exiting the modes
  won't restore whatever the previous size was.

References #9800 
Closes #11878 
Closes #11426
ghost pushed a commit that referenced this issue Jan 11, 2022
<!-- 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
Add an action to restore the last closed pane or tab. When all you have is restoring last sessions everything looks like a `std::vector<ActionAndArgs>`.

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

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Partially closes #960 
* [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
- Keep a buffer of panes/tabs that were closed recently in the form of a list
  of actions to remake it.
- To restore the pane or tab just run the list of actions.
- This (deliberately) does not restore the exact visual state as focus could
  have changed / new panes might have been created in the mean time. Mostly
  this means that restoring a pane will just attach to the currently focused
  pane instead of whatever its old neighbor was.
- Buffer is limited to 100 entries which might as well be "infinite" by most reasonable 
  standards, but prevents complaints about there being memory leaks in long running 
  instances.
- The action name could be potentially changed, but it felt unwieldy as "restoreLastClosedPaneOrTab".
- This does not handle restoring the actual running contents of a pane.

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
@Rosefield
Copy link
Contributor

What should still be done in order to enable persisted layouts for stable builds? I'm not sure if there is any telemetry for how often the feature is enabled / crashes related to it. My n=1 data says that it has been working quite well :).

@zadjii-msft zadjii-msft added the Needs-Discussion Something that requires a team discussion before we can proceed label Feb 7, 2022
@zadjii-msft
Copy link
Member Author

That's a great question. I'm personally happy with it, I kinda forgot it was preview only. IMO we should have this in 1.12 Stable, but I'll discuss with the team.

@zadjii-msft
Copy link
Member Author

Okay, team consensus. We're gonna move this to stable in 1.14. Just give it a little more time to bake. Overall, we're really happy with it, you did a great job ☺️

@zadjii-msft zadjii-msft removed the Needs-Discussion Something that requires a team discussion before we can proceed label Feb 7, 2022
zadjii-msft added a commit that referenced this issue Mar 9, 2023
  This removes all the weirdness around the way that TerminalPage needs to track
  the number of open windows. Instead of TerminalPage persisting empty state
  when the last tab closes, it lets the AppHost know that the last tab was
  closed due to _closing the tab_ (as opposed to closing the window / quitting).
  This gives AppHost an opportunity to persist empty state for that case,
  because _it_ knows how many windows there are.

  This could basically be its own PR.

  Probably worth xlinking this commit to #9800
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Remoting Communication layer between windows. Often for windowing behavior, quake mode, etc. Issue-Scenario Product-Terminal The new Windows Terminal.
Projects
None yet
Development

No branches or pull requests

3 participants