Skip to content

Commit

Permalink
Track and log changes to settings (#17678)
Browse files Browse the repository at this point in the history
Adds functionality throughout the settings model to keep track of which
settings have been set.

There are two entry points:
- AppLogic.cpp: this is where we perform a settings reload by loading
the JSON
- MainPage.cpp: this is where the Save button is clicked in the settings
UI

Both of these entry points call into
`CascadiaSettings::LogSettingChanges()` where we aggregate the list of
changes (specifically, _which_ settings changed, not _what_ their value
is).

Just about all of the settings model objects now have a
`LogSettingChanges(std::set& changes, std::string_view context)` on
them.
- `changes` is where we aggregate all of the changes to. In it being a
set, we don't need to worry about duplicates and can do things like
iterate across all of the profiles.
- `context` prepends a string to the setting. This'll allow us to better
identify where a setting was changes (i.e. "global.X" are global
settings). We also use this to distinguish between settings set in the
~base layer~ profile defaults vs individual profiles.

The change log in each object is modified via two ways:
- `LayerJson()` changes: this is useful for detecting JSON changes! All
we're doing is checking if the setting has a value (due to inheritance,
just about everything is an optional here!). If the value is set, we add
the json key to the change log
- `INHERITABLE_SETTING_WITH_LOGGING` in IInheritable.h: we already use
this macro to define getters and setters. This new macro updates the
setter to check if the value was set to something different. If so, log
it!

 Other notes:
- We're not distinguishing between `defaultAppearance` and
`unfocusedAppearance`
- We are distinguishing between `profileDefaults` and `profile` (any
other profile)
- New Tab Menu Customization:
- we really just care about the entry types. Handled in
`GlobalAppSettings`
- Font:
- We still have support for legacy values here. We still want to track
them, but just use the modern keys.
- `Theme`:
- We don't do inheritance here, so we have to approach it differently.
During the JSON load, we log each setting. However, we don't have
`LayerJson`! So instead, do the work in `CascadiaSettings` and store the
changes there. Note that we don't track any changes made via setters.
This is fine for now since themes aren't even in the settings UI, so we
wouldn't get much use out of it anyways.
- Actions:
- Actions are weird because we can have nested and iterable actions too,
but `ActionsAndArgs` as a whole add a ton of functionality. I handled it
over in `Command::LogSettingChanges` and we generally just serialize it
to JSON to get the keys. It's a lot easier than dealing with the object
model.

Epic: #10000
Auto-Save (ish): #12424
  • Loading branch information
carlos-zamora authored Aug 23, 2024
1 parent cd8c125 commit 0a9cbd0
Show file tree
Hide file tree
Showing 22 changed files with 531 additions and 10 deletions.
2 changes: 2 additions & 0 deletions .github/actions/spelling/allow/allow.txt
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ gje
godbolt
hyperlinking
hyperlinks
Kbds
kje
libfuzzer
liga
Expand All @@ -43,6 +44,7 @@ mkmk
mnt
mru
nje
NTMTo
notwrapped
ogonek
overlined
Expand Down
4 changes: 4 additions & 0 deletions src/cascadia/TerminalApp/AppLogic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,10 @@ namespace winrt::TerminalApp::implementation
return;
}
}
else
{
_settings.LogSettingChanges(true);
}

if (initialLoad)
{
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalSettingsEditor/MainPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation

void MainPage::SaveButton_Click(const IInspectable& /*sender*/, const RoutedEventArgs& /*args*/)
{
_settingsClone.LogSettingChanges(false);
_settingsClone.WriteSettingsToDisk();
}

Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalSettingsModel/ActionMap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
const auto action = cmd.ActionAndArgs().Action();
const auto id = action == ShortcutAction::Invalid ? hstring{} : cmd.ID();
_KeyMap.insert_or_assign(keys, id);
_changeLog.emplace(KeysKey);
}

// Method Description:
Expand Down
3 changes: 3 additions & 0 deletions src/cascadia/TerminalSettingsModel/ActionMap.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
Json::Value ToJson() const;
Json::Value KeyBindingsToJson() const;
bool FixupsAppliedDuringLoad() const;
void LogSettingChanges(std::set<std::string>& changes, const std::string_view& context) const;

// modification
bool RebindKeys(const Control::KeyChord& oldKeys, const Control::KeyChord& newKeys);
Expand Down Expand Up @@ -138,6 +139,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation

til::shared_mutex<std::unordered_map<hstring, std::vector<Model::Command>>> _cwdLocalSnippetsCache{};

std::set<std::string> _changeLog;

friend class SettingsModelUnitTests::KeyBindingsTests;
friend class SettingsModelUnitTests::DeserializationTests;
friend class SettingsModelUnitTests::TerminalSettingsTests;
Expand Down
34 changes: 33 additions & 1 deletion src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
// Now check if this is a command block
if (jsonBlock.isMember(JsonKey(CommandsKey)) || jsonBlock.isMember(JsonKey(ActionKey)))
{
AddAction(*Command::FromJson(jsonBlock, warnings, origin), keys);
auto command = Command::FromJson(jsonBlock, warnings, origin);
command->LogSettingChanges(_changeLog);
AddAction(*command, keys);

if (jsonBlock.isMember(JsonKey(KeysKey)))
{
Expand All @@ -105,6 +107,28 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation

// any existing keybinding with the same keychord in this layer will get overwritten
_KeyMap.insert_or_assign(keys, idJson);

if (!_changeLog.contains(KeysKey.data()))
{
// Log "keys" field, but only if it's one that isn't in userDefaults.json
static constexpr std::array<std::pair<std::wstring_view, std::string_view>, 3> userDefaultKbds{ { { L"Terminal.CopyToClipboard", "ctrl+c" },
{ L"Terminal.PasteFromClipboard", "ctrl+v" },
{ L"Terminal.DuplicatePaneAuto", "alt+shift+d" } } };
bool isUserDefaultKbd = false;
for (const auto& [id, kbd] : userDefaultKbds)
{
const auto keyJson{ jsonBlock.find(&*KeysKey.cbegin(), (&*KeysKey.cbegin()) + KeysKey.size()) };
if (idJson == id && keyJson->asString() == kbd)
{
isUserDefaultKbd = true;
break;
}
}
if (!isUserDefaultKbd)
{
_changeLog.emplace(KeysKey);
}
}
}
}

Expand Down Expand Up @@ -156,4 +180,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation

return keybindingsList;
}

