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

Track and log changes to settings #17678

Merged
merged 16 commits into from
Aug 23, 2024
Merged
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
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
12 changes: 11 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);
PankajBhojwani marked this conversation as resolved.
Show resolved Hide resolved
AddAction(*command, keys);

if (jsonBlock.isMember(JsonKey(KeysKey)))
{
Expand Down Expand Up @@ -156,4 +158,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)
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
{
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(std::string{ setting });
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
}
}

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 @@ -112,6 +112,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 @@ -642,6 +643,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);
PankajBhojwani marked this conversation as resolved.
Show resolved Hide resolved
}
settings.globals->AddTheme(*theme);
}
}
Expand Down Expand Up @@ -1206,6 +1213,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 @@ -1580,3 +1588,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
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
if (!TraceLoggingProviderEnabled(g_hSettingsModelProvider, 0, MICROSOFT_KEYWORD_MEASURES))
{
return;
}
#endif // !_DEBUG

// aggregate setting changes
std::set<std::string> changes;
static std::string globalContext{ "global" };
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
_globals->LogSettingChanges(changes, globalContext);

// Actions are not expected to change when loaded from the settings UI
static std::string actionContext{ "action" };
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
winrt::get_self<implementation::ActionMap>(_globals->ActionMap())->LogSettingChanges(changes, actionContext);

static std::string profileContext{ "profile" };
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
for (const auto& profile : _allProfiles)
{
winrt::get_self<Profile>(profile)->LogSettingChanges(changes, profileContext);
}

static std::string profileDefaultsContext{ "profileDefaults" };
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
_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",
Copy link
Member

Choose a reason for hiding this comment

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

we may get flagged with a "garrulous event". this is a lot of data. there might be a way to TraceLog an array??? i don't actually know.

my concern is that we'll get absolutely whacked with data for any even moderately complicated settings file

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a limit of 65535 bytes per event so that's my concern (though idk how valid it may be) in packing it all together into one.

Even then though, if we sent it over as an array, won't that exclude the stuff we actually want to learn? Like, if we received events with a payload of...

  • globals.initialRows, globals.wordDelimiters, globals.copyOnSelect
  • globals.wordDelimiters, globals.copyOnSelect

We would just see the data as 2 hits with a payload of an array each whereas we want to track "how many hits each setting got (i.e. globals.initialRows=1, globals.wordDelimiters=2, globals.copyOnSelect=2). Right?

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 - ");
Copy link
Member

Choose a reason for hiding this comment

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

very clever.

OutputDebugStringA(change.data());
OutputDebugStringA("\n");
#endif // !_DEBUG
}
}
53 changes: 53 additions & 0 deletions src/cascadia/TerminalSettingsModel/Command.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -741,4 +741,57 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation

return winrt::single_threaded_vector<Model::Command>(std::move(result));
}

void Command::LogSettingChanges(std::set<std::string>& changes)
PankajBhojwani marked this conversation as resolved.
Show resolved Hide resolved
{
if (_IterateOn != ExpandCommandType::None)
{
switch (_IterateOn)
{
case ExpandCommandType::Profiles:
changes.emplace(fmt::format(FMT_COMPILE("{}.{}"), IterateOnKey, "profiles"));
break;
case ExpandCommandType::ColorSchemes:
changes.emplace(fmt::format(FMT_COMPILE("{}.{}"), IterateOnKey, "schemes"));
break;
}
}

if (!_Description.empty())
{
changes.emplace(fmt::format(FMT_COMPILE("{}"), DescriptionKey));
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
}

if (IsNestedCommand())
{
changes.emplace(fmt::format(FMT_COMPILE("{}"), CommandsKey));
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
}
else
{
const auto json{ ActionAndArgs::ToJson(ActionAndArgs()) };
if (json.isString())
{
// covers actions w/out args
// - "command": "unbound" --> "unbound"
// - "command": "copy" --> "copy"
changes.emplace(fmt::format(FMT_COMPILE("{}"), json.asString()));
Copy link
Member

Choose a reason for hiding this comment

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

Technically this fmt::format is not needed. Flag for cleanup later.

}
else
{
// covers actions w/ args
// - "command": { "action": "copy", "singleLine": true } --> "copy.singleLine"
// - "command": { "action": "copy", "singleLine": true, "dismissSelection": true } --> "copy.singleLine", "copy.dismissSelection"

const std::string shortcutActionName{ json[JsonKey("action")].asString() };

auto members = json.getMemberNames();
members.erase(std::remove_if(members.begin(), members.end(), [](const auto& member) { return member == "action"; }), members.end());

for (const auto& actionArg : members)
{
changes.emplace(fmt::format(FMT_COMPILE("{}.{}"), shortcutActionName, actionArg));
}
}
}
}
}
1 change: 1 addition & 0 deletions src/cascadia/TerminalSettingsModel/Command.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
const Json::Value& json,
const OriginTag origin);
Json::Value ToJson() const;
void LogSettingChanges(std::set<std::string>& changes);

bool HasNestedCommands() const;
bool IsNestedCommand() const noexcept;
Expand Down
Loading
Loading