<AWT Dev> RFR: 8201429: Support AIX Input Method Editor (IME) for AWT Input Method Framework (IMF)
Ichiroh Takiguchi
takiguc at linux.vnet.ibm.com
Thu Feb 20 09:56:58 UTC 2020
Hello Sergey.
I'm not sure if I understand what you want to change...
XCreateGC:
The colors are created upper code, they will be overwritten.
XSetBackground:
I'm sorry, I have no idea about XSetBackground(),
I thought background might have default value, But I could not find out
the doc.
Ichiroh Takiguchi
IBM Japan, Ltd.
On 2020-02-20 14:10, Sergey Bylokhov wrote:
> Hello, Christoph.
>
> Could you shed some light on the changes below:
>
> - statusWindow->fgGC = XCreateGC(dpy, status, valuemask, &values);
> + statusWindow->fgGC = create_gc(status, FALSE);
> XSetForeground(dpy, statusWindow->fgGC, fg);
> - statusWindow->bgGC = XCreateGC(dpy, status, valuemask, &values);
> + statusWindow->bgGC = create_gc(status, TRUE);
> XSetForeground(dpy, statusWindow->bgGC, bg);
>
> The new method create_gc() is used to set the FG and BG color of the
> GC.
> But foreground color immediately replaced by the XSetForeground.
> I am asking because the create_gc() uses
> AwtScreenDataPtr.whitepixel/blackpixel
> which I would like to delete. I guess it should be possible rewrite
> this code like this:
>
> statusWindow->fgGC = XCreateGC(dpy, status, valuemask, &values);
> XSetForeground(dpy, statusWindow->fgGC, fg);
> XSetBackground(dpy, statusWindow->fgGC, bg);
> statusWindow->bgGC = XCreateGC(dpy, status, valuemask, &values);
> XSetForeground(dpy, statusWindow->bgGC, bg);
> XSetBackground(dpy, statusWindow->bgGC, fg);
>
> What do you think?
>
> On 5/28/18 5:38 am, Langer, Christoph wrote:
>> Hi Phil,
>>
>> thanks for your review. I have incorporated your suggestions:
>> http://cr.openjdk.java.net/~clanger/webrevs/8201429.3/
>>
>> I'll run it through our internal testing and run a jdk-submit job with
>> it. When all is green, I'll push it to jdk-client tomorrow.
>>
>> Best regards
>> Christoph
>>
>>> -----Original Message-----
>>> From: Philip Race [mailto:philip.race at oracle.com]
>>> Sent: Sonntag, 20. Mai 2018 01:53
>>> To: Langer, Christoph <christoph.langer at sap.com>
>>> Cc: awt-dev at openjdk.java.net; Ichiroh Takiguchi
>>> <takiguc at linux.vnet.ibm.com>; build-dev at openjdk.java.net;
>>> ppc-aix-port-
>>> dev at openjdk.java.net
>>> Subject: Re: <AWT Dev> RFR: 8201429: Support AIX Input Method Editor
>>> (IME) for AWT Input Method Framework (IMF)
>>>
>>> I think I am 99% OK with this.
>>>
>>> In general I see what you are doing here and how you've presented the
>>> webrev.
>>> Treating even the new files as moved helps see the differences but it
>>> is
>>> still
>>> a challenge to follow all the moving pieces.
>>>
>>> So before we had just
>>>
>>> abstract class unix/X11InputMethod <- class
>>> unix/XInputMethod
>>>
>>> Now we have
>>>
>>> abstract class unix/X11InputMethodBase
>>>
>>> / \
>>>
>>> / \
>>>
>>> / \
>>>
>>> / \
>>>
>>> abstract class unix/X11InputMethod abstract class
>>> aix/X11InputMethod
>>>
>>> \ /
>>> \ /
>>> \ /
>>> class unix/XInputMethod
>>>
>>>
>>>
>>> I have submitted a build job with this patch to make sure it all
>>> builds
>>> on Linux & Solaris ..
>>> and it was all fine.
>>>
>>> But testing for this would have to be manual, and I don't have cycles
>>> for that.
>>> So I'll have to accept that the testing done by IBM was enough
>>>
>>> So only minor comments ...
>>> http://cr.openjdk.java.net/~clanger/webrevs/8201429.2/src/java.desktop/u
>>> nix/classes/sun/awt/X11InputMethodBase.java.sdiff.html
>>>
>>> 730 case 0: //None of the value is set by Wnn
>>>
>>> "value is " -> "values are " ?
>>>
>>>
>>> http://cr.openjdk.java.net/~clanger/webrevs/8201429.2/src/java.desktop/u
>>> nix/native/libawt_xawt/awt/awt_InputMethod.c.sdiff.html
>>>
>>> why did you move
>>>
>>> 26 #ifdef HEADLESS
>>> 27 #error This file should not be included in headless
>>> library
>>> 28 #endif
>>>
>>>
>>> I think it should be first. It is also missing from the equivalent
>>> AIX file but that
>>> is
>>> up to you .. I really didn't look any further at the AIX files.
>>>
>>> .. and there seems no point to moving around some of the other
>>> includes
>>> except to make the webrev harder to read :-)
>>>
>>> But good job cleaning up a lot of the formatting of the native code.
>>>
>>> -phil.
>>>
>>>
>>>
>>>
>>>
>>> On 5/18/18, 4:59 AM, Langer, Christoph wrote:
>>>> Hi all,
>>>>
>>>> Here is an updated webrev:
>>>> http://cr.openjdk.java.net/~clanger/webrevs/8201429.2/
>>>> Can someone from the graphics/awt team please have a look at that
>>> change? Especially checking that we don't break non-AIX platforms?
>>> Thanks
>>> in advance.
>>>>
>>>> @Ichiroh: Thanks for your review and tests. Adressing your points:
>>>>
>>>>> resetCompositionState() was missing in
>>>>> src/java.desktop/aix/classes/sun/awt/X11InputMethod.java
>>>>>
>>> ==========================================================
>>>>> ==
>>>>> --- a/src/java.desktop/aix/classes/sun/awt/X11InputMethod.java Wed
>>> May
>>>>> 09 09:05:32 2018 +0900
>>>>> +++ b/src/java.desktop/aix/classes/sun/awt/X11InputMethod.java Mon
>>>>> May
>>>>> 14 21:25:50 2018 +0900
>>>>> @@ -56,6 +56,21 @@
>>>>> }
>>>>>
>>>>> /**
>>>>> + * Reset the composition state to the current composition
>>>>> state.
>>>>> + */
>>>>> + protected void resetCompositionState() {
>>>>> + if (compositionEnableSupported&& haveActiveClient()) {
>>>>> + try {
>>>>> + /* Restore the composition mode to the last saved
>>>>> composition
>>>>> + mode. */
>>>>> + setCompositionEnabled(savedCompositionState);
>>>>> + } catch (UnsupportedOperationException e) {
>>>>> + compositionEnableSupported = false;
>>>>> + }
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + /**
>>>>> * Activate input method.
>>>>> */
>>>>> public synchronized void activate() {
>>>>>
>>> ==========================================================
>>>> Good catch. I've incorporated that in my new webrev.
>>>>
>>>>> Otherwise,
>>>>> we could not find out any functional difference on Linux.
>>>>> we could not find out any functional difference between our
>>>>> modified
>>> code and your code on AIX.
>>>>>
>>>>> By code check, we found following difference.
>>>>>
>>> ==========================================================
>>>>> ==
>>>>> --- a/src/java.desktop/aix/native/libawt_xawt/awt/awt_InputMethod.c
>>>>> Wed May 09 09:05:32 2018 +0900
>>>>> +++ b/src/java.desktop/aix/native/libawt_xawt/awt/awt_InputMethod.c
>>>>> Mon May 14 21:25:50 2018 +0900
>>>>> @@ -248,6 +248,10 @@
>>>>> "flushText",
>>>>> "()V");
>>>>> JNU_CHECK_EXCEPTION_RETURN(env, NULL);
>>>>> + if ((*env)->ExceptionOccurred(env)) {
>>>>> + (*env)->ExceptionDescribe(env);
>>>>> + (*env)->ExceptionClear(env);
>>>>> + }
>>>>> /* IMPORTANT:
>>>>> The order of the following calls is critical since
>>>>> "imInstance" may
>>>>> point to the global reference itself, if
>>>>> "freeX11InputMethodData" is called
>>>>>
>>> ==========================================================
>>>>>
>>>>> Did you remove this code intentionally ?
>>>> Yes, I removed that one intentionally. There is no point in doing
>>>> the
>>> Exception check/clearing after a call to
>>> JNU_CHECK_EXCEPTION_RETURN(env, NULL);
>>>>
>>>>> Otherwise, I think the other changes were acceptable.
>>>> ��
>>>>
>>>> Thanks and Best regards
>>>> Christoph
>>>>
More information about the awt-dev
mailing list