Spec: Appearance configuration objects for profiles #8345
Conversation
PankajBhojwani
changed the title
| example is that an elevated state control can be rendered differently as compared to a | ||
| non-elevated one. |
There was a problem hiding this comment.
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 |
| ✔ | ✔ | ??? |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
So, when "adminState" gets added, we'll have a larger set of accepted parameters. For example...
name,tabTitle,suppressApplicationTitle--> what is my title?startingDirectoryandcommandline--> startup-related settings
Should we just accept every profile setting, then silently ignore ones that don't apply?
There was a problem hiding this comment.
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.
ghost
added
the
Needs-Author-Feedback
ghost
removed
the
Needs-Author-Feedback
| example is that an elevated state control can be rendered differently as compared to a | ||
| non-elevated one. |
There was a problem hiding this comment.
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.
| example is that an elevated state control can be rendered differently as compared to a | ||
| non-elevated one. |
There was a problem hiding this comment.
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
added
the
Needs-Author-Feedback
ghost
removed
the
Needs-Author-Feedback
PankajBhojwani
changed the title
|
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 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? |
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. |
| 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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.unfocusedConfigdefined: undefined params taken fromprofile(so the case whereprofile.unfocusedConfigis{}falls in here)profile.unfocusedConfignot defined,profile.defaults.unfocusedConfigdefined: undefined params are default values- both not defined: do nothing (so it just keeps the profile's appearance when unfocused)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| 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 |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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!
zadjii-msft
added
Issue-Docs
ghost
requested review from
|
|
||
| ### Accessibility | ||
|
|
||
| Does not affect accessibility. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We can always deprecate one version of it.
| 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 |
There was a problem hiding this comment.
We can always deprecate one version of it.
ghost
added
the
Needs-Author-Feedback
ghost
removed
the
Needs-Author-Feedback
|
just gotta merge main! |
…/pabhoj/unfocused_pane_spec
PankajBhojwani
added
AutoMerge
|
Hello @PankajBhojwani! Because this pull request has the 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 (
|
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
Summary of the Pull Request
Spec for #3062
PR Checklist
Detailed Description of the Pull Request / Additional comments
Read the spec