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

fix not updating the nav view when add/removing profiles #15162

Merged
merged 11 commits into from
Apr 17, 2023
Merged

fix not updating the nav view when add/removing profiles #15162

merged 11 commits into from
Apr 17, 2023

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Apr 11, 2023 •
edited
Loading

  • make the list of MenuItems observable, so the nav view can actually listen for changes to it
  • Use the MenuItemsSource to find the index to add at, rather than the MenuItems (which isn't accurate anymore)
  • Stash a single observable vector as the menuitemsource, and modify that whenever we need to do modifications.
    • I attempted to create a new vector, then copy into the new one, then replace the MenuItemsSource with the new vector, but that refused to work. So let's just... not.

Regressed in #14630
Closes #15140

Manually validated that this and #13673 are still fixed

DHowett
DHowett requested changes Apr 11, 2023
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bigQs

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 11, 2023
DHowett added a commit that referenced this pull request Apr 11, 2023
@DHowett
PRE-MERGE #15162 fix not updating the nav view when add/removing prof…
dcbd790 
…iles
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 11, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-SettingsUI Anything specific to the SUI Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree. labels Apr 11, 2023
@zadjii-msft zadjii-msft requested a review from DHowett April 11, 2023 20:02
DHowett
DHowett approved these changes Apr 11, 2023
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this also fix the issue where Save/Discard lose track of what profile you're looking at?

@DHowett
Copy link
Member

DHowett commented Apr 11, 2023

uh, this is weird.

now discard/save bring you back to the Startup page.

and when I discard, my first profile disappears and is replaced with a hole:

image

DHowett
DHowett requested changes Apr 11, 2023
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

above

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 11, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 13, 2023
@zadjii-msft zadjii-msft requested a review from DHowett April 13, 2023 14:47
DHowett
DHowett reviewed Apr 13, 2023
View reviewed changes
src/cascadia/TerminalSettingsEditor/MainPage.cpp Outdated
Comment on lines 130 to 132
_menuItemSource.Clear();
// now, just stick them back in.
_menuItemSource.ReplaceAll(menuItemsSTL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReplaceAll implies Clear, which suggests the Clear is either BODGY or redundant! Which is it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here.

@DHowett
Copy link
Member

DHowett commented Apr 13, 2023

I love that I can pull the unpackaged distribution from your build and just run it as-is

DHowett
DHowett approved these changes Apr 13, 2023
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested scenarios:

  • new profile
  • discard with selection
  • save with selection
  • save json outside of app
  • delete profile
  • delete profile in json while it is selected

@DHowett
Copy link
Member

DHowett commented Apr 13, 2023

The only issue I had was that it forgot the nav view scroll position... but that's better than crashing.

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 14, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot enabled auto-merge (squash) April 14, 2023 18:52
DHowett added a commit that referenced this pull request Apr 14, 2023
@DHowett
PRE-MERGE #15162 fix not updating the nav view when add/removing prof…
0ad1104 
…iles
lhecker
lhecker reviewed Apr 17, 2023
View reviewed changes
src/cascadia/TerminalSettingsEditor/MainPage.cpp Outdated
Comment on lines 130 to 132
_menuItemSource.Clear();
// now, just stick them back in.
_menuItemSource.ReplaceAll(menuItemsSTL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here.

@lhecker lhecker removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 17, 2023
@zadjii-msft zadjii-msft merged commit 1825ca1 into main Apr 17, 2023
@zadjii-msft zadjii-msft deleted the dev/migire/b/15140-settings-add-remove branch April 17, 2023 14:54
@zadjii-msft zadjii-msft mentioned this pull request Apr 20, 2023
Closed
@zadjii-msft zadjii-msft linked an issue Apr 20, 2023 that may be closed by this pull request
{Freeway-Bug}[Settings]: Contrast ratio for 'Selected' list-item is '1.37:1' with it's background, on activating 'Save' or 'Discard changes' buttons. #15150
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DHowett DHowett DHowett approved these changes

@lhecker lhecker lhecker approved these changes

Assignees
No one assigned
Labels
Area-SettingsUI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree.
Projects
None yet
Milestone
No milestone
3 participants
@zadjii-msft @DHowett @lhecker