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

Carlos Zamora (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.

Dustin L. Howett (DHowett) · 2020-09-11T18:36:42Z

+#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.

Dustin L. Howett (DHowett)

gosh i love this jsonutils stuff. good work!

Dustin L. Howett (DHowett) · 2020-09-11T18:38:02Z

+// 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.

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

Dustin L. Howett (DHowett) · 2020-09-15T18:49:36Z

+// 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 ;)

Dustin L. Howett (DHowett) · 2020-09-17T01:42:39Z


    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!

Dustin L. Howett (DHowett) · 2020-09-17T01:44:20Z

+// - 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

Dustin L. Howett (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 (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

Dustin L. Howett (DHowett) reviewed Sep 11, 2020

View reviewed changes

Comment thread src/cascadia/TerminalApp/ColorScheme.h Resolved

Dustin L. Howett (DHowett) reviewed Sep 11, 2020

View reviewed changes

PR Comments - Dustin

0241022

Dustin L. Howett (DHowett) reviewed Sep 11, 2020

View reviewed changes

Comment thread src/cascadia/TerminalApp/JsonUtils.h Resolved

Add comment for demand

57620b7

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

Carlos Zamora (carlos-zamora) assigned Dustin L. Howett (DHowett) Sep 14, 2020

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

ce4d5c6

Carlos Zamora (carlos-zamora) assigned Carlos Zamora (carlos-zamora) and unassigned Dustin L. Howett (DHowett) Sep 14, 2020

add SetColorTable()

9d4a4ca

Carlos Zamora (carlos-zamora) assigned Dustin L. Howett (DHowett) and unassigned Carlos Zamora (carlos-zamora) Sep 14, 2020

Dustin L. Howett (DHowett) reviewed Sep 14, 2020

View reviewed changes

Comment thread src/cascadia/TerminalApp/ColorScheme.idl Resolved

rename to GetColorTableEntry()

01c165a

Utkarsh Agnihotri (agniutkarsh) approved these changes Sep 15, 2020

View reviewed changes

Dustin L. Howett (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

Dustin L. Howett (DHowett) reviewed Sep 17, 2020

View reviewed changes

rename to ToJson; use til::coalesce

aa6b1e6

Dustin L. Howett (DHowett) approved these changes Sep 17, 2020

View reviewed changes

Carlos Zamora (carlos-zamora) unassigned Dustin L. Howett (DHowett) Sep 17, 2020

Leon Liang (leonMSFT) approved these changes Sep 17, 2020

View reviewed changes

Dustin L. Howett (DHowett) merged commit b70ffdf into master Sep 17, 2020

Dustin L. Howett (DHowett) deleted the dev/cazamor/tojson/basic branch September 17, 2020 18:27

Carlos Zamora (carlos-zamora) mentioned this pull request Sep 17, 2020

Feature Request - Implement Settings UI #1564

Closed

25 of 34 tasks

Conversation

Carlos Zamora (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!

Dustin L. Howett (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!

Dustin L. Howett (DHowett) commented Sep 17, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Carlos Zamora (carlos-zamora) commented Sep 11, 2020 •

edited by DHowett

Loading