Hello Sergey.
I'm not sure if I understand what you want to change...
XCreateGC:
The colors are created upper code, they will be overwritten.
XSetBackground:
I'm sorry, I have no idea about XSetBackground(),
I thought background might have default value, But I could not find out
the doc.
Ichiroh Takiguchi
IBM Japan, Ltd.
On 2020-02-20 14:10, Sergey Bylokhov wrote:
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-...@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
