Convert most of our JSON deserializers to use type-based conversion #6590

DHowett · 2020-06-19T07:10:53Z

This pull request converts the following JSON deserializers to use the
new JSON deserializer pattern:

Profile
Command
ColorScheme
Action/Args
GlobalSettings
CascadiaSettingsSerialization

This is the completion of a long-term JSON refactoring that makes our
parser and deserializer more type-safe and robust. We're finally able to
get rid of all our manual enum conversion code and unify JSON conversion
around types instead of around keys.

I've introduced another file filled with template specializations,
TerminalSettingsSerializationHelpers.h, which comprises a single unit
that holds all of the JSON deserializers (and eventually serializers)
for every type that comes from TerminalApp or TerminalSettings.

I've also moved some types out of Profile and GlobalAppSettings into a
new SettingsTypes.h to improve settings locality.

This does to some extent constitute a breaking change for already-broken
settings. Instead of parsing "successfully" (where invalid values are
null or 0 or unknown or unset), deserialization will now fail when
there's a type mismatch. Because of that, some tests had to be removed.

While I was on a refactoring spree, I removed a number of helpless
helpers, like GetWstringFromJson (which converted a u8 string to an
hstring to make a wstring out of its data pointer :|) and
_ConvertJsonToBool.

In the future, we can make the error types more robust and give them
position and type information such that a conformant application can
display rich error information ("line 3 column 3, I expected a string,
you gave me an integer").

Closes #2550.

DHowett · 2020-06-19T07:12:38Z

src/cascadia/TerminalApp/ActionArgs.h

src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp

src/cascadia/TerminalApp/GlobalAppSettings.cpp

src/cascadia/TerminalApp/Profile.cpp

src/cascadia/TerminalApp/Profile.h

src/cascadia/TerminalApp/ProfileConversionTraits.cpp

carlos-zamora

Eh. Most of my review was mostly admiring this:

Submitting a review so that I can just see the new changes next time I come around.

src/cascadia/TerminalApp/JsonUtilsNew.h

src/cascadia/TerminalApp/ActionArgs.h

src/cascadia/TerminalApp/GlobalAppSettings.cpp

src/cascadia/LocalTests_TerminalApp/KeyBindingsTests.cpp

zadjii-msft

I'm certainly not about to block this while I'm gone, though, there are still TODOs and I've got questions, so I'm just gonna "comment"

src/cascadia/TerminalApp/ActionArgs.h

zadjii-msft · 2020-06-26T14:05:21Z

src/cascadia/LocalTests_TerminalApp/KeyBindingsTests.cpp

            { "keys": ["ctrl+g"], "command": { "action": "splitPane" } },
-            { "keys": ["ctrl+h"], "command": { "action": "splitPane", "split": "auto" } },
-            { "keys": ["ctrl+i"], "command": { "action": "splitPane", "split": "foo" } }


Should we add back a test case that ensures a warning was returned for this case?

So, this causes a whole settings parsing failure... but the specific case of deserializing an unknown enum member is actually handled in the JsonUtilsTests. Should we have one specifically for a "known type of flag"?

src/cascadia/TerminalApp/JsonUtilsNew.h

src/cascadia/TerminalApp/ProfileConversionTraits.cpp

src/cascadia/TerminalApp/Profile.cpp

zadjii-msft · 2020-06-26T14:35:43Z

src/cascadia/TerminalApp/JsonUtilsNew.h

@@ -334,6 +363,18 @@ namespace TerminalApp::JsonUtils
    template<typename T, typename Converter>
    bool GetValue(const Json::Value& json, T& target, Converter&& conv)
    {
+        if constexpr (Detail::DeduceOptional<T>::IsOptional)


wat the heck is this witchcraft

if constexpr is SO GOOD. It's like, "only compile this block of code if this thing is true"

it's like std::enable_if but for an actual individual block of code instead of how enable_if uses the failure of a type to be well-formed to "hide" it from existence

src/cascadia/TerminalApp/ProfileConversionTraits.cpp

…6890) This commit updates JsonUtilsNew to support winrt `Windows::Foundation::IReference<T>` as an option type, and cleans up the optional support code by removing the optional overload on `GetValue(...)`. Instead of using an overload with a partial specialization, we're using a constexpr if with a type trait to determine option-type-ness. In addition, Carlos reported an issue with deriving from `FlagMapper` (itself templated) and referring to the base type's members without fully qualifying them. To make derivation easier, `EnumMapper` and `FlagMapper` now provide `BaseEnumMapper` and `BaseFlagMapper` type aliases. I've taken the opportunity to add a `winrt::hstring` conversion trait. Lastly, in casual use, I found out that I'd written the til::color converter wrong: it supports color strings of length 7 (`#rrggbb`) and length 4 (`#rgb`). I mistyped (and failed to test) support for 4-length color strings by pretending they were only 3 characters long. ## References Merged JsonUtils changes from #6004 and #6590. ## PR Checklist * [x] Unblocks aforementioned PRs * [x] cla * [x] Tests added/passed * [x] Documentation N/A * [x] Schema N/A * [x] Kid tested, mother approved.

x include the profile traits? PRO: remove -DH, switch smth to value PRO: remove spaces PCT: non-static PCT

GAS: remove -DH GAS: nonstatic GAS

Move all settings types out of globalapp/profile into SettingsTypes. **EVENTUALLY** they will become exported types from the Terminal Settings Model library. This is a temporary stopgap. I added a ValueType convenience helper to EnumMapper so you don't have to repeat a really obscenely long fully-qualified namespace name for every mapped value.

zadjii-msft

Let's ship this, this is great. It'll break all the in-flight PRs, but hey, lets just do it.

src/cascadia/TerminalApp/ActionArgs.h

src/cascadia/TerminalApp/JsonUtilsNew.h

DHowett · 2020-07-14T18:20:57Z

So we're still keeping the strings separate? I'm ok with it, but is there a benefit/cost to putting the strings directly into the pair_types? You don't do that in one of the other files.

forgot the ones in binding serialization, thanks

…-new

DHowett · 2020-07-15T00:58:07Z

Net code removed: 773 lines.

DHowett · 2020-07-15T17:49:14Z

I am blocking this until 1.3.

ghost · 2020-07-17T01:31:06Z

Hello @DHowett!

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-08-26T17:03:09Z

🎉Windows Terminal Preview v1.3.2382.0 has been released which incorporates this pull request.:tada:

Handy links:

DHowett commented Jun 19, 2020

View reviewed changes

src/cascadia/TerminalApp/ActionArgs.h Outdated Show resolved Hide resolved

DHowett commented Jun 19, 2020

View reviewed changes

src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp Show resolved Hide resolved

DHowett commented Jun 19, 2020

View reviewed changes

DHowett marked this pull request as draft June 19, 2020 16:33

carlos-zamora mentioned this pull request Jun 24, 2020

Add copyFormatting keybinding arg and array support #6004

Merged

4 tasks

carlos-zamora reviewed Jun 24, 2020

View reviewed changes

zadjii-msft reviewed Jun 26, 2020

View reviewed changes

carlos-zamora mentioned this pull request Jun 30, 2020

Advanced Tab Switcher #6732

Merged

DHowett commented Jul 10, 2020

View reviewed changes

src/cascadia/TerminalApp/ProfileConversionTraits.cpp Outdated Show resolved Hide resolved

This was referenced Jul 12, 2020

FontSize not working #6884

Closed

Update JsonNew for IReference+cleaner optionals, and better Mappers #6890

Merged

DHowett force-pushed the dev/duhowett/json-2-new branch from 6122eb8 to f20b623 Compare July 12, 2020 23:36

DHowett added 14 commits July 14, 2020 00:04

[BRIDGE] JsonUtils->JsonUtilsNew

112b42d

ColorScheme: Convert to JsonUtilsNew

736debe

Profile: Convert to JsonUtilsNew

f4cb75e

x include the profile traits? PRO: remove -DH, switch smth to value PRO: remove spaces PCT: non-static PCT

GlobalAppSettings: Convert to JsonUtilsNew

7aeed1c

GAS: remove -DH GAS: nonstatic GAS

CSS: Convert to JsonUtilsNew

1c02bf2

GAS: move to macro

372cb90

Profile: move to mapper

8f33d71

remove GetWstringFromJson & utils.cpp

a55198d

Some tests are now invalid

163f0f2

GAS: clean up

32ee8df

Action/Args: Convert

73b18f0

profile: convert fontweight, move const strings

785f7c0

Command: update to JU-NEW

3730707

ProfileConversions: merge constexpr strings w/ map

16ab0ae

DHowett force-pushed the dev/duhowett/json-2-new branch from f20b623 to 16ab0ae Compare July 14, 2020 07:04

code format

5d1fd5a

zadjii-msft approved these changes Jul 14, 2020

View reviewed changes

zadjii-msft added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. labels Jul 14, 2020

carlos-zamora approved these changes Jul 14, 2020

View reviewed changes

src/cascadia/TerminalApp/ActionArgs.h Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/JsonUtilsNew.h Outdated Show resolved Hide resolved

DHowett added 8 commits July 14, 2020 11:39

move strings/actargs helpers

5dbe104

fode cormat

d040df4

Merge remote-tracking branch 'origin/master' into dev/duhowett/json-2…

6f8deaf

…-new

clean

59e54d1

git is mad, push an empty commit

9801aa6

Merge remote-tracking branch 'origin/master' into dev/duhowett/json-2…

9b1e7a3

…-new

Fixup after merge

a23bc51

rename JU-NEW, fix an Oops in TargetType/ValueType

f18374e

ghost added the Issue-Task It's a feature request, but it doesn't really need a major design. label Jul 15, 2020

DHowett marked this pull request as ready for review July 15, 2020 00:57

DHowett changed the title ~~Convert most of our JSON deserializers to JsonUtilsNew~~ Convert most of our JSON deserializers to to use type-based conversion Jul 15, 2020

DHowett changed the title ~~Convert most of our JSON deserializers to to use type-based conversion~~ Convert most of our JSON deserializers to use type-based conversion Jul 15, 2020

DHowett added 2 commits July 14, 2020 18:28

update test for rename

e6c229a

remove more tests that explicitly parsed bad values

a8841d4

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 17, 2020

ghost merged commit efb1fdd into master Jul 17, 2020

ghost deleted the dev/duhowett/json-2-new branch July 17, 2020 01:31

zadjii-msft mentioned this pull request Aug 10, 2020

Providing an int value for padding will hyperexplode the Terminal on launch #7234

Closed

ghost mentioned this pull request Aug 26, 2020

Add more _templated_ JsonUtils #2550

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert most of our JSON deserializers to use type-based conversion #6590

Convert most of our JSON deserializers to use type-based conversion #6590

DHowett commented Jun 19, 2020 •

edited

Loading

DHowett commented Jun 19, 2020

carlos-zamora left a comment

zadjii-msft left a comment

zadjii-msft Jun 26, 2020

DHowett Jun 26, 2020

zadjii-msft Jun 26, 2020

DHowett Jun 26, 2020

zadjii-msft left a comment

DHowett commented Jul 14, 2020

DHowett commented Jul 15, 2020

DHowett commented Jul 15, 2020

ghost commented Jul 17, 2020

ghost commented Aug 26, 2020

Convert most of our JSON deserializers to use type-based conversion #6590

Convert most of our JSON deserializers to use type-based conversion #6590

Conversation

DHowett commented Jun 19, 2020 • edited Loading

DHowett commented Jun 19, 2020

carlos-zamora left a comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft Jun 26, 2020

Choose a reason for hiding this comment

DHowett Jun 26, 2020

Choose a reason for hiding this comment

zadjii-msft Jun 26, 2020

Choose a reason for hiding this comment

DHowett Jun 26, 2020

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

DHowett commented Jul 14, 2020

DHowett commented Jul 15, 2020

DHowett commented Jul 15, 2020

ghost commented Jul 17, 2020

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 commented Aug 26, 2020

DHowett commented Jun 19, 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.