starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-02-03 21:25:34 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

Add more _templated_ JsonUtils #3576

New Issue
Closed
opened 2026-01-30 23:24:47 +00:00 by claunia · 7 comments
claunia commented 2026-01-30 23:24:47 +00:00
Owner
Copy Link

Originally created by @zadjii-msft on GitHub (Aug 26, 2019).

Originally assigned to: @DHowett on GitHub.

In #2515 I started adding some really useful JsonUtils that let us get a property from a json object if it exists.

Unfortunately, I made them like this:


    void GetOptionalColor(const Json::Value& json,
                          std::string_view key,
                          std::optional<uint32_t>& target);

    void GetOptionalString(const Json::Value& json,
                           std::string_view key,
                           std::optional<std::wstring>& target);

    void GetOptionalGuid(const Json::Value& json,
                         std::string_view key,
                         std::optional<GUID>& target);

    void GetOptionalDouble(const Json::Value& json,
                           std::string_view key,

They really should have all been template specializations of GetOptionalValue, but I didn't want to burn a day trying to fight compiler errors.

Furthermore, we should probably also just have JsonUtils::GetValue<typename T>() that does the same thing, but w/o the optional<T>'s.

Originally created by @zadjii-msft on GitHub (Aug 26, 2019). Originally assigned to: @DHowett on GitHub. In #2515 I started adding some really useful `JsonUtils` that let us get a property from a json object if it exists. Unfortunately, I made them like this: ```c++ void GetOptionalColor(const Json::Value& json, std::string_view key, std::optional<uint32_t>& target); void GetOptionalString(const Json::Value& json, std::string_view key, std::optional<std::wstring>& target); void GetOptionalGuid(const Json::Value& json, std::string_view key, std::optional<GUID>& target); void GetOptionalDouble(const Json::Value& json, std::string_view key, ``` They really should have all been template specializations of `GetOptionalValue`, but I didn't want to burn a day trying to fight compiler errors. Furthermore, we should probably also just have `JsonUtils::GetValue<typename T>()` that does the same thing, but w/o the `optional<T>`'s.
claunia commented 2026-01-30 23:24:48 +00:00
Author
Owner
Copy Link

@DHowett-MSFT commented on GitHub (Aug 26, 2019):

Thoughts: detail namespace? Default conversion function is detail::convert_from_json<T>?

@DHowett-MSFT commented on GitHub (Aug 26, 2019): Thoughts: `detail` namespace? Default conversion function is `detail::convert_from_json<T>`?
claunia commented 2026-01-30 23:24:49 +00:00
Author
Owner
Copy Link

@DHowett-MSFT commented on GitHub (Dec 24, 2019):

I'm working on this.

However, I sorta hate? it? but I can't tell. This makes it way way easier to declare name-value mappings so we can get rid of both the parse and serialize functions.

Here's a good one:

template<>
struct JsonUtils::Details::ConversionTrait<CursorStyle> : public JsonUtils::KeyValueMapper<CursorStyle, JsonUtils::Details::ConversionTrait<CursorStyle>>
{
    static constexpr std::array<pair_type, 5> mappings = {
        pair_type{ CursorShapeBar, CursorStyle::Bar }, // DEFAULT
        pair_type{ CursorShapeVintage, CursorStyle::Vintage },
        pair_type{ CursorShapeUnderscore, CursorStyle::Underscore },
        pair_type{ CursorShapeFilledbox, CursorStyle::FilledBox },
        pair_type{ CursorShapeEmptybox, CursorStyle::EmptyBox }
    };
};
Here's a bad one 
template<>
struct JsonUtils::Details::ConversionTrait<std::tuple<HorizontalAlignment, VerticalAlignment>> : public JsonUtils::KeyValueMapper<std::tuple<HorizontalAlignment, VerticalAlignment>, JsonUtils::Details::ConversionTrait<std::tuple<HorizontalAlignment, VerticalAlignment>>>
{
    static constexpr std::array<pair_type, 9> mappings = {
        pair_type{ ImageAlignmentCenter, std::make_tuple(HorizontalAlignment::Center, VerticalAlignment::Center) }, // DEFAULT
        pair_type{ ImageAlignmentTopLeft, std::make_tuple(HorizontalAlignment::Left, VerticalAlignment::Top) },
        pair_type{ ImageAlignmentBottomLeft, std::make_tuple(HorizontalAlignment::Left, VerticalAlignment::Bottom) },
        pair_type{ ImageAlignmentLeft, std::make_tuple(HorizontalAlignment::Left, VerticalAlignment::Center) },
        pair_type{ ImageAlignmentTopRight, std::make_tuple(HorizontalAlignment::Right, VerticalAlignment::Top) },
        pair_type{ ImageAlignmentBottomRight, std::make_tuple(HorizontalAlignment::Right, VerticalAlignment::Bottom) },
        pair_type{ ImageAlignmentRight, std::make_tuple(HorizontalAlignment::Right, VerticalAlignment::Center) },
        pair_type{ ImageAlignmentTop, std::make_tuple(HorizontalAlignment::Center, VerticalAlignment::Top) },
        pair_type{ ImageAlignmentBottom, std::make_tuple(HorizontalAlignment::Center, VerticalAlignment::Bottom) }
    };
};

I love that this gets us away from all the optionals and conditionals, however:

std::optional<int> whatever;
GetValue(json, Key, whatever);
// whatever will either be nullopt or a value.

int whatever = 3;
GetValue(json, Key, whatever);
// whatever will be *3* or a value

int whatever = 100;
GetRequiredValue(json, Key, whatever);
// whatever will be a value or you're buying a one-way ticket to beef city
@DHowett-MSFT commented on GitHub (Dec 24, 2019): I'm working on this. However, I sorta hate? it? but I can't tell. This makes it _way way easier_ to declare name-value mappings so we can get rid of _both_ the parse and serialize functions. Here's a good one: ```cpp template<> struct JsonUtils::Details::ConversionTrait<CursorStyle> : public JsonUtils::KeyValueMapper<CursorStyle, JsonUtils::Details::ConversionTrait<CursorStyle>> { static constexpr std::array<pair_type, 5> mappings = { pair_type{ CursorShapeBar, CursorStyle::Bar }, // DEFAULT pair_type{ CursorShapeVintage, CursorStyle::Vintage }, pair_type{ CursorShapeUnderscore, CursorStyle::Underscore }, pair_type{ CursorShapeFilledbox, CursorStyle::FilledBox }, pair_type{ CursorShapeEmptybox, CursorStyle::EmptyBox } }; }; ``` <details> <summary>Here's a bad one</summary> ```cpp template<> struct JsonUtils::Details::ConversionTrait<std::tuple<HorizontalAlignment, VerticalAlignment>> : public JsonUtils::KeyValueMapper<std::tuple<HorizontalAlignment, VerticalAlignment>, JsonUtils::Details::ConversionTrait<std::tuple<HorizontalAlignment, VerticalAlignment>>> { static constexpr std::array<pair_type, 9> mappings = { pair_type{ ImageAlignmentCenter, std::make_tuple(HorizontalAlignment::Center, VerticalAlignment::Center) }, // DEFAULT pair_type{ ImageAlignmentTopLeft, std::make_tuple(HorizontalAlignment::Left, VerticalAlignment::Top) }, pair_type{ ImageAlignmentBottomLeft, std::make_tuple(HorizontalAlignment::Left, VerticalAlignment::Bottom) }, pair_type{ ImageAlignmentLeft, std::make_tuple(HorizontalAlignment::Left, VerticalAlignment::Center) }, pair_type{ ImageAlignmentTopRight, std::make_tuple(HorizontalAlignment::Right, VerticalAlignment::Top) }, pair_type{ ImageAlignmentBottomRight, std::make_tuple(HorizontalAlignment::Right, VerticalAlignment::Bottom) }, pair_type{ ImageAlignmentRight, std::make_tuple(HorizontalAlignment::Right, VerticalAlignment::Center) }, pair_type{ ImageAlignmentTop, std::make_tuple(HorizontalAlignment::Center, VerticalAlignment::Top) }, pair_type{ ImageAlignmentBottom, std::make_tuple(HorizontalAlignment::Center, VerticalAlignment::Bottom) } }; }; ``` </details> I _love_ that this gets us away from all the optionals and conditionals, however: ```cpp std::optional<int> whatever; GetValue(json, Key, whatever); // whatever will either be nullopt or a value. ``` ```cpp int whatever = 3; GetValue(json, Key, whatever); // whatever will be *3* or a value ``` ```cpp int whatever = 100; GetRequiredValue(json, Key, whatever); // whatever will be a value or you're buying a one-way ticket to beef city ```
claunia commented 2026-01-30 23:24:49 +00:00
Author
Owner
Copy Link

@zadjii-msft commented on GitHub (Dec 30, 2019):

@DHowett-MSFT Wow I really love that. I mean, the line on the "bad one" is excessive, but the working code, the GetValue usage, is actually really nice and clean.

@zadjii-msft commented on GitHub (Dec 30, 2019): @DHowett-MSFT Wow I really love that. I mean, the line on the "bad one" is _excessive_, but the working code, the `GetValue` usage, is actually really nice and clean.
claunia commented 2026-01-30 23:24:49 +00:00
Author
Owner
Copy Link

@DHowett-MSFT commented on GitHub (May 10, 2020):

I'd love some input on how the cases here break down. I'm having trouble coming to a decision about how this is supposed to work.

Here's what I have today.

GetValueForKey(Json::Value, string_view, T& val) (GVFK)

T not found invalid type null valid
T 🔵 unchanged 🔵 unchanged 🔵 unchanged ✔ parsed
std::optional<T> 🔵 unchanged 🔵 unchanged ✔ nullopt ✔ parsed

GetRequiredValueForKey(Json::Value, string_view, T&) (GRVFK)

T not found invalid type null valid
T ❌ exception ❌ exception ❌ exception ✔ parsed
std::optional<T> ❌ exception ❌ exception ✔ nullopt ✔ parsed

I'm wondering if it should be like this:

GVFK Invalid Type Proposal

T not found invalid type null valid
T 🔵 unchanged ❌ exception 🔵 unchanged ✔ parsed
std::optional<T> 🔵 unchanged ❌ exception ✔ nullopt ✔ parsed

I think it should be more strict.

Pros

  • If we make type errors always ❌ exception, the settings model becomes more strict.
    • I think it should be more strict.

Cons

  • This has a remote chance of breaking existing settings ("commandline": 2 parses today (!) as "2")

Further question: Should we merge GVFK and GVRFK? If the user provides T, it is required. If the user provides std::optional<T>, it is optional.

GVFK+GVRFK Merger Proposal

T not found invalid type null valid
T ❌ exception ❌ exception ❌ exception ✔ parsed
std::optional<T> 🔵 unchanged ❌ exception ✔ nullopt ✔ parsed

How do we handle null if we merge the two cases? null is an invalid type (!) that cannot go into T.

Pros

  • Cleans up how optional and non-optional values work.

Cons

  • Merging GVFK/GVRFK makes layering difficult. Layering works on the premise that a not-found key remains 🔵 unchanged
@DHowett-MSFT commented on GitHub (May 10, 2020): I'd love some input on how the cases here break down. I'm having trouble coming to a decision about how this is supposed to work. Here's what I have today. ## `GetValueForKey(Json::Value, string_view, T& val)` (GVFK) `T`|not found|invalid type|`null`|valid -|-|-|-|- `T`|🔵 unchanged|🔵 unchanged|🔵 unchanged|✔ parsed `std::optional<T>`|🔵 unchanged|🔵 unchanged|✔ `nullopt`|✔ parsed ## `GetRequiredValueForKey(Json::Value, string_view, T&)` (GRVFK) `T`|not found|invalid type|`null`|valid -|-|-|-|- `T`|❌ exception|❌ exception|❌ exception|✔ parsed `std::optional<T>`|❌ exception|❌ exception|✔ `nullopt`|✔ parsed I'm wondering if it should be like this: --- ## GVFK Invalid Type Proposal `T`|not found|invalid type|`null`|valid -|-|-|-|- `T`|🔵 unchanged|❌ exception|🔵 unchanged|✔ parsed `std::optional<T>`|🔵 unchanged|❌ exception|✔ `nullopt`|✔ parsed I think it _should_ be more strict. ### Pros * If we make type errors _always `❌ exception`_, the settings model becomes more strict. * I think it _should_ be more strict. ### Cons * This has a remote chance of breaking existing settings (`"commandline": 2` parses today (!) as `"2"`) --- Further question: Should we merge GVFK and GVRFK? If the user provides `T`, it is _required_. If the user provides `std::optional<T>`, it is _optional_. ## GVFK+GVRFK Merger Proposal `T`|not found|invalid type|`null`|valid -|-|-|-|- `T`|❌ exception|❌ exception|❌ exception|✔ parsed `std::optional<T>`|🔵 unchanged|❌ exception|✔ `nullopt`|✔ parsed How do we handle `null` if we merge the two cases? `null` is an invalid type (!) that cannot go into T. ### Pros * Cleans up how optional and non-optional values work. ### Cons * Merging GVFK/GVRFK makes layering difficult. Layering works on the premise that a not-found key remains `🔵 unchanged`
claunia commented 2026-01-30 23:24:49 +00:00
Author
Owner
Copy Link

@DHowett-MSFT commented on GitHub (May 12, 2020):

Benefit of making it throw on a conversion error...

image

@DHowett-MSFT commented on GitHub (May 12, 2020): Benefit of making it throw on a conversion error... ![image](https://user-images.githubusercontent.com/14316954/81657177-8750b600-93ec-11ea-8e93-f4c845510166.png)
claunia commented 2026-01-30 23:24:49 +00:00
Author
Owner
Copy Link

@carlos-zamora commented on GitHub (May 21, 2020):

The flag mapper should also have an all/none value. That way we can detect them and throw an error/warning when it gets combined with other stuff.

@carlos-zamora commented on GitHub (May 21, 2020): The flag mapper should also have an all/none value. That way we can detect them and throw an error/warning when it gets combined with other stuff.
claunia commented 2026-01-30 23:24:49 +00:00
Author
Owner
Copy Link

@ghost commented on GitHub (Aug 26, 2020):

:tada:This issue was addressed in #6590, which has now been successfully released as Windows Terminal Preview v1.3.2382.0.🎉

Handy links:

@ghost commented on GitHub (Aug 26, 2020): :tada:This issue was addressed in #6590, which has now been successfully released as `Windows Terminal Preview v1.3.2382.0`.:tada: Handy links: * [Release Notes](https://github.com/microsoft/terminal/releases/tag/v1.3.2382.0) * [Store Download](https://www.microsoft.com/store/apps/9n8g5rfz9xk3?cid=storebadge&ocid=badge)
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
⛺ Reserved
A11yCO
A11yMAS
A11ySev1
A11ySev2
A11ySev3
A11yTTValidated
A11yUsable
A11yVoiceAccess
A11yWCAG
Area-Accessibility
Area-AtlasEngine
Area-AzureShell
Area-Build
Area-Build
Area-Chat
Area-CmdPal
Area-CodeHealth
Area-Commandline
Area-CookedRead
Area-DefApp
Area-Extensibility
Area-Fonts
Area-GroupPolicy
Area-i18n
Area-Input
Area-Interaction
Area-Interop
Area-Localization
Area-Output
Area-Performance
Area-Portable
Area-Quality
Area-Remoting
Area-Rendering
Area-Schema
Area-Server
Area-Settings
Area-SettingsUI
Area-ShellExtension
Area-ShellExtension
Area-ShellExtension
Area-Suggestions
Area-Suggestions
Area-TerminalConnection
Area-TerminalControl
Area-Theming
Area-UserInterface
Area-VT
Area-Windowing
Area-WPFControl
AutoMerge
Blocking-Ingestion
Culprit-Centennial
Culprit-WinUI
Disability-All
Disability-Blind
Disability-LowVision
Disability-Mobility
External-Blocked-WinUI3
Fixed
Gathering-Data
good first issue
HCL-E+D
HCL-WindowsTerminal
Help Wanted
Impact-Compatibility
Impact-Compliance
Impact-Correctness
Impact-Visual
In-PR
InclusionBacklog
InclusionBacklog-Windows TerminalWin32
InclusionCommitted-202206
Issue-Bug
Issue-Docs
Issue-Feature
Issue-Feature
Issue-Question
Issue-Samples
Issue-Scenario
Issue-Task
Needs-Attention
Needs-Author-Feedback
Needs-Bisect
Needs-Discussion
Needs-Repro
Needs-Tag-Fix
Needs-Tag-Fix
Needs-Triage
No-Recent-Activity
Priority-0
Priority-1
Priority-2
Priority-3
Product-Cmd.exe
Product-Colortool
Product-Colortool
Product-Colortool
Product-Conhost
Product-Conpty
Product-Meta
Product-Powershell
Product-Terminal
Product-WSL
pull-request
Mirrored from GitHub Pull Request
 Resolution-Answered
Resolution-By-Design
Resolution-Duplicate
Resolution-External
Resolution-Fix-Available
Resolution-Fix-Committed
Resolution-No-Repro
Resolution-Won't-Fix
Severity-Blocking
Severity-Crash
Severity-DataLoss
spam
this-will-be-a-breaking-change
Tracking-External
WindowsTerminal_Win32
Work-Item
zAskModeBug
zInbox-Bug
No Label Area-CodeHealth Area-Settings Issue-Task Product-Terminal Resolution-Fix-Committed
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#3576
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?