-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
#459 IME UI does not follow the cursor in Windows Terminal #1919
#459 IME UI does not follow the cursor in Windows Terminal #1919
Conversation
…lnach/terminal into dev/philnach/459-TSF30-IME_2
This is so cool. How did you resolve the bug where switching away while the IME was open would tank all input? |
Good question, it was due to calling NotifyFocusLeave. Apparently pulling the rug out from under the IME didn't leave it in a good state. I need to follow-up on how Focus is handled with the Terminal Control and Tabs, but for now not calling NotifyFocusLeave leaves the IME in a good state. I want to get this code out there to start to gather feedback. We can tweak Focus along the way. |
originally I planned on setting them from the TerminalControl, however, this wasn't a good idea because it would put too much TSFInputControl code into the TerminalControl. Also, removed some commented out code and increased the TAB_OFFSET to 38 vs 34 amount.
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 know you're still Draft/WIP, but I gave it a quick once-over.
We now have that WinRT Utils library Michael mentioned that might be good for ColorRefToColor! 😄 |
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.
(partway through)
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 honestly? don't have any problem with this, but I'd like to see a bit of macro cleanup. I'm still going to approve it, because dang, it's way way way better than what we have.
_tsfInputControl.CompositionCompleted({ this, &TermControl::_CompositionCompleted }); | ||
_tsfInputControl.CurrentCursorPosition({ this, &TermControl::_CurrentCursorPositionHandler }); | ||
_tsfInputControl.CurrentFontInfo({ this, &TermControl::_FontInfoHandler }); | ||
container.Children().Append(_tsfInputControl); |
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.
we may need to tell the accessibility subsystem something about this, but I am not sure what. @carlos-zamora?
To the actual person who does the merge here: please copy Phil's excellent description into the commit message and get rid of the other cruft github puts in there! |
Co-Authored-By: Dustin L. Howett (MSFT) <[email protected]>
@philnach, I've got it! Anyway, it's actually in device-independent pixels... So, to go from a point size to a DIP size, you need to
then pass dips along to the TextBlock 😁 |
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.
Largely I'm happy with this, but there's a bunch of nits commented throughout, so I want to give you a chance to clean those up. I'll come back and signoff near 3pm PST regardless, because this looks so SO much better than having nothing.
Thanks!
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.
😀
double TermControl::_GetAutoScrollSpeed(double cursorDistanceFromBorder) const | ||
{ | ||
// The numbers below just feel well, feel free to change. | ||
// TODO: Maybe account for space beyond border that user has available |
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.
bare todo
Glad to have this. We're not perfect yet, but at least we now have a foundation to work from when it comes to IME. |
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.
It works, I'm shipping it.
BATON: I am making the commit here. |
🎉 Handy links: |
Summary of the Pull Request
TerminalControl doesn't use any of the built in text input and edit controls provided by XAML
for text input, which means TermianlControl needs to communicate with the Text Services Framework (TSF)
in order to provide Input Method Editor (IME) support. Just like the rest of Terminal we get
to take advantage of newer APIs (Windows.UI.Text.Core namespace) to provide support vs. the old TSF 1.0.
Windows.UI.Text.Core handles communication between a text edit control and the text services primarily
through a CoretextEditContext object.
This change introduces a new UserControl TSFInputControl which is a custom EditControl similar to the CustomEditControl sample
TSFInputControl is similar (overlay with IME text) to how old console (conimeinfo).
References
PR Checklist
Detailed Description of the Pull Request / Additional comments
TSFInputControl is a Windows.UI.Xaml.Controls.UserControl
TSFInputControl contains a Canvas control for absolution positioning a TextBlock control within it's containing control (TerminalControl).
The TextBlock control is used for displaying candidate text from the IME. When the user makes a choice in the IME the TextBlock is cleared and the text is written to the Terminal buffer like normal text.
TSFInputControl creates an instance of the CoreTextEditContext and attaches appropriate event handlers to CoreTextEditContext in order to interact with the IME.
A good write-up on how to interact with CoreTextEditContext can be found here: https://docs.microsoft.com/en-us/windows/uwp/design/input/custom-text-input
Text Updates:
Text updates from the IME come in on the TextUpdating event handler, text updates are stored in an internal buffer (_inputBuffer).
Completed Text:
Once a user selects a text in the IME, the CompositionCompleted handler is invoked. The input buffer (_inputBuffer) is written to the Terminal buffer, _inputBuffer is cleared and Canvas and TextBlock controls are hidden until the user starts a composition session again.
Positioning:
Telling the IME where to properly position itself was the hardest part of this change. The IME expects to know it's location in screen coordinates as supposed to client coordinates. This is pretty easy if you are a pure UWP, but since we are hosted inside a XAMLIsland the client to screen coordinate translation is a little harder.
How screen coordinate is calculated is by:
Once we have the right position in screen coordinates, this is supplied in the LayoutBounds of the CoreTextLayoutRequestedEventArgs which let's the IME know where to position itself on the Screen.
Font Information/Cursor/Writing to Terminal:
3 events were added to the TSFInputControl to create a loosely coupled implementation between the TerminalControl and the TSFInputControl. These events are used for obtaining Font information from the TerminalControl, getting the Cursor position and writing to the terminal buffer.
Known issues with this change:
Future considerations:
Ideally, we'd be able to interact with the console buffer directly and replace characters as the user types.
Validation Steps Performed
General steps to try functionality
Scenarios validated: