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

[PR #11619] [MERGED] Change the ControlCore layer to own a copy of its settings #28667

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

📋 Pull Request Information

Original PR: https://github.com/microsoft/terminal/pull/11619
Author: @zadjii-msft
Created: 10/26/2021
Status: ✅ Merged
Merged: 12/1/2021
Merged by: @undefined

Base: main ← Head: dev/migrie/oop/ragnarok

📝 Commits (10+)

  • 43ec102 blindly make control settings read-only, there's only 2 build breaks?
  • dad065e make this a macro in case we want to reuse this
  • 981d8cc This is a crazy idea but if it works, it'll be a game changer
  • d6989ec this is wild
  • 4b18bb4 missed one
  • 7fc2f10 move to a common header
  • c7536ed This says there's only one build error in Terminal.Control.Lib but that can't be right
  • c26dd6b This works better than it has any business doing. Plemty of bugs, but a good enough start
  • 4a1baf0 I think I needed this to get it to build. Or I started breaknig something. Idk, I wrote this a few days ago, I just need this clone for testing so git commit
  • fbba74e I think we're going to have to cut this

📊 Changes

39 files changed (+871 additions, -500 deletions)

View changed files

📝 src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp (+6 -2)
📝 src/cascadia/LocalTests_TerminalApp/TabTests.cpp (+23 -85)
📝 src/cascadia/TerminalApp/ActionPreviewHandlers.cpp (+13 -50)
📝 src/cascadia/TerminalApp/AppActionHandlers.cpp (+1 -22)
📝 src/cascadia/TerminalApp/Pane.cpp (+10 -26)
📝 src/cascadia/TerminalApp/TerminalPage.cpp (+4 -7)
➕ src/cascadia/TerminalControl/ControlAppearance.h (+49 -0)
📝 src/cascadia/TerminalControl/ControlCore.cpp (+185 -62)
📝 src/cascadia/TerminalControl/ControlCore.h (+30 -6)
📝 src/cascadia/TerminalControl/ControlCore.idl (+9 -3)
📝 src/cascadia/TerminalControl/ControlInteractivity.cpp (+13 -1)
📝 src/cascadia/TerminalControl/ControlInteractivity.h (+2 -0)
📝 src/cascadia/TerminalControl/ControlInteractivity.idl (+3 -0)
➕ src/cascadia/TerminalControl/ControlSettings.h (+81 -0)
📝 src/cascadia/TerminalControl/IControlAppearance.idl (+10 -10)
📝 src/cascadia/TerminalControl/IControlSettings.idl (+22 -19)
📝 src/cascadia/TerminalControl/ICoreState.idl (+2 -0)
📝 src/cascadia/TerminalControl/TermControl.cpp (+139 -87)
📝 src/cascadia/TerminalControl/TermControl.h (+16 -14)
📝 src/cascadia/TerminalControl/TermControl.idl (+4 -3)

...and 19 more files

📄 Description

Summary of the Pull Request

Currently, the TermControl and ControlCore recieve a settings object that implements IControlSettings. They use for this for both reading the settings they should use, and also storing some runtime overrides to those settings (namely, Opacity). The object they recieve currently is a T.S.M.TerminalSettings object, as well as another TerminalSettings object if the user wants to have an unfocusedAppearance. All these are all hosted in the same process, so everything is fine and dandy.

With the upcoming move to having the Terminal split into multiple processes, this will no longer work. If the ControlCore in the Content Process is given a pointer to a TerminalSettings in a certain Window Process, and that control is subsequently moved to another window, then there's no guarantee that the original TerminalSettings object continues to exist. In this scenario, when window 1 is closed, now the Core is unable to read any settings, because the process that owned that object no longer exists.

The solution to this issue is to have the ControlCore's own their own copy of the settings they were created with. that way, they can be confident those settings will always exist. Enter ControlSettings, a dumb struct for just storing all the contents of the Settings. I used x-macros for this, so that we don't need to copy-paste into this file every time we add a setting.

Changing this has all sorts of other fallout effects:

  • Previewing a scheme/anything is a tad bit more annoying. Before, we could just sneak the previewed scheme into a TerminalSettings that lived between the settings we created the control with, and the settings they were actually using, and it would just work. Even explaining that here, it sounds like magic, because it was. However, now, the TermControl can't use a layered TerminalSettings for the settings anymore. Now we need to actually read out the current color table, and set the whole scheme when we change it. So now there's also a Microsoft.Terminal.Core.Scheme struct for holding that data.
    • Why a struct? Because that will go across the process boundary as a blob, rather than as a pointer to an object in the other process. That way we can transit the whole struct from window to core safely.
  • A TermControl doesn't have a IControlSettings at all anymore - it initalizes itself via the settings in the Core. This will be useful for tear-out, when we need to have the TermControl initialize itself from just a ControlCore, without being able to rebuild the settings from scratch.
  • The TabTests that were written under the assumption that the Control had a layered TerminalSettings obviously broke, as they were designed to. They've been modified to reflect the new reality.
  • When we initialize the Control, we give it the settings and the UnfocusedAppearance all at once. If we don't give it an unfocusedAppearance, it will just use the focused appearance as the unfocused appearance.
  • The Control no longer can write settings to the ControlSettings. We don't want to be storing things in there. Pretty much everything we set in the control, we store somewhere other than in the settings object itself. However, opacity and useAcrylic, we need to store in a handy new RUNTIME_SETTING property. We can write those runtime overrides to those properties.
  • We no longer store the color scheme for a pane in the persisted state. I'm tracking that in #9800. I don't think it's too hard to add back, but I wanted this in front of eyes sooner than later.

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Along the way I tried to clean up code where possible, but not too agressively.

I didn't end up converting the various MockTerminalSettings classes used in tests to the x macros quite yet. I wanted to merge this with #11416 in main before I went too crazy.

Validation Steps Performed

  • Scheme previewing works
  • Adjusting the font size works
  • focused/unfocused appearances still work
  • mouse-wheeling opacity still works
  • acrylic & cleartype still does the right thing
  • saving the settings still works
  • going wild on sliding the opacity slider in the settings doesn't crash the terminal
  • toggling retro effects with a keybinding still works
  • toggling retro effects with the command palette works
  • The matrix of (useAcrylic(true,false))x(opacity(50,100))x(antialiasingMode(cleartype, grayscale)) works as expected. Slightly changed, falls back to grayscale more often, but looks more right.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/microsoft/terminal/pull/11619 **Author:** [@zadjii-msft](https://github.com/zadjii-msft) **Created:** 10/26/2021 **Status:** ✅ Merged **Merged:** 12/1/2021 **Merged by:** [@undefined](undefined) **Base:** `main` ← **Head:** `dev/migrie/oop/ragnarok` --- ### 📝 Commits (10+) - [`43ec102`](https://github.com/microsoft/terminal/commit/43ec102343539314b8be3f073b646a4919fe925e) blindly make control settings read-only, there's only 2 build breaks? - [`dad065e`](https://github.com/microsoft/terminal/commit/dad065e0feb8d91bef1c34cc1dae170af317d89d) make this a macro in case we want to reuse this - [`981d8cc`](https://github.com/microsoft/terminal/commit/981d8cc9c81e877a5010c6516f7aa3f63a00b5f5) This is a crazy idea but if it works, it'll be a game changer - [`d6989ec`](https://github.com/microsoft/terminal/commit/d6989ec9d13e67d4c68dec6d7b3e093680febb8a) this is wild - [`4b18bb4`](https://github.com/microsoft/terminal/commit/4b18bb415e925c3fec7805335ba83232e4c6c3ba) missed one - [`7fc2f10`](https://github.com/microsoft/terminal/commit/7fc2f10bbdc5a42bf174e42aaa892d493fa2968a) move to a common header - [`c7536ed`](https://github.com/microsoft/terminal/commit/c7536edfa8789adaeb2af55d65c045cfbe8a3704) This says there's only one build error in Terminal.Control.Lib but that can't be right - [`c26dd6b`](https://github.com/microsoft/terminal/commit/c26dd6b1db29dbd680b6bf8b343b3f7011f2d06a) This works better than it has any business doing. Plemty of bugs, but a good enough start - [`4a1baf0`](https://github.com/microsoft/terminal/commit/4a1baf000607dde5caf6802e5af7a3296bf9db97) I think I needed this to get it to build. Or I started breaknig something. Idk, I wrote this a few days ago, I just need this clone for testing so `git commit` - [`fbba74e`](https://github.com/microsoft/terminal/commit/fbba74e89b16465a66318b5b4b43545a88d74d45) I think we're going to have to cut this ### 📊 Changes **39 files changed** (+871 additions, -500 deletions) <details> <summary>View changed files</summary> 📝 `src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp` (+6 -2) 📝 `src/cascadia/LocalTests_TerminalApp/TabTests.cpp` (+23 -85) 📝 `src/cascadia/TerminalApp/ActionPreviewHandlers.cpp` (+13 -50) 📝 `src/cascadia/TerminalApp/AppActionHandlers.cpp` (+1 -22) 📝 `src/cascadia/TerminalApp/Pane.cpp` (+10 -26) 📝 `src/cascadia/TerminalApp/TerminalPage.cpp` (+4 -7) ➕ `src/cascadia/TerminalControl/ControlAppearance.h` (+49 -0) 📝 `src/cascadia/TerminalControl/ControlCore.cpp` (+185 -62) 📝 `src/cascadia/TerminalControl/ControlCore.h` (+30 -6) 📝 `src/cascadia/TerminalControl/ControlCore.idl` (+9 -3) 📝 `src/cascadia/TerminalControl/ControlInteractivity.cpp` (+13 -1) 📝 `src/cascadia/TerminalControl/ControlInteractivity.h` (+2 -0) 📝 `src/cascadia/TerminalControl/ControlInteractivity.idl` (+3 -0) ➕ `src/cascadia/TerminalControl/ControlSettings.h` (+81 -0) 📝 `src/cascadia/TerminalControl/IControlAppearance.idl` (+10 -10) 📝 `src/cascadia/TerminalControl/IControlSettings.idl` (+22 -19) 📝 `src/cascadia/TerminalControl/ICoreState.idl` (+2 -0) 📝 `src/cascadia/TerminalControl/TermControl.cpp` (+139 -87) 📝 `src/cascadia/TerminalControl/TermControl.h` (+16 -14) 📝 `src/cascadia/TerminalControl/TermControl.idl` (+4 -3) _...and 19 more files_ </details> ### 📄 Description ## Summary of the Pull Request Currently, the TermControl and ControlCore recieve a settings object that implements `IControlSettings`. They use for this for both reading the settings they should use, and also storing some runtime overrides to those settings (namely, `Opacity`). The object they recieve currently is a `T.S.M.TerminalSettings` object, as well as another `TerminalSettings` object if the user wants to have an `unfocusedAppearance`. All these are all hosted in the same process, so everything is fine and dandy. With the upcoming move to having the Terminal split into multiple processes, this will no longer work. If the `ControlCore` in the Content Process is given a pointer to a `TerminalSettings` in a certain Window Process, and that control is subsequently moved to another window, then there's no guarantee that the original `TerminalSettings` object continues to exist. In this scenario, when window 1 is closed, now the Core is unable to read any settings, because the process that owned that object no longer exists. The solution to this issue is to have the `ControlCore`'s own their own copy of the settings they were created with. that way, they can be confident those settings will always exist. Enter `ControlSettings`, a dumb struct for just storing all the contents of the Settings. I used x-macros for this, so that we don't need to copy-paste into this file every time we add a setting. Changing this has all sorts of other fallout effects: * Previewing a scheme/anything is a tad bit more annoying. Before, we could just sneak the previewed scheme into a `TerminalSettings` that lived between the settings we created the control with, and the settings they were actually using, and it would _just work_. Even explaining that here, it sounds like magic, because it was. However, now, the TermControl can't use a layered `TerminalSettings` for the settings anymore. Now we need to actually read out the current color table, and set the whole scheme when we change it. So now there's also a `Microsoft.Terminal.Core.Scheme` _struct_ for holding that data. - Why a `struct`? Because that will go across the process boundary as a blob, rather than as a pointer to an object in the other process. That way we can transit the whole struct from window to core safely. * A TermControl doesn't have a `IControlSettings` at all anymore - it initalizes itself via the settings in the `Core`. This will be useful for tear-out, when we need to have the `TermControl` initialize itself from just a `ControlCore`, without being able to rebuild the settings from scratch. * The `TabTests` that were written under the assumption that the Control had a layered `TerminalSettings` obviously broke, as they were designed to. They've been modified to reflect the new reality. * When we initialize the Control, we give it the settings and the `UnfocusedAppearance` all at once. If we don't give it an `unfocusedAppearance`, it will just use the focused appearance as the unfocused appearance. * The Control no longer can _write_ settings to the `ControlSettings`. We don't want to be storing things in there. Pretty much everything we set in the control, we store somewhere other than in the settings object itself. However, `opacity` and `useAcrylic`, we need to store in a handy new `RUNTIME_SETTING` property. We can write those runtime overrides to those properties. * We no longer store the color scheme for a pane in the persisted state. I'm tracking that in #9800. I don't think it's too hard to add back, but I wanted this in front of eyes sooner than later. ## References * #1256 * #5000 * #9794 has the scheme previewing in it. * #9818 is WAY more possible now. ## PR Checklist * [x] Surprisingly there wasn't ever a card or issue for this one. This was only ever a bullet point in #5000. * A bunch of these issues were fixed along the way, though I never intended to fix them: * [x] Closes #11571 * [x] Closes #11586 * [x] Closes #7219 * [x] Closes #11067 * [x] I think #11623 actually ended up resolving this one, but I'm double tapping on it here: Closes #5703 * [x] I work here * [x] Tests added/passed * [n/a] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments Along the way I tried to clean up code where possible, but not too agressively. I didn't end up converting the various `MockTerminalSettings` classes used in tests to the x macros quite yet. I wanted to merge this with #11416 in `main` before I went too crazy. ## Validation Steps Performed * [x] Scheme previewing works * [x] Adjusting the font size works * [x] focused/unfocused appearances still work * [x] mouse-wheeling opacity still works * [x] acrylic & cleartype still does the right thing * [x] saving the settings still works * [x] going wild on sliding the opacity slider in the settings doesn't crash the terminal * [x] toggling retro effects with a keybinding still works * [x] toggling retro effects with the command palette works * [x] The matrix of (`useAcrylic(true,false)`)x(`opacity(50,100)`)x(`antialiasingMode(cleartype, grayscale)`) works as expected. Slightly changed, falls back to grayscale more often, but looks more right. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
claunia added the pull-request label 2026-01-31 09:29:58 +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#28667
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?