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.

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

Spec: Appearance configuration objects for profiles #8345

Merged
19 commits merged into from
Feb 6, 2021

Conversation

PankajBhojwani
Copy link
Contributor

Summary of the Pull Request

Spec for #3062

PR Checklist

  • Closes #xxx
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Is documentation
  • Schema updated.
  • I work here

Detailed Description of the Pull Request / Additional comments

Read the spec

@PankajBhojwani PankajBhojwani changed the title [DRAFT] Spec: Configuration objects for focused/unfocused states [DRAFT] Spec: Configuration objects for profiles Nov 23, 2020
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

My main concerns are really...

  • How does this interact with other states we'll eventually introduce?
  • How does this work with profiles.defaults

doc/specs/Configuration object for profiles.md Outdated Show resolved Hide resolved
Comment on lines 15 to 16
example is that an elevated state control can be rendered differently as compared to a
non-elevated one.
Copy link
Member

Choose a reason for hiding this comment

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

How does your design scale as we add new states? Consider the following profile:

{
  "fontFace": "Cascadia Code",
  "state.unfocused" : {
    "fontFace": "Cascadia Mono"
  },
  "state.admin" : {
    "fontFace": "Consolas"
  }
}

What is the font face in the following scenarios:

Unfocused? Admin? Font
Cascadia Code
Consolas
Cascadia Mono
???

Copy link
Member

Choose a reason for hiding this comment

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

This is the single thing I'm the most worried about with this spec, but still don't have any good ideas. It doesn't make sense to have appearance.unfocused, appearance.elevated, appearance.unfocusedElevated, appearance.foo, appearance.unfocusedFoo, appearance.unfocusedElevatedFoo, etc. That pretty immediately doesn't scale.

Copy link
Member

Choose a reason for hiding this comment

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

Unless we just do something like "we'll apply appearance's in order, on top of whatever the default appearance is". So we'll always start with the focused (profile) appearance. Then if we're admin, we'll apply that on top. Then if we're focused, we'll apply that on top. When the control is unfocused, it'll go back and apply the default appearance + the elevated appearance.

Then, if we ever have a foo state, it could be inserted into that list in some order s.t. runtime appearance = elevated + foo + unfocused.

Pretty sure this doesn't make sense, but that's my idea


### Allowed parameters

