Add default values to defaults.json by carlos-zamora · Pull Request #5231 · microsoft/terminal

carlos-zamora · 2020-04-03T21:51:13Z

Summary of the Pull Request

This updates defaults.json to include the default values for all global and profile settings. Most default keybinding args are added too. This also updates a few outdated items found in the docs.

PR Checklist

Closes Make sure default settings are all set in defaults.json #5189

Validation Steps Performed

After making the changes, I made sure all of the settings are deserialized by debugging and stepping through the LayerJson code.

Global Settings

I was mainly looking for two things:

the key/value pair is found and read
the value did not change before/after the pair was read

src/cascadia/TerminalApp/defaults.json

carlos-zamora · 2020-04-03T22:41:57Z

I'm also cross-validating this with the schema by moving these settings to settings.json and running the updated schema on it. Here's some interesting findings:

Global Settings: None
Profile Settings:
- "Warning: Incorrect type. Expected string." when I set these properties to null
  - "backgroundImage"
  - "selectionBackground"
  - "cursorColor"
  - "cursorHeight"
  - "icon"
  - "tabTitle"
  - "source"
Keybindings:
- ~~"Warning: Incorrect type. Expected object." when "command" is set to null~~ Addressed by Add null, unbound to schema for keybindings #5221

I can add "null" as acceptable via the schema and double check it serializes them properly by setting them in "profiles.defaults" then trying to unset them in my "profiles.list".

But again, I want a "go-ahead" before I start doing that.

DHowett-MSFT · 2020-04-03T23:24:58Z

Null is allowable for those.

WSLUser · 2020-04-06T13:25:30Z

I like the changes if user feedback is a factor here. Use of null makes sense to me for those particular settings. You can always define those manually in profiles.json, which is what usually happens anyways.

Should I go ahead and make sure all keybindings are there too?

Yes.

I can also add in missing keybinding args and set them to their default value just to show that they exist

This would prove helpful when configuring your own keybindings (instead of having to go to docs). Basically the defaults.json is a template and profiles.json should be a mirror replica that overrides defaults.json for those settings that are explicitly different.

zadjii-msft

I like the format of using the profiles.defaults like this.
If it isn't terribly difficult to add the possible keybinding args too, that would probably also be appreciated.

DHowett-MSFT · 2020-04-07T18:04:44Z

src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp

    // If `profiles` was an object, then look for the `defaults` object
    // underneath it for the default profile settings.
-    auto defaultSettings{ Json::Value::null };
+    auto primaryDefaultSettings{ Json::Value::null };


This is implemented in somewhat the wrong order. Right now we...

Load defaults/profiles.list

Apply defaults/profiles.defaults

Apply user/profiles.defaults

Load user/profiles.list

when what we really want is to swap 1 and 2.

Yeah. This was a lot larger and more complex of a change than we hoped. Removed the code changes and opened #5276

carlos-zamora · 2020-04-07T18:06:00Z

Follow-up tasks:

Allow null for some color settings in the schema #5279: selectionBackground and cursorColor should be nullable in the schema
- this requires me doing some untangling from the Color reference. It's a sizeable change.
~~#YYYY: allow keys to be nullable~~
- similar problem as before. Requires some untangling that may be a bit too big.
- RESOLUTION: no schema change necessary. Just add in as a comment.

I'll probably do them all as one PR. But I'll update this comment with the appropriate follow-up GH issues.

DHowett-MSFT · 2020-04-07T18:07:18Z

• #YYYY: allow keys to be nullable

sorry, what? absolutely not.

carlos-zamora · 2020-04-07T18:10:53Z

• #YYYY: allow keys to be nullable

sorry, what? absolutely not.

Well, this one's annoying. So we have the closeTab keybinding action. This isn't set by default. I added it in like this:

{ "command": "closeTab", "keys": null },

but I'm not fond of it.

carlos-zamora · 2020-04-07T18:12:20Z

FYI: I omitted adding the newTerminalArgs for splitPane and newTab because they're really cumbersome. I think they'd be more confusing to see than not to see.

DHowett-MSFT · 2020-04-07T18:12:24Z

This is an example of somewhere we definitely shouldn't violate our schema just to educate users about an option they almost certainly do not want to begin with.

DHowett-MSFT · 2020-04-07T18:12:48Z

like, put that thing in a comment with a keys value and say it closes a whole tab without confirmation even if it has multiple panes in

carlos-zamora · 2020-04-07T18:20:20Z

@DHowett-MSFT hmm, should I do the same with this then? There's no real value for a user to define them as null.

#XXXX: selectionBackground and cursorColor should be nullable in the schema

src/cascadia/TerminalApp/defaults.json

DHowett-MSFT · 2020-04-07T18:21:40Z

For those, there is. Cascading settings mean that they could be inherited from some other place, and they would need to be uninherited.

doc/cascadia/SettingsSchema.md

src/cascadia/TerminalApp/defaults.json

DHowett-MSFT · 2020-04-07T22:10:50Z

src/cascadia/TerminalApp/defaults.json

+    // Miscellaneous
    "confirmCloseAllTabs": true,
+    "theme": "system",
+    "rowsToScroll": "system",


i hate this setting.

DHowett-MSFT · 2020-04-07T22:11:15Z

src/cascadia/TerminalApp/defaults.json

+            "source": null,
+
+            // Experimental Settings
+            "experimental.retroTerminalEffect": false


so we're going with the "don't make any code changes, and hope that these match the compiled-in defaults forever" option?

nope. Latest commit removed these. Unfortunately, if we have profiles as an object instead of a list, I think we just lose all of the profiles.defaults info. So, I've turned it back into a list.

Not happy about this change :/ but I'm still brainstorming a way to make it look prettier.

src/cascadia/TerminalApp/defaults.json

cinnamon-msft · 2020-04-08T18:39:00Z

Should we put somewhere that ctrl+shift+w will close a tab if there aren't any panes? Or it will close the whole window if there's only one tab?

DHowett-MSFT · 2020-04-08T18:40:26Z

If we consider it implicit that every tab contains a single pane by default, it is implied.

cinnamon-msft · 2020-04-08T18:41:32Z

I would argue people would assume there aren't any panes if they didn't split any.

DHowett-MSFT · 2020-04-08T18:42:36Z

We should rename it to closeTheSmallestThingICurrentlyHaveInFocus.

carlos-zamora · 2020-04-08T18:46:39Z

Should we put somewhere that ctrl+shift+w will close a tab if there aren't any panes? Or it will close the whole window if there's only one tab?

I think that's something worth mentioning. But defaults.json isn't the place for it. Definitely in the docs though. And the schema too, if it isn't in there already.

cinnamon-msft · 2020-04-08T18:58:31Z

src/cascadia/TerminalApp/defaults.json

        { "command": "find", "keys": "ctrl+shift+f" },

        // Tab Management
+        // "command": "closeTab" is unbound by default.


Should we bind this to something? This seems like basic functionality we should have a binding for.

The windows "standard" is ctrl+f4, but be aware that it doesn't ask for confirmation and will destroy any number of panes worth of work.

I mean, we don't ask for confirmation when you click the 'x' button on the tab now.

You're right. I'm not saying it's more dangerous than the X button, just that it's as dangerous. If we litter minefields around folks' keyboards, like we once did with ctrl+w, there's a chance for DSAT. Not a huge one, just a chance.

I'm personally ok with it being unbound be default. Mainly because there's other methods to close a tab (i.e.: exit, click X button, close pane, etc...)

I'd prefer it to be bound to ctrl+f4. Every other command except unbound has a default binding.

@cinnamon-msft and I had an offline discussion. We'll be holding off on ctrl+f4 being bound to close tab until we can get a confirmation dialog for closing the tab, if multiple panes are open.

follow-up work item: #5301

zadjii-msft

Seems good to me

src/cascadia/TerminalApp/defaults.json

DHowett-MSFT · 2020-04-09T17:27:16Z

Any changes you make to this file please make sure you reflect to the universal file. Do remember the profiles differ 😄

src/cascadia/TerminalApp/defaults.json

Co-Authored-By: Dustin L. Howett (MSFT) <[email protected]>

ghost · 2020-04-09T23:13:34Z

Hello @carlos-zamora!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2020-04-22T17:15:42Z

🎉Windows Terminal Preview v0.11.1121.0 has been released which incorporates this pull request.:tada:

Handy links:

fix json

c34c43f

carlos-zamora added the Area-Settings Issues related to settings and customizability, for console or terminal label Apr 3, 2020

carlos-zamora requested review from DHowett-MSFT and cinnamon-msft April 3, 2020 21:51

carlos-zamora commented Apr 3, 2020

View reviewed changes

src/cascadia/TerminalApp/defaults.json Outdated Show resolved Hide resolved

zadjii-msft reviewed Apr 6, 2020

View reviewed changes

carlos-zamora added 3 commits April 7, 2020 09:45

fix typo in defaults.json and update theme

6dc7916

[CODE] allow defaults.json to have profiles.defaults

8a56ac0

add nullable profile settings + update keybindings

3653ab3

DHowett-MSFT reviewed Apr 7, 2020

View reviewed changes

add comment about 'closeTab' and fix minor spacing issue

8bf734b

DHowett-MSFT reviewed Apr 7, 2020

View reviewed changes

src/cascadia/TerminalApp/defaults.json Outdated Show resolved Hide resolved

remove code changes

8914042

carlos-zamora force-pushed the dev/cazamor/settings-defaults branch from e85da26 to 8914042 Compare April 7, 2020 22:04

reword comment

e59110b

DHowett-MSFT suggested changes Apr 7, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 7, 2020

undo changing profiles into an object

d91a41c

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 7, 2020

address Dustin's comments. Try something new with profiles

de9461c

DHowett-MSFT reviewed Apr 7, 2020

View reviewed changes

src/cascadia/TerminalApp/defaults.json Outdated Show resolved Hide resolved

remove comment. Keep order of 'unique' then 'same/alphabetical'

eb8b879

carlos-zamora marked this pull request as ready for review April 8, 2020 00:05

Merge branch 'master' into dev/cazamor/settings-defaults

fdc2dab

cinnamon-msft reviewed Apr 8, 2020

View reviewed changes

zadjii-msft approved these changes Apr 8, 2020

View reviewed changes

leonMSFT reviewed Apr 9, 2020

View reviewed changes

src/cascadia/TerminalApp/defaults.json Outdated Show resolved Hide resolved

update defaults-universal and os integration comment

9a0c8dd

DHowett-MSFT approved these changes Apr 9, 2020

View reviewed changes

src/cascadia/TerminalApp/defaults.json Outdated Show resolved Hide resolved

Add missing space

0780a9d

Co-Authored-By: Dustin L. Howett (MSFT) <[email protected]>

cinnamon-msft self-requested a review April 9, 2020 23:00

cinnamon-msft approved these changes Apr 9, 2020

View reviewed changes

carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 9, 2020

ghost merged commit 2bd1e39 into master Apr 9, 2020

ghost deleted the dev/cazamor/settings-defaults branch April 9, 2020 23:14

ghost mentioned this pull request Apr 22, 2020

Make sure default settings are all set in defaults.json #5189

Closed

This pull request was closed.

Conversation

carlos-zamora commented Apr 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

PR Checklist

Validation Steps Performed

Uh oh!

Uh oh!

carlos-zamora commented Apr 3, 2020

Uh oh!

DHowett-MSFT commented Apr 3, 2020

Uh oh!

WSLUser commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlos-zamora commented Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DHowett-MSFT commented Apr 7, 2020

Uh oh!

carlos-zamora commented Apr 7, 2020

Uh oh!

carlos-zamora commented Apr 7, 2020

Uh oh!

DHowett-MSFT commented Apr 7, 2020

Uh oh!

DHowett-MSFT commented Apr 7, 2020

Uh oh!

carlos-zamora commented Apr 7, 2020

Uh oh!

Uh oh!

DHowett-MSFT commented Apr 7, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cinnamon-msft commented Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DHowett-MSFT commented Apr 8, 2020

Uh oh!

cinnamon-msft commented Apr 8, 2020

Uh oh!

DHowett-MSFT commented Apr 8, 2020

Uh oh!

carlos-zamora commented Apr 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlos-zamora Apr 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

carlos-zamora commented Apr 3, 2020 •

edited

Loading

WSLUser commented Apr 6, 2020 •

edited

Loading

carlos-zamora commented Apr 7, 2020 •

edited

Loading

cinnamon-msft commented Apr 8, 2020 •

edited

Loading

carlos-zamora Apr 9, 2020 •

edited

Loading

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 (`@msftbot`) and give me an instruction to get started! Learn more here.