<AWT Dev> RFR: 8201429: Support AIX Input Method Editor (IME) for AWT Input Method Framework (IMF)

Langer, Christoph christoph.langer at sap.com
Thu Feb 20 10:32:31 UTC 2020


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 at linux.vnet.ibm.com>
> Sent: Donnerstag, 20. Februar 2020 10:57
> To: Sergey Bylokhov <Sergey.Bylokhov at oracle.com>
> Cc: Langer, Christoph <christoph.langer at sap.com>; Philip Race
> <philip.race at oracle.com>; awt-dev at openjdk.java.net; build-
> dev at openjdk.java.net; ppc-aix-port-dev at 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 at oracle.com]
> >>> Sent: Sonntag, 20. Mai 2018 01:53
> >>> To: Langer, Christoph <christoph.langer at sap.com>
> >>> Cc: awt-dev at openjdk.java.net; Ichiroh Takiguchi
> >>> <takiguc at linux.vnet.ibm.com>; build-dev at openjdk.java.net;
> >>> ppc-aix-port-
> >>> dev at 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
> >>>>


More information about the awt-dev mailing list