-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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
Ctrl+Backspace works, and two other small fixes #3934
Conversation
NAK on combining these three different bug fixes into one pull request -- sorry! We find ourselves bisecting fairly often, and it makes it a lot easier to reason about when each change is fairly pure. For the future, though: github only works "properly" if you repeat the word |
(also, thank for you doing this: I appreciate how willing you are to jump into things and deal with our somewhat opaque (at times) review requirements!) |
No problem. I'll reopen separate ones for each. |
Oh dang, excellent catch! Thanks! |
@@ -382,15 +382,18 @@ bool Terminal::SetWindowTitle(std::wstring_view title) | |||
// - true iff we successfully updated the color table entry. | |||
bool Terminal::SetColorTableEntry(const size_t tableIndex, const COLORREF dwColor) | |||
{ | |||
if (tableIndex > _colorTable.size()) | |||
try |
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.
Note: I am only looking at this because I am invested in the ctrl+backspace functionality, I have no idea what coding standards are going on here.
You should probably nix the exception handling and use _colorTable.find
like this:
auto colorTableItr = _colorTable.find(tableIndex);
if (colorTableItr != _colorTable.end()) {
*colorTableItr = dwColor;
...
}
else {
return 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.
_colorTable
is a std::array
which doesn't have a find
function. Would be an clean way if it did support find
though.
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.
Ah, in that case you could std::find(_colorTable.begin(), _colorTable.end(), tableIndex)
! (it just does a linear search)
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.
Sticking with random access+exception makes the most sense for this function. Calling it with an invalid value is an exceptional event which requires some really arcane input to repro.
Summary of the Pull Request
Changes the Ctrl+Backspace input sequence and how it is processed by
InputStateMachineEngine
. Now Ctrl+Backspace deletes a whole word at a time (tested on WSL, CMD, and PS).PR also fixes two other issues which just needed some someone to implement them.
References
PR Checklist
ResizePseudoConsole
returnsS_OK
#3447, Trying to define color 256 causes temporary hang #3720Detailed Description of the Pull Request / Additional comments
Changed the input sequence for Ctrl+Backspace to
\x1b\x8
so the sequence would pass through_DoControlCharacter
. Changed_DoControlCharacter
to process\b
in a way which forms the correctINPUT_RECORD
s to delete whole words.Fixed #3447 by verifying that
COORD
X,Y values are positive.Fixed #3720 by wrapping the
at
call in a try-catch.Validation Steps Performed
Ctrl+Backspace works 🎉
Can no longer repro #3720