Make CascadiaSettings a WinRT object #7457

carlos-zamora · 2020-08-28T22:51:20Z

CascadiaSettings is now a WinRT object in the TerminalApp project.

References

#7141 - CascadiaSettings is a settings object
#885 - this new settings object will be moved to a new TerminalSettingsModel project

This one looks big, but most of it is really just propagating the
changes to the tests. In fact, you can probably save yourself some time
because the tests were about an hour of Find&Replace.

CascadiaSettings::GetCurrentAppSettings() was only being used in
Pane.cpp. So I ripped out the 3 lines of code and stuffed them in there.

Follow-up work:

There's a few places in AppLogic where I get_self to be able to get
the warnings out. This will go away in the next PR (wrapping up Replace TerminalSettings runtimeclasses with interfaces only #885)

Validation Steps Performed

Tests passed
Deployment succeeded

Closes #7141

DHowett

15/17

src/cascadia/TerminalApp/TerminalPage.cpp

DHowett · 2020-09-04T20:55:03Z

src/cascadia/TerminalApp/CascadiaSettings.cpp

            if (equals(profileGuid, guid))
            {
-                std::iter_swap(_profiles.begin() + pIndex, _profiles.begin() + gIndex);
+                auto prof1 = _profiles.GetAt(pIndex);


i bet there's a way to use iterators here ;P

So, the problem is more the +pIndex and +gIndex. When I do this:

std::iter_swap(_profiles.First() + pIndex, _profiles.First() + gIndex);

there isn't a + operator found :(

Ugh, there's a bunch of iterator helpers that are private. Bah.

src/cascadia/TerminalApp/CascadiaSettings.cpp

src/cascadia/TerminalApp/CascadiaSettings.h

src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp

miniksa · 2020-09-09T17:38:50Z

src/cascadia/TerminalApp/CascadiaSettings.cpp

-                                   _profiles.end(),
-                                   [](auto&& profile) { return profile.Hidden(); }),
-                    _profiles.end());
+    for (uint32_t i{}; i < _profiles.Size();)


Oh, please just assign 0 to i. I know the {} will technically default construct it to 0, but that's so much more obtuse than the thing everyone recognizes.

ha! you know, i thought you were part of our discussions about zero-initializing things with {}, and perhaps with the ayes and not the nays

miniksa

Looks like it does what it says on the box. I'd prefer to have Dustin also sign, though, as he has a bit more WinRT experience and sees things I don't.

DHowett · 2020-09-09T20:45:42Z

src/cascadia/TerminalApp/AppLogic.cpp

@@ -616,9 +629,9 @@ namespace winrt::TerminalApp::implementation
        try
        {
            auto newSettings = _isUwp ? CascadiaSettings::LoadUniversal() : CascadiaSettings::LoadAll();
-            _settings.copy_from(winrt::get_self<CascadiaSettings>(newSettings));
+            _settings = newSettings;


to save eight nanoseconds you could do uh

Suggested change

_settings = newSettings;

_settings = std::move(newSettings);

or just _settings = isUwp ? xxx : yyy

ghost · 2020-09-09T20:49:29Z

Hello @DHowett!

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 · 2020-09-22T16:01:54Z

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

Handy links:

ghost added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Aug 28, 2020

This comment has been minimized.

Sign in to view

carlos-zamora force-pushed the dev/cazamor/set/winrt-app-obj-top branch from 7d78e47 to 700290e Compare August 28, 2020 22:53

carlos-zamora mentioned this pull request Sep 4, 2020

Introduce KeyMapping and Move TerminalSettings construction #7537

Merged

2 tasks

DHowett requested changes Sep 4, 2020

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Sep 4, 2020

This comment has been minimized.

Sign in to view

carlos-zamora mentioned this pull request Sep 9, 2020

Add serialization error handling to settings projection layer #7576

Merged

2 tasks

carlos-zamora and others added 3 commits September 9, 2020 10:03

Make CascadiaSettings a WinRT object

41e5cfa

Make Profiles an observable vector

2471885

use projected type + other PR comments

8ebefd6

miniksa reviewed Sep 9, 2020

View reviewed changes

fix Jumplist

c3d3f93

miniksa approved these changes Sep 9, 2020

View reviewed changes

PR comments - michael

1885822

carlos-zamora force-pushed the dev/cazamor/set/winrt-app-obj-top branch from 641e1c7 to 1885822 Compare September 9, 2020 18:04

fix tests

87865d1

carlos-zamora assigned DHowett Sep 9, 2020

DHowett reviewed Sep 9, 2020

View reviewed changes

DHowett approved these changes Sep 9, 2020

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 9, 2020

ghost merged commit c5cf7b8 into master Sep 9, 2020

ghost deleted the dev/cazamor/set/winrt-app-obj-top branch September 9, 2020 20:49

carlos-zamora restored the dev/cazamor/set/winrt-app-obj-top branch September 9, 2020 20:50

ghost mentioned this pull request Sep 22, 2020

Transform TerminalApp settings objects into WinRT objects #7141

Closed

DHowett deleted the dev/cazamor/set/winrt-app-obj-top branch February 5, 2021 19:07

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make CascadiaSettings a WinRT object #7457

Make CascadiaSettings a WinRT object #7457

carlos-zamora commented Aug 28, 2020 •

edited by DHowett

Loading

This comment has been minimized.

DHowett left a comment

DHowett Sep 4, 2020

carlos-zamora Sep 8, 2020

DHowett Sep 8, 2020

This comment has been minimized.

miniksa Sep 9, 2020

DHowett Sep 9, 2020

miniksa left a comment

DHowett Sep 9, 2020

ghost commented Sep 9, 2020

ghost commented Sep 22, 2020

Make CascadiaSettings a WinRT object #7457

Make CascadiaSettings a WinRT object #7457

Conversation

carlos-zamora commented Aug 28, 2020 • edited by DHowett Loading

References

Validation Steps Performed

This comment has been minimized.

DHowett left a comment

Choose a reason for hiding this comment

DHowett Sep 4, 2020

Choose a reason for hiding this comment

carlos-zamora Sep 8, 2020

Choose a reason for hiding this comment

DHowett Sep 8, 2020

Choose a reason for hiding this comment

This comment has been minimized.

miniksa Sep 9, 2020

Choose a reason for hiding this comment

DHowett Sep 9, 2020

Choose a reason for hiding this comment

miniksa left a comment

Choose a reason for hiding this comment

DHowett Sep 9, 2020

Choose a reason for hiding this comment

ghost commented Sep 9, 2020

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 commented Sep 22, 2020

carlos-zamora commented Aug 28, 2020 •

edited by DHowett

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.