starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-02-04 05:35:20 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

[PR #8175] Rework JsonUtils' optional handling to let Converters see null #27132

New Issue
Open
opened 2026-01-31 09:20:11 +00:00 by claunia · 0 comments
claunia commented 2026-01-31 09:20:11 +00:00
Owner
Copy Link

Original Pull Request: https://github.com/microsoft/terminal/pull/8175

State: closed
Merged: Yes

The JsonUtils changes in #8018 revealed that we need more robust,
configurable optional handling. We learned that there's a class of
values that was previously underrepresented in our API: strings that
have an explicit empty value.

The Settings model supports starting directory, icon, background image
et al values that are empty. That emptiness overrides a value set in a
lower layer, so it is not sufficient to represent the empty value for
any one of those fields as an unset optional.

There are a couple other settings for which we've implemented a
hand-rolled option type (for roughly the same reason): foreground,
background, any color fields that override values from the color scheme
or the lower layer profile.

These requirements are best fulfilled by better optional support in
JsonUtils. Where the library would originally detect known types of
optional and pre-filter them out during GetValue and SetValue, it
will now defer to another conversion trait.

This commit introduces a helper conversion trait and an "option oracle".
The conversion trait will use the option oracle to detect emptiness,
generate empty option values, and read values out of option types. In so
doing, the trait is insulated from the implementation details of any
specific option type.

Any special logic for handling JSON null and option types has been
stripped from GetValue. Due to this, there is an express change in
behavior for some converters:

  • GetValue<T>(jsonNull) where T is not an option type[1] has
    been upgraded from a silent no-op to an exception.

Further, I took the opportunity to replace NullableSetting with
std::optional<std::optional>, which accurately represents "setting
that the user might explicitly clear". I've added a test to
JsonUtilsTests to make sure it can serialize/deserialize double
optionals the way we expect it to.

Tests (Local, Unit for TerminalApp/SettingsModel):
Summary: Total=140, Passed=140, Failed=0, Blocked=0, Not Run=0, Skipped=0

[1]: Explicitly, if T is not an option type and the converter does
not support null.

**Original Pull Request:** https://github.com/microsoft/terminal/pull/8175 **State:** closed **Merged:** Yes --- The JsonUtils changes in #8018 revealed that we need more robust, configurable optional handling. We learned that there's a class of values that was previously underrepresented in our API: _strings that have an explicit empty value_. The Settings model supports starting directory, icon, background image et al values that are empty. That emptiness _overrides_ a value set in a lower layer, so it is not sufficient to represent the empty value for any one of those fields as an unset optional. There are a couple other settings for which we've implemented a hand-rolled option type (for roughly the same reason): foreground, background, any color fields that override values from the color scheme _or_ the lower layer profile. These requirements are best fulfilled by better optional support in JsonUtils. Where the library would originally detect known types of optional and pre-filter them out during `GetValue` and `SetValue`, it will now defer to another conversion trait. This commit introduces a helper conversion trait and an "option oracle". The conversion trait will use the option oracle to detect emptiness, generate empty option values, and read values out of option types. In so doing, the trait is insulated from the implementation details of any specific option type. Any special logic for handling JSON null and option types has been stripped from GetValue. Due to this, there is an express change in behavior for some converters: * `GetValue<T>(jsonNull)` where `T` is **not** an option type[1] has been upgraded from a silent no-op to an exception. Further, I took the opportunity to replace NullableSetting with std::optional<std::optional<T>>, which accurately represents "setting that the user might explicitly clear". I've added a test to JsonUtilsTests to make sure it can serialize/deserialize double optionals the way we expect it to. Tests (Local, Unit for TerminalApp/SettingsModel): Summary: Total=140, Passed=140, Failed=0, Blocked=0, Not Run=0, Skipped=0 [1]: Explicitly, if `T` is not an option type _and the converter does not support null_.
claunia added the pull-request label 2026-01-31 09:20:11 +00:00
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 pull-request
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#27132
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?