<AWT Dev> RFR: 8201429: Support AIX Input Method Editor (IME) for AWT Input Method Framework (IMF)

[Philip Race]

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/unix/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/unix/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
>