Skip to content

Commit

Permalink
Hide profiles by default if they aren't new (#10910)
Browse files Browse the repository at this point in the history
Let's say a user doesn't know that they need to write `"hidden": true` in
order to prevent a profile from showing up (and a settings UI doesn't exist).
Naturally they would open settings.json and try to remove the profile object.
This section of code recognizes if a profile was seen before and marks it as
`"hidden": true` by default and thus ensures the behavior the user expects:
Profiles won't show up again after they've been removed from settings.json.

## References

#8324 - Application State

## PR Checklist
* [x] Closes #8270
* [x] I work here
* [x] Tests added/passed

## Validation Steps Performed

* settings.json/state.json are created if they don't exist ✔️
* Removing any profile from settings.json doesn't cause it to appear again ✔️
* Hitting save in SUI creates profiles with `"hidden": true` ✔️
* Removing a default profile and hitting save in SUI works ❌
  An empty object is added instead.
  • Loading branch information
lhecker authored Aug 16, 2021
1 parent 0220f71 commit 5d36e5d
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 8 deletions.
7 changes: 6 additions & 1 deletion src/cascadia/TerminalControl/TermControl.h
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation

inline bool _IsClosing() const noexcept
{
#ifndef NDEBUG
// _closing isn't atomic and may only be accessed from the main thread.
assert(Dispatcher().HasThreadAccess());
if (const auto dispatcher = Dispatcher())
{
assert(dispatcher.HasThreadAccess());
}
#endif
return _closing;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,15 +155,14 @@ winrt::Microsoft::Terminal::Settings::Model::CascadiaSettings CascadiaSettings::
const auto hardcodedDefaultGuid = resultPtr->GlobalSettings().DefaultProfile();

std::optional<std::string> fileData = _ReadUserSettings();
const bool foundFile = fileData.has_value();

// Make sure the file isn't totally empty. If it is, we'll treat the file
// like it doesn't exist at all.
const bool fileHasData = foundFile && !fileData.value().empty();
const bool fileHasData = fileData && !fileData->empty();
bool needToWriteFile = false;
if (fileHasData)
{
resultPtr->_ParseJsonString(fileData.value(), false);
resultPtr->_ParseJsonString(*fileData, false);
}

// Load profiles from dynamic profile generators. _userSettings should be
Expand Down Expand Up @@ -204,6 +203,35 @@ winrt::Microsoft::Terminal::Settings::Model::CascadiaSettings CascadiaSettings::
_CatchRethrowSerializationExceptionWithLocationInfo(resultPtr->_userSettingsString);
}

// Let's say a user doesn't know that they need to write `"hidden": true` in
// order to prevent a profile from showing up (and a settings UI doesn't exist).
// Naturally they would open settings.json and try to remove the profile object.
// This section of code recognizes if a profile was seen before and marks it as
// `"hidden": true` by default and thus ensures the behavior the user expects:
// Profiles won't show up again after they've been removed from settings.json.
{
const auto state = winrt::get_self<implementation::ApplicationState>(ApplicationState::SharedInstance());
auto generatedProfiles = state->GeneratedProfiles();
bool generatedProfilesChanged = false;

for (auto profile : resultPtr->_allProfiles)
{
if (generatedProfiles.emplace(profile.Guid()).second)
{
generatedProfilesChanged = true;
}
else if (profile.Origin() != OriginTag::User)
{
profile.Hidden(true);
}
}

if (generatedProfilesChanged)
{
state->GeneratedProfiles(generatedProfiles);
}
}

// After layering the user settings, check if there are any new profiles
// that need to be inserted into their user settings file.
needToWriteFile = resultPtr->_AppendDynamicProfilesToUserSettings() || needToWriteFile;
Expand Down Expand Up @@ -352,7 +380,6 @@ void CascadiaSettings::_LoadDynamicProfiles()
}
}

const GUID nullGuid{ 0 };
for (auto& generator : _profileGenerators)
{
const std::wstring generatorNamespace{ generator->GetNamespace() };
Expand Down Expand Up @@ -711,7 +738,7 @@ bool CascadiaSettings::_AppendDynamicProfilesToUserSettings()
wbuilder.settings_["indentation"] = " ";
wbuilder.settings_["enableYAMLCompatibility"] = true; // suppress spaces around colons

auto isInJsonObj = [](const auto& profile, const auto& json) {
static const auto isInJsonObj = [](const auto& profile, const auto& json) {
for (auto profileJson : _GetProfilesJsonObject(json))
{
if (profileJson.isObject())
Expand Down Expand Up @@ -745,8 +772,16 @@ bool CascadiaSettings::_AppendDynamicProfilesToUserSettings()

for (const auto& profile : _allProfiles)
{
// Skip profiles that are in the user settings or the default settings.
if (isInJsonObj(profile, _userSettings) || isInJsonObj(profile, _defaultSettings))
// Skip profiles that are:
// * hidden
// Because when a user manually removes profiles from settings.json,
// we mark them as hidden in LoadAll(). Adding those profiles right
// back into settings.json would feel confusing, while the
// profile that was just erased is added right back.
// * in the user settings or the default settings
// Because we don't want to add profiles which are already
// in the settings.json (explicitly or implicitly).
if (profile.Hidden() || isInJsonObj(profile, _userSettings) || isInJsonObj(profile, _defaultSettings))
{
continue;
}
Expand Down

0 comments on commit 5d36e5d

Please sign in to comment.