Re: [PATCH v3] winex11.drv: call XIMReset on CPS_COMPLETE

[Akihiro Sagawa]

On Thu, 31 Mar 2022 14:30:23 +0900, Alex Kwak wrote:
> The composition on the IME message is end, but the XIM string is
> in-composition state, and the same string remains on the XIM
> afterwards.
> so, call X11DRV_ForceXIMReset and finish composition state in the XIM.
>
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52754
> Signed-off-by: Alex Kwak
>
> v2: When XmbResetIC is called and KeyEvent occurs, it is a problem with
> some IME (i.e., ibus-hangul).
> v3: implement X11DRV_FindXIC(..) for getting exists XIC.
>
> dlls/winex11.drv/ime.c    |  2 ++
>  dlls/winex11.drv/x11drv.h |  1 +
>  dlls/winex11.drv/xim.c    | 31 +++++++++++++++++++++++++++++--
>  3 files changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/dlls/winex11.drv/ime.c b/dlls/winex11.drv/ime.c
> index c1584930861..2fe78234671 100644
> --- a/dlls/winex11.drv/ime.c
> +++ b/dlls/winex11.drv/ime.c
> @@ -753,6 +753,8 @@ BOOL WINAPI NotifyIME(HIMC hIMC, DWORD dwAction, DWORD dwIndex, DWORD dwValue)
>                      myPrivate->bInComposition = FALSE;
>                      ImmUnlockIMCC(lpIMC->hPrivate);
> -
>                X11DRV_ForceXIMReset(lpIMC->hWnd);
> -

I understood the basic concept of this patch, but...
>                  bRet = TRUE;
>              }
>              break;
>
> diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h
> index f389f3e0836..de882a532f3 100644
> --- a/dlls/winex11.drv/x11drv.h
> +++ b/dlls/winex11.drv/x11drv.h
> @@ -827,6 +827,7 @@ extern BOOL X11DRV_InitXIM( const char *input_style ) DECLSPEC_HIDDEN;
>  extern XIC X11DRV_CreateIC(XIM xim, struct x11drv_win_data *data) DECLSPEC_HIDDEN;
>  extern void X11DRV_SetupXIM(void) DECLSPEC_HIDDEN;
>  extern void X11DRV_XIMLookupChars( const char *str, DWORD count ) DECLSPEC_HIDDEN;
> +XIC X11DRV_FindXIC(HWND hwnd) DECLSPEC_HIDDEN;

You might want to use "extern" for consistency.
> extern void X11DRV_ForceXIMReset(HWND hwnd) DECLSPEC_HIDDEN;
>  extern void X11DRV_SetPreeditState(HWND hwnd, BOOL fOpen) DECLSPEC_HIDDEN;
>
> diff --git a/dlls/winex11.drv/xim.c b/dlls/winex11.drv/xim.c
> index 3994c2106cc..7a4ec432bbf 100644
> --- a/dlls/winex11.drv/xim.c
> +++ b/dlls/winex11.drv/xim.c
> @@ -249,9 +249,36 @@ static void XIMPreEditCaretCallback(XIC ic, XPointer client_data,
>      TRACE("Finished\n");
>  }
>
> +/**
> -
> - X11DRV_FindXIC
> -
> -
> -
> - This function finds the already created XIC in itself and parents.
> -
> - If only the specified handle is get via X11DRV_get_ic, the XIC can be exists
> -
> - in the parents and cannot be get.
> - */
>
> +XIC X11DRV_FindXIC(HWND hwnd)
> +{
> - HWND hwndParent = hwnd;

You can name the parameter hwndParent.
> - XIC ic;
> - if (!hwndParent)
> -
>    return NULL;
> - for (;;)
> - {
> -
>    ic = get_win_data( hwndParent )->xic;
> -
>    if (ic)
> -
>        break;
> -
>    else
> -
>        hwndParent = GetParent(hwnd);
> -
> -
>    if (!hwndParent)
> -
>        return NULL;
> - }

Because I'm not an expert at Wine's XIC handling, I wonder whether this
approach is acceptable. Anyway, it's better to rewrite this code snippet
with while loop.
> - return ic;
>
> +}
> -
>
> void X11DRV_ForceXIMReset(HWND hwnd)
>  {
> - XIC ic = X11DRV_get_ic(hwnd);
> - XIC ic = X11DRV_FindXIC(hwnd);
> if (ic)
> {
>     char* leftover;
>
> @@ -267,7 +294,7 @@ void X11DRV_SetPreeditState(HWND hwnd, BOOL fOpen)
>      XIMPreeditState state;
>      XVaNestedList attr;
> - ic = X11DRV_get_ic(hwnd);
> - ic = X11DRV_FindXIC(hwnd);
> if (!ic)
>     return;

These code change the preexisting behaviors.
Would you mind dividing this patch into two or more?
e.g.
 - part 1: introduces X11DRV_FindXIC() and updates X11DRV_ForceXIMReset(),
 - part 2: calls X11DRV_FindXIC() in X11DRV_SetPreeditState() (this part
might merge into part 1)
 - part 3: calls X11DRV_ForceXIMRest() in NotifyIME(CPS_COMPLETE).

Akihiro Sagawa