Include profile nav menu items to consider for retaining position #10618

mimvdb · 2021-07-11T20:25:26Z

Summary of the Pull Request

When discarding or saving settings, the current navigation should be retained.

References

Issue introduced by #10390

PR Checklist

Closes Settings UI: Current nav item is reset when discarding/saving on a profile nav item #10617
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Documentation updated. If checked, please file a pull request on our docs repo 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

zadjii-msft

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

zadjii-msft · 2021-07-12T13:44:35Z

@msftbot make sure that @carlos-zamora and @lhecker sign off on this PR

ghost · 2021-07-12T13:44:39Z

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:

I'll only merge this pull request if it's approved by @carlos-zamora and @lhecker

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".

lhecker

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.)

mimvdb · 2021-07-12T16:05:25Z

Would you mind changing this to the following?

No problem, that will work because the first item is never a profile.

There's another use

@lhecker Just to make sure we're on the same line: this PR changes menuItemsSTL to menuItems and not the other way around.

lhecker · 2021-07-12T16:15:09Z

OOOOOOOH Now I got it. Damn that's embarrassing. 😄 I'm sorry.

It might make sense to change _InitializeProfilesList to not modify the menuItems list directly in the future to make this more obvious. It's faster anyways to bulk-read a COM vector into a STL container, modify the container and bulk-write it back into the COM vector.

mimvdb · 2021-07-12T16:25:45Z

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

mimvdb · 2021-07-12T17:50:43Z

I was typing in a hurry and my point got away from me. On my machine menuItems.ReplaceAll took longer than _InitializeProfilesList (though only slightly). So while

It's faster anyways to bulk-read a COM vector into a STL container, modify the container and bulk-write it back into the COM vector

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)

carlos-zamora

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!

ghost · 2021-07-12T18:16:35Z

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.

carlos-zamora · 2021-07-12T18:19:30Z

@DHowett tagging these as both XXX-Service-Consider. I'm a bit confused as to which branch/release this change should target because of the new til::feature workflow, so sorry if you can't actually port this to 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)

ghost · 2021-07-14T17:01:11Z

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

Handy links:

ghost · 2021-07-14T17:05:29Z

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

Handy links:

Include profile nav menu items to consider for retaining position

e31a3fb

zadjii-msft reviewed Jul 12, 2021

View reviewed changes

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 12, 2021

zadjii-msft added the Area-Settings UI Anything specific to the SUI label Jul 12, 2021

lhecker approved these changes Jul 12, 2021

View reviewed changes

Use menuItemsSTL vector where possible to reduce overhead

232b607

ghost removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 12, 2021

carlos-zamora approved these changes Jul 12, 2021

View reviewed changes

carlos-zamora added the Product-Terminal The new Windows Terminal. label Jul 12, 2021

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

carlos-zamora added zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. labels Jul 12, 2021

ghost added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Jul 12, 2021

lhecker merged commit ef8ba20 into microsoft:main Jul 12, 2021

ghost mentioned this pull request Jul 14, 2021

Settings UI: Current nav item is reset when discarding/saving on a profile nav item #10617

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include profile nav menu items to consider for retaining position #10618

Include profile nav menu items to consider for retaining position #10618

mimvdb commented Jul 11, 2021 •

edited by lhecker

Loading

zadjii-msft left a comment

zadjii-msft commented Jul 12, 2021

ghost commented Jul 12, 2021

lhecker left a comment

mimvdb commented Jul 12, 2021

lhecker commented Jul 12, 2021

mimvdb commented Jul 12, 2021

mimvdb commented Jul 12, 2021

carlos-zamora left a comment

ghost commented Jul 12, 2021

carlos-zamora commented Jul 12, 2021

ghost commented Jul 14, 2021

ghost commented Jul 14, 2021

Include profile nav menu items to consider for retaining position #10618

Include profile nav menu items to consider for retaining position #10618

Conversation

mimvdb commented Jul 11, 2021 • edited by lhecker Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft commented Jul 12, 2021

ghost commented Jul 12, 2021

lhecker left a comment

Choose a reason for hiding this comment

mimvdb commented Jul 12, 2021

lhecker commented Jul 12, 2021

mimvdb commented Jul 12, 2021

mimvdb commented Jul 12, 2021

carlos-zamora left a comment

Choose a reason for hiding this comment

ghost commented Jul 12, 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.

carlos-zamora commented Jul 12, 2021

ghost commented Jul 14, 2021

ghost commented Jul 14, 2021

mimvdb commented Jul 11, 2021 •

edited by lhecker

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.