-
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
Include profile nav menu items to consider for retaining position #10618
Include profile nav menu items to consider for retaining position #10618
Conversation
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.
This seems reasonable to me, but I think I want @carlos-zamora and @lhecker to sign off, since they're the owner and author of the original PR
@msftbot make sure that @carlos-zamora and @lhecker sign off on this PR |
Hello @zadjii-msft! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
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.
There's another use of menuItems
on line 174. Would you mind changing this to the following?
const auto& firstItem{ menuItemsSTL.at(0) };
(Completely optional of course.)
No problem, that will work because the first item is never a profile.
@lhecker Just to make sure we're on the same line: this PR changes menuItemsSTL to menuItems and not the other way around. |
OOOOOOOH Now I got it. Damn that's embarrassing. 😄 I'm sorry. It might make sense to change |
Yes that might make sense. It might also be nicer to update instead of replace the profile navs where possible (ICONS FLICKER WHEN RELOADING). I tried that for a bit for #10609 but wasn't successful (I got some very weird behavior probably due to unfamiliarity with winrt/winui). When spamming save/discard I get 100% cpu split between menuItems.ReplaceAll and _InitializeProfilesList |
I was typing in a hurry and my point got away from me. On my machine
is probably true all things being equal, it might still be faster to append on startup because most of the menu items are already loaded by the XAML. I'll leave the PR like this (unless there are other comments of course) |
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.
Love it! Thanks for doing this!
Regarding the _InitializeProfilesList
discussion and the "icon flickering" issue, happy to take contributions for that too. If you want to do it, go for it. Otherwise, I'm down for you to open an issue so that we can track these code-health/performance things. Thanks!
Hello @lhecker! 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 (
|
@DHowett tagging these as both |
…0618) ## Summary of the Pull Request When discarding or saving settings, the current navigation should be retained. ## References Issue introduced by #10390 ## PR Checklist * [x] Closes #10617 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx ## Detailed Description of the Pull Request / Additional comments `menuItemsSTL` is filled with all _non_ profile navItems, then `menuItemsSTL` fills `menuItems`, then the profile navItems are added to `menuItems`. So to include the profile nav items in the iteration, `menuItems` needs to be used ## Validation Steps Performed Spam discard and save buttons (cherry picked from commit ef8ba20)
🎉 Handy links: |
🎉 Handy links: |
Summary of the Pull Request
When discarding or saving settings, the current navigation should be retained.
References
Issue introduced by #10390
PR Checklist
Detailed Description of the Pull Request / Additional comments
menuItemsSTL
is filled with all non profile navItems, thenmenuItemsSTL
fillsmenuItems
, then the profile navItems are added tomenuItems
. So to include the profile nav items in the iteration,menuItems
needs to be usedValidation Steps Performed
Spam discard and save buttons