-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Spec for Default Profile Settings #3569
Conversation
##### Harder to mentally parse | ||
Maybe not as easy to mentally picture how one profile inherits from another. The | ||
user would probably need to manually build the tree of profile inheritance in | ||
their own head to understand how a profile gets its settings. |
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.
One additional concern:
I still want a place to define a default settings block that isn't one of my profiles.
- If it has to be a real live profile, would it be.. hidden=true?
- Would the inheritee inherit the hidden=true?
- If it wasn't, what would stop it from showing up in my list?
- If it had to be a real profile, would I then have to override the commandline?
Imagine this scenario:
"profiles": [
{
"guid": "{abcd}",
"name": "cmd",
"fontFace": "Cascadia Code",
"background": "#f00ff0",
"commandline": "cmd.exe"
},
{
"guid": "{defg}",
"name": "WSL",
"inheritsFrom": "{abcd}", // inherit from cmd because i put my defaults in there
"source": "Microsoft.Terminal.Wsl" // AUTOGENERATED DYNAMIC PROFILE
}
]
Does "WSL" inherit the commandline
? It should, since it inherits everything from cmd.
we probably do want inheritance eventually: I want to be able to do this:
"profiles": [
{
"guid": "{defg}",
"name": "Ubuntu",
"source": "Microsoft.Terminal.Wsl" // AUTOGENERATED DYNAMIC PROFILE
},
{
"name": "Ubuntu (dark theme)",
"inheritsFrom": "{defg}", // Ubuntu
"background": "#000000",
"colorScheme": "Really quite dark",
// I don't know the commandline: it's based on the ubuntu profile.
// I shouldn't _need_ to know it.
"source": "Microsoft.Terminal.Wsl"
}
]
that is out of spec for this proposal.
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 concur. 4 seems like the weakest proposal on paper, for this feature. Good overall, but far too heavy-handed for "these settings apply to every profile".
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.
It seems like source
and inheritsFrom
are kinda trying to do the same thing then. What if we combined them? source
is either a...
string
(i.e.:Microsoft.Terminal.WSL
or other dynamic profile ones)guid
: directly inherit the resolved profile with this guid
We already check for duplicate guids so we know it's a one-to-one relationship between a guid and a profile.
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.
Argh. I want it but you're probably right about it being too heavy-handed for this feature. 😭
`"profiles"` object. Makes sense. | ||
|
||
#### Concerns | ||
##### Mysterious `__defaults__` GUID |
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.
also ugh the schema violation
- `rootSettings` | ||
- `globalSettings`: again maybe conflicts a bit with other concepts/properties | ||
- `profileSettings` | ||
- `profilePrototype` |
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 do not consider a conflict with defaults.json to be a reason to not do a thing. If the problem is one of user education, we can get the word out on the streets in a different 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.
I strongly agree.
My opinion on the matter is that proposal 1 is the easiest to identify it's purpose, mentally parse, and easiest to change in the file. I don't believe that That's my opinion though, and I'm susceptible to being convinced otherwise. |
I personally like 4 the most for its infinite flexibility, but I'm biased since I am the one who verbally suggested that in the meeting. I would like to throw out 2 because magic constants are terrible. But if I have to lose on my "infinitely flexible" one, I prefer 3 to 1 because I like the scoped use of the word "default" with the object. |
I'm partial to approach 3. I like how it looks the most. It's the most intuitive for me personally, and it can get rid of confusion (if there's any) around I'm okay with 1. I agree with you @zadjii-msft, any confusion around approach 1 could get ironed out with some documentation. One thing I'd like to propose is to rename |
|
||
During the course of the pull request review on [#3369], the original pull request for this feature's implementation, it became apparent that the entire team has differing opinions on how this feature should be exposed to the user. This doc is born from that discussion. | ||
|
||
## Solution Proposals |
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.
At one point there was a 5th proposal:
- any of the settings for profiles should freely go in
globals
.
{
"defaultProfile": "<guid>",
"fontFace": "Cascadia Code",
"profiles": [ ... ]
}
From what I remember, this was abandoned because in a situation like the one above, it is unclear if "fontFace" would change the App's font vs the profiles' font.
It might be unintuitive that one profile from the list of profiles affects all | ||
the others. | ||
|
||
### Proposal 3: Change `profiles` to an object with a `list` of profiles and a `defaults` object |
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.
Personal favorite (though I really just want inheritance). I really like the following aspects of it...
- Easy schema change
- Supporting the
"profiles": [ ... ]
vs"profiles": { "defaults": { ... }, "list": [ ... ] }
syntax change is clever. I like the idea of just overloading it. Then maybe deprecating support for the legacy version at some point. - Scoping this to "profiles" is really easy to understand. No doubt that I am specifically making changes to my profiles
I'm thinking about what this would look like with the Settings UI. It would feel pretty intuitive. You could choose which specific profile you want to edit. Or just choose "edit my default profile settings" and it would edit this thing directly.
Love it.
Given that we can do 3 with a good fallback for "just treat it as an array", I think I'm sold on it. |
There's a dev team quorum on proposal 3, so I'll update the spec to reflect this. Thanks for the discussion all! |
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.
Marking Request Changes now that we've settled on proposal 3.
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.
Gracias
@miniksa / @leonMSFT / @cinnamon-msft can I get a third on this spec? |
@microsoft/windows-console-team this needs a third. |
## Summary of the Pull Request _This is attempt 2 at this feature_. The original PR can be found at #3369. These are settings that apply to _every_ profile, before user customizations. If the user wants to add "default profile settings", they can make the `"profiles"` property an _object_, instead of a list, and add `"defaults"` key underneath that object. The users list of profiles should then be under the `list` property of the `profiles` object. ## References #2515, #2603, #3369, #3569 ## PR Checklist * [x] Closes #2325 * [x] I work here * [x] Tests added/passed * [x] schema, docs updated ## Detailed Description of the Pull Request / Additional comments ~~Discussion in #2325 itself serves as the "spec" for this task. I thought we'd need more discussion on the topic, but it ended up being pretty straightforward.~~ I should not have said that in the original PR. We've had a better spec review now that I think we're happier with. ## Validation Steps Performed _ran the tests_
Refer to the original issue: Default Profile for Common Profile Settings #2325
So this is my summary of everything we discussed regarding "default profile settings". The original PR was #3369, but we were not in agreement on the UX, so this PR is for discussion about that.
I put forth 4 proposals that were mentioned in the discussion.
We'll iterate on this in the comments below, and if we can come to a consensus, I'll update the spec accordingly.