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

Hide profiles by default if they aren't new #10910

Merged
3 commits merged into from
Aug 16, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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,47 @@ 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;
size_t hiddenProfiles = 0;

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

// In case that a user removed all of their profiles, we want to default to pretend as if
// the user asked for a "reset" of the profile list and readd all profiles as visible.
if (hiddenProfiles == resultPtr->_allProfiles.Size())
{
for (auto profile : resultPtr->_allProfiles)
{
profile.Hidden(false);
}
}

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 +392,6 @@ void CascadiaSettings::_LoadDynamicProfiles()
}
}

const GUID nullGuid{ 0 };
for (auto& generator : _profileGenerators)
{
const std::wstring generatorNamespace{ generator->GetNamespace() };
Expand Down Expand Up @@ -746,7 +785,7 @@ 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))
if (profile.Hidden() || isInJsonObj(profile, _userSettings) || isInJsonObj(profile, _defaultSettings))
Copy link
Member

Choose a reason for hiding this comment

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

So technically this is the same as if (profile.Hidden() || profile.Origin() == OriginTag::User) right? Just doesn't make sense to change it because it'll probably have some kind of unintended consequence.

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 this is just there, because it would be confusing otherwise, if the profile you deleted suddenly shows up again. It felt weird, when I tested it. 😅

{
continue;
}
Expand Down