-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Create a new page for "Add new profile" in the SUI #9352
Conversation
screen-shoot it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/TerminalSettingsEditor/Resources/en-US/Resources.resw
Outdated
Show resolved
Hide resolved
src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw
Outdated
Show resolved
Hide resolved
src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome job! Thanks!
src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little worried about some of the edge cases with duplicating profiles and the names, guids assigned 😬
// - 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
_allProfiles.Append(*duplicated); | ||
|
||
const auto newName{ fmt::format(L"{} ({})", source.Name(), RS_(L"CopySuffix")) }; | ||
duplicated->Name(winrt::hstring(newName)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay hear me out - what if you have two profiles already: ["A", "A Copy"]. If you duplicate "A", what happens? Two "A Copy" profiles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow up sounds good - though I would also like some discussion before that on whether this is strictly necessary!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)
|
||
const auto newName{ fmt::format(L"{} ({})", source.Name(), RS_(L"CopySuffix")) }; | ||
duplicated->Name(winrt::hstring(newName)); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not manually assign any guid here, later on a guid is automatically generated for the duplicated profile from its name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meh, I'm fine with that for now. Just so long as we're aware of it 😉
Misspellings found, please review:
To accept these changes, run the following commands from this repository on this branch
✏️ Contributor please read thisBy 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.
If the listed items are:
See the 🔬 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 File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
notes mailed to Pankaj |
…/pabhoj/add_profile_page
…/pabhoj/add_profile_page
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
break; | ||
} | ||
// There is a theoretical unsigned integer wraparound, which is OK | ||
newName = fmt::format(L"{} ({}{})", source.Name(), RS_(L"CopySuffix"), candidateIndex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Hello @PankajBhojwani! Because this pull request has the 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 (
|
🎉 Handy links: |
Profile::CopySettings
andCascadiaSettings::DuplicateProfile
👎Notes from bug bash (checked bugs have been resolved):
seems a little clearer to me.
to the defaults and you duplicate a profile
PR Checklist