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 4 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
94 changes: 94 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,94 @@
---
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 its focused as compared to when its unfocused. Another
PankajBhojwani marked this conversation as resolved.
Show resolved Hide resolved
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

A new class in the `TerminalControl` namespace, called `CustomRenderConfig`, that will contain rendering
parameters. `TermControl` and `TerminalCore` will use objects of this class to pass along rendering parameters
to the renderer.

Our `TerminalSettingsModel` will parse out that object from the settings json file and pipe it over to
`TermControl`/`TerminalCore` to use. This will be done through `IControlSettings` and `ICoreSettings`, which
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.


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?

do not want parameters which would cause a resize in this object.) Here are the list of parameters we
will allow:

- Anything regarding colors: `colorScheme`, `foreground`, `background`, `cursorColor` etc
- `cursorShape`
- `backgroundImage`

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