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.

Sign up for GitHub

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

Jump to bottom

[BASH] Settings UI: Two issues in automatic profile naming #9714

Closed
DHowett opened this issue Apr 6, 2021 · 5 comments
Closed

[BASH] Settings UI: Two issues in automatic profile naming #9714

DHowett opened this issue Apr 6, 2021 · 5 comments
Labels
Area-Settings UI Anything specific to the SUI good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v2.0

Comments

@DHowett
Copy link
Member

DHowett commented Apr 6, 2021 •
edited by zadjii-msft

Windows Terminal version (or Windows build number)

1.8

Other Software

No response

Steps to reproduce

  • Add a new profile. The automatic name is (for example) "Profile 8".
  • Rename it to "Profile 9" (because you are trying to break Terminal, and for no other reason).
  • Add a new profile, which will be named "Profile 9".

Expected Behavior

No response

Actual Behavior

When you hit save, one disappears from the UI.

originated in 4/2 bug bash
@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 Apr 6, 2021
@DHowett DHowett changed the title Settings UI: Two issues in automatic profile naming [BASH] Settings UI: Two issues in automatic profile naming Apr 6, 2021
@zadjii-msft zadjii-msft added Area-Settings UI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. labels Apr 6, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Apr 6, 2021
@zadjii-msft zadjii-msft added this to the Terminal v2.0 milestone Apr 6, 2021
@zadjii-msft
Copy link
Member

Two issues?

@DHowett
Copy link
Member Author

DHowett commented Apr 6, 2021

Duplication of the number, stomping of the entry. I also don't think it matters that much. There's easier ways to break Terminal 😄

@zadjii-msft zadjii-msft added Priority-3 A description (P3) and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Apr 6, 2021
@carlos-zamora
Copy link
Member

carlos-zamora commented Apr 7, 2021 •
edited by DHowett

I'm marking this as help wanted. CascadiaSettings::CreateNewProfile() just needs to check if the newly generated name is taken, and, if it is, generate a new one.

Relevant code:

terminal/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

Lines 222 to 232 in 361877c

winrt::Microsoft::Terminal::Settings::Model::Profile CascadiaSettings::CreateNewProfile()
{
auto newProfile{ _userDefaultProfileSettings->CreateChild() };
_allProfiles.Append(*newProfile);
// Give the new profile a distinct name so a guid is properly generated
const winrt::hstring newName{ fmt::format(L"Profile {}", _allProfiles.Size()) };
newProfile->Name(newName);
return *newProfile;
}

Today, we're naming the new profile "Profile <num of profiles>". We should probably just try "Profile <num of profiles + 1>" and keep incrementing it until we find one that works? I'm mildly annoyed that from that point forward, all new profiles would be called "Profile <num of profiles + 1>" but there's no reason for that to be annoying haha.

@carlos-zamora carlos-zamora added Help Wanted We encourage anyone to jump in on these. good first issue This is a fix that might be easier for someone to do as a first contribution labels Apr 7, 2021
@carlos-zamora carlos-zamora mentioned this issue Apr 7, 2021
Merged
6 tasks
@Don-Vito Don-Vito mentioned this issue Apr 14, 2021
Merged
6 tasks
@ghost ghost added In-PR This issue has a related PR 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 Apr 14, 2021
@ghost ghost closed this as completed in 3368e60 Apr 14, 2021
mpela81 pushed a commit to mpela81/terminal that referenced this issue Apr 17, 2021
@Don-Vito @mpela81
Fix profile name generation to allocate unique name (microsoft#9816)
baae2b2 
## PR Checklist
* [x] Closes microsoft#9714
* [x] CLA signed. 
* [ ] Tests added/passed
* [ ] Documentation updated. 
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already. 

## Detailed Description of the Pull Request / Additional comments
Attempts to generate a name Profile X, where X is the index of the new profile (1-based).
As long as name is already taken, generates new name by incrementing X by 1
DHowett pushed a commit that referenced this issue May 14, 2021
@Don-Vito @DHowett
Fix profile name generation to allocate unique name (#9816)
16ce16e 
## PR Checklist
* [x] Closes #9714
* [x] CLA signed.
* [ ] Tests added/passed
* [ ] Documentation updated.
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already.

## Detailed Description of the Pull Request / Additional comments
Attempts to generate a name Profile X, where X is the index of the new profile (1-based).
As long as name is already taken, generates new name by incrementing X by 1

(cherry picked from commit 3368e60)
DHowett pushed a commit that referenced this issue May 14, 2021
@Don-Vito @DHowett
Fix profile name generation to allocate unique name (#9816)
c2cb365 
## PR Checklist
* [x] Closes #9714
* [x] CLA signed.
* [ ] Tests added/passed
* [ ] Documentation updated.
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already.

## Detailed Description of the Pull Request / Additional comments
Attempts to generate a name Profile X, where X is the index of the new profile (1-based).
As long as name is already taken, generates new name by incrementing X by 1

(cherry picked from commit 3368e60)
@ghost
Copy link

ghost commented May 25, 2021

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

Handy links:

@ghost
Copy link

ghost commented May 25, 2021

🎉This issue was addressed in #9816, which has now been successfully released as Windows Terminal Preview v1.9.1445.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
Assignees
No one assigned
Labels
Area-Settings UI Anything specific to the SUI good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) 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
Milestone
Terminal v2.0
Development

No branches or pull requests
3 participants
@DHowett @carlos-zamora @zadjii-msft