RFR JDK-8204938: Add a test case to automatically check the updated LSR data
Hi, Please review the fix for JDK-8204938 Bug: https://bugs.openjdk.java.net/browse/JDK-8204938 Webrev: http://cr.openjdk.java.net/~nishjain/8204938/webrev.02/ Fix: Added a test case to cross check the LSR data generated for the JDK APIs. So, for each lsr data update, the test case need not be updated, it automatically cross checks the updated lsr data. Regards, Nishit Jain
Re: Proposal:X11 default visual support for IM status window on VNC
Hello. New fixed code is in: http://cr.openjdk.java.net/~aleonard/defvis/webrev.02/ Could you check fixed files again ? I only updated following part between webrev.01 and webrev.02 == diff -r 70a582d110a1 -r 6f04164a9d62 src/java.desktop/unix/native/libawt_xawt/awt/awt_InputMethod.c --- a/src/java.desktop/unix/native/libawt_xawt/awt/awt_InputMethod.c Wed Jun 06 21:03:25 2018 +0900 +++ b/src/java.desktop/unix/native/libawt_xawt/awt/awt_InputMethod.c Wed Jun 20 16:54:24 2018 +0900 @@ -677,7 +677,7 @@ return NULL; } statusWindow->w = status; -//12-point font +//12, 13-point fonts statusWindow->fontset = XCreateFontSet(dpy, "-*-*-medium-r-normal-*-*-120-*-*-*-*," \ "-*-*-medium-r-normal-*-*-130-*-*-*-*", == Please create bugid and handle it. Thanks, Ichiroh Takiguchi IBM Japan, Ltd. On 2018-06-20 04:59, Naoto Sato wrote: Please change the comment in line 680, which should also mention 13 point font. Naoto On 6/19/18 12:54 PM, Naoto Sato wrote: Looks OK wrt awt_InputMethod.c change. Naoto On 6/19/18 11:32 AM, Phil Race wrote: Where's the bug ID ? The review should have a bug ID in the subject line so we can all find it later ! Is this changing the default visual for all WIndows, not just the IM status window? I think we need to understand the implications before this can be accepted. Similarly for the fontset change .. this might change what others get. The fontset spec. there seems very loose to me .. I think I18N-DEV should be asked about this too. -phil. On 06/19/2018 11:07 AM, Sergey Bylokhov wrote: Looks fine, if there are no other comments I'll push the fix using the new bugid. On 06/06/2018 17:54, Ichiroh Takiguchi wrote: Hello Sergey. Thank you for your review. Could you review following patch ? http://cr.openjdk.java.net/~aleonard/defvis/webrev.01/ Thanks, Ichiroh Takiguchi IBM Japan, Ltd. On 2018-06-06 07:15, Sergey Bylokhov wrote: Hi, Ichiroh. The approach looks fine, but maybe it is possible to decrees code duplication in findWithTemplate(). After the fix it will have two similar loops. On 24/05/2018 22:24, Ichiroh Takiguchi wrote: Hello, IBM would like to contribute X11 default visual support for IM status window patch to OpenJDK project. Issue: Java's Native IM status window is not displayed even if it's there. Because of this issue, user cannot get proper visual feedback during key input operation. We found this issue on Tiger VNC. Reason: Java may pick up unexpected visual for Java's Native IM status window when Xserver supports multiple visual. Workaround: X11 default visual can be changed by FORCEDEFVIS environment variable, but it's not easy to find out default visual id. I'd like contribute following 2 files: M src/java.desktop/unix/native/libawt_xawt/awt/awt_GraphicsEnv.c (Change X11 visual setting) M src/java.desktop/unix/native/libawt_xawt/awt/awt_InputMethod.c (Support 13 point X11 misc fonts (like k14 font for Japanese), since the fonts may defined for unscaled fonts.) webrev files are in http://cr.openjdk.java.net/~aleonard/defvis/ I appreciate any feedback please, and how I would go about obtaining a sponsor and contributor? Thanks, Ichiroh Takiguchi IBM Japan, Ltd.
Re: Proposal:X11 default visual support for IM status window on VNC
My question has not been answered. I don't think this is ready to go in. -phil. On 6/20/18, 4:23 AM, Ichiroh Takiguchi wrote: Hello. New fixed code is in: http://cr.openjdk.java.net/~aleonard/defvis/webrev.02/ Could you check fixed files again ? I only updated following part between webrev.01 and webrev.02 == diff -r 70a582d110a1 -r 6f04164a9d62 src/java.desktop/unix/native/libawt_xawt/awt/awt_InputMethod.c --- a/src/java.desktop/unix/native/libawt_xawt/awt/awt_InputMethod.c Wed Jun 06 21:03:25 2018 +0900 +++ b/src/java.desktop/unix/native/libawt_xawt/awt/awt_InputMethod.c Wed Jun 20 16:54:24 2018 +0900 @@ -677,7 +677,7 @@ return NULL; } statusWindow->w = status; -//12-point font +//12, 13-point fonts statusWindow->fontset = XCreateFontSet(dpy, "-*-*-medium-r-normal-*-*-120-*-*-*-*," \ "-*-*-medium-r-normal-*-*-130-*-*-*-*", == Please create bugid and handle it. Thanks, Ichiroh Takiguchi IBM Japan, Ltd. On 2018-06-20 04:59, Naoto Sato wrote: Please change the comment in line 680, which should also mention 13 point font. Naoto On 6/19/18 12:54 PM, Naoto Sato wrote: Looks OK wrt awt_InputMethod.c change. Naoto On 6/19/18 11:32 AM, Phil Race wrote: Where's the bug ID ? The review should have a bug ID in the subject line so we can all find it later ! Is this changing the default visual for all WIndows, not just the IM status window? I think we need to understand the implications before this can be accepted. Similarly for the fontset change .. this might change what others get. The fontset spec. there seems very loose to me .. I think I18N-DEV should be asked about this too. -phil. On 06/19/2018 11:07 AM, Sergey Bylokhov wrote: Looks fine, if there are no other comments I'll push the fix using the new bugid. On 06/06/2018 17:54, Ichiroh Takiguchi wrote: Hello Sergey. Thank you for your review. Could you review following patch ? http://cr.openjdk.java.net/~aleonard/defvis/webrev.01/ Thanks, Ichiroh Takiguchi IBM Japan, Ltd. On 2018-06-06 07:15, Sergey Bylokhov wrote: Hi, Ichiroh. The approach looks fine, but maybe it is possible to decrees code duplication in findWithTemplate(). After the fix it will have two similar loops. On 24/05/2018 22:24, Ichiroh Takiguchi wrote: Hello, IBM would like to contribute X11 default visual support for IM status window patch to OpenJDK project. Issue: Java's Native IM status window is not displayed even if it's there. Because of this issue, user cannot get proper visual feedback during key input operation. We found this issue on Tiger VNC. Reason: Java may pick up unexpected visual for Java's Native IM status window when Xserver supports multiple visual. Workaround: X11 default visual can be changed by FORCEDEFVIS environment variable, but it's not easy to find out default visual id. I'd like contribute following 2 files: M src/java.desktop/unix/native/libawt_xawt/awt/awt_GraphicsEnv.c (Change X11 visual setting) M src/java.desktop/unix/native/libawt_xawt/awt/awt_InputMethod.c (Support 13 point X11 misc fonts (like k14 font for Japanese), since the fonts may defined for unscaled fonts.) webrev files are in http://cr.openjdk.java.net/~aleonard/defvis/ I appreciate any feedback please, and how I would go about obtaining a sponsor and contributor? Thanks, Ichiroh Takiguchi IBM Japan, Ltd.
Re: [11] Review Request: 8202696 glyphs in textfield only shown when thai baht-character is added
+1 (approved). -phil. On 06/18/2018 12:29 AM, Dipak Kumar wrote: Hi Phil, I am able to make the test automated after incorporating your suggestion to limit the test case to logical fonts. I observed that "canDisplayUpTo()" which internally uses "canDisplay()" does return expected values for logical fonts. Thanks for your guidance. Please find the updated webrev at http://cr.openjdk.java.net/~dkumar/8202696/webrev.02/ . Request you to review it again and let me know your comments. Thanks, Dipak -Original Message- From: Phil Race Sent: 16 June 2018 00:11 To: Dipak Kumar ; 2d-...@openjdk.java.net; i18n-dev@openjdk.java.net; Naoto Sato Subject: Re: [11] Review Request: 8202696 glyphs in textfield only shown when thai baht-character is added The exclusion ranges only apply to the logical fonts, so the iteration you are doing looking for any font is now not testing the case you are trying to fix, except by chance. You will need to limit the iteration to the logical fonts. I don't know why canDisplay(..) did not do what I would expect .. particularly since canDisplayUpTo should be using the same information. -phil. On 06/14/2018 12:07 AM, Dipak Kumar wrote: Hi Phil, Thanks for reviewing the patch and providing useful information. Regarding your suggestion of using "canDisplay()" in test, I tried using this function to make the test headless. But this function is returning true even without proposed fix in fontconfig.properties file. Font exclusion is not coming into picture in this function call. In the previously written test case, I have added a check for font support (whether characters can be displayed or not using function "canDisplayUpTo()"). Please find the updated webrev at http://cr.openjdk.java.net/~dkumar/8202696/webrev.01/ . Request you to have a look into this again and let me know your inputs. Many thanks, Dipak -Original Message- From: Phil Race Sent: 13 June 2018 03:26 To: Dipak Kumar ; 2d-...@openjdk.java.net; i18n-dev@openjdk.java.net; Naoto Sato Subject: Re: [11] Review Request: 8202696 glyphs in textfield only shown when thai baht-character is added BTW I think the synopsis really needs updating ! Once you know what is going on, continuing to title it after the random empirical observation of the reporter is not necessary. I've changed it to : "Remove exclusion range for phonetic chars in windows fontconfig.properties" Please use that in the commit - when you get to it. -phil. On 06/12/2018 01:46 PM, Phil Race wrote: I can only guess why this range was excluded. My guess is that it was something to do with an attempt at allowing as many of these glyphs as possible to come from (for example) a Chinese font in a chinese locale even though we always put the latin font first .. The interesting reason why adding a Thai Baht magically makes them appears is Thai forces TextLayout .. and it (apparently) bypasses the exclusion range. I suspect this is because layout needs to operate on physical fonts so gets the list of physical fonts and operates on these explicitly bypassing CompositeFont which is where this support exists. You can prove that just by using one of these phonetic chars in Font2DTest without adding Thai or anything else and switching to TextLayout .. lo and behold .. it appears. I wonder how long that has been the case ? Perhaps so long as to be effectively forever .. That makes a case for getting rid of all these ranges. The use for the ranges is to control what physical font is used for glyphs in cases where some font that may be "earlier" in the list supplies glyphs that you'd actually prefer to come from some other font. That may be useful if the exclusion ranges were able to be applied depending on your locale, but this alphabetic exclusion is particularly questionable. So the change is OK, but can't the test be automated .. and headless ? You just need to do something like ask if the font "canDisplay()" these code points. However the test is fragile in the sense that it assumes that logical fonts on Windows are capable of supporting them. -phil On 06/04/2018 11:32 PM, Dipak Kumar wrote: Hi, Please review below fix : Bug: https://bugs.openjdk.java.net/browse/JDK-8202696 Webrev: http://cr.openjdk.java.net/~dkumar/8202696/webrev.00/ Characters which are not getting displayed actually belong to Phonetic Extensions (https://en.wikipedia.org/wiki/Phonetic_Extensions ). These characters are not getting rendered properly because they are in exclusion range mentioned in the windows.fontconfig.properties file. In the above fix, font exclusion ranges have been adjusted so that these characters do not fall within these ranges Font related Jtreg tests and Mach5 client tests are fine. Thanks, Dipak
Re: RFR JDK-8204938: Add a test case to automatically check the updated LSR data
Looks good. Naoto On 6/20/18 3:32 AM, Nishit Jain wrote: Hi, Please review the fix for JDK-8204938 Bug: https://bugs.openjdk.java.net/browse/JDK-8204938 Webrev: http://cr.openjdk.java.net/~nishjain/8204938/webrev.02/ Fix: Added a test case to cross check the LSR data generated for the JDK APIs. So, for each lsr data update, the test case need not be updated, it automatically cross checks the updated lsr data. Regards, Nishit Jain
