Update ColorScheme with Json Serializer and color table API by carlos-zamora · Pull Request #7609 · microsoft/terminal

carlos-zamora · 2020-09-11T18:25:24Z

Add ToJson() to the ConversionTraits in JsonUtils. This can be used
to serialize settings objects into JSON.

As a proof of concept, ToJson and UpdateJson were added to
ColorScheme.

Getters and setters for members and colors in the color table were added
and polished.

References

#1564 - Settings UI

ColorScheme is a particularly easy example of serialization because it
has no fallback.

Added a few tests for JSON serializers.

src/cascadia/TerminalApp/ColorScheme.h

DHowett · 2020-09-11T18:36:42Z

src/cascadia/TerminalApp/ColorScheme.h

+#define GETSET_COLORTABLEPROPERTY(name)                                               \
+public:                                                                               \
+    winrt::Windows::UI::Color name() const noexcept { return _table[##name##Index]; } \
+    void name(const winrt::Windows::UI::Color& value) noexcept { _table[##name##Index] = value; }


do we need an observable/notifier here?

Maybe? I'll hold off on that for when we get the UI hooked up.

DHowett

gosh i love this jsonutils stuff. good work!

src/cascadia/ut_app/JsonUtilsTests.cpp

DHowett · 2020-09-11T18:38:02Z

src/cascadia/inc/cppwinrt_utils.h

+// This should only be used for color types where there's no logic in the
+// getter/setter beyond just accessing/updating the value.
+// This takes advantage of til::color
+#define GETSET_COLORPROPERTY(name, ...)                                             \
+public:                                                                             \
+    winrt::Windows::UI::Color name() const noexcept { return _##name; }             \
+    void name(const winrt::Windows::UI::Color& value) noexcept { _##name = value; } \
+                                                                                    \
+private:                                                                            \


is this really a generic-enough helper that it belongs in utils

Eh? 🤷‍♂️ Yes, because WinUI::Color should be how we expose colors across layers, and we want to leverage til::color as much as we can? Only 50% confident here haha

I think that some of this C++/winrt stuff might magically take care of it for us ...

so what happens if you just use GETSET_PROPERTY(til::color, Foreground) or whatever?

There's a trick to c++/winrt, and that trick is that your implementation methods don't actually need to match your signatures at all ;)

Okay, I think that this won't automatically work because the type it's looking for is actually a struct_Windows_UI_Color and the detach_from helper does not allow for type coercion.

src/cascadia/TerminalApp/JsonUtils.h

Co-authored-by: Dustin L. Howett <duhowett@microsoft.com>

src/cascadia/TerminalApp/ColorScheme.idl

.github/actions/spell-check/dictionary/apis.txt

src/cascadia/TerminalApp/ColorScheme.cpp

src/cascadia/TerminalApp/ColorScheme.h

DHowett · 2020-09-15T18:49:36Z

src/cascadia/inc/cppwinrt_utils.h

+// This should only be used for color types where there's no logic in the
+// getter/setter beyond just accessing/updating the value.
+// This takes advantage of til::color
+#define GETSET_COLORPROPERTY(name, ...)                                             \
+public:                                                                             \
+    winrt::Windows::UI::Color name() const noexcept { return _##name; }             \
+    void name(const winrt::Windows::UI::Color& value) noexcept { _##name = value; } \
+                                                                                    \
+private:                                                                            \


I think that some of this C++/winrt stuff might magically take care of it for us ...

so what happens if you just use GETSET_PROPERTY(til::color, Foreground) or whatever?

There's a trick to c++/winrt, and that trick is that your implementation methods don't actually need to match your signatures at all ;)

src/cascadia/ut_app/JsonTests.cpp

src/cascadia/ut_app/JsonUtilsTests.cpp

DHowett · 2020-09-17T01:42:39Z

src/cascadia/ut_app/JsonUtilsTests.cpp


    template<typename TExpected, typename TJson>
-    static void TryBasicType(TExpected&& expected, TJson&& json)
+    static void TryBasicType(TExpected&& expected, TJson&& json, std::optional<std::string> overrideToJsonOutput = std::nullopt)


shouldn't this be an optional json::value? so you can specify {1} or something?

then later you can use expectedJson[key] = override.value_or(jsonObject); and simplify the code?

Ah. I was trying to do that, but I messed up by making it a std::optional<TJson> instead of std::optional<Json::Value>. Derp!

DHowett · 2020-09-17T01:44:20Z

src/cascadia/TerminalApp/ColorScheme.cpp

+// - json: an object which will be a serialization of a ColorScheme object.
+// Return Value:
+// <none>
+void ColorScheme::UpdateJson(Json::Value& json)


i 100% expected you to go a different direction with this and just .. make it a function called ToJson that returned the json object.

Is there ever a reason for us to call UpdateJson multiple times with different color schemes? (or profiles, etc.)?

Ideally, I could name it LayerJson with an out param because we're really just layering our values onto the Json. But I guess that isn't something we're doing yet, so yeah, renaming to ToJson as you expected haha

DHowett · 2020-09-17T18:27:15Z

I'm going to admin override this because it's failed on #7654

carlos-zamora added 3 commits September 10, 2020 11:49

Add SetValueForKey to JsonUtils

be3a5a0

Add ToJson to ColorScheme

a5fccc6

polish ColorScheme

c69572b

carlos-zamora added Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. labels Sep 11, 2020

update spell bot

0de07b8

DHowett reviewed Sep 11, 2020

View reviewed changes

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

DHowett reviewed Sep 11, 2020

View reviewed changes

PR Comments - Dustin

0241022

DHowett reviewed Sep 11, 2020

View reviewed changes

src/cascadia/TerminalApp/JsonUtils.h Show resolved Hide resolved

Add comment for demand

57620b7

Co-authored-by: Dustin L. Howett <duhowett@microsoft.com>

carlos-zamora assigned DHowett Sep 14, 2020

Merge branch 'master' into dev/cazamor/tojson/basic

ce4d5c6

carlos-zamora assigned carlos-zamora and unassigned DHowett Sep 14, 2020

add SetColorTable()

9d4a4ca

carlos-zamora assigned DHowett and unassigned carlos-zamora Sep 14, 2020

DHowett reviewed Sep 14, 2020

View reviewed changes

src/cascadia/TerminalApp/ColorScheme.idl Outdated Show resolved Hide resolved

rename to GetColorTableEntry()

01c165a

agniutkarsh approved these changes Sep 15, 2020

View reviewed changes

DHowett requested changes Sep 15, 2020

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 Sep 15, 2020

carlos-zamora added 2 commits September 16, 2020 18:34

PR comments - Dustin

f217c7a

I knew I missed a code format. whoops!

2dde34b

DHowett reviewed Sep 17, 2020

View reviewed changes

rename to ToJson; use til::coalesce

aa6b1e6

DHowett approved these changes Sep 17, 2020

View reviewed changes

carlos-zamora unassigned DHowett Sep 17, 2020

leonMSFT approved these changes Sep 17, 2020

View reviewed changes

DHowett merged commit b70ffdf into master Sep 17, 2020

DHowett deleted the dev/cazamor/tojson/basic branch September 17, 2020 18:27

carlos-zamora mentioned this pull request Sep 17, 2020

Feature Request - Implement Settings UI #1564

Closed

34 tasks

Conversation

carlos-zamora commented Sep 11, 2020 • edited by DHowett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett commented Sep 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

carlos-zamora commented Sep 11, 2020 •

edited by DHowett

Loading