Handling Nullable Profile Settings

Windows Terminal version: 1.3.2382.0

Related to...

Add test for null Profile.StartingDirectory #7423: missing test for nullable startingDirectory
Make Profile a WinRT object #7283: winrt-ify Profile

Some settings in Profile are std::optional and others aren't. This leads to an inconsistent handling of setting the property to null in the JSON. Consider the following examples:

"backgroundImage" (std::optional<std::wstring>)
- explicitly do not have a background image
"snapOnInput" (bool)
- fall back to profiles.defaults value
"startingDirectory" (std::optional<std::wstring>)
- inherit the startingDirectory of the parent process
"commandline" (std::wstring)
- fall back to profiles.defaults value

From an implementation standpoint, it makes sense. std:optionals treat null as a meaningful value, whereas non-std::optionals treat null as fall back.

The problem here is 2-fold:

The schema does not always accept null as a value, even though we do
- startingDirectory is particularly an example of this where it has a unique behavior from null but is not in the schema
A user has no way of knowing whether setting a profile setting to null will consistently fallback or set the value.

Proposed Implementation/Solution

Make none of the profile settings nullable. If the user...

omits the setting --> fall back
sets to null --> validate a potential type mismatch

We should also add a special enum value for startingDirectory to inherit from the process.

From a Settings UI standpoint, it won't be possible to set settings to null either. So we should just purge null-ness as an option, and add specific enums for special behavior. Fallback and other special behavior must be represented in the Settings UI as their own button/option.

NAK on a special magic enum value for startingDirectory. null is that value.

No matter what you name that enum value, if it is a string, somebody could name a directory after it and then wonder, "why does Terminal hate my directory named inherit?"

I disagree with some of these assertions of inconsistent behavior.

"backgroundImage" (std::optional<std::wstring>)

explicitly do not have a background image

"snapOnInput" (bool)

fall back to profiles.defaults value

"startingDirectory" (std::optional<std::wstring>)

inherit the startingDirectory of the parent process

"commandline" (std::wstring)

fall back to profiles.defaults value

(2): snapOnInput: null falls back to the compiled-in value (true), and unsets the profiles.defaults value
(4): commandline: null falls back to the compiled-in value (cmd.exe), and unsets the profiles.defaults value

null never falls back to the value in profiles.defaults. That's the point of null.
If you wanted it to fall back to profiles.defaults you omit the setting.

null is an explicit statement that "I DO NOT WANT THE NAMED SETTING TO APPLY HERE". That is distinctly different from falling back.

Ok, then at least with startingDirectory we're explicitly allowing null to be a value. We just have to better document that it has special meaning.

I tested "commandline" and "snapOnInput". It falls back to profile.defaults. Here's the test JSON:

    "profiles": {
        "defaults": {
            "commandline": "wsl.exe",
            "snapOnInput": false
        },
        "list": [
            {
                "name": "test",
                "commandline": null,
                "snapOnInput": null
            }
        ]
    }

Profile "test" runs wsl.exe and does not snap on input.

We can change the behavior to be that when you set these settings to null, you fall back to the compiled-in value (as you'd expect). But I still think that that's not the right move. A regular person does not know what the compiled-in value is.

The user explicitly set something to null that is not nullable. I feel that that by definition is a type mismatch.

As for "backgroundImage", I'm fine with interpreting that as nullable. That makes sense. The user is explicitly stating that they do not want a background image.

I would not treat the schema as the source of truth. We've pretty consistently forgotten to update that with PRs, and we'll often do it half-assedly.

This deserves a longer reply but that's the thoughts off the top of the dome.

The case that's the most curious to me is the snapOnInput one - that's not really an optional. We might be treating null as false in that case (which is why it seems like it reverts to the profiles.defaults value (which is false in the above example))

optional<GUID> _guid{ nullopt };
optional<wstring> _source{ nullopt };
optional<GUID> _connectionType;

optional<wstring> _schemeName;

optional<color> _defaultForeground;

optional<color> _defaultBackground;

optional<color> _selectionBackground;

optional<color> _cursorColor;

optional<wstring> _tabTitle;

optional<color> _tabColor;

optional<wstring> _startingDirectory;

optional<wstring> _backgroundImage;
optional<double> _backgroundImageOpacity;
optional<WUX::Media::Stretch> _backgroundImageStretchMode;
optional<tuple<WUX::HorizontalAlignment, WUX::VerticalAlignment>> _backgroundImageAlignment;

optional<TerminalControl::ScrollbarState> _scrollbarState;

optional<wstring> _icon;

optional<bool> _retroTerminalEffect;

Property	Hardcoded default	What does `null` mean?
`_schemeName`	`"Campbell"`	I want no scheme??? this doesn't make sense
`_defaultForeground`	`nullopt`	Use the foreground from the scheme
`_tabTitle`	`nullopt`	Use whatever the title from the application is
`_tabColor`	`nullopt`	Use whatever the color from the theme/application is
`_startingDirectory`	`nullopt`	Use the CWD of the terminal itself
`_backgroundImage`	`nullopt`	I don't what an image
`_scrollbarState`	`nullopt`	?????????? probably "visible"
`_icon`	`nullopt`	I don't want to set an image as the icon
`_retroTerminalEffect`	`nullopt`	Don't enable retro effects

From those, _scrollbarState, _icon, _retroTerminalEffect all don't really make sense as optionals. I suppose we're currently just treating null as "use the hardcoded default value". I suppose _backgroundImage falls into this category as well. When you set that setting to null, you're saying "I want to clear out whatever the current value is".

The ones that do make sense are the ones where there's some other value that the Termianl might use in the absence of a value in the Profile. _tabTitle, _tabColor - For both of them, the absence of a value has meaning. Same with _startingDirectory.

I might be losing the track a bit.

Profile "test" runs wsl.exe and does not snap on input.

That might be a bug in parsing strings with JsonUtils. I'd think that parsing null would set the commandline to null or "", but that seems like we see null and decide "Well that's not a string" and skip processing it, leaving the value from the defaults.

Dustin and I discussed that we might be able to fix this with converting almost all optional<string>'s into just strings where "" is "no value", and having null explicitly map to the empty string.

(in addition to getting rid of the aforementioned optionals that aren't really optionals)

Make Profile a WinRT object (#7283)

Navigation Menu

Repository navigation

Handling Nullable Profile Settings #7435

Proposed Implementation/Solution

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Handling Nullable Profile Settings #7435

Description

Proposed Implementation/Solution

Activity

DHowett commented on Aug 27, 2020

DHowett commented on Aug 27, 2020

carlos-zamora commented on Aug 27, 2020

zadjii-msft commented on Aug 27, 2020

zadjii-msft commented on Aug 27, 2020

zadjii-msft commented on Aug 27, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions