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

lhecker · 2021-08-09T21:41:40Z

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

Closes Default profiles appear even if removed from the config.json #8270
I work here
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.

carlos-zamora

Ok! It took me a while to understand it but I think I get it now. So it'll work something like this:

settings.json has profiles A, B, C, D (D = dynamic profile)
- GeneratedProfiles: A,B,C,D
user removes D and saves
- GeneratedProfiles: A,B,C
- D is loaded, but it's marked as hidden
_AppendDynamicProfilesToUserSettings
- D is hidden, so we don't append it to the JSON
  Right?

Either way, you showed me that it works earlier, and it looks great!

carlos-zamora · 2021-08-09T23:30:21Z

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

@@ -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))


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.

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. 😅

WSLUser · 2021-08-10T16:08:53Z

Naturally they would open settings.json and try to remove the profile object

If someone is trying to delete that profile, wouldn't using "disabledProfileSources": be a better solution? For example, not everyone uses the Azure profile, a user can disable sourcing it but they first have to know about it. If they don't, then they'll simply try removing it by hand and of course it's regenerated. Marking something like that as "hidden" isn't the right solution, that just hides what the user will consider "bloatware". It should be removed altogether in this case.

zadjii-msft · 2021-08-10T16:23:53Z

If someone is trying to delete that profile, wouldn't using "disabledProfileSources": be a better solution?

After the literally 100 issues that have been filed on us for "i deleted the thing why does it come back", I'm gonna say no. Most folks don't know how dynamic profiles work, and they honsetly don't care. They just want to delete the thing and have it go away.

DHowett · 2021-08-10T16:24:41Z

Naturally they would open settings.json and try to remove the profile object

If someone is trying to delete that profile, wouldn't using "disabledProfileSources": be a better solution? For example, not everyone uses the Azure profile, a user can disable sourcing it but they first have to know about it. If they don't, then they'll simply try removing it by hand and of course it's regenerated. Marking something like that as "hidden" isn't the right solution, that just hides what the user will consider "bloatware". It should be removed altogether in this case.

Allowing users to delete individual profiles from a generator solves the case where they only want to delete one WSL profile--not all of them--because they've made a custom version of one that they want to detach from it. Disabling an entire generator is "the nuclear option." 😄

However, I tend to agree with the rest of this statement. I'm more in favor of making the profile object not exist (so it never gets re-serialized on save, and doesn't show up in the settings UI) when the user deletes it. That will improve uniformity with user-origin profiles and allow us to simplify the logic behind [Delete] in the Settings UI.

Reading the code review, it looks like one of the intermediate nodes is marked as hidden... so we will never write a new entry for a deleted-then-hidden profile to the JSON. This is awesome.

DHowett · 2021-08-10T16:32:27Z

Due to GitHub being somewhat down, I cannot review using the web UI. However:

+            for (auto profile : resultPtr->_allProfiles)
+            {
+                if (generatedProfiles.emplace(profile.Guid()).second)
+                {
+                    generatedProfilesChanged = true;

This is going to add user-origin profiles to the list, is is not? We'll eventually have a list of all profiles ever seen, rather than a list of profiles that were generated by a profile generator. Is this a worthwhile concern?

+            // 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);

We can remove the validation step in _Validate... that checks that at least one profile is visible if we're going to do this 😄

         // 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))

This comment could use some updating!

lhecker · 2021-08-10T22:28:06Z

@DHowett Regarding your diffs in the order you wrote them:

Even generated profiles will be tagged as User, if they're in the settings.json during load. If we ignore User profiles, we'd end up in a situation where no profile is added to the set, if someone removes state.json, but not settings.json.
I've removed that code
Comment added 👍

DHowett · 2021-08-11T20:25:25Z

@msftbot make sure @zadjii-msft signs off

ghost · 2021-08-11T20:25:29Z

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @zadjii-msft

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

DHowett · 2021-08-11T20:35:03Z

Okay, so confirming the settings UI experience:

We will want to let users delete these profiles, right?
When they do, should we set hidden or delete the profile?
- If we delete it, it will show back up as hidden when they save. Maybe that makes sense?
- If we set it hidden, it will not disappear from the list.
  - We need a visual treatment for hidden profiles so that we don't get complaints that "delete does nothing, but it goes away from the menu"

We should file a followup (does somebody want to own it for the 1.11 cycle?) for making the settings UI do the "right" thing here.

lhecker · 2021-08-12T01:24:39Z

We will want to let users delete these profiles, right?

What are "these" profiles? Custom or generated ones? Because I believe the former is unaffected by this change. The hidden attribute is only set on non-User profiles after all...

I've tried this PR and deleting profiles in the SUI works as expected I believe.

DHowett · 2021-08-12T01:25:52Z

We will want to let users delete these profiles, right?

What are "these" profiles? Custom or generated ones? Because I believe the former is unaffected by this change. The hidden attribute is only set on non-User profiles after all...

I've tried this PR and deleting profiles in the SUI works as expected I believe.

Ah I mean Generated ones :)

carlos-zamora · 2021-08-12T16:43:47Z

Okay, so confirming the settings UI experience:

We will want to let users delete these profiles, right?

When they do, should we set hidden or delete the profile?

If we delete it, it will show back up as hidden when they save. Maybe that makes sense?

If we set it hidden, it will not disappear from the list.

We need a visual treatment for hidden profiles so that we don't get complaints that "delete does nothing, but it goes away from the menu"

We should file a followup (does somebody want to own it for the 1.11 cycle?) for making the settings UI do the "right" thing here.

I think half of the problem here is we need an extensions page. I feel like the proper design should be something like this:

Scenario 1: If you remove the profile from the JSON --> the profile won't appear in the dropdown or SUI
- this is done using AppState::GeneratedProfiles
Scenario 2: If you delete the profile from SUI (now, all profiles should be delete-able) --> do the same as Scenario 1
- handled the same way as Scenario 1
Scenario 3: The user wants a deleted auto-generated profile to come back --> go to the Extensions page and re-enable it

For now, I think this implementation is fine because it helps JSON-only users not see the profile. Thoughts?

DHowett · 2021-08-12T16:44:46Z

Yes, that's the proper design. However: what's going to happen today and how much do we need to fix for 1.11?

zadjii-msft

Yea I'm cool with this. We obviously have more stuff to work out in post, but we always did. This doesn't really make it that much worse.

The only real caution here is the scenario where WSL just fails to load entirely (cause it does that sometimes), then we fail to find the profile source, then we don't include it in the settings.json when we reserialize. But actually, that was a problem before too. Yea this is better than before and we'll work out the kinks.

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.

ghost · 2021-08-31T17:04:48Z

🎉Windows Terminal Preview v1.10.2383.0 has been released which incorporates this pull request.:tada:

Handy links:

Hide profiles by default if they aren't new

3a50435

ghost added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Aug 9, 2021

carlos-zamora approved these changes Aug 9, 2021

View reviewed changes

lhecker added 2 commits August 11, 2021 00:13

Address reviewer comments

2457c78

Address Dustin's comments

ad2c346

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 11, 2021

DHowett mentioned this pull request Aug 13, 2021

Azure Cloud Shell should not be available by default since I haven't installed it, haven't requested it #5908

Closed

This comment has been minimized.

Sign in to view

microsoft deleted a comment from Alacritous Aug 13, 2021

zadjii-msft approved these changes Aug 16, 2021

View reviewed changes

ghost merged commit 5d36e5d into main Aug 16, 2021

ghost deleted the dev/lhecker/app-state-generated-profiles branch August 16, 2021 13:32

zadjii-msft mentioned this pull request Aug 16, 2021

[megathread] Deleting profiles is often confusing #9997

Open

13 tasks

ghost mentioned this pull request Aug 31, 2021

Default profiles appear even if removed from the config.json #8270

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lhecker commented Aug 9, 2021

carlos-zamora left a comment

carlos-zamora Aug 9, 2021

lhecker Aug 10, 2021

WSLUser commented Aug 10, 2021

zadjii-msft commented Aug 10, 2021

DHowett commented Aug 10, 2021 •

edited

Loading

DHowett commented Aug 10, 2021

lhecker commented Aug 10, 2021

DHowett commented Aug 11, 2021

ghost commented Aug 11, 2021

DHowett commented Aug 11, 2021

lhecker commented Aug 12, 2021

DHowett commented Aug 12, 2021

carlos-zamora commented Aug 12, 2021

DHowett commented Aug 12, 2021

This comment has been minimized.

This comment has been minimized.

zadjii-msft left a comment

ghost commented Aug 31, 2021

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

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

Conversation

lhecker commented Aug 9, 2021

References

PR Checklist

Validation Steps Performed

carlos-zamora left a comment

Choose a reason for hiding this comment

carlos-zamora Aug 9, 2021

Choose a reason for hiding this comment

lhecker Aug 10, 2021

Choose a reason for hiding this comment

WSLUser commented Aug 10, 2021

zadjii-msft commented Aug 10, 2021

DHowett commented Aug 10, 2021 • edited Loading

DHowett commented Aug 10, 2021

lhecker commented Aug 10, 2021

DHowett commented Aug 11, 2021

ghost commented Aug 11, 2021

DHowett commented Aug 11, 2021

lhecker commented Aug 12, 2021

DHowett commented Aug 12, 2021

carlos-zamora commented Aug 12, 2021

DHowett commented Aug 12, 2021

This comment has been minimized.

This comment has been minimized.

zadjii-msft left a comment

Choose a reason for hiding this comment

ghost commented Aug 31, 2021

DHowett commented Aug 10, 2021 •

edited

Loading