Add an Appearances xaml object and AppearanceViewModel to TSE #10066

PankajBhojwani · 2021-05-10T07:45:04Z

Summary of the Pull Request

Implements an Appearances xaml object and an AppearanceViewModel in the SettingsEditor project. Updates Profiles to use these new objects for its default appearance.

This is the first step towards getting UnfocusedAppearance into the SUI.

PankajBhojwani · 2021-05-11T07:52:05Z

src/cascadia/TerminalSettingsEditor/Appearances.h

+        // These settings are not defined in AppearanceConfig, so we grab them
+        // from the source profile itself. The reason we still want them in the
+        // AppearanceViewModel is so we can continue to have the 'Text' grouping
+        // we currently have in xaml, since that grouping has some settings that
+        // are defined in AppearanceConfig and some that are not.
+        OBSERVABLE_PROJECTED_SETTING(_appearance.SourceProfile(), FontFace);
+        OBSERVABLE_PROJECTED_SETTING(_appearance.SourceProfile(), FontSize);
+        OBSERVABLE_PROJECTED_SETTING(_appearance.SourceProfile(), FontWeight);


When we add unfocused appearance to the SUI, we will set a bool on these settings in xaml to only be shown for the default appearance (we do not want to add these settings to AppearanceConfig because we had concerns about causing a resize when transitioning between focused/unfocused states)

That's fair. Pretty straightforward too. We'll just make their visibility depend on some bool in the view model. Kinda like how we used IsBaseLayer to force all of the settings to always appear.

PankajBhojwani · 2021-05-11T08:00:12Z

@msftbot make sure @carlos-zamora signs off on this.

ghost · 2021-05-11T08:00:19Z

Hello @PankajBhojwani!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @carlos-zamora

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

carlos-zamora

Looks good! Mostly just shuffling things around to introduce the AppearanceViewModel. I have a few nits, but nothing major.

carlos-zamora · 2021-05-11T21:49:15Z

src/cascadia/TerminalSettingsEditor/Appearances.idl

+        Windows.Foundation.Collections.IObservableVector<Font> CompleteFontList { get; };
+        Windows.Foundation.Collections.IObservableVector<Font> MonospaceFontList { get; };


Can we keep these two on Profiles? We'll be using the same font lists across all appearance configs so maybe we can just reuse them. Ideally, we could reuse the same list across all of the profile pages but that's definitely out of scope here haha.

Hm, we would need to give AppearanceViewModel a reference to the ProfileViewModel then...

IIRC MainPage passes in some sort of "state" to every new page it navigates to right? Perhaps the font lists (or maybe even some sort of font list manager obj) could be passed into any new profile that's navigated to? LMK if I'm missing a part of the puzzle tho 😄

Ah the ProfileViewModel already had the font lists, the issue here is how the Appearances or AppearanceViewModel can get access to the font lists in ProfileViewModel.

Just made a change to give the Appearances object a ref to the ProfileViewModel, so it grabs the font lists from there now!

Yup! MainPage can totally pass in the font lists via the NavigationState. And that's probably the architecture we'll want. That way, we'll only ever generate the font lists once, and we can reuse them across each Profiles page.

The only reason I think we should hold off on that change rn is I think the right approach would be to lazy load them. Like, I'm running into a similar problem with the Actions page right now. If we had MainPage load up all of the actions and all of the fonts when you open the SUI, it'll take forever to open it.

~~Mind filing a task so that we do this later (probably in the v2.0 timeframe)?~~ I think it's worth doing, just not in this PR. I've added this as a part of #9207.

I do not think we should continue using NavigationState as a bad viewmodel. We need a viewmodel. Navigation state objects are supposed to get temporary information into a page that is necessary TO NAVIGATE. If we have a real viewmodel, we don't even need a navigation state class.

src/cascadia/TerminalSettingsEditor/Appearances.h

carlos-zamora · 2021-05-11T21:52:52Z

src/cascadia/TerminalSettingsEditor/Appearances.h

+        // These settings are not defined in AppearanceConfig, so we grab them
+        // from the source profile itself. The reason we still want them in the
+        // AppearanceViewModel is so we can continue to have the 'Text' grouping
+        // we currently have in xaml, since that grouping has some settings that
+        // are defined in AppearanceConfig and some that are not.
+        OBSERVABLE_PROJECTED_SETTING(_appearance.SourceProfile(), FontFace);
+        OBSERVABLE_PROJECTED_SETTING(_appearance.SourceProfile(), FontSize);
+        OBSERVABLE_PROJECTED_SETTING(_appearance.SourceProfile(), FontWeight);


That's fair. Pretty straightforward too. We'll just make their visibility depend on some bool in the view model. Kinda like how we used IsBaseLayer to force all of the settings to always appear.

src/cascadia/TerminalSettingsEditor/Appearances.cpp

carlos-zamora

You're close. Just move those two properties into the AppearanceViewModel and we'll be good.

src/cascadia/TerminalSettingsEditor/Appearances.idl

src/cascadia/TerminalSettingsEditor/Appearances.cpp

DHowett

pretty straightforward all things considered! just a big nit.

src/cascadia/TerminalSettingsEditor/Appearances.xaml

src/cascadia/TerminalSettingsEditor/Appearances.h

…/pabhoj/sui_appearances

PankajBhojwani · 2021-06-18T18:07:52Z

As #10317 targets this, please don't automerge this!

PankajBhojwani added 13 commits April 30, 2021 06:48

initial

b923b4b

hm

85a6392

cursor shape/height mostly work

d750d56

remove navigated to

ad35c70

Color scheme ish

5920408

background image stuff ish

7eea902

bg image

f5078b8

Remove can delete appearance for now

f6cf9dd

format

0800d64

dependency callback

81a8b8b

font stuff

f179248

format

c8ee89f

comment

62db2fa

PankajBhojwani commented May 11, 2021

View reviewed changes

deduplicate file picker

5f047a1

PankajBhojwani marked this pull request as ready for review May 11, 2021 07:58

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label May 11, 2021

PankajBhojwani removed the AutoMerge Marked for automatic merge by the bot when requirements are met label May 11, 2021

Remove color scheme from profiles

6c7d997

carlos-zamora approved these changes May 11, 2021

View reviewed changes

PankajBhojwani added 3 commits May 11, 2021 23:17

abstract

3c3fede

font lists back in profile, ref to profile

05666f2

format

ef71e80

carlos-zamora requested changes May 14, 2021

View reviewed changes

src/cascadia/TerminalSettingsEditor/Appearances.idl Show resolved Hide resolved

src/cascadia/TerminalSettingsEditor/Appearances.cpp Outdated Show resolved Hide resolved

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 May 14, 2021

conflict

3dca0dd

carlos-zamora approved these changes May 17, 2021

View reviewed changes

PankajBhojwani added 2 commits May 20, 2021 21:55

conflict, updates from merge main

e384b00

format, rearrange

8e604ba

This comment has been minimized.

Sign in to view

PankajBhojwani added the Needs-Second It's a PR that needs another sign-off label Jun 1, 2021

ghost requested review from zadjii-msft, miniksa and lhecker June 1, 2021 16:36

conflict

9ac1407

This comment has been minimized.

Sign in to view

PankajBhojwani mentioned this pull request Jun 3, 2021

Allow creating and editing unfocused appearances in the SUI #10317

Merged

6 tasks

% sign changes

901d4bb

DHowett requested changes Jun 16, 2021

View reviewed changes

src/cascadia/TerminalSettingsEditor/Appearances.xaml Show resolved Hide resolved

src/cascadia/TerminalSettingsEditor/Appearances.h Outdated Show resolved Hide resolved

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 Jun 16, 2021

PankajBhojwani added 3 commits June 18, 2021 09:06

Merge branch 'main' of https://github.com/microsoft/terminal into dev…

7e0264d

…/pabhoj/sui_appearances

move file/image picker to utils.h/cpp

f5b5bb9

tabstop false

fbfdb2d

format

6177c26

PankajBhojwani requested a review from DHowett June 28, 2021 17:32

PankajBhojwani assigned DHowett Jun 28, 2021

PankajBhojwani added 4 commits July 1, 2021 16:01

conflict

d7290a0

vcxproj conflict

c089c20

fix

bbaf0ec

clinclude

b3df4af

DHowett approved these changes Jul 9, 2021

View reviewed changes

DHowett changed the title ~~Add an Appearances xaml object and an AppearanceViewModel to SettingsEditor~~ Add an Appearances xaml object and AppearanceViewModel to TSE Jul 9, 2021

DHowett merged commit c18e0f5 into main Jul 9, 2021

DHowett deleted the dev/pabhoj/sui_appearances branch July 9, 2021 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an Appearances xaml object and AppearanceViewModel to TSE #10066

Add an Appearances xaml object and AppearanceViewModel to TSE #10066

PankajBhojwani commented May 10, 2021 •

edited by DHowett

Loading

PankajBhojwani May 11, 2021

carlos-zamora May 11, 2021

PankajBhojwani commented May 11, 2021

ghost commented May 11, 2021

carlos-zamora left a comment

carlos-zamora May 11, 2021

PankajBhojwani May 12, 2021

leonMSFT May 13, 2021

PankajBhojwani May 14, 2021

carlos-zamora May 14, 2021 •

edited

Loading

DHowett May 14, 2021

carlos-zamora May 11, 2021

carlos-zamora left a comment

This comment has been minimized.

This comment has been minimized.

DHowett left a comment

PankajBhojwani commented Jun 18, 2021

		Windows.Foundation.Collections.IObservableVector<Font> CompleteFontList { get; };
		Windows.Foundation.Collections.IObservableVector<Font> MonospaceFontList { get; };

Add an Appearances xaml object and AppearanceViewModel to TSE #10066

Add an Appearances xaml object and AppearanceViewModel to TSE #10066

Conversation

PankajBhojwani commented May 10, 2021 • edited by DHowett Loading

Summary of the Pull Request

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PankajBhojwani commented May 11, 2021

ghost commented May 11, 2021

carlos-zamora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora May 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

DHowett left a comment

Choose a reason for hiding this comment

PankajBhojwani commented Jun 18, 2021

PankajBhojwani commented May 10, 2021 •

edited by DHowett

Loading

carlos-zamora May 14, 2021 •

edited

Loading