[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-awt-dev
Subject: Re: <AWT Dev> RFR: 8201429: Support AIX Input Method Editor (IME) for AWT Input Method Framework (IM
From: "Langer, Christoph" <christoph.langer () sap ! com>
Date: 2020-02-20 10:32:31
Message-ID: AM0PR02MB57149E79F0722BDBC02C70A48A130 () AM0PR02MB5714 ! eurprd02 ! prod ! outlook ! com
[Download RAW message or body]
Hi Sergey,
from what I can see your proposed changes seem to make sense, given that \
XSetForeground and XSetBackground do their job.
The change itself comes from Ichiroh-san, I only helped to review/sponsor it at the \
time and ran a few tests in our infrastructure. I suggest you prepare a patch and we \
run it through our testing to see whether it builds or causes regressions on AIX. \
However, as for the IME/IMF context, I really would like to see Ichiroh verify it, \
since he's got an environment where he can test the graphics stuff on AIX and is \
knowledgeable about IMs needed for e.g. Japanese.
Cheers
Christoph
> -----Original Message-----
> From: Ichiroh Takiguchi <takiguc@linux.vnet.ibm.com>
> Sent: Donnerstag, 20. Februar 2020 10:57
> To: Sergey Bylokhov <Sergey.Bylokhov@oracle.com>
> Cc: Langer, Christoph <christoph.langer@sap.com>; Philip Race
> <philip.race@oracle.com>; awt-dev@openjdk.java.net; build-
> dev@openjdk.java.net; ppc-aix-port-dev@openjdk.java.net
> Subject: Re: <AWT Dev> RFR: 8201429: Support AIX Input Method Editor
> (IME) for AWT Input Method Framework (IMF)
>
> 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.race@oracle.com]
> > > > Sent: Sonntag, 20. Mai 2018 01:53
> > > > To: Langer, Christoph <christoph.langer@sap.com>
> > > > Cc: awt-dev@openjdk.java.net; Ichiroh Takiguchi
> > > > <takiguc@linux.vnet.ibm.com>; build-dev@openjdk.java.net;
> > > > ppc-aix-port-
> > > > dev@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
> > > > >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic