Add profiles.json schema to doc folder #2704

cinnamon-msft · 2019-09-09T20:21:12Z

Summary of the Pull Request

Updated the JSON schema referenced in #1003
Adding it to our repo so we can reference it in the profiles.json file

References

#1003

PR Checklist

Closes profile.json should have a schema #1003
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Requires documentation to be 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

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

zadjii-msft

I don't think any of these comments rise above the level of "nit"

zadjii-msft · 2019-09-09T20:30:13Z

doc/cascadia/profiles.schema.json

+  "definitions": {
+    "Color": {
+      "default": "#",
+      "pattern": "^#[A-Fa-f0-9]{6}$",


This can actually be a #rgb color now as well

zadjii-msft · 2019-09-09T20:30:29Z

doc/cascadia/profiles.schema.json

+      "pattern": "^#[A-Fa-f0-9]{6}$",
+      "type": "string"
+    },
+    "ProfileKey": {


Suggested change

"ProfileKey": {

"ProfileGuid": {

maybe?

zadjii-msft · 2019-09-09T20:31:44Z

doc/cascadia/profiles.schema.json

+      "type": "string"
+    },
+    "ProfileKey": {
+      "default": "{}",


I'm not totally sure this is true, but I'm not sure what a better way to put this would be

zadjii-msft · 2019-09-09T20:32:17Z

doc/cascadia/profiles.schema.json

+              "keys": {
+                "description": "Defines the key combinations used to call the command.",
+                "items": {
+                  "pattern": "^(ctrl\\+|alt\\+|shift\\+){0,3}([0-9a-zA-Z]|backspace|tab|enter|esc|space|pgup|pgdn|end|home|left|up|right|down|insert|delete|numpad_([0-9]|multiply|plus|minus|period|divide)|f[1-9]|f1[0-9]|f2[0-4]|plus|,|-|\\.)$",


holy regular expression batman

i am extremely alarmed about this pattern.

also, this pattern is hyper-specific (backspace, tab, enter, etc.), but it misses out on all the extended keys like {}[]_+-=|\\

also, this pattern allows ctrl+ctrl+ctrl+c

Maybe I can help with this one @cinnamon_msft - Question - should we force the use of at least one modifier through regex? Else we're letting people redefine things like Enter, for example.

people actually want enter, now that a failed keybinding dispatch can pass through. As a binding for copy, it'll copy if there's a selection or send a CR if there isn't.

If you want me to alter the logic, let me know. Regex101 is an excellent tool and will explain how the expression works, but it does take a certain twisted effort to comprehend, and for many people regex is a write-only exercise so if it's hurting everyone's brains, that's okay with me lol

I put out a Twitter challenge to improve it: https://twitter.com/oising/status/1172226517331525632 -- maybe we can make it even less readable / compact

@oising I'm glad to see another regex lover is around 😁. I've just been caught up with how to make CTRL+, ALT+, and SHIFT+ not repeat (as Dustin aluded to the CTRL+CTRL+CTRL+C issue above).

At least we only need to solve this problem once, so as ugly as it gets, we'll never have to touch it again 🤞

Yeah, it's only really possible with lookbehind assertions (to prevent dupes), and it's also lucky we're using javascript since PCRE only supports fixed width assertions, though .NET added lookbehind assertions specifically. I don't know if it support backreferences though. Another thing to think about was possibly using subroutines (to possibly add some feel-good DRY), but man, I only had 20 minutes :D

So yeah, just for information, a subroutine call lets you "call" a previous capture group from later in the expression, but in the later context. It's not supported in javascript it seems, but if it was, we could remove the second and third modifier capture group, and just refer to the first one by its index with (?2):

^(?<modifier>(ctrl|alt|shift)\+?((?2)(?<!\2)\+?)?((?2)(?<!\2|\4))?\+?)?(?<key>\w+?)?(?<=\w)$

zadjii-msft · 2019-09-09T20:33:19Z

doc/cascadia/profiles.schema.json

+            "description": "Sets the background color of the profile. Overrides background set in color scheme if colorscheme is set. Uses hex color format: \"#rrggbb\". Default \"#000000\" (black)."
+          },
+          "backgroundImage": {
+            "description": "(Not in SettingsSchema.md)",


zadjii-msft · 2019-09-09T20:33:49Z

doc/cascadia/profiles.schema.json

+          },
+          "colorScheme": {
+            "default": "Campbell",
+            "description": "Name of the terminal color scheme to use. Color schemes are defined under schemes.",


Suggested change

"description": "Name of the terminal color scheme to use. Color schemes are defined under schemes.",

"description": "Name of the terminal color scheme to use. Color schemes are defined under \"schemes\".",

zadjii-msft · 2019-09-09T20:34:37Z

doc/cascadia/profiles.schema.json

+            "type": "string"
+          },
+          "connectionType": {
+            "$ref": "#/definitions/ProfileKey",


Oh maybe this should just be Guid

yes we need a separate guid type

zadjii-msft · 2019-09-09T20:35:10Z

doc/cascadia/profiles.schema.json

+          },
+          "connectionType": {
+            "$ref": "#/definitions/ProfileKey",
+            "default": "{d9fcfdfa-a479-412c-83b7-c5640e61cd62}",


Shouldn't this not have a default, as I'm pretty sure the default is the null guid?

ABSOLUTELY NOT

bitcrazed · 2019-09-10T22:45:10Z

doc/cascadia/profiles.schema.json

+{
+  "$id": "https://github.com/microsoft/terminal/terminal.profile.schema.json",
+  "$schema": "http://json-schema.org/draft-07/schema#",
+  "title": "Microsoft Terminal Profile Settings",


Shouldn't this be "Microsoft's Windows Terminal Settings Profile Schema" or similar?

DHowett-MSFT · 2019-09-10T22:46:53Z

doc/cascadia/profiles.schema.json

@@ -0,0 +1,449 @@
+{
+  "$id": "https://github.com/microsoft/terminal/terminal.profile.schema.json",


I don't know what the $id is supposed to represent, but this is not a valid URL

DHowett-MSFT · 2019-09-10T22:47:12Z

doc/cascadia/profiles.schema.json

+  "title": "Microsoft Terminal Profile Settings",
+  "definitions": {
+    "Color": {
+      "default": "#",


What does # mean as a default?

When editing the profiles.json, it'll auto-fill the "#" for the color code

DHowett-MSFT · 2019-09-10T22:47:53Z

doc/cascadia/profiles.schema.json

+  "properties": {
+    "globals": {
+      "additionalProperties": false,
+      "description": "Properties affect the entire window, regardless of the profile settings.",


Suggested change

"description": "Properties affect the entire window, regardless of the profile settings.",

"description": "Properties that affect the entire window, regardless of the profile settings.",

DHowett-MSFT · 2019-09-10T22:48:07Z

doc/cascadia/profiles.schema.json

+      "properties": {
+        "alwaysShowTabs": {
+          "default": true,
+          "description": "When set to true, tabs are always displayed. When set to false and showTabsInTitlebar is set to false, tabs only appear after typing Ctrl + T.",


after "opening a new tab" (since it can be rebound)

DHowett-MSFT · 2019-09-10T22:48:37Z

doc/cascadia/profiles.schema.json

+        },
+        "defaultProfile": {
+          "$ref": "#/definitions/ProfileKey",
+          "description": "Sets the default profile. Opens by typing Ctrl + T or by clicking the '+' icon. The guid of the desired default profile is used as the value."


note about the new tab binding again

doc/cascadia/profiles.schema.json

DHowett-MSFT · 2019-09-10T22:55:17Z

doc/cascadia/profiles.schema.json

+            "type": "string"
+          },
+          "scrollbarState": {
+            "description": "Defines the visibility of the scrollbar. Possible values: \"visible\", \"hidden\"",


DHowett-MSFT · 2019-09-10T22:55:38Z

doc/cascadia/profiles.schema.json

+            "type": "boolean"
+          },
+          "startingDirectory": {
+            "default": "%USERPROFILE%",


this is actually not the default, we just .. faked it into being default. if they remove it, the default is "the current directory"

DHowett-MSFT · 2019-09-10T22:56:00Z

doc/cascadia/profiles.schema.json

+          }
+        },
+        "required": [
+          "acrylicOpacity",


only guid, colorScheme, commandline and name are required.

DHowett-MSFT · 2019-09-10T22:56:20Z

doc/cascadia/profiles.schema.json

+            "$ref": "#/definitions/Color",
+            "description": "Sets the color used as ANSI green."
+          },
+          "name": {


move name up

doc/cascadia/profiles.schema.json

DHowett-MSFT · 2019-09-18T19:41:13Z

notes from use: source is allowed in profile, globals is not required (all globals objects can be merged with the root optionally), and name or guid are the only truly required fields for Profile as of #754.

DHowett-MSFT · 2019-09-20T16:36:30Z

and key bindings fail to validate for \ and | and all symbols; suggest widening \w to [^+\s] (all characters that are not plus or space)

DHowett-MSFT · 2019-09-20T16:39:31Z

doc/cascadia/profiles.schema.json

doc/cascadia/SettingsSchema.md

doc/cascadia/profiles.schema.json

DHowett-MSFT · 2019-09-24T20:01:24Z

doc/cascadia/profiles.schema.json

+    "profiles",
+    "schemes"
+  ],
+  "type": "object"


we still have the problem where defaultProfile can be inside globals or outside of it, keybindings can be inside globals or outside, etc.
Anything that can exist in globals can also exist outside of globals, so we need some sort of reusable object mapping that says so.

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

Ne4to · 2019-09-26T21:52:43Z

doc/cascadia/profiles.schema.json

+            "default": false,
+            "description": "When set to true, the window will have an acrylic background. When set to false, the window will have a plain, untextured background.",
+            "type": "boolean"
+          }


The "hidden" property is missing.

Suggested change

}

},

"hidden": {

"default": false,

"description": "If set to true, the profile will not appear in the list of profiles. This can be used to hide default profiles and dynamicially generated profiles, while leaving them in your settings file.",

"type": "boolean"

}

DHowett-MSFT · 2019-09-27T18:15:14Z

@bitcrazed i'm dismissing your review because you left one comment and it's since been fixed.

ut supra

added profiles.json schema to doc folder

4a16478

cinnamon-msft requested a review from a team September 9, 2019 20:21

zadjii-msft approved these changes Sep 9, 2019

View reviewed changes

zadjii-msft added the Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs label Sep 9, 2019

bitcrazed previously requested changes Sep 10, 2019

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 Sep 10, 2019

DHowett-MSFT suggested changes Sep 10, 2019

View reviewed changes

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 11, 2019

cinnamon-msft added 2 commits September 11, 2019 16:29

addressed most comments

cdfd597

addressed most comments

c6c65c4

carlos-zamora reviewed Sep 12, 2019

View reviewed changes

doc/cascadia/profiles.schema.json Outdated Show resolved Hide resolved

fixed keybindings regex and color table

c273874

DHowett-MSFT mentioned this pull request Sep 18, 2019

Add a schema reference to the userDefaults and patch one into user data #2803

Merged

2 tasks

carlos-zamora reviewed Sep 24, 2019

View reviewed changes

doc/cascadia/profiles.schema.json Outdated Show resolved Hide resolved

cinnamon-msft and others added 2 commits September 24, 2019 12:36

updated schema and settings documentation

614025c

Delete dlfk

d50ce32

DHowett-MSFT suggested changes Sep 24, 2019

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 Sep 24, 2019

Update doc/cascadia/SettingsSchema.md

91d64e3

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

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 24, 2019

Update doc/cascadia/profiles.schema.json

2fd03b7

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

Ne4to reviewed Sep 26, 2019

View reviewed changes

updated schema

c30f7c5

DHowett-MSFT approved these changes Sep 27, 2019

View reviewed changes

Merge branch 'master' into cinnamon/add-schema-to-docs

639f581

cinnamon-msft merged commit 258c8b4 into master Sep 27, 2019

cinnamon-msft deleted the cinnamon/add-schema-to-docs branch January 30, 2020 21:17

	"description": "Name of the terminal color scheme to use. Color schemes are defined under schemes.",
	"description": "Name of the terminal color scheme to use. Color schemes are defined under \"schemes\".",

		@@ -0,0 +1,449 @@
		{
		"$id": "https://github.com/microsoft/terminal/terminal.profile.schema.json",

	"description": "Properties affect the entire window, regardless of the profile settings.",
	"description": "Properties that affect the entire window, regardless of the profile settings.",

Add profiles.json schema to doc folder #2704

Add profiles.json schema to doc folder #2704

Conversation

cinnamon-msft commented Sep 9, 2019 • edited Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oising Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

oising Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT commented Sep 18, 2019

DHowett-MSFT commented Sep 20, 2019 • edited Loading

DHowett-MSFT commented Sep 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT commented Sep 27, 2019

cinnamon-msft commented Sep 9, 2019 •

edited

Loading

oising Sep 12, 2019 •

edited

Loading

oising Sep 12, 2019 •

edited

Loading

DHowett-MSFT commented Sep 20, 2019 •

edited

Loading