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

Implement CascadiaSettings::Copy() #7877

Merged
merged 5 commits into from
Oct 16, 2020
Merged

Implement CascadiaSettings::Copy() #7877

merged 5 commits into from
Oct 16, 2020

Conversation

carlos-zamora
Copy link
Member

Summary of the Pull Request

This implements the Copy function for CascadiaSettings. Copy performs a deep copy of a CascadiaSettings object. This is needed for data binding in the Terminal Settings Editor.

The Copy function was basically implemented in every settings model object. This was mostly just repetitive work.

References

#7667 - TSM
#1564 - Settings UI

PR Checklist

  • Tests added/passed

@carlos-zamora carlos-zamora added Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. labels Oct 10, 2020
@zadjii-msft zadjii-msft self-assigned this Oct 15, 2020
zadjii-msft
zadjii-msft approved these changes Oct 15, 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.

It's a shame that this doesn't work well with a macro for auto-synthesizing these Copy methods. I highly expect either a merge conflict with this PR causing a property to not get copied, or for a future PR to forget to Copy a property

src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/Command.cpp Outdated Show resolved Hide resolved
leonMSFT
leonMSFT approved these changes Oct 15, 2020
View reviewed changes
Copy link
Contributor

@leonMSFT leonMSFT left a comment

Choose a reason for hiding this comment

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

Seems pretty straightforward, do you think you could make a small test to test out the whole SUI scenario of making a copy, making some changes, and then replacing the original? Then check that the settings you've changed are what you expect them to be in the original copy.

@carlos-zamora carlos-zamora mentioned this pull request Oct 16, 2020
Merged
DHowett
DHowett requested changes Oct 16, 2020
View reviewed changes
src/cascadia/TerminalSettingsModel/ActionArgs.h Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/Command.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp Outdated Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 16, 2020
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 16, 2020
@carlos-zamora carlos-zamora assigned DHowett and unassigned zadjii-msft Oct 16, 2020
@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 16, 2020
@ghost
Copy link

ghost commented Oct 16, 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.

@carlos-zamora carlos-zamora merged commit 9045266 into master Oct 16, 2020
@carlos-zamora carlos-zamora deleted the dev/cazamor/tsm/copy branch October 16, 2020 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PankajBhojwani PankajBhojwani PankajBhojwani left review comments

@leonMSFT leonMSFT leonMSFT approved these changes

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

@DHowett DHowett DHowett approved these changes

Assignees
No one assigned
Labels
Area-Settings Issues related to settings and customizability, for console or terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@carlos-zamora @DHowett @zadjii-msft @PankajBhojwani @leonMSFT