Skip to content

Use X-macros to simplify new setting creation in SettingsModel#11416

Merged
19 commits merged intomainfrom
dev/pabhoj/simplify_add_setting
Nov 3, 2021
Merged

Use X-macros to simplify new setting creation in SettingsModel#11416
19 commits merged intomainfrom
dev/pabhoj/simplify_add_setting

Conversation

@PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented Oct 4, 2021

Introduces X-macros to reduce the number of places we need to write essentially the same line of code but for a different setting (declaring it in the header file, in Copy, LayerJson, ToJson, etc).

@PankajBhojwani PankajBhojwani changed the title Use X-macros to simplify new setting creation (Part 1) Use X-macros to simplify new setting creation in SettingsModel Oct 8, 2021
@PankajBhojwani PankajBhojwani marked this pull request as ready for review October 8, 2021 19:44
@SnirBroshi
Copy link
Contributor

I love this, will you do the same for actions?

Also, can you use this to generate the settings schema, or the other way around? Using the C-preprocessor to generate JSON is weird but possible, but maybe a python script will do

@zadjii-msft
Copy link
Member

@NotWearingPants you might be interested in #3475. I originally wanted to do this using a python script+the schema to generate all of the rest of the action plumbing, but in retrospect, the xmacros are way easier to use. So it'll probably end up being "update the xmacro and the schema". That's way better than what it is today ☺️

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple nits but overall I'm really happy with this

zadjii-msft
zadjii-msft previously approved these changes Oct 26, 2021
@zadjii-msft zadjii-msft added the Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. label Oct 26, 2021
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 26, 2021
MTSM_GLOBAL_SETTINGS(GLOBAL_SETTINGS_TO_JSON)
#undef GLOBAL_SETTINGS_TO_JSON

// clang-format on
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now that we're not keeping a huge list of aligned columns, you can remove the clang-format guards and let the formatter act natural. this applies to any place where you did this 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, turns out this was the only place that still had the guards! Removed

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 26, 2021
@DHowett
Copy link
Member

DHowett commented Oct 28, 2021

alas, it's conflicted with the paste trimmer!

@zadjii-msft zadjii-msft assigned DHowett and unassigned DHowett Nov 1, 2021
@ghost ghost dismissed stale reviews from zadjii-msft via 430669f November 2, 2021 13:46
@DHowett
Copy link
Member

DHowett commented Nov 2, 2021

Unfortunately, new policies require re-reviews when new commits are pushed.

profile->_Commandline = _Commandline;
profile->_StartingDirectory = _StartingDirectory;
profile->_AntialiasingMode = _AntialiasingMode;
profile->_ForceFullRepaintRendering = _ForceFullRepaintRendering;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm surprised you didn't move these into the macro . . . but it seems like you did?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, what? This is supported as a profile setting?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very confused.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are in the global settings macro, not in the profile settings macro

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we don't actually use them in Profile::LayerJson or Profile::ToJson

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So... they just should have never been in Profile in the first place?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PankajBhojwani, can you follow up (separate PR) to remove these from Profile if they're not used? Thx

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oen q about the rendering settings -- are they per profile or are they per globals?

profile->_Commandline = _Commandline;
profile->_StartingDirectory = _StartingDirectory;
profile->_AntialiasingMode = _AntialiasingMode;
profile->_ForceFullRepaintRendering = _ForceFullRepaintRendering;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, what? This is supported as a profile setting?

profile->_Commandline = _Commandline;
profile->_StartingDirectory = _StartingDirectory;
profile->_AntialiasingMode = _AntialiasingMode;
profile->_ForceFullRepaintRendering = _ForceFullRepaintRendering;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very confused.

@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Nov 2, 2021
@zadjii-msft zadjii-msft removed their assignment Nov 3, 2021
@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 3, 2021
@ghost
Copy link

ghost commented Nov 3, 2021

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 726b428 into main Nov 3, 2021
@ghost ghost deleted the dev/pabhoj/simplify_add_setting branch November 3, 2021 15:01
DHowett pushed a commit that referenced this pull request Nov 4, 2021
The `ForceFullRepaintRendering` and `SoftwareRendering` are global only and for some reason were in profile. This commit removes them.

Reference: #11416 (comment)
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.
ghost pushed a commit that referenced this pull request Jan 31, 2022
Introduced in #11416

We weren't using these macros for duplicating as well, so I forgot to duplicate a couple settings. This PR switches duplicating over to using the macros as well, which shou;d reduce future bugs. 

Also adds notes to which properties are intentionally omitted from these macros.

* [x] closes #12265
* [x] Verified manually that #12120 still works as expected
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. AutoMerge Marked for automatic merge by the bot when requirements are met

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants