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.

Sign up for GitHub

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

Clean up TSFInputControl a bit #13677

Merged
merged 1 commit into from
Aug 8, 2022
Merged

Clean up TSFInputControl a bit #13677

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 32 additions & 65 deletions src/cascadia/TerminalControl/TSFInputControl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
#include "TSFInputControl.h"
#include "TSFInputControl.g.cpp"

#include <Utils.h>

using namespace winrt::Windows::Foundation;
using namespace winrt::Windows::Graphics::Display;
using namespace winrt::Windows::UI::Core;
Expand All @@ -16,17 +14,7 @@ using namespace winrt::Windows::UI::Xaml;

namespace winrt::Microsoft::Terminal::Control::implementation
{
TSFInputControl::TSFInputControl() :
_editContext{ nullptr },
_inComposition{ false },
_activeTextStart{ 0 },
_focused{ false },
_currentTerminalCursorPos{ 0, 0 },
_currentCanvasWidth{ 0.0 },
_currentTextBlockHeight{ 0.0 },
_currentTextBounds{ 0, 0, 0, 0 },
_currentControlBounds{ 0, 0, 0, 0 },
_currentWindowBounds{ 0, 0, 0, 0 }
TSFInputControl::TSFInputControl()
{
InitializeComponent();

Expand Down Expand Up @@ -83,11 +71,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// - <none>
void TSFInputControl::NotifyFocusEnter()
{
if (_editContext != nullptr)
{
_editContext.NotifyFocusEnter();
_focused = true;
}
_editContext.NotifyFocusEnter();
_focused = true;
Comment on lines +74 to +75
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing in this class sets _editContext to nullptr. 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Good point. We'll totally blow up in the ctor long before we get here.

}

// Method Description:
Expand All @@ -99,11 +84,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// - <none>
void TSFInputControl::NotifyFocusLeave()
{
if (_editContext != nullptr)
{
_editContext.NotifyFocusLeave();
_focused = false;
}
_editContext.NotifyFocusLeave();
_focused = false;
}

// Method Description:
Expand All @@ -117,14 +99,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
if (!_inputBuffer.empty())
{
TextBlock().Text(L"");
const auto bufLen = ::base::ClampedNumeric<int32_t>(_inputBuffer.length());
_inputBuffer.clear();
_selection = {};
_activeTextStart = 0;
_editContext.NotifyFocusLeave();
_editContext.NotifyTextChanged({ 0, bufLen }, 0, { 0, 0 });
_editContext.NotifyTextChanged({ 0, INT32_MAX }, 0, _selection);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does passing INT32_MAX here instead of _inputBuffer.length get us? Is there

Oh right we're clearing the buffer. Yea, blow away everything. makes sense.

_editContext.NotifyFocusEnter();
_activeTextStart = 0;
_inComposition = false;
TextBlock().Text({});
}
}

Expand Down Expand Up @@ -303,12 +284,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void TSFInputControl::_compositionCompletedHandler(CoreTextEditContext sender, const CoreTextCompositionCompletedEventArgs& /*args*/)
{
_inComposition = false;

// only need to do work if the current buffer has text
if (!_inputBuffer.empty())
{
_SendAndClearText();
}
Comment on lines -308 to -311
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_inputBuffer.empty() doesn't really matter. What matters is whether _activeTextStart == _inputBuffer.size(), because _activeTextStart indicates the actual start of the text we currently edit.

_SendAndClearText();
}

// Method Description:
Expand Down Expand Up @@ -336,16 +312,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// - <none>
void TSFInputControl::_textRequestedHandler(CoreTextEditContext sender, const CoreTextTextRequestedEventArgs& args)
{
// the range the TSF wants to know about
const auto range = args.Request().Range();

try
{
const auto textEnd = ::base::ClampMin<size_t>(range.EndCaretPosition, _inputBuffer.length());
const auto length = ::base::ClampSub<size_t>(textEnd, range.StartCaretPosition);
const auto textRequested = _inputBuffer.substr(range.StartCaretPosition, length);

args.Request().Text(textRequested);
const auto range = args.Request().Range();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need to clamp here anymore?

const auto text = _inputBuffer.substr(
range.StartCaretPosition,
range.EndCaretPosition - range.StartCaretPosition);
args.Request().Text(text);
}
CATCH_LOG();
}
Expand All @@ -360,8 +333,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// - args: CoreTextSelectionRequestedEventArgs for providing data for the SelectionRequested event. Not used in method.
// Return Value:
// - <none>
void TSFInputControl::_selectionRequestedHandler(CoreTextEditContext sender, const CoreTextSelectionRequestedEventArgs& /*args*/)
void TSFInputControl::_selectionRequestedHandler(CoreTextEditContext sender, const CoreTextSelectionRequestedEventArgs& args)
{
args.Request().Selection(_selection);
}

// Method Description:
Expand All @@ -374,8 +348,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// - args: CoreTextSelectionUpdatingEventArgs for providing data for the SelectionUpdating event. Not used in method.
// Return Value:
// - <none>
void TSFInputControl::_selectionUpdatingHandler(CoreTextEditContext sender, const CoreTextSelectionUpdatingEventArgs& /*args*/)
void TSFInputControl::_selectionUpdatingHandler(CoreTextEditContext sender, const CoreTextSelectionUpdatingEventArgs& args)
{
_selection = args.Selection();
args.Result(CoreTextSelectionUpdatingResult::Succeeded);
}

// Method Description:
Expand All @@ -388,24 +364,18 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// - <none>
void TSFInputControl::_textUpdatingHandler(CoreTextEditContext sender, const CoreTextTextUpdatingEventArgs& args)
{
const auto incomingText = args.Text();
const auto range = args.Range();

try
{
// When a user deletes the last character in their current composition, some machines
// will fire a CompositionCompleted before firing a TextUpdating event that deletes the last character.
// The TextUpdating will have a lower StartCaretPosition, so in this scenario, _activeTextStart
// needs to update to be the StartCaretPosition.
// A known issue related to this behavior is that the last character that's deleted from a composition
// will get sent to the terminal before we receive the TextUpdate to delete the character.
// See GH #5054.
_activeTextStart = ::base::ClampMin(_activeTextStart, ::base::ClampedNumeric<size_t>(range.StartCaretPosition));
const auto incomingText = args.Text();
const auto range = args.Range();

_inputBuffer = _inputBuffer.replace(
range.StartCaretPosition,
::base::ClampSub<size_t>(range.EndCaretPosition, range.StartCaretPosition),
range.EndCaretPosition - range.StartCaretPosition,
incomingText);
_selection = args.NewSelection();
// GH#5054: Pressing backspace might move the caret before the _activeTextStart.
_activeTextStart = ::base::ClampMin(_activeTextStart, ::base::ClampedNumeric<size_t>(range.StartCaretPosition));
Comment on lines +377 to +378
Copy link
Member Author

@lhecker lhecker Aug 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think "some machines will fire [...]". As far as I can see all machines do that, but it depends on the circumstances. I've thus changed the comment to just be about backspacing.


// Emojis/Kaomojis/Symbols chosen through the IME without starting composition
// will be sent straight through to the terminal.
Expand All @@ -432,22 +402,19 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
}

// Method Description:
// - Send the portion of the textBuffer starting at _activeTextStart to the end of the buffer.
// Then clear the TextBlock and hide it until the next time text is received.
// Arguments:
// - <none>
// Return Value:
// - <none>
void TSFInputControl::_SendAndClearText()
{
const auto text = _inputBuffer.substr(_activeTextStart);
if (text.empty())
{
return;
}

_CompositionCompletedHandlers(text);

_activeTextStart = _inputBuffer.length();
_activeTextStart = _inputBuffer.size();

TextBlock().Text(L"");
TextBlock().Text({});

// After we reset the TextBlock to empty string, we want to make sure
// ActualHeight reflects the respective height. It seems that ActualHeight
Expand Down
30 changes: 15 additions & 15 deletions src/cascadia/TerminalControl/TSFInputControl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void _textUpdatingHandler(winrt::Windows::UI::Text::Core::CoreTextEditContext sender, const winrt::Windows::UI::Text::Core::CoreTextTextUpdatingEventArgs& args);
void _formatUpdatingHandler(winrt::Windows::UI::Text::Core::CoreTextEditContext sender, const winrt::Windows::UI::Text::Core::CoreTextFormatUpdatingEventArgs& args);

void _SendAndClearText();
void _RedrawCanvas();

winrt::Windows::UI::Text::Core::CoreTextEditContext::TextRequested_revoker _textRequestedRevoker;
winrt::Windows::UI::Text::Core::CoreTextEditContext::SelectionRequested_revoker _selectionRequestedRevoker;
winrt::Windows::UI::Text::Core::CoreTextEditContext::FocusRemoved_revoker _focusRemovedRevoker;
Expand All @@ -68,22 +71,19 @@ namespace winrt::Microsoft::Terminal::Control::implementation
winrt::Windows::UI::Text::Core::CoreTextEditContext::CompositionStarted_revoker _compositionStartedRevoker;
winrt::Windows::UI::Text::Core::CoreTextEditContext::CompositionCompleted_revoker _compositionCompletedRevoker;

Windows::UI::Text::Core::CoreTextEditContext _editContext;

Windows::UI::Text::Core::CoreTextEditContext _editContext{ nullptr };
std::wstring _inputBuffer;

bool _inComposition;
size_t _activeTextStart;
void _SendAndClearText();
void _RedrawCanvas();
bool _focused;

til::point _currentTerminalCursorPos;
double _currentCanvasWidth;
double _currentTextBlockHeight;
winrt::Windows::Foundation::Rect _currentControlBounds;
winrt::Windows::Foundation::Rect _currentTextBounds;
winrt::Windows::Foundation::Rect _currentWindowBounds;
winrt::Windows::UI::Text::Core::CoreTextRange _selection{};
size_t _activeTextStart = 0;
bool _inComposition = false;
bool _focused = false;

til::point _currentTerminalCursorPos{};
double _currentCanvasWidth = 0.0;
double _currentTextBlockHeight = 0.0;
winrt::Windows::Foundation::Rect _currentControlBounds{};
winrt::Windows::Foundation::Rect _currentTextBounds{};
winrt::Windows::Foundation::Rect _currentWindowBounds{};
};
}
namespace winrt::Microsoft::Terminal::Control::factory_implementation
Expand Down