Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework JsonUtils' optional handling to let Converters see null #8175

Merged
merged 5 commits into from Nov 9, 2020

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Nov 6, 2020

The JsonUtils changes in #8018 revealed that we need more robust,
configurable optional handling. We learned that there's a class of
values that was previously underrepresented in our API: strings that
have an explicit empty value
.

The Settings model supports starting directory, icon, background image
et al values that are empty. That emptiness overrides a value set in a
lower layer, so it is not sufficient to represent the empty value for
any one of those fields as an unset optional.

There are a couple other settings for which we've implemented a
hand-rolled option type (for roughly the same reason): foreground,
background, any color fields that override values from the color scheme
or the lower layer profile.

These requirements are best fulfilled by better optional support in
JsonUtils. Where the library would originally detect known types of
optional and pre-filter them out during GetValue and SetValue, it
will now defer to another conversion trait.

This commit introduces a helper conversion trait and an "option oracle".
The conversion trait will use the option oracle to detect emptiness,
generate empty option values, and read values out of option types. In so
doing, the trait is insulated from the implementation details of any
specific option type.

Any special logic for handling JSON null and option types has been
stripped from GetValue. Due to this, there is an express change in
behavior for some converters:

  • GetValue<T>(jsonNull) where T is not an option type[1] has
    been upgraded from a silent no-op to an exception.

Further, I took the opportunity to replace NullableSetting with
std::optional<std::optional>, which accurately represents "setting
that the user might explicitly clear". I've added a test to
JsonUtilsTests to make sure it can serialize/deserialize double
optionals the way we expect it to.

Tests (Local, Unit for TerminalApp/SettingsModel):
Summary: Total=140, Passed=140, Failed=0, Blocked=0, Not Run=0, Skipped=0

[1]: Explicitly, if T is not an option type and the converter does
not support null
.

@DHowett DHowett marked this pull request as ready for review November 6, 2020 21:35
@DHowett DHowett changed the title Rework JsonUtils' optional handling to allow Converters access to null values Rework JsonUtils' optional handling to give Converters access to null values Nov 6, 2020
@DHowett DHowett changed the title Rework JsonUtils' optional handling to give Converters access to null values Rework JsonUtils' optional handling to give Converters access to null values Nov 6, 2020
Comment on lines +104 to +107
struct hstring_like
{
std::string value;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What? Why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, was there something in the test that made this unclear?

@DHowett
Copy link
Member Author

DHowett commented Nov 6, 2020

Summary of Non-passing Tests:
    TerminalAppLocalTests::TabTests::TryZoomPane [Failed]
    TerminalAppLocalTests::TabTests::MoveFocusFromZoomedPane [Failed]
    TerminalAppLocalTests::TabTests::CloseZoomedPane [Failed]

Summary: Total=143, Passed=140, Failed=3, Blocked=0, Not Run=0, Skipped=0

These test failures are not caused by this change.

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this so much. Double approve haha

{ \
return *val.setting; \
return **val; \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hehe

@DHowett
Copy link
Member Author

DHowett commented Nov 9, 2020

Since this changed the behavior of some more settings, it'll require @zadjii-msft to sign off. 😄

@@ -201,12 +197,12 @@ private: \
/*iterate through parents to find a value*/ \
for (auto parent : _parents) \
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (auto parent : _parents) \
for (const auto& parent : _parents) \

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk how we missed this in code review

shouldn't we have had a helper for "walk parents and do a thing"? I thought that was part of the brief.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem I ran into by taking this out into its own helper method was that we're returning a value and calling a specific _get##name##Impl(). Figured it'd be less of a headache to throw this in a macro.

The const & thing is my bad tho. Sorry! I think I went through all of the other ones in the PR, but I just forgot to check the macro?

@zadjii-msft zadjii-msft self-assigned this Nov 9, 2020
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I feel good about this because there are good tests for this, not because I understand templates

@DHowett DHowett changed the title Rework JsonUtils' optional handling to give Converters access to null values Rework JsonUtils' optional handling to let Converters see null Nov 9, 2020
@DHowett DHowett merged commit 3a5c33b into main Nov 9, 2020
@DHowett DHowett deleted the dev/duhowett/json-utils-null branch November 9, 2020 23:13
@ghost
Copy link

ghost commented Jan 28, 2021

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

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants