Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Persist window layout on window close #10972
Changes from 3 commits
405c853
6297417
a265e81
48cde20
4b897d0
773cc79
37e5bd4
1b6dfc5
c1722c7
d6997f9
8c64dd5
97bce32
ec79c93
d7ce2de
91c3d69
ef80c66
0a3ccd1
8d6a7ee
0a6d4f7
c0f7eaa
2062eb3
464ca59
3b02719
d204f00
1cd14b3
cfe3583
072822a
b8b7d48
4b1bbbd
50cf7fb
81af065
ef21ca6
ba56ded
6aeabb1
7f14c7d
8e22231
2f39ed9
735de10
230a1ab
8f38e1b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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!
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
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()
actionThere 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 man would you look at that, I'm ahead of myself
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)