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

IME Composition Truncation + Mouse Bugs #5378

Closed
wants to merge 40 commits into from
Closed

IME Composition Truncation + Mouse Bugs #5378

wants to merge 40 commits into from

Conversation

zreedy
Copy link
Contributor

@zreedy zreedy commented Mar 4, 2022

Description

This PR addresses the following issues:

  • Text from IME Candidates and final submissions are truncated if larger than 32 bytes.
  • No way for application to query if the IME candidate window is currently open.
  • No way for application to dismiss IME candidate window.
  • Changes to how candidate window is placed to address some rare issues positioning.
  • Whitespace being striped from the end of IME strings incorrectly
  • Traditional Chinese IMEs issuing a placeholder in the form of an empty space which is inconsistent with expected behavior from other IMEs. Suppressing this seems to be the agreed upon solution (looking at Chromium source and Windows IME sample code).
  • Incorrect assumption that mouse buttons are released when a new window is created (e.g. tear dragging a tab from a workspace)

IME Candidate/Submission Truncation
A new SDL hint flag SDL_HINT_IME_SUPPORT_EXTENDED_TEXT has been added. When disabled (SDL_FALSE) legacy behavior is retained with no side-effects.
When enabled (SDL_TRUE) the following occurs instead:

  • SDL_TextEditingExtEvent (SDL_TEXTEDITING_EXT) is issued if the buffer would overflow (previously truncated). Otherwise the original SDL_TextEditingEvent is still issued. This is to reduce the amount of heap allocation that needs to happen.
  • SDL_TextInputEvent will be dispatched multiple times with as much text that can be stored in the text field buffer (32 bytes) as possible (respecting UTF-8 codepoint boundaries).

The text field of SDL_TextEditingExtEvent must be freed by the consumer, this is inline with the behavior of SDL_FileDrop.

Further details can be found in the thread for issue 5363

No way for an application to query if the IME candidate window is open
This is needed for software applications (and games) using SDL to interact with IME an appropriately. Without knowing if the IME is currently actively taking input from the user or displayed it's not possible for an application to handle certain input events from the mouse appropriately.
This was fixed by adding SDL_IsTextInputShown. This returns an SDL_bool.

No way for application to dismiss IME candidate window
This is needed for software applications (and games) using SDL to interact with an IME appropriately. When a user clicks somewhere within the application, it is important to instruct the IME to dismiss whatever candidate was provided without it dispatching events to the user. The added SDL_ClearComposition does this without the overhead of disabling/re-enabling the IME subsystem (which was previously required).

Existing Issue(s)

zreedy and others added 30 commits September 14, 2021 15:35
Fixed: Whitespace being striped from the end of IME strings incorrectly
Added: SDL_ClearComposition function + Fix event order when selecting candidates in Japanese IME
…ting a window is not necessarily accurate.

For example; if a window is created from dragging out a gadget/tab, the mouse button is still held down and has not been released yet.
SDL_DYNAPI_PROC(SDL_bool,SDL_IsTextInputActive,(void),(),return)
SDL_DYNAPI_PROC(SDL_bool,SDL_IsTextInputShown,(void),(),return)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not insert new procs, but append them in dynapi: running the gendynapi.pl perl script will do that for you.


// Get the correct caret position if we've selected a candidate from the candidate window
if (videodata->ime_cursor == 0 && length > 0) {
int32_t start = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not using SDL's own Sint32 here?

WCHAR buffer[SDL_TEXTEDITINGEVENT_TEXT_SIZE];
const size_t size = SDL_arraysize(buffer);
buffer[0] = 0;
WCHAR *buffer = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

NULL, really (and the same for s just above it)

@@ -173,6 +177,7 @@ WIN_ResetDeadKeys()

GetKeyboardState(keyboardState);


Copy link
Contributor

Choose a reason for hiding this comment

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

Unwanted addition of a stray newline?

@sezero
Copy link
Contributor

sezero commented Mar 4, 2022

Way too many commits (some irrelevant, some already reverted) - can this not be rebased to current main and squashed?

@slouken
Copy link
Collaborator

slouken commented Mar 4, 2022

As discussed in #5363, I made the change to always split text input events as needed in 2e801b1

Please rebase your changes and squash into a single or a few functionally independent commits for review.

Thanks!

@zreedy
Copy link
Contributor Author

zreedy commented Mar 4, 2022

I did not realize you had made that change in main already!
I will address everyone's suggestions in a new PR as I will need to rebase.

Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants