<AWT Dev> RFR: 8201429: Support AIX Input Method Editor (IME) for AWT Input Method Framework (IMF)
Langer, Christoph
christoph.langer at sap.com
Mon May 28 12:38:41 UTC 2018
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 ppc-aix-port-dev
mailing list