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

Add native Windows IME support #4941

Merged
merged 21 commits into from
Jan 11, 2022
Merged

Add native Windows IME support #4941

merged 21 commits into from
Jan 11, 2022

Conversation

Susko3
Copy link
Member

@Susko3 Susko3 commented Dec 17, 2021

SDL2 text input shows some problems, mainly the 10 character limit and selection not working properly.
Existing issues on SDL's side: libsdl-org/SDL#1879 and libsdl-org/SDL#3385.

So I decided to write my own implementation for Windows IME handling. This is completely transparent to TextBox and even to SDL2DesktopWindowTextInput. The composition text is no longer arbitrarily limited, and selection works as expected.

It's unfortunate that we can't use SDL_SetWindowsMessageHook as we do in WindowsMouseHandler (reasoning: eb44a5c). Seems a bit strange that this was the case, but the alternative way of receiving window messages works just fine (albeit with a minor issue).

Some native IMM/IME code adapted from chromium.


With this in, we're one step closer to providing an in-game candidate window for composition. I've already got the native part working, but adding a candidate window in-game is a bit daunting to me.

Some IME messages didn't get sent trough `SDL_SetWindowsMessageHook`, so this alternative is required.
bdach
bdach previously requested changes Dec 18, 2021
osu.Framework/Platform/SDL2/SDL2Extensions.cs Outdated Show resolved Hide resolved
osu.Framework/Platform/SDL2DesktopWindow.cs Outdated Show resolved Hide resolved
osu.Framework/Platform/Windows/Native/Imm.cs Outdated Show resolved Hide resolved
osu.Framework/Platform/Windows/Native/Imm.cs Outdated Show resolved Hide resolved
osu.Framework/Platform/Windows/Native/Imm.cs Outdated Show resolved Hide resolved
osu.Framework/Platform/Windows/WindowsWindow.cs Outdated Show resolved Hide resolved
osu.Framework/Platform/Windows/WindowsWindow.cs Outdated Show resolved Hide resolved
osu.Framework/Platform/Windows/WindowsWindow.cs Outdated Show resolved Hide resolved
@bdach
Copy link
Collaborator

bdach commented Dec 18, 2021

Generally disappointed to see more platform-specific handling, especially that it seems that it's bundled with workarounds to disable some SDL behaviour.

…e queue

Also changes Marshal.PtrToStructure so the nullref check isn't needed.
The hack for getting the result is no longer needed since the events are now
handled before being posted in the queue (early enough so that internal IME
state is preserved).

Tested to work as expected with the edge case mentioned in the (now removed) comment.
{
start = cursorPosition;
if (handle.IsClosed || handle.IsInvalid)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not super sure about these checks... Can this ever realistically happen in a scenario other than "this object has been disposed"? If so, then the usual way to proceed is to throw ObjectDisposedException or similar rather than just soft-fail like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

From my understanding of SafeHandle, IsClosed can only happen when it's disposed. Since this means improper use of the class, I'll change that to a throw.

IsInvalid can happen if a bad hWnd was provided, or when ImmGetContext doesn't provide a valid handle for some reason. In this case, we shouldn't throw as the consumer-facing methods are TryGet.... Returning false is fine. We already return false for other Imm errors, such as those in tryGetCompositionSize/String.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Bad HWND sounds pretty fatal and I'd say it would be fine to throw on that too. Not sure about the imm function itself returning a zero/negative handle, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't think we can differentiate between the two, Imm only returns a handle, and the docs don't mention any error values.

osu.Framework/Platform/Windows/WindowsWindow.cs Outdated Show resolved Hide resolved
osu.Framework/Platform/Windows/WindowsWindow.cs Outdated Show resolved Hide resolved
@peppy peppy self-requested a review January 3, 2022 04:03
Copy link
Sponsor Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

Haven't tested, but looks sane on a code pass.

osu.Framework/Platform/Windows/WindowsWindow.cs Outdated Show resolved Hide resolved
@bdach bdach dismissed their stale review January 6, 2022 09:00

looks resolved, pending testing

@bdach
Copy link
Collaborator

bdach commented Jan 10, 2022

Tested this briefly today, seems to work okay as far as I can tell (although not a frequent IME user soooo...)

@peppy peppy enabled auto-merge January 11, 2022 11:44
@peppy peppy merged commit 04d438d into ppy:master Jan 11, 2022
@Susko3 Susko3 deleted the windows-ime branch January 11, 2022 12:09
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.

None yet

3 participants