Merged
Conversation
zadjii-msft
requested review from
DHowett-MSFT,
carlos-zamora,
cinnamon-msft,
leonMSFT and
miniksa
zadjii-msft
added
Area-Settings
DHowett-MSFT
reviewed
Dec 9, 2019
Contributor
DHowett-MSFT
left a comment
DHowett-MSFT
left a comment
There was a problem hiding this comment.
Reviewed the layering code, looks god. Haven’t looked at tests or schema yet, so I’m holding my green Chex mix
DHowett-MSFT
approved these changes
Dec 9, 2019
doc/user-docs/UsingJsonSettings.md
Outdated
|
|
||
| What if I wanted a profile to have a different value for a property other than | ||
| the default? Simply set the property in the profile's entry to override the | ||
| value from `defaults`. Let's say you want the `cmd` profile to have `Consolas` |
Contributor
There was a problem hiding this comment.
Inconsistent formatting for font names. Shall we split the difference and always italicize them?
Co-Authored-By: Dustin L. Howett (MSFT) <[email protected]>
…bjectifying-settings # Conflicts: # src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp
…hub.com/Microsoft/Terminal into dev/migrie/f/2325-objectifying-settings
DHowett-MSFT
reviewed
Dec 9, 2019
DHowett-MSFT
reviewed
Dec 9, 2019
|
|
||
| // Remove the `guid` member from the default settings. That'll | ||
| // hyper-explode, so just don't let them do that. | ||
| _userDefaultProfileSettings.removeMember({ "guid" }); |
Member
There was a problem hiding this comment.
Shouldn't we also remove hidden?
Member
Author
There was a problem hiding this comment.
I mean, people could totally just hide all their profiles though, right? Then only unhide profiles if they want them visible (i.e hide all WSL profiles, and only re-enable the cmd profile)
Contributor
There was a problem hiding this comment.
yeah i honestly think that's fine
carlos-zamora
approved these changes
Dec 11, 2019
carlos-zamora
added
the
AutoMerge
|
Hello @carlos-zamora! 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 (
|
ghost
merged commit 
5 tasks
zadjii-msft
pushed a commit
that referenced
this pull request
Dec 30, 2019
<!-- 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 Adds proper `type` for `ProfilesObject` definition to avoid warnings about matches of multiple schemas. <!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> ## References Original issue: #3909 Related PR: #3892 Relates VSCode issue: microsoft/vscode#86738 <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist * [X] Closes #3909 * [X] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] No new tests ~Tests added/passed~ * [ ] No docs update needed ~Requires documentation to be updated~ * [X] 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: #3909 (marked as help wanted) <!-- 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 1. Download `doc/cascadia/profiles.schema.json` locally 1. Open `profiles.json` from WT in VSCode 1. Replace `$schema` value with path to local copy (verified that all errors are still in place and validations works as before) 1. Update it with `type` on `ProfilesObject` 1. Check that `Matches multiple schemas when only one must validate` warning is fixed
|
🎉 Handy links: |
ghost
mentioned this pull request
Rican7
added a commit
to Rican7/dotfiles
that referenced
this pull request
Jan 15, 2020
configurations thanks to the new "profile defaults" feature (See microsoft/terminal#3892)
Rican7
added a commit
to Rican7/dotfiles
that referenced
this pull request
Feb 10, 2024
configurations thanks to the new "profile defaults" feature (See microsoft/terminal#3892)
This pull request was closed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
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 thelistproperty of theprofilesobject.References
#2515, #2603, #3369, #3569
PR Checklist
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