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

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

Closed
Tracked by #9997
ghost opened this issue Nov 14, 2020 · 12 comments · Fixed by MicrosoftDocs/terminal#186 or #10910
Closed
Tracked by #9997

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

ghost opened this issue Nov 14, 2020 · 12 comments · Fixed by MicrosoftDocs/terminal#186 or #10910
Labels
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. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@ghost
Copy link

ghost commented Nov 14, 2020

Description of the new feature/enhancement

When one of the default profiles (PowerShell, Command Prompt, Azure Cloud Shell) is deleted from the configuration file, it still appears in the Terminal. As it is not in the configuration file, it can no longer be edited.

It would be more intuitive if the profiles are either re-created in the config file, or they disappear from the new session dropdown in the terminal.
If they were to be removed from the terminal, it would make sense if a dynamic profile was added to the configuration file when a new shell is installed, so that the user would be able to configure or delete it.

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Nov 14, 2020
@Don-Vito
Copy link
Contributor

Don-Vito commented Nov 14, 2020

@dev-kittens - to "remove" auto-generated profile you need to mark it as "hidden"

You can find more details and explanations about this here: https://docs.microsoft.com/en-us/windows/terminal/dynamic-profiles

@ghost
Copy link
Author

ghost commented Nov 14, 2020

@Don-Vito Thank you for the link. I've added the unwanted profiles to disabledProfileSources in my config.

I'm not satisfied with this answer, though. It's quite unintuitive to have profiles appear even though they were deleted in the configuration file.
I would suggest for Windows Terminal to either re-add the items to the JSON file on start or to only generate dynamic profiles when creating a configuration or when a new shell is installed.

@Don-Vito
Copy link
Contributor

@dev-kittens - I am not from the team, but what you write makes sense, the behavior is not the most intuitive. Probably you should update the issue title and expected behavior. So once the team gets to it, they know that you are aware of current behavior but ask for an improvement.

@ghost ghost changed the title Default profiles cannot be removed Default profiles appear even if removed from the config.json Nov 14, 2020
@ghost
Copy link
Author

ghost commented Nov 14, 2020

Oh, sure. I've just done that.

@zadjii-msft
Copy link
Member

Thanks for the feedback! We actually went throguh a pretty lengthy design discussion about this last year when we were implementing cascading settings and dynamic profiles the first time. If you really want to read more of our decision making process, you can refer to #1258 and #1321. While this wasn't the most intuitive feature I've ever shipped, this behavior had the least drawbacks.

I've added some additional docs over at MicrosoftDocs/terminal#186, hopefully that should help mitigate the confusion a bit.

@zadjii-msft zadjii-msft added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Product-Terminal The new Windows Terminal. Resolution-By-Design It's supposed to be this way. Sometimes for compatibility reasons. labels Nov 16, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 16, 2020
@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Nov 16, 2020
@markdall
Copy link

markdall commented Jun 1, 2021

So, in order to stop seeing a shell for something I don't have and don't use yet (Azure) I needed to add "disabledProfileSources": ["Windows.Terminal.Azure"], to settings.json. I honestly feel like just deleting it should have been sufficient, but I understand that it probably wasn't up to you.

@DaveEM
Copy link

DaveEM commented Jul 19, 2021

@Don-Vito / @zadjii-msft - I'm just another Windows Terminal user here, but here are a few thoughts:

  1. Can the Settings UI indicate when a profile is dynamic and provide a one-click option to delete it and do whatever is needed to keep it from showing up again?

  2. For people editing the JSON directly, it seems like a comment for dynamically generated profiles saying how to keep it from getting re-created is needed at a minimum. Even better, have a setting per dynamic profile to disable the profile (hiding it leaves it in the settings UI). On a related note, can multiple dynamic profiles come from one profile source? If so, disabling at the per-profile level seems like a better option than disabling the whole profile source.

Also, I ran into the same issue as @spiralw where I deleted the profile from the config file and found it odd that the deleted profile was restored in the UI but wasn't in the config file. The suggested solution of Windows Terminal re-adding the items to the JSON file on start seems reasonable to eliminate this confusion.

@zadjii-msft
Copy link
Member

(1 A) IIRC, the "delete profile" button on a profile in the SUI should hide dynamic profiles, instead of just removing them from the json. @carlos-zamora can correct me if I'm wrong on that. If it doesn't today, the works that's being done as a follow up to #10513, #8324 should take care of this

(1 B) RE:

Can the Settings UI indicate when a profile is dynamic

I thought we were going to be adding that text to the SUI at some point, but clearly we forgot to do that.

Also, I ran into the same issue as @spiralw where I deleted the profile from the config file and found it odd that the deleted profile was restored in the UI but wasn't in the config file. The suggested solution of Windows Terminal re-adding the items to the JSON file on start seems reasonable to eliminate this confusion.

That's definitely unexpected. When the settings UI loads up the settings, it actually just uses the contents of the file, so perhaps it's possible that your editor had the file open and didn't notice that the file contents changed out from underneath it?

@DaveEM
Copy link

DaveEM commented Jul 20, 2021

(1 A) IIRC, the "delete profile" button on a profile in the SUI should hide dynamic profiles, instead of just removing them from the json. @carlos-zamora can correct me if I'm wrong on that. If it doesn't today, the works that's being done as a follow up to #10513, #8324 should take care of this

The problems in this case are:
a. The Delete Profile button for the Windows PowerShell profile is greyed out. I added a comment to #10547 about this since that issue is currently closed as a dupe of the work to remember which dynamic profiles have been deleted but won't be fully resolved unless the block on deleting the profile is removed.

b. Hiding the profile doesn't remove it from the Settings menu. If I delete a profile I expect it to act like it's fully deleted - at least in the UI.

(1 B) RE:

Can the Settings UI indicate when a profile is dynamic

I thought we were going to be adding that text to the SUI at some point, but clearly we forgot to do that.

Thanks for getting an issue opened for this!

Also, I ran into the same issue as @spiralw where I deleted the profile from the config file and found it odd that the deleted profile was restored in the UI but wasn't in the config file. The suggested solution of Windows Terminal re-adding the items to the JSON file on start seems reasonable to eliminate this confusion.

That's definitely unexpected. When the settings UI loads up the settings, it actually just uses the contents of the file, so perhaps it's possible that your editor had the file open and didn't notice that the file contents changed out from underneath it?

This definitely repros:

  1. Run Windows Terminal on Windows 10 19043.1147.
  2. Go to Settings -> Open JSON file
  3. Remove the "Windows PowerShell" profile from the settings JSON.
  4. Close Windows Terminal.
  5. Close the editor.
  6. Run Windows Terminal again.
  7. Go to Settings (notice that Windows PowerShell is still there still) -> Open JSON file
  8. See that the "Windows PowerShell" profile has not been restored.
  9. For extra bonus points, go back to Windows Terminal, click the Save to save Settings, and then see that an empty profile entry has been added to the profile list (i.e. "{}")

@carlos-zamora
Copy link
Member

(1 A) IIRC, the "delete profile" button on a profile in the SUI should hide dynamic profiles, instead of just removing them from the json. @carlos-zamora can correct me if I'm wrong on that. If it doesn't today, the works that's being done as a follow up to #10513, #8324 should take care of this

In box and dynamic profiles disable the delete button and display a text explaining why it's disabled:

  • In box profiles (CMD and Windows PowerShell):
    This profile cannot be deleted because it is included by default.
  • Dynamic profiles (WSL distros, PowerShell Core)
    This profile cannot be deleted because it is automatically generated.

We still show hidden profiles in the SUI so hiding it won't fix the issue. We need #8324 so that we can know when the user deleted (via JSON or SUI) the profile and we make sure it doesn't show up again. Not great but we're getting there.

@DaveEM You're seeing the same bug here. The Windows PowerShell profile should basically be an empty profile IIRC. The problem shouldn't repro for non-dynamic non-in-box profiles (brand new profiles you created yourself).

@carlos-zamora
Copy link
Member

@zadjii-msft removing the Resolution-By-Design tag since we'll want to do this after #8324.

@carlos-zamora carlos-zamora added Issue-Task It's a feature request, but it doesn't really need a major design. and removed Resolution-By-Design It's supposed to be this way. Sometimes for compatibility reasons. Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs labels Jul 26, 2021
@ghost ghost added the In-PR This issue has a related PR label Aug 9, 2021
@ghost ghost closed this as completed in #10910 Aug 16, 2021
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Aug 16, 2021
ghost pushed a commit that referenced this issue Aug 16, 2021
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.
DHowett pushed a commit that referenced this issue Aug 25, 2021
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
Copy link

ghost commented Aug 31, 2021

🎉This issue was addressed in #10910, which has now been successfully released as Windows Terminal Preview v1.10.2383.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
5 participants