Switch to jsoncpp as our json library #1005

zadjii-msft · 2019-05-24T20:15:46Z

Summary of the Pull Request

Switch to using jsoncpp as our json library. This lets us pretty-print the json file by default, and lets users place comments in the json file.

PR Checklist

Closes Switch to a JSON serializer that supports pretty printing #475
CLA signed. If not, go over here and sign the CLA
Tests added/passed - ~~this would really be a good opportunity to add some tests~~ Yea this is gonna be a follow-up, see Refactor pure C++ types from TerminalApp.dll into a lib #1042
Requires documentation to be updated - @cinnamon-msft it's now a race between this PR and yours to see who needs to update the documentation.
I've discussed this with @DHowett-MSFT extensively this week

Detailed Description of the Pull Request / Additional comments

We will now only re-write the file when the actual logical structure of the json object changes, not only when the serialization changes.

Unfortunately, this will remove any existing ordering of profiles, and make the order random. We don't terribly care though, because when #754 lands, this will be less painful.

It also introduces a top-level globals object to hold all the global properties, including keybindings. Existing profiles should gracefully upgrade.

* Load messages from the Resources.resw file * Display a message when we fail to parse the settings on an initial parse, or on a reload. * Unfortunately, the dialog is styled incorrectly when the app theme is opposite the system theme. This can result in the button being invisible.

…or-dialog # Conflicts: # src/cascadia/CascadiaPackage/Resources/en-US/Resources.resw

… of it.

…or-dialog

# Conflicts: # src/cascadia/TerminalApp/App.cpp

They're no longer ordered, which I hate, but we'll live with.

Involves moving serialization out of AppKeyBindings, because all we have is a cppwinrt type, and Json::Value is not a cppwinrt type

# Conflicts: # src/cascadia/TerminalApp/App.cpp # src/cascadia/TerminalApp/App.h # src/cascadia/TerminalApp/AppKeyBindings.cpp # src/cascadia/TerminalApp/AppKeyBindings.h # src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp # src/cascadia/TerminalApp/ColorScheme.cpp # src/cascadia/TerminalApp/GlobalAppSettings.cpp # src/cascadia/TerminalApp/Profile.cpp

zadjii-msft

I am disappointed I can't block my own PR

dep/jsoncpp/README.md

src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp

src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp

src/cascadia/TerminalApp/Utils.cpp

src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp

src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp

src/cascadia/TerminalApp/ColorScheme.cpp

src/cascadia/TerminalApp/ColorScheme.h

src/cascadia/TerminalApp/GlobalAppSettings.cpp

src/cascadia/TerminalApp/pch.h

dlong11 · 2019-05-25T03:36:58Z

@zadjii-msft Just a note about jsoncpp - Our open source scanners report two issues with version 1.8.4. (I think this is the version that you are using)

https://www.cvedetails.com/cve/CVE-2018-17850/
https://www.cvedetails.com/cve/CVE-2018-17851/

Another reference - https://tools.cisco.com/security/center/viewAlert.x?alertId=58971

Just fyi - I saw jsoncpp it rang a bell.

DHowett-MSFT · 2019-05-27T20:38:07Z

Our open source scanners report two issues with version 1.8.4. (I think this is the version that you are using)
Just fyi - I saw jsoncpp it rang a bell.

Thanks for keeping an eye out, @dlong11! We'll evaluate these (and make sure our governance tool knows about them as well). Since we're not putting JSON on an RPC path right now, it might be at an acceptable level of risk... we'll have to think about that.

src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp

src/cascadia/TerminalApp/AppKeyBindingsSerialization.h

src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp

src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp

miniksa · 2019-05-30T15:59:04Z

@zadjii-msft, you requested my review here but it already looks full of comments by @DHowett-MSFT and @adiviness that are probably sufficient so I'll move onto another one for now. Ping me if you really want me to look at this.

DHowett-MSFT · 2019-05-30T18:14:03Z

src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp

-static constexpr std::wstring_view SCHEMES_KEY{ L"schemes" };
+static constexpr std::string_view PROFILES_KEY{ "profiles" };
+static constexpr std::string_view KEYBINDINGS_KEY{ "keybindings" };
+static constexpr std::string_view GLOBALS_KEY{ "globals" };


Is globals the right name for this? It's like application settings... but also, i dunno, why aren't they at root level?

frankly I just wanted to bundle them up. now that we lose the ordering we had before, the file would contain something like

{ "global1": val, "global2": val, "profiles": [ { ... }, { ... }, ], "global3": val, "colorSchemes": [ { ... } ], "global4": val, }

with the globals intermixed with things like the profiles, color schemes, etc, which I thought was annoying. I can switch it back if we want.

src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp

# Conflicts: # src/cascadia/TerminalApp/Profile.cpp

src/cascadia/TerminalApp/AppKeyBindingsSerialization.h

src/cascadia/TerminalApp/Profile.cpp

and the remaining PR nits

zadjii-msft · 2019-06-04T16:27:31Z

@DHowett-MSFT @miniksa can I get a second on this?

DHowett-MSFT · 2019-06-04T16:39:34Z

Arg I thought I seconded. Sorry!

DHowett-MSFT · 2019-06-04T16:44:10Z

src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp

-    return { FileIO::ReadTextAsync(storageFile, UnicodeEncoding::Utf8).get() };
+    auto buffer = FileIO::ReadBufferAsync(storageFile).get();
+    auto bufferData = buffer.data();
+    std::vector<uint8_t> bytes{ bufferData, bufferData + buffer.Length() };


