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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
Nov 9, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dfea59b
Rework JsonUtils' optional handling to allow Converters access to nul…
DHowett Nov 6, 2020
abcfcd9
Fix more tests that expected null to mean nothing
DHowett Nov 6, 2020
60050f4
Replace NullableSetting with opt<opt<T>>, add JsonUtils test
DHowett Nov 6, 2020
bb82b70
Make OptionOracle public, change names, decay more
DHowett Nov 6, 2020
97045e7
well make the tests _work_
DHowett Nov 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/actions/spell-check/expect/expect.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2277,6 +2277,7 @@ tcome
tcommandline
tcommands
tcon
TDelegated
TDP
TEAMPROJECT
tearoff
Expand Down
17 changes: 9 additions & 8 deletions doc/cascadia/Json-Utility-API.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,20 @@ return a JSON value coerced into the specified type.
When reading into existing storage, it returns a boolean indicating whether that storage was modified.

If the JSON value cannot be converted to the specified type, an exception will be generated.
For non-nullable type conversions (most POD types), `null` is considered to be an invalid type.

```c++
std::string one;
std::optional<std::string> two;

JsonUtils::GetValue(json, one);
// one is populated or unchanged.
// one is populated or an exception is thrown.

JsonUtils::GetValue(json, two);
// two is populated, nullopt or unchanged
// two is populated, nullopt or an exception is thrown

auto three = JsonUtils::GetValue<std::string>(json);
// three is populated or zero-initialized
// three is populated or an exception is thrown

auto four = JsonUtils::GetValue<std::optional<std::string>>(json);
// four is populated or nullopt
Expand Down Expand Up @@ -225,14 +226,14 @@ auto v = JsonUtils::GetValue<int>(json, conv);

-|json type invalid|json null|valid
-|-|-|-
`T`|❌ exception|🔵 unchanged|✔ converted
`T`|❌ exception|❌ exception|✔ converted
`std::optional<T>`|❌ exception|🟨 `nullopt`|✔ converted

### GetValue&lt;T&gt;() (returning)

-|json type invalid|json null|valid
-|-|-|-
`T`|❌ exception|🟨 `T{}` (zero value)|✔ converted
`T`|❌ exception|❌ exception|✔ converted
`std::optional<T>`|❌ exception|🟨 `nullopt`|✔ converted

### GetValueForKey(T&) (type-deducing)
Expand All @@ -242,14 +243,14 @@ a "key not found" state. The remaining three cases are the same.

val type|key not found|_json type invalid_|_json null_|_valid_
-|-|-|-|-
`T`|🔵 unchanged|_❌ exception_|_🔵 unchanged_|_✔ converted_
`std::optional<T>`|_🔵 unchanged_|_❌ exception_|_🟨 `nullopt`_|_✔ converted_
`T`|🔵 unchanged|_❌ exception_|_❌ exception_|_✔ converted_
`std::optional<T>`|🔵 unchanged|_❌ exception_|_🟨 `nullopt`_|_✔ converted_

### GetValueForKey&lt;T&gt;() (return value)

val type|key not found|_json type invalid_|_json null_|_valid_
-|-|-|-|-
`T`|🟨 `T{}` (zero value)|_❌ exception_|_🟨 `T{}` (zero value)_|_✔ converted_
`T`|🟨 `T{}` (zero value)|_❌ exception_|_❌ exception_|_✔ converted_
`std::optional<T>`|🟨 `nullopt`|_❌ exception_|_🟨 `nullopt`_|_✔ converted_

### Future Direction
Expand Down
13 changes: 1 addition & 12 deletions src/cascadia/LocalTests_SettingsModel/CommandTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,6 @@ namespace SettingsModelLocalTests
// of from keybindings.

const std::string commands0String{ R"([
{ "name": "command0", "command": { "action": "splitPane", "split": null } },
{ "name": "command1", "command": { "action": "splitPane", "split": "vertical" } },
{ "name": "command2", "command": { "action": "splitPane", "split": "horizontal" } },
{ "name": "command4", "command": { "action": "splitPane" } },
Expand All @@ -157,18 +156,8 @@ namespace SettingsModelLocalTests
VERIFY_ARE_EQUAL(0u, commands.Size());
auto warnings = implementation::Command::LayerJson(commands, commands0Json);
VERIFY_ARE_EQUAL(0u, warnings.size());
VERIFY_ARE_EQUAL(5u, commands.Size());
VERIFY_ARE_EQUAL(4u, commands.Size());

{
auto command = commands.Lookup(L"command0");
VERIFY_IS_NOT_NULL(command);
VERIFY_IS_NOT_NULL(command.Action());
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, command.Action().Action());
const auto& realArgs = command.Action().Args().try_as<SplitPaneArgs>();
VERIFY_IS_NOT_NULL(realArgs);
// Verify the args have the expected value
VERIFY_ARE_EQUAL(SplitState::Automatic, realArgs.SplitStyle());
}
{
auto command = commands.Lookup(L"command1");
VERIFY_IS_NOT_NULL(command);
Expand Down
12 changes: 1 addition & 11 deletions src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,6 @@ namespace SettingsModelLocalTests
void KeyBindingsTests::TestSplitPaneArgs()
{
const std::string bindings0String{ R"([
{ "keys": ["ctrl+c"], "command": { "action": "splitPane", "split": null } },
{ "keys": ["ctrl+d"], "command": { "action": "splitPane", "split": "vertical" } },
{ "keys": ["ctrl+e"], "command": { "action": "splitPane", "split": "horizontal" } },
{ "keys": ["ctrl+g"], "command": { "action": "splitPane" } },
Expand All @@ -335,17 +334,8 @@ namespace SettingsModelLocalTests
VERIFY_IS_NOT_NULL(keymap);
VERIFY_ARE_EQUAL(0u, keymap->_keyShortcuts.size());
keymap->LayerJson(bindings0Json);
VERIFY_ARE_EQUAL(5u, keymap->_keyShortcuts.size());
VERIFY_ARE_EQUAL(4u, keymap->_keyShortcuts.size());

{
KeyChord kc{ true, false, false, static_cast<int32_t>('C') };
auto actionAndArgs = ::TestUtils::GetActionAndArgs(*keymap, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SplitPaneArgs>();
VERIFY_IS_NOT_NULL(realArgs);
// Verify the args have the expected value
VERIFY_ARE_EQUAL(SplitState::Automatic, realArgs.SplitStyle());
}
{
KeyChord kc{ true, false, false, static_cast<int32_t>('D') };
auto actionAndArgs = ::TestUtils::GetActionAndArgs(*keymap, kc);
Expand Down
50 changes: 23 additions & 27 deletions src/cascadia/TerminalSettingsModel/IInheritable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation

// This is like std::optional, but we can use it in inheritance to determine whether the user explicitly cleared it
template<typename T>
struct NullableSetting
{
std::optional<T> setting{ std::nullopt };
bool set{ false };
};
using NullableSetting = std::optional<std::optional<T>>;
DHowett marked this conversation as resolved.
Show resolved Hide resolved
}

// Use this macro to quickly implement both getters and the setter for an
Expand All @@ -93,27 +89,27 @@ public: \
bool Has##name() const \
{ \
return _##name.has_value(); \
}; \
} \
\
/* Returns the resolved value for this setting */ \
/* fallback: user set value --> inherited value --> system set value */ \
type name() const \
{ \
const auto val{ _get##name##Impl() }; \
return val ? *val : type{ __VA_ARGS__ }; \
}; \
} \
\
/* Overwrite the user set value */ \
void name(const type& value) \
{ \
_##name = value; \
}; \
} \
\
/* Clear the user set value */ \
void Clear##name() \
{ \
_##name = std::nullopt; \
}; \
} \
\
private: \
std::optional<type> _##name{ std::nullopt }; \
Expand All @@ -137,7 +133,7 @@ private: \
\
/*no value was found*/ \
return std::nullopt; \
};
}

// This macro is similar to the one above, but is reserved for optional settings
// like Profile.Foreground (where null is interpreted
Expand All @@ -148,51 +144,51 @@ public: \
/* Returns true if the user explicitly set the value, false otherwise*/ \
bool Has##name() const \
{ \
return _##name.set; \
}; \
return _##name.has_value(); \
} \
\
/* Returns the resolved value for this setting */ \
/* fallback: user set value --> inherited value --> system set value */ \
winrt::Windows::Foundation::IReference<type> name() const \
{ \
const auto val{ _get##name##Impl() }; \
if (val.set) \
if (val) \
{ \
if (val.setting) \
if (*val) \
{ \
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

} \
return nullptr; \
} \
return winrt::Windows::Foundation::IReference<type>{ __VA_ARGS__ }; \
}; \
} \
\
/* Overwrite the user set value */ \
void name(const winrt::Windows::Foundation::IReference<type>& value) \
{ \
if (value) /*set value is different*/ \
{ \
_##name.setting = value.Value(); \
_##name = std::optional<type>{ value.Value() }; \
} \
else \
{ \
_##name.setting = std::nullopt; \
/* note we're setting the _inner_ value */ \
_##name = std::optional<type>{ std::nullopt }; \
} \
_##name.set = true; \
}; \
} \
\
/* Clear the user set value */ \
void Clear##name() \
{ \
_##name.set = false; \
}; \
_##name = std::nullopt; \
} \
\
private: \
NullableSetting<type> _##name{}; \
NullableSetting<type> _get##name##Impl() const \
{ \
/*return user set value*/ \
if (Has##name()) \
if (_##name) \
{ \
return _##name; \
} \
Expand All @@ -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?

{ \
auto val{ parent->_get##name##Impl() }; \
if (val.set) \
if (auto val{ parent->_get##name##Impl() }) \
{ \
return val; \
} \
} \
\
/*no value was found*/ \
return { std::nullopt, false }; \
};
return std::nullopt; \
}
Loading