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.

Sign up for GitHub

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

Jump to bottom

Spec: Appearance configuration objects for profiles #8345

Merged
19 commits merged into from
Feb 6, 2021
Merged

Spec: Appearance configuration objects for profiles #8345

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
963e624
draft spec
PankajBhojwani Nov 20, 2020
a24d166
my alias is a misspelling
PankajBhojwani Nov 20, 2020
d5f6ced
future consideration
PankajBhojwani Nov 20, 2020
83b78ba
general object, not just for focused/unfocused
PankajBhojwani Nov 23, 2020
887af3b
clearer implementation details
PankajBhojwani Nov 30, 2020
7ddb322
renaming
PankajBhojwani Nov 30, 2020
5cf8216
inheritance
PankajBhojwani Dec 1, 2020
0e7b578
elaboration on inheritance
PankajBhojwani Dec 1, 2020
6812b76
further details
PankajBhojwani Dec 3, 2020
ad352df
updates from brownbag
PankajBhojwani Dec 4, 2020
df039d2
ICore/ControlAppearance
PankajBhojwani Dec 4, 2020
95ab38c
moved elevated state to future considerations, no longer experimental
PankajBhojwani Dec 9, 2020
2f18add
1 more risk, naming consistency, spelling
PankajBhojwani Dec 10, 2020
7445282
notes on why multiple states is now a future consideration
PankajBhojwani Dec 11, 2020
8a62a44
spell
PankajBhojwani Dec 11, 2020
5048f8f
additional notes from PR comments
PankajBhojwani Dec 14, 2020
df9627b
updates from recent changes
PankajBhojwani Dec 21, 2020
c50df8d
address comments
PankajBhojwani Feb 5, 2021
e00e259
Merge branch 'main' of https://github.com/microsoft/terminal into dev…
PankajBhojwani Feb 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/actions/spell-check/dictionary/names.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ nvda
oising
oldnewthing
osgwiki
pabhojwa
paulcam
pauldotknopf
PGP
Expand Down
122 changes: 122 additions & 0 deletions doc/specs/Configuration object for profiles.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
---
PankajBhojwani marked this conversation as resolved.
Show resolved Hide resolved
author: <Pankaj> <Bhojwani> <pabhojwa@microsoft.com>
created on: <2020-11-20>
last updated: <2020-11-20>
issue id: <#8345>
---

# Configuration object for profiles

## Abstract

This spec outlines how we can support 'configuration objects' in our profiles, which
will allow us to render differently depending on the state of the control. For example, a
control can be rendered differently if it's focused as compared to when it's unfocused. Another
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


## Inspiration

Reference: [#3062](https://github.com/microsoft/terminal/issues/3062)

Users want there to be a more visible indicator than the one we have currently for which
pane is focused and which panes are unfocused. This change would grant us that feature.

## Solution Design

We will add an new interface in the `TerminalControl` namespace, called `IAppearance`, which defines how
`TerminalControl` and `TerminalCore` will ask for the rendering settings they need to know about (such as `CursorShape`).
`TerminalApp` will implement this interface through a class called `AppAppearanceConfig`.
PankajBhojwani marked this conversation as resolved.
Show resolved Hide resolved

We will also have `IControlSettings` require `IAppearance`. That way, the control `settings` object can
itself also be used as an object that implements `IAppearance`. We do this so we do not need a separate
'focused' configuration object when we wish to switch back to the 'regular' appearance from the unfocused
appearance - we can simply pass in the settings.

On the Settings Model side, there will be a new interface called `IAppearanceConfig`, which is essentially a
mirror of the `IAppearance` interface described earlier. A new class, `AppearanceConfig`, will implement this
interface and so will `Profile` itself (for the same reason as earlier - so that no new configuration object is
needed for the regular appearance).

When we parse out the settings json file, each state-appearance will be stored in an object of the `AppearanceConfig`
class. Later on, these values get piped over to the `AppAppearanceConfig` objects in `TerminalApp`. This is the
similar to the way we already pipe over information such as `FontFace` and `CursorStyle` from the settings
model to the app.

### 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.


For now, these states are meant to be entirely appearance-based. So, not all parameters which can be
defined in a `Profile` can be defined in this new object (for example, we do not want parameters which
would cause a resize in this object.) Here is the list of parameters we will allow:

- Anything regarding colors: `colorScheme`, `foreground`, `background`, `cursorColor` etc
- Anything regarding background image: `path`, `opacity`, `alignment`, `stretchMode`
- `cursorShape`

We may wish to allow further parameters in these objects in the future (like `bellStyle`?). The addition
of further parameters can be discussed in the future and is out of scope for this spec.

### Inheritance

We have to decide how we want to deal with the case(s) where not all parameters are defined - i.e. what
values should we use for undefined parameters? The current proposal is as follows:

If the profile defines an `unfocusedState`, any parameters not explicitly defined within it will adopt
the values from the profile itself. If the profile does not define an `unfocusedState`, then the global/default `unfocusedState` 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 •
edited
Loading

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 •
edited
Loading

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

while still having a global/default unfocused state appearance, they simply need to define an empty `unfocusedState`
for that profile (similarly, they could define just 1 or 2 parameters if they wish for minimal changes between the focused
and unfocused states for that profile). If they do not define any `unfocusedState` for the profile, then
the global/default one will be used.

## UI/UX Design

Users will be able to add a new setting to their profiles that will look like this:

```
"unfocusedState":
PankajBhojwani marked this conversation as resolved.
Show resolved Hide resolved
{
"colorScheme": "Campbell",
"cursorColor": "#888",
"cursorShape": "emptyBox",
"foreground": "#C0C0C0",
"background": "#000000"
}
```

## Capabilities

### 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.


### Security

Does not affect security.

### Reliability

This is another location in the settings where parsing/loading the settings may fail. However, this is the case
for any new setting we add so I would say that this is a reasonable cost for this feature.

### Compatibility

Should not affect compatibility.

### Performance, Power, and Efficiency
PankajBhojwani marked this conversation as resolved.
Show resolved Hide resolved

## Potential Issues
PankajBhojwani marked this conversation as resolved.
Show resolved Hide resolved

Inactive tabs will be 'rendered' in the background with the `UnfocusedRenderingParams` object, we need to make
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.

## Future considerations
PankajBhojwani marked this conversation as resolved.
Show resolved Hide resolved

We will need to decide how this will look in the settings UI.

## Resources