-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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 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.
@@ -1318,7 +1323,7 @@ void AdaptDispatch::RequestChecksumRectangularArea(const VTInt id, const VTInt p | |||
{ | |||
uint16_t checksum = 0; | |||
// If this feature is not enabled, we'll just report a zero checksum. | |||
if constexpr (Feature_VtChecksumReport::IsEnabled()) | |||
if (Feature_VtChecksumReport::IsEnabled() && _vtChecksumReportEnabled) |
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 thought the compiler would complain if you put a constexpr bool into the same if condition as a regular bool. Are the warnings broken for the adapter project? Hmm...
(It doesn't really matter though because if constexpr
is mostly syntactic sugar. The compiler can also remove all other constant if conditions if optimizations are enabled.)
If the feature now has a bool member, do we still need the feature flag?
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.
Hmm... For WT, probably not. I'm guessing we'll still want it for WindowsInbox
though? I'll update the feature flag to reflect that.
CC @DHowett
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.
Though I guess _vtChecksumReportEnabled
is false
by default so it'd be the same either way haha. Imma just remove the flag altogether 😅
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 will want to keep it turned off for WindowsInbox. Therefore: keep the if constexpr
and wrap it around the if (not constant expression)
, or flip the sense of the constexpr if to reduce nesting.
FWIW: I prefer that we keep all velocity-related if constexpr
separate and on their own condition lines. It makes it easier to clean up, and easier to diff when cleaned up.
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 ( |
@@ -35,6 +35,7 @@ dataobject | |||
dcomp | |||
debugbreak | |||
delayimp | |||
DECRQRCA |
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.
Instead of adding the incorrect spelling to the dictionary, could we not just use the correct spelling in the code? 😉 It should be DECRQCRA
.
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. |
Summary of the Pull Request
Adds a compatibility page to the settings UI. This page exposes several existing settings and introduces a few new settings:
Several smaller changes were accomplished as a part of this PR:
experimental.input.forceVT
was renamed tocompatibility.input.forceVT
compatibility.allowVtChecksumReport
settingcompatibility.allowKeypadMode
settingcompatibility.allowHeadless
(which was missing)Feature_VtChecksumReport
feature flagA part of #10000
Closes #16672