void ActionMap::LogSettingChanges(std::set<std::string>& changes, const std::string_view& context) const
{
for (const auto& setting : _changeLog)
{
changes.emplace(fmt::format(FMT_COMPILE("{}.{}"), context, setting));
}
}
}
38 changes: 37 additions & 1 deletion src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -90,27 +90,42 @@ Json::Value AppearanceConfig::ToJson() const
void AppearanceConfig::LayerJson(const Json::Value& json)
{
JsonUtils::GetValueForKey(json, ForegroundKey, _Foreground);
_logSettingIfSet(ForegroundKey, _Foreground.has_value());

JsonUtils::GetValueForKey(json, BackgroundKey, _Background);
_logSettingIfSet(BackgroundKey, _Background.has_value());

JsonUtils::GetValueForKey(json, SelectionBackgroundKey, _SelectionBackground);
_logSettingIfSet(SelectionBackgroundKey, _SelectionBackground.has_value());

JsonUtils::GetValueForKey(json, CursorColorKey, _CursorColor);
_logSettingIfSet(CursorColorKey, _CursorColor.has_value());

JsonUtils::GetValueForKey(json, LegacyAcrylicTransparencyKey, _Opacity);
JsonUtils::GetValueForKey(json, OpacityKey, _Opacity, JsonUtils::OptionalConverter<float, IntAsFloatPercentConversionTrait>{});
_logSettingIfSet(OpacityKey, _Opacity.has_value());

if (json["colorScheme"].isString())
{
// to make the UI happy, set ColorSchemeName.
JsonUtils::GetValueForKey(json, ColorSchemeKey, _DarkColorSchemeName);
_LightColorSchemeName = _DarkColorSchemeName;
_logSettingSet(ColorSchemeKey);
}
else if (json["colorScheme"].isObject())
{
// to make the UI happy, set ColorSchemeName to whatever the dark value is.
JsonUtils::GetValueForKey(json["colorScheme"], "dark", _DarkColorSchemeName);
JsonUtils::GetValueForKey(json["colorScheme"], "light", _LightColorSchemeName);

_logSettingSet("colorScheme.dark");
_logSettingSet("colorScheme.light");
}

#define APPEARANCE_SETTINGS_LAYER_JSON(type, name, jsonKey, ...) \
JsonUtils::GetValueForKey(json, jsonKey, _##name);
JsonUtils::GetValueForKey(json, jsonKey, _##name); \
_logSettingIfSet(jsonKey, _##name.has_value());

MTSM_APPEARANCE_SETTINGS(APPEARANCE_SETTINGS_LAYER_JSON)
#undef APPEARANCE_SETTINGS_LAYER_JSON
}
Expand Down Expand Up @@ -156,3 +171,24 @@ winrt::hstring AppearanceConfig::ExpandedBackgroundImagePath()
return winrt::hstring{ wil::ExpandEnvironmentStringsW<std::wstring>(path.c_str()) };
}
}

void AppearanceConfig::_logSettingSet(const std::string_view& setting)
{
_changeLog.emplace(setting);
}

void AppearanceConfig::_logSettingIfSet(const std::string_view& setting, const bool isSet)
{
if (isSet)
{
_logSettingSet(setting);
}
}

void AppearanceConfig::LogSettingChanges(std::set<std::string>& changes, const std::string_view& context) const
{
for (const auto& setting : _changeLog)
{
changes.emplace(fmt::format(FMT_COMPILE("{}.{}"), context, setting));
}
}
5 changes: 5 additions & 0 deletions src/cascadia/TerminalSettingsModel/AppearanceConfig.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
static winrt::com_ptr<AppearanceConfig> CopyAppearance(const AppearanceConfig* source, winrt::weak_ref<Profile> sourceProfile);
Json::Value ToJson() const;
void LayerJson(const Json::Value& json);
void LogSettingChanges(std::set<std::string>& changes, const std::string_view& context) const;

Model::Profile SourceProfile();

Expand All @@ -52,5 +53,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation

private:
winrt::weak_ref<Profile> _sourceProfile;
std::set<std::string> _changeLog;

void _logSettingSet(const std::string_view& setting);
void _logSettingIfSet(const std::string_view& setting, const bool isSet);
};
}
5 changes: 5 additions & 0 deletions src/cascadia/TerminalSettingsModel/CascadiaSettings.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
std::unordered_map<winrt::hstring, winrt::com_ptr<implementation::ColorScheme>> colorSchemes;
std::unordered_map<winrt::hstring, winrt::hstring> colorSchemeRemappings;
bool fixupsAppliedDuringLoad{ false };
std::set<std::string> themesChangeLog;

void clear();
};
Expand Down Expand Up @@ -96,6 +97,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
void _executeGenerator(const IDynamicProfileGenerator& generator);

std::unordered_set<std::wstring_view> _ignoredNamespaces;
std::set<std::string> themesChangeLog;
// See _getNonUserOriginProfiles().
size_t _userProfileCount = 0;
};
Expand Down Expand Up @@ -150,6 +152,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation

void ExpandCommands();

void LogSettingChanges(bool isJsonLoad) const;

private:
static const std::filesystem::path& _settingsPath();
static const std::filesystem::path& _releaseSettingsPath();
Expand Down Expand Up @@ -180,6 +184,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
winrt::com_ptr<implementation::Profile> _baseLayerProfile = winrt::make_self<implementation::Profile>();
winrt::Windows::Foundation::Collections::IObservableVector<Model::Profile> _allProfiles = winrt::single_threaded_observable_vector<Model::Profile>();
winrt::Windows::Foundation::Collections::IObservableVector<Model::Profile> _activeProfiles = winrt::single_threaded_observable_vector<Model::Profile>();
std::set<std::string> _themesChangeLog{};

// load errors
winrt::Windows::Foundation::Collections::IVector<Model::SettingsLoadWarnings> _warnings = winrt::single_threaded_vector<Model::SettingsLoadWarnings>();
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalSettingsModel/CascadiaSettings.idl
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ namespace Microsoft.Terminal.Settings.Model

CascadiaSettings Copy();
void WriteSettingsToDisk();
void LogSettingChanges(Boolean isJsonLoad);

String Hash { get; };

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ void ParsedSettings::clear()
profilesByGuid.clear();
colorSchemes.clear();
fixupsAppliedDuringLoad = false;
themesChangeLog.clear();
}

// This is a convenience method used by the CascadiaSettings constructor.
Expand Down Expand Up @@ -658,6 +659,12 @@ void SettingsLoader::_parse(const OriginTag origin, const winrt::hstring& source
// versions of these themes overriding the built-in ones.
continue;
}

if (origin != OriginTag::InBox)
{
static std::string themesContext{ "themes" };
theme->LogSettingChanges(settings.themesChangeLog, themesContext);
}
settings.globals->AddTheme(*theme);
}
}
Expand Down Expand Up @@ -1222,6 +1229,7 @@ CascadiaSettings::CascadiaSettings(SettingsLoader&& loader) :
_allProfiles = winrt::single_threaded_observable_vector(std::move(allProfiles));
_activeProfiles = winrt::single_threaded_observable_vector(std::move(activeProfiles));
_warnings = winrt::single_threaded_vector(std::move(warnings));
_themesChangeLog = std::move(loader.userSettings.themesChangeLog);

_resolveDefaultProfile();
_resolveNewTabMenuProfiles();
Expand Down Expand Up @@ -1596,3 +1604,73 @@ void CascadiaSettings::_resolveNewTabMenuProfilesSet(const IVector<Model::NewTab
}
}
}

void CascadiaSettings::LogSettingChanges(bool isJsonLoad) const
{
#ifndef _DEBUG
// Only do this if we're actually being sampled
if (!TraceLoggingProviderEnabled(g_hSettingsModelProvider, 0, MICROSOFT_KEYWORD_MEASURES))
{
return;
}
#endif // !_DEBUG

// aggregate setting changes
std::set<std::string> changes;
static constexpr std::string_view globalContext{ "global" };
_globals->LogSettingChanges(changes, globalContext);

// Actions are not expected to change when loaded from the settings UI
static constexpr std::string_view actionContext{ "action" };
winrt::get_self<implementation::ActionMap>(_globals->ActionMap())->LogSettingChanges(changes, actionContext);

static constexpr std::string_view profileContext{ "profile" };
for (const auto& profile : _allProfiles)
{
winrt::get_self<Profile>(profile)->LogSettingChanges(changes, profileContext);
}

static constexpr std::string_view profileDefaultsContext{ "profileDefaults" };
_baseLayerProfile->LogSettingChanges(changes, profileDefaultsContext);

// Themes are not expected to change when loaded from the settings UI
// DO NOT CALL Theme::LogSettingChanges!!
// We already collected the changes when we loaded the JSON
for (const auto& change : _themesChangeLog)
{
changes.insert(change);
}

// report changes
for (const auto& change : changes)
{
#ifndef _DEBUG
// A `isJsonLoad ? "JsonSettingsChanged" : "UISettingsChanged"`
// would be nice, but that apparently isn't allowed in the macro below.
// Also, there's guidance to not send too much data all in one event,
// so we'll be sending a ton of events here.
if (isJsonLoad)
{
TraceLoggingWrite(g_hSettingsModelProvider,
"JsonSettingsChanged",
TraceLoggingDescription("Event emitted when settings.json change"),
TraceLoggingValue(change.data()),
TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES),
TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage));
}
else
{
TraceLoggingWrite(g_hSettingsModelProvider,
"UISettingsChanged",
TraceLoggingDescription("Event emitted when settings change via the UI"),
TraceLoggingValue(change.data()),
TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES),
TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage));
}
#else
OutputDebugStringA(isJsonLoad ? "JsonSettingsChanged - " : "UISettingsChanged - ");
OutputDebugStringA(change.data());
OutputDebugStringA("\n");
#endif // !_DEBUG
}
}
Loading

0 comments on commit 0a9cbd0

Please sign in to comment.