Make ColorScheme a WinRT object #7238

carlos-zamora · 2020-08-10T21:37:44Z

Summary of the Pull Request

ColorScheme is now a WinRT object in the TerminalApp project.

References

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

PR Checklist

Tests passed

Detailed Description of the Pull Request / Additional comments

All of the JSON stuff can't be exposed via the idl. So the plan here is that we'll have the TerminalSettingsModel project handle all of the serialization when it's moved over. These functions will be exposed off of the implementation namespace, not projected namespace.

Validation Steps Performed

Tests passed
Deployment succeeded

DHowett · 2020-08-11T00:16:18Z

src/cascadia/TerminalApp/ColorScheme.idl

+        UInt32 Foreground { get; };
+        UInt32 Background { get; };
+        UInt32 SelectionBackground { get; };
+        UInt32 CursorColor { get; };


let's not dig this hole deeper. Introduce a Color struct type.

you can specialize its conversion to/from til::color

we probably should have just gone with Windows.UI.Color - my bad

Updated ColorScheme to use Windows.UI.Color. Should I go ahead and propagate that change to ICoreSettings too? I'm already here.

src/cascadia/TerminalApp/lib/TerminalAppLib.vcxproj

src/cascadia/TerminalApp/ColorScheme.cpp

src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp

src/cascadia/ut_app/JsonTests.cpp

zadjii-msft

Okay other than this one comment, I'm okay with this

src/cascadia/TerminalApp/CascadiaSettings.h

carlos-zamora

Forgot to remove a few more TerminalSettings references

src/cascadia/TerminalApp/ColorScheme.cpp

src/cascadia/TerminalApp/ColorScheme.h

DHowett · 2020-08-12T18:13:54Z

I don't get it. All that 7a3ca00 did was rename some variables.

The responsibility to apply a color scheme to an IControlSettings lies inside TerminalApp where TerminalSettings lives. You haven't done anything to break the dependency from colorscheme to terminal control (where IControlSettings now lives)

carlos-zamora · 2020-08-12T18:26:09Z

I don't get it. All that 7a3ca00 did was rename some variables.

The responsibility to apply a color scheme to an IControlSettings lies inside TerminalApp where TerminalSettings lives. You haven't done anything to break the dependency from colorscheme to terminal control (where IControlSettings now lives)

At some point I had ColorScheme.idl referencing a TerminalSettings. That shouldn't be allowed. Making it reference an IControlSettings is better, but you're right, all of that logic should be moved to TermApp.

I was thinking of this little change being a stepping stone to #885's last step (Move the new WinRT settings objects into TerminalSettings project). We don't need the separation until then, at which point I'll also move the similar logic over for Profile.

TLDR: referencing IControlSettings > TerminalSettings

DHowett · 2020-08-12T18:28:33Z

I actually disagree that it’s better!

Eventually, the interfaces for settings will become read-only and you will need to transition this code back to using a class type that has setters on it. Moving to the interface right now is going to be a roadblock for when you inevitably have to concentrate the logic in App.

DHowett · 2020-08-12T18:29:31Z

(cf #7219)

This reverts commit 7a3ca00.

src/cascadia/TerminalApp/ColorScheme.cpp

src/cascadia/TerminalApp/ColorScheme.idl

src/cascadia/TerminalApp/GlobalAppSettings.cpp

src/cascadia/TerminalApp/TerminalPage.cpp

DHowett · 2020-08-15T00:50:31Z

src/cascadia/TerminalApp/ColorScheme.cpp

-        result[i] = _table[i];
-    }
+    winrt::com_array<Color> result{ COLOR_TABLE_SIZE };
+    std::transform(_table.begin(), _table.end(), result.begin(), [](til::color c) -> Color { return c; });


this is fine because a til::color is small enough to fit in a single register (or two registers), but it could be an expensive copy for any other struct. prefer const x& unless otherwise okay

carlos-zamora force-pushed the dev/cazamor/set/winrt-app-obj branch 2 times, most recently from c60f366 to dc958d4 Compare August 10, 2020 23:14

carlos-zamora added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Aug 10, 2020

DHowett requested changes Aug 11, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 11, 2020

zadjii-msft requested changes Aug 11, 2020

View reviewed changes

src/cascadia/TerminalApp/ColorScheme.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp Outdated Show resolved Hide resolved

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 11, 2020

carlos-zamora force-pushed the dev/cazamor/set/winrt-app-obj branch from dc958d4 to d1a30bd Compare August 11, 2020 22:44

carlos-zamora marked this pull request as ready for review August 11, 2020 22:44

carlos-zamora assigned DHowett and zadjii-msft Aug 11, 2020

DHowett reviewed Aug 11, 2020

View reviewed changes

src/cascadia/ut_app/JsonTests.cpp Outdated Show resolved Hide resolved

DHowett reviewed Aug 11, 2020

View reviewed changes

src/cascadia/ut_app/JsonTests.cpp Outdated Show resolved Hide resolved

zadjii-msft requested changes Aug 12, 2020

View reviewed changes

src/cascadia/TerminalApp/CascadiaSettings.h Outdated Show resolved Hide resolved

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 12, 2020

zadjii-msft assigned carlos-zamora and unassigned zadjii-msft Aug 12, 2020

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 12, 2020

carlos-zamora commented Aug 12, 2020

View reviewed changes

src/cascadia/TerminalApp/ColorScheme.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/ColorScheme.h Show resolved Hide resolved

carlos-zamora assigned zadjii-msft and unassigned carlos-zamora Aug 12, 2020

carlos-zamora added 3 commits August 13, 2020 14:40

Make ColorScheme a WinRT object

a44ef99

fix build

8417c9d

fix tests

7c42199

carlos-zamora added 4 commits August 13, 2020 14:40

fix filter

27c6207

remove TerminalSettings dependency

d9cdc19

Revert "remove TerminalSettings dependency"

3f712e7

This reverts commit 7a3ca00.

fix test + PR comment

1fb018d

carlos-zamora force-pushed the dev/cazamor/set/winrt-app-obj branch from 07dfdb7 to 1fb018d Compare August 13, 2020 21:41

zadjii-msft approved these changes Aug 14, 2020

View reviewed changes

zadjii-msft removed their assignment Aug 14, 2020

DHowett reviewed Aug 14, 2020

View reviewed changes

src/cascadia/TerminalApp/ColorScheme.cpp Outdated Show resolved Hide resolved

DHowett requested changes Aug 14, 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 Aug 14, 2020

carlos-zamora added 2 commits August 14, 2020 12:09

PR Comments

38760fe

one less wstring<-->hstring

c2518b5

DHowett reviewed Aug 15, 2020

View reviewed changes

DHowett approved these changes Aug 15, 2020

View reviewed changes

DHowett merged commit e9a7053 into master Aug 15, 2020

DHowett deleted the dev/cazamor/set/winrt-app-obj branch August 15, 2020 00:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ColorScheme a WinRT object #7238

Make ColorScheme a WinRT object #7238

carlos-zamora commented Aug 10, 2020 •

edited

Loading

DHowett Aug 11, 2020

DHowett Aug 11, 2020

zadjii-msft Aug 11, 2020

carlos-zamora Aug 11, 2020

zadjii-msft left a comment

carlos-zamora left a comment

DHowett commented Aug 12, 2020

carlos-zamora commented Aug 12, 2020

DHowett commented Aug 12, 2020

DHowett commented Aug 12, 2020

DHowett Aug 15, 2020

Make ColorScheme a WinRT object #7238

Make ColorScheme a WinRT object #7238

Conversation

carlos-zamora commented Aug 10, 2020 • edited Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

DHowett Aug 11, 2020

Choose a reason for hiding this comment

DHowett Aug 11, 2020

Choose a reason for hiding this comment

zadjii-msft Aug 11, 2020

Choose a reason for hiding this comment

carlos-zamora Aug 11, 2020

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

DHowett commented Aug 12, 2020

carlos-zamora commented Aug 12, 2020

DHowett commented Aug 12, 2020

DHowett commented Aug 12, 2020

DHowett Aug 15, 2020

Choose a reason for hiding this comment

carlos-zamora commented Aug 10, 2020 •

edited

Loading