starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-09-25 00:15:10 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

Handling Nullable Profile Settings #10337

New Issue
Closed
opened 2026-01-31 02:18:45 +00:00 by claunia · 7 comments
claunia commented 2026-01-31 02:18:45 +00:00
Owner
Copy Link

Originally created by @carlos-zamora on GitHub (Aug 27, 2020).

Windows Terminal version: 1.3.2382.0

Related to...

  • #7423: missing test for nullable startingDirectory
  • #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:

  1. "backgroundImage" (std::optional<std::wstring>)
    • explicitly do not have a background image
  2. "snapOnInput" (bool)
    • fall back to profiles.defaults value
  3. "startingDirectory" (std::optional<std::wstring>)
    • inherit the startingDirectory of the parent process
  4. "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:

  1. 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
  2. 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.

Originally created by @carlos-zamora on GitHub (Aug 27, 2020). ``` Windows Terminal version: 1.3.2382.0 ``` Related to... - #7423: missing test for nullable startingDirectory - #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: 1. `"backgroundImage"` (`std::optional<std::wstring>`) - explicitly do not have a background image 2. `"snapOnInput"` (`bool`) - fall back to profiles.defaults value 3. `"startingDirectory"` (`std::optional<std::wstring>`) - inherit the startingDirectory of the parent process 4. `"commandline"` (`std::wstring`) - fall back to profiles.defaults value From an implementation standpoint, it makes sense. `std:optional`s treat `null` as a meaningful value, whereas non-`std::optional`s treat null as fall back. The problem here is 2-fold: 1. 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 2. 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.
claunia commented 2026-01-31 02:18:46 +00:00
Author
Owner
Copy Link

@DHowett commented on GitHub (Aug 27, 2020):

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?"

@DHowett commented on GitHub (Aug 27, 2020): 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`?"
claunia commented 2026-01-31 02:18:46 +00:00
Author
Owner
Copy Link

@DHowett commented on GitHub (Aug 27, 2020):

I disagree with some of these assertions of inconsistent behavior.

  1. "backgroundImage" (std::optional<std::wstring>)
    • explicitly do not have a background image
  2. "snapOnInput" (bool)
    • fall back to profiles.defaults value
  3. "startingDirectory" (std::optional<std::wstring>)
    • inherit the startingDirectory of the parent process
  4. "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.

@DHowett commented on GitHub (Aug 27, 2020): I disagree with some of these assertions of inconsistent behavior. > 1. `"backgroundImage"` (`std::optional<std::wstring>`) > - explicitly do not have a background image > 2. `"snapOnInput"` (`bool`) > - fall back to profiles.defaults value > 3. `"startingDirectory"` (`std::optional<std::wstring>`) > - inherit the startingDirectory of the parent process > 4. `"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.
claunia commented 2026-01-31 02:18:46 +00:00
Author
Owner
Copy Link

@carlos-zamora commented on GitHub (Aug 27, 2020):

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.

@carlos-zamora commented on GitHub (Aug 27, 2020): 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.
claunia commented 2026-01-31 02:18:47 +00:00
Author
Owner
Copy Link

@zadjii-msft commented on GitHub (Aug 27, 2020):

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))

@zadjii-msft commented on GitHub (Aug 27, 2020): 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))
claunia commented 2026-01-31 02:18:47 +00:00
Author
Owner
Copy Link

@zadjii-msft commented on GitHub (Aug 27, 2020):


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.

@zadjii-msft commented on GitHub (Aug 27, 2020): ```c++ 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`. <hr> 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.
claunia commented 2026-01-31 02:18:47 +00:00
Author
Owner
Copy Link

@zadjii-msft commented on GitHub (Aug 27, 2020):

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)

@zadjii-msft commented on GitHub (Aug 27, 2020): 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)
claunia commented 2026-01-31 02:18:48 +00:00
Author
Owner
Copy Link

@ghost commented on GitHub (Sep 22, 2020):

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

Handy links:

@ghost commented on GitHub (Sep 22, 2020): :tada:This issue was addressed in #7283, which has now been successfully released as `Windows Terminal Preview v1.4.2652.0`.:tada: Handy links: * [Release Notes](https://github.com/microsoft/terminal/releases/tag/v1.4.2652.0) * [Store Download](https://www.microsoft.com/store/apps/9n8g5rfz9xk3?cid=storebadge&ocid=badge)
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 Area-Settings Issue-Task Needs-Triage Product-Terminal Resolution-Fix-Committed
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#10337
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?