starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-02-03 21:25:34 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

[PR #19262] Remove TerminalSettings from the TerminalSettingsModel project #31773

New Issue
Open
opened 2026-01-31 09:49:26 +00:00 by claunia · 0 comments
claunia commented 2026-01-31 09:49:26 +00:00
Owner
Copy Link

Original Pull Request: https://github.com/microsoft/terminal/pull/19262

State: closed
Merged: Yes

The idea with IControlSettings (and friends) was always that a consumer
of the terminal control could implement it in whatever way they pleased.

Windows Terminal (the application) was intended to be only one
consumer. It has a whole JSON settings model. Nobody wants to think
about JSON at the Terminal Control level. We could have an "adapter" in
TerminalApp, which spoke Terminal JSON Settings on one side and Terminal
Control on the other side.

That worked until we added the settings editor. The settings editor
needed to display a control, and that control's settings needed to be
based on the JSON settings. Oops. We took the expedient route of moving
the adapter into TerminalSettingsModel itself, and poking a bunch of
holes in it so that TerminalApp and TerminalSettingsEditor could tweak
it as needed.

Later, we doubled down on the control settings interface by having every
Terminal Control make its own ControlSettings when we were going to do
the multi-process model. This reduced the number of IPC round trips for
every settings query to 0. Later we built color scheme previewing on top
of that--adding structs to carry color schemes and stuff which was
already in the Appearance config. Sheesh. Layers and layers and layers.

This pull request moves it back into its own library and strips it from
the surface of TerminalSettingsModel. It also deletes ControlSettings
and struct CoreScheme. That library is called
TerminalSettingsAppAdapterLib, and it contains a hidden WinRT
implements type rather than a full-fledged activatable runtimeclass.
It also implements one-level inheritance on its own rather than using
IInheritable.

It adheres to the following principles:

  • The control will never modify its settings in a way that is visible to
    the control's consumer; therefore, none of the properties have setters
  • The settings should never contain things of interest only to the
    Application that the Application uses to communicate data back to
    itself     (see ProfileName, removed in 68b723c and KeyBindings,
    removed in fa09141). This generalizes to "we should never store stuff
    in an unrelated object passed between layers solely for the purpose of
    getting it back".

I made a few changes to the settings interface, including introducing a
new ICoreScheme interface that only contains color scheme info. This
is designed to support the Preview/Set color scheme actions, which no
longer work by app backing up the scheme and restoring it later. All
of that machinery lives inside TermControl/ControlCore now.

ICoreScheme no longer supports GetColorAtIndex; you must read all 16
colors at the same time. I am not sorry. Every consumer did that
already, so now we have 15 fewer COM calls for every color scheme.

The new TerminalSettings is mostly consumed via
com_ptr<TerminalSettings>, so a bunch of . (projected) accesses had
to turn into -> (com_ptr dereferencing) accesses.

I also realized, in the course of this work, that the old
TerminalSettings contained a partial hand-written reimplementation of
every setting in ControlProperties. Every contributor had to add
every new setting to both places--why? I can't figure it out. I'm using
ControlProperties comprehensively now. I propagated any setting whose
default value was different from that in ControlProperties back to
ControlProperties.

This is part X in a series of pull requests that will remove all mention
of Microsoft.Terminal.Control and Microsoft.Terminal.Core from the
settings model. Once that is done, the settings model can consume only
the base WinRT types and build very early and test more easily.

Previewing is fun. I introduced a new place to stash an entire color
table on ControlCore, which we use to save the "active" colors while we
temporarily overwrite them. SetColorScheme is also fun. We now have a
slot for overriding only the focused color scheme on ControlCore. It's
fine. It's clearer than "back up the focused appearance, overwrite the
focused appearance, create a child of the user's settings and apply the
color scheme to it, etc.".

There is a bug/design choice in color scheme overriding, which may or
may not matter: overlaying a color scheme on a terminal with an
unfocused appearance which does not have its own color scheme will
result in the previously-deleted overridden focused color scheme peeking
through when the terminal is not focused.

I also got rid of our only in-product use of
Terminal::CreateFromSettings which required us to set InitialRows
and InitialCols on the incoming settings object (see core tenet 2).

Refs #19261
Refs #19314
Refs #19254

**Original Pull Request:** https://github.com/microsoft/terminal/pull/19262 **State:** closed **Merged:** Yes --- The idea with IControlSettings (and friends) was always that a consumer of the terminal control could implement it in whatever way they pleased. Windows Terminal (the application) was intended to be only one consumer. It has a whole JSON settings model. Nobody wants to think about JSON at the Terminal Control level. We could have an "adapter" in TerminalApp, which spoke Terminal JSON Settings on one side and Terminal Control on the other side. That worked until we added the settings editor. The settings editor needed to display a control, and that control's settings needed to be based on the JSON settings. Oops. We took the expedient route of moving the adapter into TerminalSettingsModel itself, and poking a bunch of holes in it so that TerminalApp and TerminalSettingsEditor could tweak it as needed. Later, we doubled down on the control settings interface by having every Terminal Control _make its own ControlSettings_ when we were going to do the multi-process model. This reduced the number of IPC round trips for every settings query to 0. Later we built color scheme previewing on top of that--adding structs to carry color schemes and stuff which was already in the Appearance config. Sheesh. Layers and layers and layers. This pull request moves it back into its own library and strips it from the surface of TerminalSettingsModel. It also deletes `ControlSettings` and `struct CoreScheme`. That library is called `TerminalSettingsAppAdapterLib`, and it contains a hidden WinRT _implements_ type rather than a full-fledged activatable `runtimeclass`. It also implements one-level inheritance on its own rather than using IInheritable. It adheres to the following principles: - The control will never modify its settings in a way that is visible to the control's consumer; therefore, none of the properties have setters - The settings should never contain things of interest only to the Application that the Application uses to communicate data _back to itself_ (see `ProfileName`, removed in 68b723c and `KeyBindings`, removed in fa09141). This generalizes to "we should never store stuff in an unrelated object passed between layers solely for the purpose of getting it back". I made a few changes to the settings interface, including introducing a new `ICoreScheme` interface that _only_ contains color scheme info. This is designed to support the Preview/Set color scheme actions, which no longer work by _app backing up the scheme and restoring it later._ All of that machinery lives inside TermControl/ControlCore now. `ICoreScheme` no longer supports `GetColorAtIndex`; you must read all 16 colors at the same time. I am not sorry. Every consumer did that already, so now we have 15 fewer COM calls for every color scheme. The new TerminalSettings is mostly consumed via `com_ptr<TerminalSettings>`, so a bunch of `.` (projected) accesses had to turn into `->` (com_ptr dereferencing) accesses. I also realized, in the course of this work, that the old TerminalSettings contained a partial hand-written reimplementation of _every setting_ in `ControlProperties`. Every contributor had to add every new setting to both places--why? I can't figure it out. I'm using ControlProperties comprehensively now. I propagated any setting whose default value was different from that in ControlProperties back to ControlProperties. This is part X in a series of pull requests that will remove all mention of Microsoft.Terminal.Control and Microsoft.Terminal.Core from the settings model. Once that is done, the settings model can consume _only_ the base WinRT types and build very early and test more easily. Previewing is fun. I introduced a new place to stash an entire color table on ControlCore, which we use to save the "active" colors while we temporarily overwrite them. SetColorScheme is _also_ fun. We now have a slot for overriding only the focused color scheme on ControlCore. It's fine. It's clearer than "back up the focused appearance, overwrite the focused appearance, create a child of the user's settings and apply the color scheme to it, etc.". There is a bug/design choice in color scheme overriding, which may or may not matter: overlaying a color scheme on a terminal with an unfocused appearance which _does not_ have its own color scheme will result in the previously-deleted overridden focused color scheme peeking through when the terminal is not focused. I also got rid of our only in-product use of `Terminal::CreateFromSettings` which required us to set `InitialRows` and `InitialCols` on the incoming settings object (see core tenet 2). Refs #19261 Refs #19314 Refs #19254
claunia added the pull-request label 2026-01-31 09:49:26 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
⛺ Reserved
A11yCO
A11yMAS
A11ySev1
A11ySev2
A11ySev3
A11yTTValidated
A11yUsable
A11yVoiceAccess
A11yWCAG
Area-Accessibility
Area-AtlasEngine
Area-AzureShell
Area-Build
Area-Build
Area-Chat
Area-CmdPal
Area-CodeHealth
Area-Commandline
Area-CookedRead
Area-DefApp
Area-Extensibility
Area-Fonts
Area-GroupPolicy
Area-i18n
Area-Input
Area-Interaction
Area-Interop
Area-Localization
Area-Output
Area-Performance
Area-Portable
Area-Quality
Area-Remoting
Area-Rendering
Area-Schema
Area-Server
Area-Settings
Area-SettingsUI
Area-ShellExtension
Area-ShellExtension
Area-ShellExtension
Area-Suggestions
Area-Suggestions
Area-TerminalConnection
Area-TerminalControl
Area-Theming
Area-UserInterface
Area-VT
Area-Windowing
Area-WPFControl
AutoMerge
Blocking-Ingestion
Culprit-Centennial
Culprit-WinUI
Disability-All
Disability-Blind
Disability-LowVision
Disability-Mobility
External-Blocked-WinUI3
Fixed
Gathering-Data
good first issue
HCL-E+D
HCL-WindowsTerminal
Help Wanted
Impact-Compatibility
Impact-Compliance
Impact-Correctness
Impact-Visual
In-PR
InclusionBacklog
InclusionBacklog-Windows TerminalWin32
InclusionCommitted-202206
Issue-Bug
Issue-Docs
Issue-Feature
Issue-Feature
Issue-Question
Issue-Samples
Issue-Scenario
Issue-Task
Needs-Attention
Needs-Author-Feedback
Needs-Bisect
Needs-Discussion
Needs-Repro
Needs-Tag-Fix
Needs-Tag-Fix
Needs-Triage
No-Recent-Activity
Priority-0
Priority-1
Priority-2
Priority-3
Product-Cmd.exe
Product-Colortool
Product-Colortool
Product-Colortool
Product-Conhost
Product-Conpty
Product-Meta
Product-Powershell
Product-Terminal
Product-WSL
pull-request
Mirrored from GitHub Pull Request
 Resolution-Answered
Resolution-By-Design
Resolution-Duplicate
Resolution-External
Resolution-Fix-Available
Resolution-Fix-Committed
Resolution-No-Repro
Resolution-Won't-Fix
Severity-Blocking
Severity-Crash
Severity-DataLoss
spam
this-will-be-a-breaking-change
Tracking-External
WindowsTerminal_Win32
Work-Item
zAskModeBug
zInbox-Bug
No Label pull-request
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#31773
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?