-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add a Compatibility and Terminal page to the Settings UI #17895
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also move the compatibility.textMeasurement
setting from the rendering page over to this new one.
This comment was marked as outdated.
This comment was marked as outdated.
Is it too late to change the JSON names used for the VT settings? Because I think it would be nice if they could be consistently named after the control sequences that they apply to, especially since we could be adding quite a few more of these at some point in the future. One possibility would be to use the control sequence acronyms, i.e. something like this:
Or if you prefer a more descriptive name, then maybe something that more closely matches the actual control names, like this:
That said, have you tested either of the input mode settings? Because I have a strong suspicion that they won't actually work. As far I understand it, the VT input is largely handled in conhost, so a setting that is only applied to Windows terminal isn't necessarily going to work the way you're expecting it to. Another point I wanted to raise was whether these settings might be better at the profile level? Because I can imagine there will be some VT operations that someone might need for compatibility with a specific application, but which they wouldn't necessarily want enabled in general because of security concerns ( |
Are all of the compatibility settings globals? I thought that most of them were actually profile settings. How do we resolve that? How do users set them per-profile? FWIW about debugFeatures: that one is hidden for a reason. I do not know if we should introduce it to the SUI. |
We're able to change them. We'd just have to still support the old keys, but we do that all the time. If we're renaming them, I'd prefer to use the control sequence acronyms for the JSON, actually. It's shorter to type out and the schema provides additional details to make it more user friendly. We can provide a more descriptive name and help text in the settings UI.
I haven't, but these settings have been around for a while. I'm just exposing them to the settings UI. You're talking specifically about the
Nope. They're all globals. We could convert them to profile settings like this though:
What's the reason? We're already exposing this setting via the JSON. We're just adding another method to toggle it. I think it's important to carry it over so that we treat SUI as a first-class citizen like the JSON. |
Yeah, me too.
Well forceVT has only been an experimental setting which I suspect nobody has used (I recently tried it and couldn't get it to work meaningfully). And keypadMode is only a build setting, which works because it applies to both conhost and WT - making that a runtime setting is where it becomes problematic. And if it was a just a matter of bug fixing, I wouldn't have a problem in leaving that for later. But my concern is that these options may not be feasible at all, in which case it seems premature to be adding them to the UI. Although if someone else has an idea of how they could be made to work, that's fine. I might be missing something obvious. |
This comment has been minimized.
This comment has been minimized.
Got it. That's fair. I'll take a closer look to see if these work tomorrow then. |
Demo (Updated for bb1521b) |
src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw
Outdated
Show resolved
Hide resolved
src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
Outdated
Show resolved
Hide resolved
259de45
to
cb661bc
Compare
This comment has been minimized.
This comment has been minimized.
Alright! Got a chance to take a look at the two input mode settings. Let me preface this by saying that this isn't an area I'm too familiar with, but trying my best here (and any help/feedback would be very appreciated 🙂). Here's my findings.
|
@carlos-zamora I think perhaps it's just me not understanding what it was intended to do. I expected it would disable win32 input mode completely, so it would be impossible for an application to turn that on (which is why I was suggesting the name However, if it's just meant to control the internal conpty encoding, then I think it's fine as it is. And in that case I think the current setting name is also fine as it is. |
(It really should - we just hooked it up to the part that requests conpty to request win32im, rather than the part of terminal that responds to it. You're totally right.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some style nits! Thanks for doing so much!
@@ -90,6 +90,11 @@ void Terminal::UpdateSettings(ICoreSettings settings) | |||
_autoMarkPrompts = settings.AutoMarkPrompts(); | |||
_rainbowSuggestions = settings.RainbowSuggestions(); | |||
|
|||
if (_stateMachine) | |||
{ | |||
SetVtChecksumReportSupport(settings.AllowVtChecksumReport()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if stateMachine
comes along later but there's no UpdateSettings call?
Visibility="{x:Bind ViewModel.DebugFeaturesAvailable}"> | ||
<ToggleSwitch IsOn="{x:Bind ViewModel.DebugFeaturesEnabled, Mode=TwoWay}" | ||
Style="{StaticResource ToggleSwitchInExpanderStyle}" | ||
Visibility="{x:Bind ViewModel.DebugFeaturesEnabled}" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this binding will actually make the switch disappear when it's off lmao
@@ -552,10 +552,46 @@ | |||
<value>Always on top</value> | |||
<comment>Header for a control to toggle if the app will always be presented on top of other windows, or is treated normally (when disabled).</comment> | |||
</data> | |||
<data name="Profile_ForceVTInput.Header" xml:space="preserve"> | |||
<value>Force the terminal to use the legacy input encoding</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<value>Force the terminal to use the legacy input encoding</value> | |
<value>Use the legacy input encoding</value> |
<comment>Header for a control to toggle support for Windows Terminal to run in the background.</comment> | ||
</data> | ||
<data name="Globals_IsolatedMode.Header" xml:space="preserve"> | ||
<value>Force each window to be its own process</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<value>Force each window to be its own process</value> | |
<value>Force each window to run in its own process</value> |
<comment>Header for a control to toggle making each window be its own process.</comment> | ||
</data> | ||
<data name="Globals_DebugFeaturesEnabled.Header" xml:space="preserve"> | ||
<value>Debug Mode</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<value>Debug Mode</value> | |
<value>Enable debugging features (useful for troubleshooting or developing Terminal)</value> |
??
<comment>Additional description for what the "allow headless" setting does. Presented near "Globals_AllowHeadless.Header".</comment> | ||
</data> | ||
<data name="Globals_IsolatedMode.HelpText" xml:space="preserve"> | ||
<value>Certain features including but not limited to global hotkeys, tab drag and drop, and running commandlines in existing windows won't work.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<value>Certain features including but not limited to global hotkeys, tab drag and drop, and running commandlines in existing windows won't work.</value> | |
<value>Certain features will not work properly, including—but not limited to—global hotkeys, tab drag and drop, and wt.exe window targeting.</value> |
</data> | ||
<data name="Globals_IsolatedMode.HelpText" xml:space="preserve"> | ||
<value>Certain features including but not limited to global hotkeys, tab drag and drop, and running commandlines in existing windows won't work.</value> | ||
<comment>Additional description for what the "isolated mode" setting does. Presented near "Globals_IsolatedMode.Header".</comment> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<comment>Additional description for what the "isolated mode" setting does. Presented near "Globals_IsolatedMode.Header".</comment> | |
<comment>{Locked="wt.exe"} Additional description for what the "isolated mode" setting does. Presented near "Globals_IsolatedMode.Header".</comment> |
@@ -660,6 +700,10 @@ | |||
<value>Advanced</value> | |||
<comment>Header for a sub-page of profile settings focused on more advanced scenarios.</comment> | |||
</data> | |||
<data name="Profile_Terminal.Header" xml:space="preserve"> | |||
<value>Terminal</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<value>Terminal</value> | |
<value>Terminal Emulation</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
terminal emulation is what we do!
@@ -124,8 +124,6 @@ namespace SettingsModelUnitTests | |||
"trimPaste": true, | |||
"experimental.input.forceVT": false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a test for migration? optional
@@ -179,4 +179,11 @@ | |||
</alwaysEnabledBrandingTokens> | |||
</feature> | |||
|
|||
<feature> | |||
<name>Feature_DebugMode</name> | |||
<description>Enables the debug mode setting</description> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<description>Enables the debug mode setting</description> | |
<description>Enables UI access to the debug mode setting</description> |
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the feature name makes it sound quite ominous
Summary of the Pull Request
Adds a global Compatibility page to the settings UI. This page exposes several existing settings and introduces a few new settings:
This also adds a Terminal subpage for profiles in the settings UI. This page includes:
Several smaller changes were accomplished as a part of this PR:
experimental.input.forceVT
was renamed tocompatibility.input.forceVT
compatibility.allowDECRQCRA
settingcompatibility.allowHeadless
(which was missing)Feature_DebugMode
feature flag to control if debug features should be shown in the SUIA part of #10000
Closes #16672