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

[RDY] win32 ime #6223

Closed
wants to merge 14 commits into from
Closed

[RDY] win32 ime #6223

wants to merge 14 commits into from

Conversation

yatli
Copy link
Contributor

@yatli yatli commented Jul 9, 2021

wip, does not work yet

wip, partially working.

Almost ready now.

Problems/TODO:

  • What is the origin for the rect in SetCursorRect? Treating them as screen coords kind of works, but the relative position inside a control will offset the IME with the cursor.
    • Screen coords work as intended
  • It seems that when coords are off by too much (out of screen?), the composition win goes back to the top left of screen, and candidate window jumps between top left and bottom right.
    • This is a TextBox bug. A TextBoxTextInputMethodClient bug.
  • Need to detect current locale and handle different IME placement conventions.
  • Need a way for the window impl to notify InputMethodManager that IME has been switched -- otherwise TextInputMethodClientRequestedEvent won't fire to register the new IME.
  • Composition content support (Let's save it for the next time...)
  • Bonus: client-side IME toggle

@yatli yatli changed the title win32 ime wip [RDY] win32 ime Jul 13, 2021
@robloo robloo mentioned this pull request Jul 17, 2021
@maxkatz6 maxkatz6 requested a review from kekekeks July 21, 2021 19:54
}
case WindowsMessage.WM_IME_SETCONTEXT:
{
// TODO if we implement preedit, disable the composition window:
Copy link
Member

Choose a reason for hiding this comment

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

Technically preedit and other advanced features are already in the API:

bool SupportsPreedit { get; }
/// <summary>
/// Sets the non-committed input string
/// </summary>
void SetPreeditText(string text);
/// <summary>
/// Indicates if text input client is capable of providing the text around the cursor
/// </summary>
bool SupportsSurroundingText { get; }
/// <summary>
/// Returns the text around the cursor, usually the current paragraph, the cursor position inside that text and selection start position
/// </summary>
TextInputMethodSurroundingText SurroundingText { get; }
/// <summary>
/// Should be fired when surrounding text changed
/// </summary>
event EventHandler SurroundingTextChanged;
/// <summary>
/// Returns the text before the cursor. Must return a non-empty string if cursor is not at the beginning of the text entry
/// </summary>
string TextBeforeCursor { get; }
/// <summary>
/// Returns the text before the cursor. Must return a non-empty string if cursor is not at the end of the text entry
/// </summary>
string TextAfterCursor { get; }

but we have this chicken and egg problem: platform code doesn't handle preedit and textbox doesn't support it as well.

Copy link
Member

Choose a reason for hiding this comment

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

I guess we need some control in the control catalog or sandbox to test the platform support.

NotifyInputMethodUpdated(root);
}

public void NotifyInputMethodUpdated(ITextInputMethodRoot? root)
Copy link
Member

Choose a reason for hiding this comment

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

Why exactly do we need this? Is it needed to handle keyboard layout switch between basic and IME-enabled languages?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if that should be handled on the xplat layer though. I'd expect that state to be tracked by the platform code that encapsulates such switches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how other users do this, but for me I heavily rely on win+space to switch between en/zh on the fly.
The ime manager queries client capabilities when it gets focus (or init?), but doesn't handle IME switch when the control is focused.

Copy link
Member

Choose a reason for hiding this comment

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

Well, I'd expect the WindowImpl.cs to always have an active Imm32InputMethod instance (just like X11Window does) that maintains its state rather than going through the requery process. Your approach also adds more ties between input method handling and toplevels while we are kinda planning to add built-in OSK support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WindowImpl always have that. InputMethodManager doesn't.

Copy link
Member

Choose a reason for hiding this comment

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

The initial idea was that InputMethodManager synchronizes the control with the native window implementation. The native window implementation is supposed to keep track of what was previously passed to it rather than requery that information on demand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. Maybe it's possible to put the mutable states in IMM32InputMethod.

@Mikolaytis
Copy link
Contributor

Good Job! can't wait to see this merged!

@Mikolaytis
Copy link
Contributor

I've merged this(with minor tweaks) to my private fork and our QA will test this until the weekend. I will notify about the result. For the first look - everything is perfect.

@Mikolaytis
Copy link
Contributor

Mikolaytis commented Nov 23, 2021

We are missing some things:

  1. Ability to toggle IME on/off on a InputElement like WPF's InputMethodEnabled
  2. IME is not applying FontSize to intermediate input.
  3. No way to configure margin/padding or offset of intermediate input.
  4. No way to disable intermediate input and/or candidates list.

Possible Improvement:
Currently TextInput event raised only by WM_CHAR windows message. To get the best IME support, like in chromium we should listen to all of the IME messages WM_IME_STARTCOMPOSITION,WM_IME_COMPOSITION,WM_IME_ENDCOMPOSITION and handle them properly, like WPF do.

@Mikolaytis
Copy link
Contributor

Mikolaytis commented Nov 24, 2021

Also here is a some bugs:

  1. IME is not working after any dialog window launch(OpenFileDialog/SaveFiledialog is working fine).
  2. On Windows 10 emoji panel position is not updated at all. Only the first show is in correct position. To update the position again - you should switch to another app and back.

@Mikolaytis Mikolaytis mentioned this pull request Nov 24, 2021
8 tasks
@maxkatz6
Copy link
Member

maxkatz6 commented Dec 13, 2021

This PR was completed with that one #7007
Thanks @yatli and @Mikolaytis

@maxkatz6 maxkatz6 closed this Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants