Allow generated profiles to be deleted #11007

lhecker · 2021-08-22T14:30:10Z

Re-enables the delete button for generated profiles in the settings UI.
Additionally fixes "Startup Profiles" to only list active profiles.

Profiles are considered deleted if they're absent from settings.json, but their
GUID has been encountered before. Or in other words, from a user's perspective:
Generated profiles are added to the settings.json automatically only once.
Thus if the user chooses to delete the profile (e.g. using the delete button)
they aren't re-added automatically and thus appear to have been deleted.

Meanwhile those generated profiles are actually only marked as "hidden"
as well as "deleted", but still exist in internal profile lists.
The "hidden" attribute hides them from all existing menus. The "deleted" one
hides them from the settings UI and prevents them from being written to disk.

It would've been preferrable of course to just not generate and
add deleted profile to internal profile lists in the first place.
But this would've required far more wide-reaching changes.
The settings UI for instance requires a list of all profiles in order to
allow a user to re-create previously deleted profiles. Such an approach was
attempted but discarded because of it's current complexity overhead.

References

Part of [megathread] Deleting profiles is often confusing #9997
A sequel to 5d36e5d

PR Checklist

Closes Saving settings with removed default profiles creates them as empty objects #10960
I work here
Tests added/passed

Validation Steps Performed

"Startup Profiles" doesn't list deleted profiles ✔️
Manually removing an item from settings.json removes the profile ✔️
Removing cmd.exe and saving doesn't create empty objects (Saving settings with removed default profiles creates them as empty objects #10960) ✔️
"Add a new profile" lists deleted profiles ✔️
"Duplicate" recreates previously deleted profiles ✔️
Profiles are always created with GUIDs ✔️

src/cascadia/TerminalSettingsEditor/Profiles.cpp

src/cascadia/TerminalSettingsEditor/Launch.xaml

DHowett

Let's do this. This feels right.

Known caveat, which we will want to have an answer for in the future: You can create a new profile based on a deleted generated profile, but it is a full deep copy. There's no way to get the "stub" back (the one that auto-updates when we change the generators(!)) yet. We can work on that, and I suspect that that's going to be a pretty narrow case. I'm only thinking about it from a settings model consistency standpoint. 😄

DHowett · 2021-08-23T00:43:15Z

src/cascadia/TerminalSettingsModel/Profile.idl

@@ -46,6 +46,8 @@ namespace Microsoft.Terminal.Settings.Model
        void CreateUnfocusedAppearance();
        void DeleteUnfocusedAppearance();

+        // True if the user explicitly removed this Profile from settings.json.
+        Boolean Deleted;


you can probably make this getter-only since CascadiaSettings(Serialization) uses the members directly ... right? Or do we want to expose setting this to the public API?

DHowett · 2021-08-23T00:46:43Z

do we have an issue which tracks deleting generated profiles

we used to!

[pr body]

If you don't mind, I would prefer a bit more of the explanation of the how in the PR body when it merges to become the commit. The reason we ended up doing it this way instead of another way/other things we considered... just so that when somebody runs a git blame to figure out which commit introduced WINRT_PROPERTY(deleted...) they can learn why instead of just that "yes indeed, we did this because we did it."

zadjii-msft · 2021-08-23T13:26:17Z

do we have an issue which tracks deleting generated profiles

we used to!

There's #9997, which has a collection of related thoughts, but I'm not sure it's comprehensive.

carlos-zamora · 2021-08-23T17:27:36Z

It would've been preferrable of course to just not generate and
add deleted profile to internal profile lists in the first place.
But this would've required far more wide-reaching changes.
The settings UI for instance requires a list of all profiles in order to
allow a user to re-create previously deleted profiles. Such an approach was
attempted but discarded because of it's current complexity overhead.

Should we track this in a separate issue and throw it in the backlog as a CodeHealth task? It'd be nice if we could take steps to get to that imo. (note, haven't read the PR yet, just speculation)

DHowett · 2021-08-23T17:29:22Z

It would've been preferrable of course to just not generate and
add deleted profile to internal profile lists in the first place.

It might be preferable from a "cleanliness" standpoint, but it can be considered valuable to have a list of profiles the user has explicitly suppressed so we can surface it on an Extensions page.

Also, Leonard's going to rewrite settings anyway, workitem or not.

miniksa · 2021-08-23T21:23:00Z

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

-    winrt::hstring newName{};
-    for (uint32_t candidateIndex = 0; candidateIndex < _allProfiles.Size() + 1; candidateIndex++)
+    std::wstring newName;
+    for (uint32_t candidateIndex = 0, count = _allProfiles.Size() + 1; candidateIndex < count; candidateIndex++)


Can/should you const uint32_t the count initialization?

Also... multiple initializations in for loops always confuse me. But I'm not willing to block over it.

Unfortunately you cannot have multiple different types in for loops in C++.
You can only write something like for (auto [a, b, ...] = std::tuple{ A, B, ... }; ...

Sad. Whatever. I'll survive.

zadjii-msft · 2021-08-23T21:30:56Z

src/cascadia/TerminalSettingsEditor/Profiles.xaml

-                                    </Flyout>
-                                </Button.Flyout>
-                            </Button>
-                            <TextBlock x:Name="DeleteButtonDisclaimer"


We can really get rid of this disclaimer? I was under the impression that deleting a inbox profile from your settings.json would still have it be present in the list of profiles, since those are in defaults.json (they're not from a dynamic source).

Or do we log the default profile guids in state.json somewhere? (I may have missed this)

Even default profiles can be removed now. If a profile isn't present in the user's settings.json, it's origin isn't OriginTag::User. In such cases the profile will now always be marked as "hidden" (and "deleted") unless it's the first time we're encountering it.

This works even for inbox profiles.

Oh and for your last question specifically: Yeah! We're logging all GUIDs to state.json.

oh yea okay, I see it now with the else if (profile.Origin() != OriginTag::User) okay that makes sense!

ghost · 2021-08-23T21:38:17Z

Hello @lhecker!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Re-enables the delete button for generated profiles in the settings UI. Additionally fixes "Startup Profiles" to only list active profiles. Profiles are considered deleted if they're absent from settings.json, but their GUID has been encountered before. Or in other words, from a user's perspective: Generated profiles are added to the settings.json automatically only once. Thus if the user chooses to delete the profile (e.g. using the delete button) they aren't re-added automatically and thus appear to have been deleted. Meanwhile those generated profiles are actually only marked as "hidden" as well as "deleted", but still exist in internal profile lists. The "hidden" attribute hides them from all existing menus. The "deleted" one hides them from the settings UI and prevents them from being written to disk. It would've been preferrable of course to just not generate and add deleted profile to internal profile lists in the first place. But this would've required far more wide-reaching changes. The settings UI for instance requires a list of _all_ profiles in order to allow a user to re-create previously deleted profiles. Such an approach was attempted but discarded because of it's current complexity overhead. * Part of #9997 * A sequel to 5d36e5d * [x] Closes #10960 * [x] I work here * [x] Tests added/passed * "Startup Profiles" doesn't list deleted profiles ✔️ * Manually removing an item from settings.json removes the profile ✔️ * Removing cmd.exe and saving doesn't create empty objects (#10960) ✔️ * "Add a new profile" lists deleted profiles ✔️ * "Duplicate" recreates previously deleted profiles ✔️ * Profiles are always created with GUIDs ✔️

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

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

Handy links:

ghost added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. labels Aug 22, 2021

lhecker commented Aug 22, 2021

View reviewed changes

src/cascadia/TerminalSettingsEditor/Profiles.cpp Show resolved Hide resolved

lhecker force-pushed the dev/lhecker/delete-generated-profiles branch 2 times, most recently from dd56496 to 775d00a Compare August 22, 2021 16:25

lhecker commented Aug 22, 2021

View reviewed changes

src/cascadia/TerminalSettingsEditor/Launch.xaml Outdated Show resolved Hide resolved

Allow generated profiles to be deleted

8cc552c

lhecker force-pushed the dev/lhecker/delete-generated-profiles branch from 775d00a to 8cc552c Compare August 22, 2021 20:23

DHowett added the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label Aug 23, 2021

DHowett approved these changes Aug 23, 2021

View reviewed changes

DHowett requested review from carlos-zamora and zadjii-msft August 23, 2021 16:58

miniksa reviewed Aug 23, 2021

View reviewed changes

miniksa approved these changes Aug 23, 2021

View reviewed changes

zadjii-msft reviewed Aug 23, 2021

View reviewed changes

Address Dustin's comments

4f16fa1

zadjii-msft approved these changes Aug 23, 2021

View reviewed changes

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

ghost merged commit 608a49e into main Aug 23, 2021

ghost deleted the dev/lhecker/delete-generated-profiles branch August 23, 2021 22:00

zadjii-msft mentioned this pull request Aug 24, 2021

[megathread] Deleting profiles is often confusing #9997

Open

13 tasks

DHowett removed the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label Aug 25, 2021

ghost mentioned this pull request Aug 31, 2021

Saving settings with removed default profiles creates them as empty objects #10960

Closed

lhecker mentioned this pull request Sep 2, 2022

Deleting the reference to a profile from a JSON fragment file prevents the profile from showing back up again even when the profile still exists in the fragment file. #13916

Open

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow generated profiles to be deleted #11007

Allow generated profiles to be deleted #11007

lhecker commented Aug 22, 2021 •

edited

Loading

DHowett left a comment

DHowett Aug 23, 2021

DHowett commented Aug 23, 2021

zadjii-msft commented Aug 23, 2021

carlos-zamora commented Aug 23, 2021

DHowett commented Aug 23, 2021

miniksa Aug 23, 2021

lhecker Aug 23, 2021

miniksa Aug 23, 2021

zadjii-msft Aug 23, 2021

lhecker Aug 23, 2021

lhecker Aug 23, 2021

zadjii-msft Aug 23, 2021

ghost commented Aug 23, 2021

ghost commented Aug 31, 2021

Allow generated profiles to be deleted #11007

Allow generated profiles to be deleted #11007

Conversation

lhecker commented Aug 22, 2021 • edited Loading

References

PR Checklist

Validation Steps Performed

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett commented Aug 23, 2021

zadjii-msft commented Aug 23, 2021

carlos-zamora commented Aug 23, 2021

DHowett commented Aug 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 23, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Aug 31, 2021

lhecker commented Aug 22, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.