Skip to content

Hook up the Save and Reset buttons#8348

Merged
6 commits merged intofeature/settings-uifrom
dev/cazamor/sui/save-button
Dec 1, 2020
Merged

Hook up the Save and Reset buttons#8348
6 commits merged intofeature/settings-uifrom
dev/cazamor/sui/save-button

Conversation

@carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Nov 20, 2020 •
edited
Loading

Summary of the Pull Request

Adds an event handler for the Save and Reset buttons. "Save" writes the settings to disk using the API introduced by #8018. "Reset" simply overwrites the settingsClone (what the Settings UI reads from) with settingsSource (provided by TermApp on Settings UI initialization).

References

#1564 - Settings UI

Validation Steps Performed

The following scenarios were tested and are verified to work properly:

  • Change default profile in SUI, then save/reset
  • Hide a profile, then save/reset
  • Modifying the settings.json directly propagates changes to SUI
  • "Reset" updates the current page

@carlos-zamora carlos-zamora marked this pull request as ready for review November 23, 2020 19:16
@carlos-zamora
Copy link
Member Author

carlos-zamora commented Nov 30, 2020

g, don't you need to init the profile list here? don't make a copy of this logic, unify it with the logic on lines 41-47 😬

@DHowett somehow you looked at the commit that isn't HEAD haha. The logic for refreshing the list of profiles was added in 23f08e8. I don't see how I can consolidate that with the constructor though. UpdateSettings needs to remove the list of profiles, then initialize it again. And the constructor has that InitializeComponent that I'm guessing we don't want to call in UpdateSettings.

zadjii-msft
zadjii-msft reviewed Dec 1, 2020
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

I'd sign off if not for the one UnparsedDefaultProfile question

@zadjii-msft zadjii-msft added the Area-SettingsUI Anything specific to the SUI label Dec 1, 2020
@zadjii-msft zadjii-msft mentioned this pull request Dec 1, 2020
Open
34 tasks
@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 1, 2020
@ghost
Copy link

ghost commented Dec 1, 2020

Hello @carlos-zamora!

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.

@ghost ghost merged commit 2f747a7 into feature/settings-ui Dec 1, 2020
@ghost ghost deleted the dev/cazamor/sui/save-button branch December 1, 2020 21:06
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zadjii-msft zadjii-msft zadjii-msft approved these changes

Assignees

@zadjii-msft zadjii-msft

Labels

Area-SettingsUI Anything specific to the SUI AutoMerge Marked for automatic merge by the bot when requirements are met

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@carlos-zamora @DHowett @zadjii-msft