<AWT Dev> RFR: 8201429: Support AIX Input Method Editor (IME) for AWT Input Method Framework (IMF)
Sergey Bylokhov
Sergey.Bylokhov at oracle.com
Thu Feb 20 05:10:41 UTC 2020
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
>>>
--
Best regards, Sergey.
More information about the build-dev
mailing list