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
