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

[Alex Kwak]

Hi,

Thanks for your reviewing. great helpful to me.

I submit a new patch reflecting feedback.

https://www.winehq.org/pipermail/wine-devel/2022-April/212598.html

Regards.,

Alex

22. 4. 2. 22:57에 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