Wait, do we need to make a copy into a vector and THEN a copy into a string?

zadjii-msft · 2019-06-04T18:55:49Z

/azp run

azure-pipelines · 2019-06-04T18:56:09Z

Azure Pipelines successfully started running 1 pipeline(s).

zadjii-msft added 24 commits May 7, 2019 13:59

start work on an error dialog

d0121a1

This unfortunately didn't work

00a8da4

Merge remote-tracking branch 'origin/master' into dev/migrie/f/gh-err…

643a177

…or-dialog # Conflicts: # src/cascadia/CascadiaPackage/Resources/en-US/Resources.resw

I'm just committing all of this, even though I really don't need most…

b51d03f

… of it.

Cleanup for PR

c1d17f6

Merge remote-tracking branch 'origin/master' into dev/migrie/f/gh-err…

cb55215

…or-dialog

Include jsoncpp in the solution

809a180

Merge branch 'master' into dev/migrie/f/gh-error-dialog

e8f5fb8

# Conflicts: # src/cascadia/TerminalApp/App.cpp

PR feedback

5547ca5

Merge branch 'dev/migrie/f/gh-error-dialog' into dev/migrie/f/jsoncpp

86e92c0

Serialize just colorschemes

dae4213

They're no longer ordered, which I hate, but we'll live with.

Serialize Profiles

c88f69f

Serialize the keybindings

c84b8b3

Involves moving serialization out of AppKeyBindings, because all we have is a cppwinrt type, and Json::Value is not a cppwinrt type

Start work on parsing the json with jsoncpp

d2d8df6

Serialize Colorschemes

4b8fcd1

Deserialize profiles

96f49ac

AppKeyBindings deserialization

a8a1bba

Roundtrip the everything

87b795a

Remove all the old serialization code

ddbbc12

gah serialize the starting directory too

6c268a0

Remove all the _2's

a2aa9db

throw a proper exception

f60bfb3

zadjii-msft commented May 24, 2019

View reviewed changes

DHowett-MSFT suggested changes May 24, 2019

View reviewed changes

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

adiviness suggested changes May 28, 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 May 28, 2019

zadjii-msft mentioned this pull request May 30, 2019

Remove deserialization of legacy json keys #1069

Closed

DHowett-MSFT reviewed May 30, 2019

View reviewed changes

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

zadjii-msft added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels May 30, 2019

zadjii-msft added 3 commits May 30, 2019 13:35

Merge remote-tracking branch 'origin/master' into dev/migrie/f/jsoncpp

511a66c

# Conflicts: # src/cascadia/TerminalApp/Profile.cpp

Remaining PR nits

c4b2eef

Merge remote-tracking branch 'origin/master' into dev/migrie/f/jsoncpp

097b7dd

# Conflicts: # src/cascadia/TerminalApp/Profile.cpp

DHowett-MSFT reviewed May 30, 2019

View reviewed changes

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

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

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

Change the case of all the keys to PascalCase

63e0a23

and the remaining PR nits

zadjii-msft requested review from DHowett-MSFT and adiviness and removed request for DHowett-MSFT May 31, 2019 13:33

adiviness approved these changes May 31, 2019

View reviewed changes

Add a NOTICES file

a68a9bb

DHowett-MSFT approved these changes Jun 4, 2019

View reviewed changes

zadjii-msft merged commit 8a69be0 into master Jun 4, 2019

zadjii-msft deleted the dev/migrie/f/jsoncpp branch June 4, 2019 21:55

BrennanConroy mentioned this pull request Jun 19, 2019

Allow the SignalR C++ client to be built without using cpprestsdk dotnet/aspnetcore#8718

Closed

zadjii-msft mentioned this pull request Jul 1, 2019

Try to flow together JSON serialization for keybindings #1378

Closed

zadjii-msft mentioned this pull request Aug 6, 2019

Use JSONC / allow comments in JSON per other Microsoft products #2292

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to jsoncpp as our json library #1005

Switch to jsoncpp as our json library #1005

zadjii-msft commented May 24, 2019 •

edited

Loading

zadjii-msft left a comment •

edited

Loading

dlong11 commented May 25, 2019

DHowett-MSFT commented May 27, 2019

miniksa commented May 30, 2019

DHowett-MSFT May 30, 2019

zadjii-msft May 30, 2019

zadjii-msft commented Jun 4, 2019

DHowett-MSFT commented Jun 4, 2019

DHowett-MSFT Jun 4, 2019

zadjii-msft commented Jun 4, 2019

azure-pipelines bot commented Jun 4, 2019

Switch to jsoncpp as our json library #1005

Switch to jsoncpp as our json library #1005

Conversation

zadjii-msft commented May 24, 2019 • edited Loading

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

zadjii-msft left a comment • edited Loading

Choose a reason for hiding this comment

dlong11 commented May 25, 2019

DHowett-MSFT commented May 27, 2019

miniksa commented May 30, 2019

DHowett-MSFT May 30, 2019

Choose a reason for hiding this comment

zadjii-msft May 30, 2019

Choose a reason for hiding this comment

zadjii-msft commented Jun 4, 2019

DHowett-MSFT commented Jun 4, 2019

DHowett-MSFT Jun 4, 2019

Choose a reason for hiding this comment

zadjii-msft commented Jun 4, 2019

azure-pipelines bot commented Jun 4, 2019

zadjii-msft commented May 24, 2019 •

edited

Loading

zadjii-msft left a comment •

edited

Loading