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

[Alex Kwak]

> 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?

If WM_IME_ENDCOMPOSITION comes, I think WM_IME_STARTCOMPOSITION
should come next time.
  but it was not.
If continue to expect composition, I don't think WM_IME_ENDCOMPOSITION
should not come.
> 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?

I agree with unstable on CJK inputs.

Unlike Windows,
it is implemented without considering what language you are typing and what
characteristics each language is input to, so I thought it would not be
possible to apply it to all CJK collectively. ( also, XIM doesn't support )

The same situation as the case you mentioned cannot happen in Hangul.
because in Hangul, when a word is completed, it must be one clause or the
composition must be exited with non-composition character(or event).

so, I'm worried about causing problems in Chinese and Japanese.

22. 3. 26. 19:44에 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