Enable previewing the color scheme in the command palette #9794
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
removed
the
Needs-Author-Feedback
miniksa
approved these changes
Apr 15, 2021
PankajBhojwani
requested changes
Apr 15, 2021
There was a problem hiding this comment.
Blocking just for the runtime colors question!
Also I think you already know this, but the color scheme preview will not work if the profile has an unfocusedAppearance. I'm okay with fixing that separately/in-post though since it looks like we need to have some discussion on how we want to tackle that.
| } | ||
|
|
||
| // ApplyColorScheme(nullptr) will clear the old color scheme. | ||
| controlSettings.ApplyColorScheme(nullptr); |
There was a problem hiding this comment.
Have a question about this - why do we clear the runtime colors? I understood the 'control-decided' settings to only be mutable by the control.
Also, wouldn't this mean that the 'PreviewColorScheme' appearance may not match up to the 'SetColorScheme' appearance? Because it seems that we don't clear out the runtime colors when previewing the new scheme, but we do when we set the new scheme (so if there were some colors in the runtime settings, they will show up in the preview but will disappear after setting the new scheme)
There was a problem hiding this comment.
Yea, this is a rough one. It probably should only be mutable by the control, but if it is, then the SetColorScheme action would do nothing at all when the user's already run colortool, which is not really what we want, right? So the only option here is to clear out the runtime settings.
Weirdly enough, this seemed to work fine when doing the colortool -> preview a scheme. I agree that it seems like it shouldn't. I could dig into it and investigate further if need be
ghost
added
the
Needs-Author-Feedback
ghost
removed
the
Needs-Author-Feedback
This comment has been minimized.
This comment has been minimized.
carlos-zamora
approved these changes
Apr 21, 2021
| @@ -2751,4 +2752,5 @@ namespace winrt::TerminalApp::implementation | |||
| WindowRenamer().IsOpen(false); | |||
| } | |||
| } | |||
|
|
|||
PankajBhojwani
approved these changes
Apr 21, 2021
carlos-zamora
added
the
AutoMerge
|
Hello @carlos-zamora! 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 (
|
ghost
merged commit
|
🎉 Handy links: |
ghost
mentioned this pull request
18 tasks
ghost
pushed a commit
that referenced
this pull request
Dec 1, 2021
## 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.
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of the Pull Request
Allow schemes to be previewed as the user hovers over them in the Command Palette.
References
Potential follow-ups
PR Checklist
Detailed Description of the Pull Request / Additional comments
This works by inserting a "preview"
TerminalSettingsinto the settings hierarchy, before theTermControl's runtime settings, and after the ones from the actualCascadiaSettings. This allows us to modify that preview settings object, then discard it when we're done with the preview.This could also be used for other settings in the future - I built it to be extensible to other
ShortcutActions, though I haven't implemented those yet.Validation Steps Performed
colortool -x <scheme>after selecting a scheme - colortool overrides the selected schemecolortool -x <scheme>after selecting a scheme - the scheme in the palette becomes the active one