-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notes about where string views
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits - but I am close
src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
Outdated
Show resolved
Hide resolved
src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
Outdated
Show resolved
Hide resolved
src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
Outdated
Show resolved
Hide resolved
src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
Outdated
Show resolved
Hide resolved
TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage)); | ||
} | ||
#else | ||
OutputDebugStringA(isJsonLoad ? "JsonSettingsChanged - " : "UISettingsChanged - "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very clever.
if (isJsonLoad) | ||
{ | ||
TraceLoggingWrite(g_hSettingsModelProvider, | ||
"JsonSettingsChanged", |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You already marked the static std::string
s as resolved. I'm assuming you'll push a commit for this? I'm otherwise ✅ with this PR.
@@ -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())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elsewhere you wrote _changeLog.emplace(KeysKey);
. Does this not support the contains call without the .data()
call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope :/. Even with the transparent comparators
// covers actions w/out args | ||
// - "command": "unbound" --> "unbound" | ||
// - "command": "copy" --> "copy" | ||
changes.emplace(fmt::format(FMT_COMPILE("{}"), json.asString())); |
There was a problem hiding this comment.
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.
There's an unnecessary `fmt::format` here caught in the code review: #17678 (comment) This simply removes it.
There's an unnecessary `fmt::format` here caught in the code review: #17678 (comment) This simply removes it. (cherry picked from commit 93d592b) Service-Card-Id: PVTI_lADOAF3p4s4AmhmQzgSwIS4 Service-Version: 1.22
Adds functionality throughout the settings model to keep track of which settings have been set.
There are two entry points:
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 thebase layerprofile 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 logINHERITABLE_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:
defaultAppearance
andunfocusedAppearance
profileDefaults
andprofile
(any other profile)GlobalAppSettings
Theme
:LayerJson
! So instead, do the work inCascadiaSettings
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.ActionsAndArgs
as a whole add a ton of functionality. I handled it over inCommand::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