Not all parameters which can be defined in a `Profile` can be defined in this new object (for example, we
Copy link
Member

Choose a reason for hiding this comment

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

useAcrylic and acrylicOpacity should be allowed (conceptually), but aren't because of a something out of our control (at least for now). Should we specifically throw a warning when somebody tries to set these in the "unfocusedState" object?

is the way we already pipe over information that these interfaces need to know regarding rendering (such as
`CursorStyle` and `FontFace`).

### Allowed parameters
Copy link
Member

Choose a reason for hiding this comment

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

So, when "adminState" gets added, we'll have a larger set of accepted parameters. For example...

  • name, tabTitle, suppressApplicationTitle --> what is my title?
  • startingDirectory and commandline --> startup-related settings

Should we just accept every profile setting, then silently ignore ones that don't apply?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking that at least for now, these additional state configurations should be entirely appearance-based, so we don't accept things like commandline, startingDirectory and probably nothing to do with the title either.

doc/specs/Configuration object for profiles.md Outdated Show resolved Hide resolved
doc/specs/Configuration object for profiles.md Outdated Show resolved Hide resolved
doc/specs/Configuration object for profiles.md 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 Nov 25, 2020
@zadjii-msft zadjii-msft self-assigned this Nov 30, 2020
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 30, 2020
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.

For purely selfish reasons, I'm mostly concerned about the unfocused/admin/foo appearance problem, so I want to make sure we have a plan for that.

doc/specs/Configuration object for profiles.md Outdated Show resolved Hide resolved
Comment on lines 15 to 16
example is that an elevated state control can be rendered differently as compared to a
non-elevated one.
Copy link
Member

Choose a reason for hiding this comment

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

This is the single thing I'm the most worried about with this spec, but still don't have any good ideas. It doesn't make sense to have appearance.unfocused, appearance.elevated, appearance.unfocusedElevated, appearance.foo, appearance.unfocusedFoo, appearance.unfocusedElevatedFoo, etc. That pretty immediately doesn't scale.

Comment on lines 15 to 16
example is that an elevated state control can be rendered differently as compared to a
non-elevated one.
Copy link
Member

Choose a reason for hiding this comment

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

Unless we just do something like "we'll apply appearance's in order, on top of whatever the default appearance is". So we'll always start with the focused (profile) appearance. Then if we're admin, we'll apply that on top. Then if we're focused, we'll apply that on top. When the control is unfocused, it'll go back and apply the default appearance + the elevated appearance.

Then, if we ever have a foo state, it could be inserted into that list in some order s.t. runtime appearance = elevated + foo + unfocused.

Pretty sure this doesn't make sense, but that's my idea

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 3, 2020
@microsoft microsoft deleted a comment Dec 3, 2020
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 4, 2020
@PankajBhojwani PankajBhojwani changed the title [DRAFT] Spec: Configuration objects for profiles Spec: Appearance configuration objects for profiles Dec 4, 2020
@PankajBhojwani PankajBhojwani marked this pull request as ready for review December 4, 2020 22:47
@zadjii-msft
Copy link
Member

Alright so I've thought on this for the weekend, and had a thought. I know that I was the one really pushing for the elevated config states, but this whole proposal would be like, 1000% simpler if we didn't concern ourselves with that. We couldn't find prior art for this, so maybe it's okay to just not do it?

We'll have the shield we're adding for #1939 to indicate a window is elevated. Maybe that's good enough for now. If we don't bother with the combinatorial logic of having multiple states, and only worry about unfocusedAppearance, then this whole spec is really straightforward.

Maybe I can loop back on having a separate elevated appearance post-#4000. Someone else mentioned doing this as an extension and I think I'm coming around on that idea - then we can make the logic whatever silly thing we want, and not have it officially supported, since it'll be coming from @zadjii not @zadjii-msft.

Thoughts?

@PankajBhojwani
Copy link
Contributor Author

Maybe I can loop back on having a separate elevated appearance post-#4000.

I like this idea of coming back to the 'elevated appearance' later - hopefully by then we have some idea of whether there are other states that users really want.

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.

Okay I'm not blocking this anymore, I just wanted a clarification.

the values from the profile itself. If the profile does not define an `unfocusedConfig`, then the global/default `unfocusedConfig` is used
for this profile.

Thus, if a user wishes for the unfocused state to look the same as the focused state for a particular profile,
Copy link
Member

Choose a reason for hiding this comment

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

Okay, so just to be sure, the ordering is

profile.unfocusedState or
profiles.defaults.unfocusedState or
profile.appearance or
profile.defaults.appearance

(where profile.appearance is just the appearance of the profile itself)

and we just return whatever the first non-null one is? And if the profile.unfocusedState is {}, we'll just use profile.appearance?

Copy link
Contributor Author

@PankajBhojwani PankajBhojwani Dec 11, 2020

Choose a reason for hiding this comment

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

Yes, though there is also the question of what we do about the undefined parameters within the unfocused config object, so I prefer to think of it as:

  • profile.unfocusedConfig defined: undefined params taken from profile (so the case where profile.unfocusedConfig is {} falls in here)
  • profile.unfocusedConfig not defined, profile.defaults.unfocusedConfig defined: undefined params are default values
  • both not defined: do nothing (so it just keeps the profile's appearance when unfocused)

Copy link
Member

Choose a reason for hiding this comment

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

OKay I think case 2 is weird, but it's well defined at least. I guess I'd expect undefined params in that case to use the profile ones still, but I understand how (from a layering perspective), that's just impossible. I think it'd be weird to throw an unfocused config in the defaults...

okay though what if I wanted all my profiles to be gray when they're unfocused. I'd throw a unfocusedConfig: { scheme: gray } in the defaults, and be happy. But then the rest of the unfocused config is coming from the default settings, not the profile's settings.

Like yea that makes sense, it's just weird

Copy link
Member

Choose a reason for hiding this comment

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

nit: I would add a small comment somewhere explaining why we're doing this "all-or-nothing" approach as opposed to the "per-setting" approach.

Copy link
Contributor Author

@PankajBhojwani PankajBhojwani Dec 14, 2020

Choose a reason for hiding this comment

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

Like yea that makes sense, it's just weird

Yeah but its a clean way given we don't want to define our own default unfocusedConfig and we don't want to mess up when no unfocusedConfig is defined anywhere in the file

nit: I would add a small comment somewhere explaining why we're doing this "all-or-nothing" approach as opposed to the "per-setting" approach.

Sounds good, will do

doc/specs/Configuration object for profiles.md Outdated Show resolved Hide resolved
sure that switching to an inactive tab (and so causing the renderer to update with the 'normal' parameters)
does not cause the window to flash/show a jarring indicator that the rendering values changed.

When certain appearance settings are changed via OSC sequences (such as the background color), only the focused/regular
Copy link
Member

Choose a reason for hiding this comment

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

I think there's a issue laying around about "make all the settings properties getters-only", and that touches on this subject. The settings should really only be read-only, but TermControl is kinda also using them as the bucket for it's runtime appearance as well. So it needs to kinda be applied as

runtimeAppearance 
	+ (unfocused? unfocusedAppearance : defaultAppearance)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to clarify, you're saying that if we get an OSC sequence to change some appearance value, we should apply it to the unfocused appearance if the control is not focused when the OSC is processed?

Copy link
Member

Choose a reason for hiding this comment

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

Oh derp. No, sorry for confusing you. I meant appearance = focused? default + runtime changes : unfocused

So I guess I'm imagining that if someone changes the BG color with an OSC< that only changes the focused BG color, never the unfocused one.

Plus, this way, when the settings reload, the runtime changes would still persist. This would apply for things like changing the font size too (though that's not as relevant for this spec)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I guess I'm imagining that if someone changes the BG color with an OSC< that only changes the focused BG color, never the unfocused one.

Makes sense, I'll add that in!

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.

Yea alright, lets ship this

@zadjii-msft zadjii-msft removed their assignment Jan 7, 2021
@zadjii-msft zadjii-msft added Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal. labels Feb 4, 2021
@ghost ghost requested review from miniksa, DHowett and leonMSFT February 4, 2021 13:58
doc/specs/Configuration object for profiles.md Outdated Show resolved Hide resolved
doc/specs/Configuration object for profiles.md Outdated Show resolved Hide resolved

### Accessibility

Does not affect accessibility.
Copy link
Member

Choose a reason for hiding this comment

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

Not directly. I guess a user could choose for themselves some unfocused states that would make the UI highly inaccessible to themselves (poor contrast ratio.)

What happens for high-contrast mode? Is the user allowed to still override high-contrast mode colors to the point where they might not be high-contrast anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the user can override high-contrast mode colors already in their current profiles, then yes they can do so in the unfocused appearance as well. I.e. this change doesn't provide additional functionality with regards to a specific appearance, it just gives users a second appearance for their profiles, so any accessibility concerns with regards to users messing around with their unfocused appearance already exist because they could do the same with the one appearance they have currently.

these appearance objects can scale/layer over each other. We had a lot of discussion about this and could not find
a suitable solution to the problem of multiple states being valid at the same time (like unfocused and elevated).
This, along with the fact that it is uncertain if there even will be more states we would want to add led us to
the conclusion that we should only support the unfocused state for now, and come back to this issue later. If there
Copy link
Member

Choose a reason for hiding this comment

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

Well.... the way the json snippet is... it's a bit awkward to come back later. Unless the json snippet is changed to be like { "stateChangeConfig" : { "mode" : "unfocused", "overrides" : { ... blah blah blah } } }... in which case it becomes future proof and can become "mode" : "elevated" or "mode" : "elevated | unfocused" in the future.

I'm just thinking that if you have to make >1 in the future, it's a little awkward to have a bunch of different formats for different states.

Copy link
Member

Choose a reason for hiding this comment

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

We can always deprecate one version of it.

doc/specs/Configuration object for profiles.md Outdated Show resolved Hide resolved
doc/specs/Configuration object for profiles.md Outdated Show resolved Hide resolved
doc/specs/Configuration object for profiles.md Outdated Show resolved Hide resolved
these appearance objects can scale/layer over each other. We had a lot of discussion about this and could not find
a suitable solution to the problem of multiple states being valid at the same time (like unfocused and elevated).
This, along with the fact that it is uncertain if there even will be more states we would want to add led us to
the conclusion that we should only support the unfocused state for now, and come back to this issue later. If there
Copy link
Member

Choose a reason for hiding this comment

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

We can always deprecate one version of it.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 5, 2021
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 5, 2021
@DHowett
Copy link
Member

DHowett commented Feb 5, 2021

just gotta merge main!

@PankajBhojwani PankajBhojwani added AutoMerge Marked for automatic merge by the bot when requirements are met and removed Needs-Second It's a PR that needs another sign-off labels Feb 5, 2021
@ghost
Copy link

ghost commented Feb 5, 2021

Hello @PankajBhojwani!

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 1962767 into main Feb 6, 2021
@ghost ghost deleted the dev/pabhoj/unfocused_pane_spec branch February 6, 2021 00:05
ghost pushed a commit that referenced this pull request Apr 8, 2021
This pull request adds an appearance configuration object to our
settings model and app lib, allowing the control to be rendered
differently depending on its state, and then uses it to add support for
an "unfocused" appearance that the terminal will use when it's not in
focus.

To accomplish this, we isolated the appearance-related settings from
Profile (into AppearanceConfig) and TerminalSettings (into the
IControlAppearance and ICoreAppearance interfaces). A bunch of work was
done to make inheritance work.

The unfocused appearance inherits from the focused one _for that
profile_. This is important: If you define a
defaults.unfocusedAppearance, it will apply all of defaults' settings to
any leaf profile when a terminal in that profile is out of focus.

Specified in #8345 
Closes #3062
Closes #2316
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants