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