Re: [PATCH] winex11.drv: Fix IME_SetResultString for Korean(hangul) IME.

[Akihiro Sagawa]

On Sun, 20 Mar 2022 21:17:27 +0900, Alex Kwak wrote:
> In a special case for Hangul(Korean), when Composition is completed and
> this function is called, the new Composition disappears. so, calling
> IME_SetCompositionString again for makes composition will be
> starting.

Please add Wine-Bug header before Signed-off-by line.
> Signed-off-by: Alex Kwak
>
> dlls/winex11.drv/ime.c    |  4 ++--
>  dlls/winex11.drv/x11drv.h | 27 +++++++++++++++++++++++++++
>  dlls/winex11.drv/xim.c    | 13 +++++++++++++
>  3 files changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/dlls/winex11.drv/ime.c b/dlls/winex11.drv/ime.c
> index c1584930861..f7827b31635 100644
> --- a/dlls/winex11.drv/ime.c
> +++ b/dlls/winex11.drv/ime.c
> @@ -1050,8 +1050,8 @@ void IME_SetResultString(LPWSTR lpResult, DWORD dwResultLen)
>      GenerateIMEMessage(imc, WM_IME_COMPOSITION, lpResult[0], GCS_RESULTSTR|GCS_RESULTCLAUSE);
>      GenerateIMEMessage(imc, WM_IME_ENDCOMPOSITION, 0, 0);
> - if (!inComp)
> -
>    ImmSetOpenStatus(imc, FALSE);
> -
>
> myPrivate->bInComposition = FALSE;
> -
>
> ImmSetOpenStatus(imc, FALSE);
>
> ImmUnlockIMC(imc);
>
> }

I'm not sure about this change.
It always turns off IME open status. Since composition might be
continuous at this moment, it can't be set unconditionally.
If we should do so for some reasons, could you explain in detail?
> diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h
> index f389f3e0836..8ce3a7e8528 100644
> --- a/dlls/winex11.drv/x11drv.h
> +++ b/dlls/winex11.drv/x11drv.h

[..]
> --- a/dlls/winex11.drv/xim.c
> +++ b/dlls/winex11.drv/xim.c
> @@ -117,6 +117,19 @@ void X11DRV_XIMLookupChars( const char *str, DWORD count )
>  IME_SetResultString(wcOutput, dwOutput);
>  HeapFree(GetProcessHeap(), 0, wcOutput);
> -
> - /*
> -
> * In a special case for Hangul(Korean), when Composition is completed and
> -
> * this function is called, the new Composition disappears. so, calling
> -
> * IME_SetCompositionString again for makes composition will be
> -
> * starting.
> -
> */
> - if (CompositionString && dwCompStringLength >= sizeof(WCHAR) &&
> -
>    IsHangul(((const WCHAR*) CompositionString)[0]))
> - {
> -
>    IME_SetCompositionString(SCS_SETSTR, CompositionString,
> -
>                             dwCompStringLength, NULL, 0);
> - }
>
> }
>
> static BOOL XIMPreEditStateNotifyCallback(XIC xic, XPointer p, XPointer data)

Could you revert Hangul(Korean) specific changes? This issue seems not
to be Korean specific because I can occasionally reproduce it with
Japanese input method.

From my point of view, the main cause of this issue is that the order of
PreeditCallbacks and XmbLookupString() aren't stable. The current wine's
implementation seems to rely on getting the result of XmbLookupString()
before PreeditCallback for the next clause. However, when one or more
clauses are left after commiting a clause, PreeditCallback for the left
clauses sometimes (or always in Hangul?) called before getting the
result of XmbLookupString.
What do you think about these XIM situations? Does your patch works
well for both cases?

Regards,
Akihiro Sagawa