Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

mkitzan
Copy link
Contributor

@mkitzan mkitzan commented Dec 12, 2019

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

Detailed 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 correct INPUT_RECORDs 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

@DHowett-MSFT
Copy link
Contributor

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 closes. Closes #x, closes #y, closes #z. 😄

@mkitzan mkitzan closed this Dec 12, 2019
@DHowett-MSFT
Copy link
Contributor

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

@mkitzan
Copy link
Contributor Author

mkitzan commented Dec 12, 2019

No problem. I'll reopen separate ones for each.

@mkitzan mkitzan deleted the three-piece-fixes branch December 12, 2019 18:39
@mkitzan
Copy link
Contributor Author

mkitzan commented Dec 12, 2019

For the future, though: github only works "properly" if you repeat the word closes. Closes #x, closes #y, closes #z. 😄

If that's the case then issue #2248 should be closed because of this PR.

@DHowett-MSFT
Copy link
Contributor

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

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;
}

Copy link
Contributor Author

@mkitzan mkitzan Dec 12, 2019

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.

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)

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants