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.r...@oracle.com] > Sent: Sonntag, 20. Mai 2018 01:53 > To: Langer, Christoph <christoph.lan...@sap.com> > Cc: awt-...@openjdk.java.net; Ichiroh Takiguchi > <taki...@linux.vnet.ibm.com>; build-dev@openjdk.java.net; ppc-aix-port- > d...@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 > >
