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.

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

Color scheme preview via command palette and settings reload resets zoom level #11067

Closed
fran-f opened this issue Aug 27, 2021 · 5 comments · Fixed by #11619
Closed

Color scheme preview via command palette and settings reload resets zoom level #11067

fran-f opened this issue Aug 27, 2021 · 5 comments · Fixed by #11619
Assignees
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@fran-f
Copy link

fran-f commented Aug 27, 2021

Windows Terminal version (or Windows build number)

Terminal 1.9.1942.0, Windows 10.0.19043.1165

Other Software

No response

Steps to reproduce

  • Open Terminal
  • Increase the font size with Ctrl-+
  • Open the command palette with Ctrl-Shift-P
  • Find "Select color scheme" and hit Enter to show the scheme list

As soon as the first color scheme is previewed, the zoom level is reset.

Expected Behavior

I would expect the zoom level to be indipendent from color schemes. Previewing or applying a new scheme should not reduce or increase the font size.

Actual Behavior

The color scheme preview affects the zoom level. Backing out of the menu will not revert this change.

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Aug 27, 2021
@DHowett
Copy link
Member

DHowett commented Aug 27, 2021

You know, I could have sworn we had an issue tracking this! Right now, anything that causes a settings reload (or: "for new settings to be pushed into a terminal pane") destroys fragile things like font size and application-originated color scheme changes and the like. I searched the backlog high and low, however, and couldn't find one.

Congrats! This is now the tracking issue for that.

@DHowett DHowett changed the title Color scheme preview via command palette resets zoom level Color scheme preview via command palette and settings reload resets zoom level Aug 27, 2021
@DHowett DHowett added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Aug 27, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Aug 27, 2021
@DHowett DHowett added this to the Terminal Backlog milestone Aug 27, 2021
@DHowett
Copy link
Member

DHowett commented Aug 27, 2021

@zadjii-msft might be able to find it, too, but for now I'm just going to pull the triage tag off.

@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Aug 27, 2021
@fran-f
Copy link
Author

fran-f commented Aug 27, 2021

I did look for one as well, and I could not find any. The closest was #2874.

@zadjii-msft
Copy link
Member

I don't think I ever filed anything like that. Turns out I'm probably gonna blow this all up anyways while working through an element of #5000, so I'm just gonna throw this on me for 1.12.

@zadjii-msft zadjii-msft self-assigned this Aug 30, 2021
@zadjii-msft zadjii-msft added Needs-Discussion Something that requires a team discussion before we can proceed and removed Needs-Discussion Something that requires a team discussion before we can proceed labels Aug 30, 2021
@ghost ghost added the In-PR This issue has a related PR label Oct 26, 2021
@ghost ghost closed this as completed in #11619 Dec 1, 2021
ghost pushed a commit that referenced this issue 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.
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Dec 1, 2021
@ghost
Copy link

ghost commented Feb 3, 2022

🎉This issue was addressed in #11619, which has now been successfully released as Windows Terminal Preview v1.13.10336.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants