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

Add Serializer to CascadiaSettings #8018

Merged
25 commits merged into from
Nov 17, 2020
Merged

Add Serializer to CascadiaSettings #8018

25 commits merged into from
Nov 17, 2020

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Oct 22, 2020

Summary of the Pull Request

This adds ToJson functions to Profile, GlobalAppSettings, and ColorScheme. They are used in CascadiaSettings to completely serialize an instance of the settings model. Thanks to #7923, all of the settings are std::optional, and JsonUtils only writes out values that are actually populated.

CascadiaSettings::WriteSettingsToDisk serializes the current settings and writes them to the settings.json. A backup file is created with your old contents.

Limitations:

  • all of the color schemes are serialized regardless of them coming from defaults.json or settings.json
  • keybindings/actions are copied/pasted

References

#1564 - Settings UI
TSM Specs (#6904 and #7876)

PR Checklist

  • Tests added/passed

@carlos-zamora

This comment has been minimized.

@carlos-zamora

This comment has been minimized.

@ghost ghost closed this Oct 27, 2020
@ghost ghost deleted the branch main October 27, 2020 17:35
@carlos-zamora carlos-zamora reopened this Oct 27, 2020
@carlos-zamora carlos-zamora changed the base branch from dev/cazamor/tsm/inheritance to main October 27, 2020 17:44
@carlos-zamora carlos-zamora changed the base branch from main to dev/cazamor/tsm/hidden-vs-active-profiles October 28, 2020 07:07
@carlos-zamora carlos-zamora marked this pull request as ready for review October 28, 2020 07:28
@zadjii-msft zadjii-msft self-assigned this Oct 28, 2020
ghost pushed a commit that referenced this pull request Oct 28, 2020
## Summary of the Pull Request
This PR replaces `CascadiaSettings::_profiles` with...
- `_allProfiles`: the list of all available profiles in the settings model (i.e. settings.json, dynamic profiles, etc...)
- `_activeProfiles`: the list of all non-hidden profiles (used for the new tab dropdown)

## References
#8018: maintaining a list of all profiles allows us to serialize hidden profiles
#1564: Settings UI can link to `AllProfiles()` instead of `ActiveProfiles()` to expose hidden profiles

## PR Checklist
* [x] Closes #4139 
* [x] Tests added/passed

## Validation Steps Performed
Deploy and testing succeeded
@ghost ghost closed this Oct 28, 2020
@ghost ghost deleted the branch main October 28, 2020 16:22
@DHowett
Copy link
Member

DHowett commented Oct 28, 2020

If you know that a PR targets another branch, you may need to not use automerge ;)

@carlos-zamora carlos-zamora reopened this Oct 28, 2020
@carlos-zamora carlos-zamora changed the base branch from dev/cazamor/tsm/hidden-vs-active-profiles to main October 28, 2020 16:54
DHowett added a commit that referenced this pull request Nov 9, 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<T>>, 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_.
…lization

# Conflicts:
#	src/cascadia/TerminalSettingsModel/JsonUtils.h

// write current settings to current settings file
const auto json{ ToJson() };
_WriteSettings(json.toStyledString(), settingsPath);
Copy link
Member

Choose a reason for hiding this comment

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

i hope to heck that this doesn't use the json format with the spaces before the colons for some reason

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. Just switched to a StreamWriterBuilder. Looks better now. The only thing that really bugs me, is that everything is written in alphabetical order. So we'll get some global settings written all the way at the bottom.

src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp Outdated Show resolved Hide resolved
@@ -291,6 +291,7 @@ void Profile::LayerJson(const Json::Value& json)
JsonUtils::GetValueForKey(json, NameKey, _Name);
JsonUtils::GetValueForKey(json, GuidKey, _Guid);
JsonUtils::GetValueForKey(json, HiddenKey, _Hidden);
JsonUtils::GetValueForKey(json, SourceKey, _Source);
Copy link
Member

Choose a reason for hiding this comment

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

'scuse me?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed

Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove it? Why had you added it? It seems very important to serialize it, but i’m surprised it wasn’t there before

Don’t just go deleting lines of code because someone commented on them!

Copy link
Member Author

Choose a reason for hiding this comment

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

Oof. I forgot to run the tests anyways. So, this is interesting. I looked through the history of Profile.cpp and it looks like it was always missing (checked the inheritance and winrt-ification PRs). But, we need this so that the following roundtrip test passes:

            {
                "name": "Weird Profile",
                "tabColor": null,
                "foreground": null,
                "source": "local"
            }

I think we just always relied on source being set by the dynamic profile generator, so we never had to care. But now with inheritance and serialization, we need this? Still a bit weird because really the user shouldn't be setting a source, Terminal should. Thoughts? Is this even a valid test?

Copy link
Member

Choose a reason for hiding this comment

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

it was set by "generateStub", if I recall correctly.

If there is a source, it must be present in the user's settings.

Copy link
Member

Choose a reason for hiding this comment

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

because source is part of the primary key we use to match up dynamic profiles.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. Then I added it back in.

@ghost 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 Nov 11, 2020
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Lets do this

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 17, 2020
@ghost
Copy link

ghost commented Nov 17, 2020

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 6b503ba into main Nov 17, 2020
@ghost ghost deleted the dev/cazamor/tsm/serialization branch November 17, 2020 00:37
ghost pushed a commit that referenced this pull request Dec 1, 2020
## Summary of the Pull Request
Adds an event handler for the Save and Reset buttons. "Save" writes the settings to disk using the API introduced by #8018. "Reset" simply overwrites the `settingsClone` (what the Settings UI reads from) with `settingsSource` (provided by TermApp on Settings UI initialization).

## References
#1564 - Settings UI

## Validation Steps Performed
The following scenarios were tested and are verified to work properly:
- Change default profile in SUI, then save/reset
- Hide a profile, then save/reset
- Modifying the settings.json directly propagates changes to SUI
- "Reset" updates the current page
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants