Create a new page for "Add new profile" in the SUI by PankajBhojwani · Pull Request #9352 · microsoft/terminal

PankajBhojwani · 2021-03-03T00:34:47Z

Whenever we add a new profile setting from now on we have to update
Profile::CopySettings and CascadiaSettings::DuplicateProfile 👎

Notes from bug bash (checked bugs have been resolved):

The duplicate list can be very long if you have profiles
DH: "Create new" seems too vague. "New empty profile" or something
seems a little clearer to me.
There is no deduplication counter for name
Crash when your settings file is corrupt and we had to fall back
to the defaults and you duplicate a profile
Crash due to Crash in main when writing settings to disk twice #10003

PR Checklist

Closes "Add a profile" should lead to its own page #9121

DHowett · 2021-03-03T20:15:58Z

screen-shoot it!

carlos-zamora

I have a lot of comments, and they're all nits, really. Only requesting changes so I can come back and check it when you've addressed my stuff. You've done a fantastic job here. Thank you so much 😊!

Definitely want @cinnamon-msft to take a look at the design (after the minor XAML changes I've suggested). It feels great!

I was originally thinking that selecting a profile to duplicate would be in a combo box, but since the page is fairly empty right now, I think this makes more sense (and changing it to a combo box is like no work, really).

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

src/cascadia/TerminalSettingsEditor/AddProfile.idl

src/cascadia/TerminalSettingsEditor/AddProfile.h

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

carlos-zamora

Awesome job! Thanks!

src/cascadia/TerminalSettingsEditor/AddProfile.h

src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw

zadjii-msft

I'm a little worried about some of the edge cases with duplicating profiles and the names, guids assigned 😬

src/cascadia/TerminalSettingsEditor/MainPage.cpp

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

zadjii-msft · 2021-03-15T16:47:34Z

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

+// - Duplicate a new profile based off another profile's settings
+// Return Value:
+// - a reference to the new profile
+winrt::Microsoft::Terminal::Settings::Model::Profile CascadiaSettings::DuplicateProfile(Model::Profile source)


Is there some way we could add a test for this? To maybe prevent us from forgetting this in the future?

Maybe like if we did something where we:

duplicate a profile ("A" -> "A Copy")

manually change the name of "A Copy" back to "A"

Call ToJson on both profiles and compare the results?

Just to clarify - what would be the purpose of this test? If we want to check whether duplicating is working as intended, we could just compare the profile objects themselves without calling ToJson right?

I'm not sure that a Profile actually implements any sort of logical compare with other profiles - so I was thinking the ToJson would be a handy shortcut, rather writing code that actually compares the values of each and every property.

This is more of a thought experiment so you can ignore it for now

zadjii-msft · 2021-03-15T16:49:24Z

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

+    _allProfiles.Append(*duplicated);
+
+    const auto newName{ fmt::format(L"{} ({})", source.Name(), RS_(L"CopySuffix")) };
+    duplicated->Name(winrt::hstring(newName));


okay hear me out - what if you have two profiles already: ["A", "A Copy"]. If you duplicate "A", what happens? Two "A Copy" profiles?

Only one of the Copy profiles will show up after the user hits 'save', which is the same thing that happens if a user manually creates two profiles with the same name so I feel like it should be okay?

Should we do the thing that like, explorer (et. al) do where it's like "Foo" -> "Foo Copy" -> "Foo Copy(1)" -> "Foo Copy(2)" -> etc? Like, it seems to me that the action that's being requested by the user is "make me a new profile", so we should definitely give them a fresh one.

I suppose we could address both this and "if a user manually creates two profiles with the same name (using the UI)" at the same time in a follow up

Follow up sounds good - though I would also like some discussion before that on whether this is strictly necessary!

For the record, this is prone to a similar bug as #9714. So I see it as the same level of importance/severity as that. I agree though, something we should be aware of as a follow up. Mind filing an issue and tagging it here (heck, maybe even expand #9714 to encompass this since it'll be a similar fix?)

zadjii-msft · 2021-03-15T16:52:54Z

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

+
+    const auto newName{ fmt::format(L"{} ({})", source.Name(), RS_(L"CopySuffix")) };
+    duplicated->Name(winrt::hstring(newName));
+


In the same vein - what do we do about the duplicated profile's guid? Do we manually assign it a fresh one?

If I manually generate the guid for a profile named "Foo Copy" (say it's {1}), and set the guid for a profile "Bar" to the guid to {1}, then duplicate "Foo", does that duplicated profile now conflict with "Bar"?

We do not manually assign any guid here, later on a guid is automatically generated for the duplicated profile from its name

Maybe we should, to avoid the (incredibly rare but absolutely possible) case from above? I think it'd be totally unexpected if you duplicated "Foo" and ended up in the settings for "Bar"

I'm not sure I agree that this is on us to solve. If we're going as far as to consider what if the user manually inputs a very specific guid that will conflict with a guid we generate, then couldn't they achieve the same thing by simply manually assigning the same guid to two different profiles in their settings file? It feels like one of those 'do bad things get bad results' that I'm not sure we should try to work around

Meh, I'm fine with that for now. Just so long as we're aware of it 😉

src/cascadia/TerminalSettingsEditor/AddProfile.xaml

…/pabhoj/add_profile_page

github-actions · 2021-04-12T19:00:24Z

Misspellings found, please review:

duplciate

To accept these changes, run the following commands from this repository on this branch

pushd $(git rev-parse --show-toplevel)
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"aef aspnet boostorg BSODs Cac COINIT dahall DEFAPP DEFCON fde fea fmtlib HPCON isocpp mintty msvcrtd NVDA pinam QOL remoting Unk unte vcrt what3words xamarin "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/7df4b3c823d1b64a60bf6089b249adb14ecfd458.txt";
use File::Path qw(make_path);
make_path ".github/actions/spelling/expect";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"cac coinit defapp duplciate hpcon MSVCRTD Remoting unk "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
popd

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

... misspelled, then please correct them instead of using the command.
... names, please add them to .github/actions/spelling/dictionary/names.txt.
... APIs, you can add them to a file in .github/actions/spelling/dictionary/.
... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

🗜️ If you see a bunch of garbage and it relates to a binary-ish string, please add a file path to the .github/actions/spelling/excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

DHowett

i'm blocking to re-review. this is WYA too hot for 1.8 (which is already built)

carlos-zamora · 2021-04-19T17:50:05Z

i'm blocking to re-review. this is WYA too hot for 1.8 (which is already built)

@PankajBhojwani @DHowett Now that v1.8 is out, can we get this merged for the bug bash on Friday?

DHowett · 2021-04-23T22:34:14Z

notes mailed to Pankaj

…/pabhoj/add_profile_page

DHowett · 2021-05-05T02:40:14Z

moved notes-

DHowett

Pankaj- automerge this when you have fixed the name counter so that it.. doesn't say "Copy0" and looks more ergonomic (like, starting at "Copy 2" and having a space)

DHowett · 2021-05-05T02:36:57Z

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

Won't this create Blah (Copy0)? That seems somewhat unergonomic.

You may want to use candidateIndex+2 (so that we skip "copy 1") and split the number and the copy suffix out with a space, like Blah (Copy 2)

ghost · 2021-05-05T04:15:22Z

Hello @PankajBhojwani!

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.

ghost · 2021-05-25T19:10:33Z

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

Handy links:

initial

3402193

ghost added Area-SettingsUI Anything specific to the SUI Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Mar 3, 2021

add new works

7d94b5e

PankajBhojwani added 4 commits March 3, 2021 16:14

events

ff4971c

duplicate button

0fc1578

works now

b3adf6b

okay now it works

e4d1a02

PankajBhojwani marked this pull request as ready for review March 6, 2021 00:59

zadjii-msft self-assigned this Mar 8, 2021

carlos-zamora requested changes Mar 10, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 10, 2021

zadjii-msft assigned PankajBhojwani and unassigned zadjii-msft Mar 10, 2021

PankajBhojwani added 2 commits March 10, 2021 13:13

addressing comments

c472b60

conflict

2b23b94

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 10, 2021

abstract, localize

4326dd8

carlos-zamora approved these changes Mar 12, 2021

View reviewed changes

src/cascadia/TerminalSettingsEditor/AddProfile.h Outdated Show resolved Hide resolved

src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved

nits

9d9a733

PankajBhojwani added the Needs-Second It's a PR that needs another sign-off label Mar 12, 2021

ghost requested review from leonMSFT, miniksa and zadjii-msft March 12, 2021 19:17

zadjii-msft requested changes Mar 15, 2021

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Mar 15, 2021

address comments

981dfdc

PankajBhojwani added 4 commits April 7, 2021 16:32

format

2b0416c

Merge branch 'main' of https://github.com/microsoft/terminal into dev…

7a57a99

…/pabhoj/add_profile_page

appearance setting macro

d24b425

duplicate now includes unfocusedAppearance

dec8950

PankajBhojwani added 2 commits April 12, 2021 14:17

conflict

2d31527

format

1b9bb9b

PankajBhojwani requested a review from DHowett April 12, 2021 22:33

DHowett requested changes Apr 12, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 12, 2021

PankajBhojwani removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 19, 2021

ghost requested a review from lhecker April 26, 2021 02:14

PankajBhojwani added 6 commits April 25, 2021 20:04

rename the create new button

aa5e0d7

check for user default profile settings before using it

51ec91c

Merge branch 'main' of https://github.com/microsoft/terminal into dev…

2c31c87

…/pabhoj/add_profile_page

deduplication counter

db1358e

conflict

62ff394

Merge branch 'main' of https://github.com/microsoft/terminal into dev…

947efeb

…/pabhoj/add_profile_page

PankajBhojwani requested a review from DHowett May 3, 2021 01:55

DHowett approved these changes May 5, 2021

View reviewed changes

fix copy number

294d4e2

PankajBhojwani added the AutoMerge Marked for automatic merge by the bot when requirements are met label May 5, 2021

ghost merged commit b53bd67 into main May 5, 2021

ghost deleted the dev/pabhoj/add_profile_page branch May 5, 2021 04:15

ghost mentioned this pull request May 25, 2021

"Add a profile" should lead to its own page #9121

Closed

This pull request was closed.


		const auto newName{ fmt::format(L"{} ({})", source.Name(), RS_(L"CopySuffix")) };
		duplicated->Name(winrt::hstring(newName));

Conversation

PankajBhojwani commented Mar 3, 2021 • edited by DHowett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

DHowett commented Mar 3, 2021

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PankajBhojwani Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PankajBhojwani Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Apr 12, 2021

Misspellings found, please review:

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

carlos-zamora commented Apr 19, 2021

Uh oh!

DHowett commented Apr 23, 2021

Uh oh!

DHowett commented May 5, 2021

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PankajBhojwani commented Mar 3, 2021 •

edited by DHowett

Loading

PankajBhojwani Mar 15, 2021 •

edited

Loading

PankajBhojwani Mar 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.