-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Conversation
- 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.
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)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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)
…f closing the window specifically)
@check-spelling-bot ReportUnrecognized words, please review:
Previously acknowledged words that are now absentSPACEBAR Unregister xIcon yIconTo 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
✏️ Contributor please read thisBy 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.
If the listed items are:
See the 🔬 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 garbageIf it relates to a ... well-formed patternSee if there's a pattern that would match it. If not, try writing one and adding it to a 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 stringPlease add a file path to the File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
|
@check-spelling-bot ReportUnrecognized words, please review:
Previously acknowledged words that are now absentSPACEBAR Unregister xIcon yIconTo 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
✏️ Contributor please read thisBy 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.
If the listed items are:
See the 🔬 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 garbageIf it relates to a ... well-formed patternSee if there's a pattern that would match it. If not, try writing one and adding it to a 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 stringPlease add a file path to the File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
|
@check-spelling-bot ReportUnrecognized words, please review:
Previously acknowledged words that are now absentSPACEBAR Unregister xIcon yIconTo 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
✏️ Contributor please read thisBy 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.
If the listed items are:
See the 🔬 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 garbageIf it relates to a ... well-formed patternSee if there's a pattern that would match it. If not, try writing one and adding it to a 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 stringPlease add a file path to the File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
|
…an stay the stay if we choose to have multiple windows saved in the future.
(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
I guess one problem with touching 41 different files is that you get merge conflicts all the time 🙃. |
There was a problem hiding this 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 😊
other.Profile() == _Profile && | ||
other.SuppressApplicationTitle() == _SuppressApplicationTitle && | ||
other.ColorScheme() == _ColorScheme; | ||
auto otherAsUs = other.try_as<NewTerminalArgs>(); |
There was a problem hiding this comment.
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!
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. |
@DHowett is taking one last look at this, I think his review is in progress as we speak 😉 |
There was a problem hiding this 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. --> |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Thanks.
// 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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
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
assert(_IsLeaf()); | ||
|
||
NewTerminalArgs args{}; | ||
auto controlSettings = _control.Settings().as<TerminalSettings>(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
Hello @DHowett! Because this pull request has the 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 (
|
Automerge: LET'S DO THIS |
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
<!-- 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)
🎉 Handy links: |
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? |
Greet news! ;)) Tks! |
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). |
@kwkaiwang can you file a new issue? Make sure to include |
Check this: #11639 |
This commit adds initial support for saving window layout on application
close.
Done:
then save if you are the last window closing.
button or through closing all panes on the tab) then remove any saved
state.
perform to restore its layout and the window size/position
information.
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:
window.
appear to be focused on opening.
Next Steps:
necessary.
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.
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).
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:
-> reopen and see tabs. Tested with powershell/cmd/wsl windows.
session.
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