Skip to content

Commit

Permalink
Adjust TSFInputControl alignment (#5609)
Browse files Browse the repository at this point in the history
This PR fixes a couple of issues with TSFInputControl alignment:

1. The emoji picker IME in particular would show up overlapping the
   current buffer row because I stupidly didn't realize that
   `TextBlock.ActualHeight` is 0 right after initialization and before
   it has any text. So, the emoji picker will show up on the bottom of
   the 0 height TextBlock, which overlaps the current buffer row. This
   isn't a problem with CJK because inputting text _causes_ the IME to
   show up, so by the time the IME shows up, the TextBlock has text and
   accordingly has a height.
2. It turns out the emoji picker IME doesn't follow the `TextBlock`
   bottom, unlike Chinese and Japanese IME, so if a user were to compose
   near the edge of the screen and let the `TextBlock` start line
   wrapping, the emoji IME doesn't follow the bottom of the `TextBlock`
   as it grows. This means that the `TextBlock` position doesn't update
   in the middle of composition, and the `LayoutRequested` event that it
   fires at the beginning of composition is the only chance we get to
   tell the emoji IME where to place itself. It turns out when we reset
   `TextBlock.Text`, the ActualHeight doesn't get immediately reset back
   to the min size. So if a user were to bring up the emoji IME before
   `ActualHeight` is reset, the IME will show up way below the current
   buffer row.
3. We don't currently `TryRedrawCanvas` when the window position changes
   (resizing, dragging the window around), so sometimes dragging or
   resizing the Terminal doesn't update the position of the IME. Ideally
   it should be listening to some "window position changed" event, but
   alas we don't have that and it would be much harder than this
   incoming fix. We'll just track the window bounds as part of
   `TryRedrawCanvas` and redraw if it changes. For the most part, this
   will allow the IME to update to where the new window position is, but
   it'll only be called if we receive a `LayoutRequested` event.

## PR Checklist
* [x] Closes #5470
* [x] CLA signed.
* [x] Tests added/passed

## Validation Steps Performed
I play with it for quite a bit.
  • Loading branch information
leonMSFT committed Apr 28, 2020
1 parent 97d6456 commit 64f8bbf
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 8 deletions.
33 changes: 25 additions & 8 deletions src/cascadia/TerminalControl/TSFInputControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
_currentCanvasWidth{ 0.0 },
_currentTextBlockHeight{ 0.0 },
_currentTextBounds{ 0, 0, 0, 0 },
_currentControlBounds{ 0, 0, 0, 0 }
_currentControlBounds{ 0, 0, 0, 0 },
_currentWindowBounds{ 0, 0, 0, 0 }
{
InitializeComponent();

Expand Down Expand Up @@ -144,20 +145,22 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
_CurrentCursorPositionHandlers(*this, *cursorArgs);
const til::point cursorPos{ gsl::narrow_cast<ptrdiff_t>(cursorArgs->CurrentPosition().X), gsl::narrow_cast<ptrdiff_t>(cursorArgs->CurrentPosition().Y) };

const double actualCanvasWidth = Canvas().ActualWidth();

const double actualTextBlockHeight = TextBlock().ActualHeight();
const double actualCanvasWidth{ Canvas().ActualWidth() };
const double actualTextBlockHeight{ TextBlock().ActualHeight() };
const auto actualWindowBounds{ CoreWindow::GetForCurrentThread().Bounds() };

if (_currentTerminalCursorPos == cursorPos &&
_currentCanvasWidth == actualCanvasWidth &&
_currentTextBlockHeight == actualTextBlockHeight)
_currentTextBlockHeight == actualTextBlockHeight &&
_currentWindowBounds == actualWindowBounds)
{
return;
}

_currentTerminalCursorPos = cursorPos;
_currentCanvasWidth = actualCanvasWidth;
_currentTextBlockHeight = actualTextBlockHeight;
_currentWindowBounds = actualWindowBounds;

_RedrawCanvas();
}
Expand Down Expand Up @@ -192,12 +195,20 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation

// calculate FontSize in pixels from Points
const double fontSizePx = (fontSize.height<double>() * 72) / USER_DEFAULT_SCREEN_DPI;
const double unscaledFontSizePx = fontSizePx / scaleFactor;

// Make sure to unscale the font size to correct for DPI! XAML needs
// things in DIPs, and the fontSize is in pixels.
TextBlock().FontSize(fontSizePx / scaleFactor);
TextBlock().FontSize(unscaledFontSizePx);
TextBlock().FontFamily(Media::FontFamily(fontArgs->FontFace()));

// TextBlock's actual dimensions right after initialization is 0w x 0h. So,
// if an IME is displayed before TextBlock has text (like showing the emoji picker
// using Win+.), it'll be placed higher than intended.
TextBlock().MinWidth(unscaledFontSizePx);
TextBlock().MinHeight(unscaledFontSizePx);
_currentTextBlockHeight = std::max(unscaledFontSizePx, _currentTextBlockHeight);

const auto widthToTerminalEnd = _currentCanvasWidth - clientCursorInDips.x<double>();
// Make sure that we're setting the MaxWidth to a positive number - a
// negative number here will crash us in mysterious ways with a useless
Expand All @@ -207,8 +218,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation

// Get window in screen coordinates, this is the entire window including
// tabs. THIS IS IN DIPs
const auto windowBounds{ CoreWindow::GetForCurrentThread().Bounds() };
const til::point windowOrigin{ til::math::flooring, windowBounds };
const til::point windowOrigin{ til::math::flooring, _currentWindowBounds };

// Get the offset (margin + tabs, etc..) of the control within the window
const til::point controlOrigin{ til::math::flooring,
Expand Down Expand Up @@ -434,6 +444,13 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation

TextBlock().Text(L"");

// After we reset the TextBlock to empty string, we want to make sure
// ActualHeight reflects the respective height. It seems that ActualHeight
// isn't updated until there's new text in the TextBlock, so the next time a user
// invokes "Win+." for the emoji picker IME, it would end up
// using the pre-reset TextBlock().ActualHeight().
TextBlock().UpdateLayout();

// hide the controls until text input starts again
Canvas().Visibility(Visibility::Collapsed);
}
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalControl/TSFInputControl.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
double _currentTextBlockHeight;
winrt::Windows::Foundation::Rect _currentControlBounds;
winrt::Windows::Foundation::Rect _currentTextBounds;
winrt::Windows::Foundation::Rect _currentWindowBounds;
};
}
namespace winrt::Microsoft::Terminal::TerminalControl::factory_implementation
Expand Down

0 comments on commit 64f8bbf

Please sign in to comment.