Re: RFR 8177552: Compact Number Formatting support
Hi Naoto,
To add to my previous mail comment, the DecimalFormat spec also says that
/*"DecimalFormat can be instructed to format and parse scientific
notation only via a pattern; there is currently no factory method that
creates a scientific notation format. In a pattern, the exponent
character immediately followed by one or more digit characters indicates
scientific notation. "
*/That is, exponent formatting and parsing is instructed only via a
scientific notation pattern and I think should not be there with
*general number* formatting.
Updated webrev based on the other comments
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.02/
> Some more comments (all in CompactNumberFormat.java)
> line 807: expandAffix() seems to treat localizable special pattern
characters, but currently the implementation only cares for the minus
sign. Should other localizable pattern chars be taken care of, such as
percent sign?
- Other special characters like '%' percent sign are not allowed as per
CNF compact pattern spec
> line 869, 888: Define what -1 means as a ret value.
- OK.
> line 897: iterMultiplier be better all capitalized as it is a
constant. And it could be statically defined in the class to be shared
with other locations that use "10" for arithmetic operation.
- OK, made it static final and renamed it as RANGE_MULTIPLIER
> line 1531: Any possibility this could lead to divide-by-zero?
- None which I am aware of, unless you are pointing to the issue like
JDK-8211161, which we know is not an issue.
Regards,
Nishit Jain
On 23-11-2018 15:55, Nishit Jain wrote:
Hi Naoto,
> I think DecimalFormat and CNF should behave the same, ie. 'E' should
be treated as the exponent without a quote.
Personally I don't think that the exponential parsing should be
supported by CompactNumberFormat, because the objective of compact
numbers is to represent numbers in short form. So, parsing of number
format like "1.05E4K" should not be expected from CompactNumberFormat,
I am even doubtful that such forms ("1.05E4K") are used anywhere where
exponential and compact form are together used. If formatting and
parsing of exponential numbers are needed it should be done by
DecimalFormat scientific instance *not *with the general number
instance.So, I don't think that we should allow parsing of exponential
numbers.Comments welcome.
Regards,
Nishit Jain
On 22-11-2018 02:02, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/21/18 12:53 AM, Nishit Jain wrote:
Hi Naoto,
Updated the webrev based on suggestions
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/
Changes made:
- Replaced List with String[] to be added to the the
resource bundles
Good.
- refactored DecimalFormat.subparse() to be used by the CNF.parse(),
to reduce code duplication.
I presume CNF is calling package-private methods in DF to share the
same code. Some comments noting the sharing would be helpful.
- Also updated it with other changes as suggested in the comments
Sorry I missed your question the last time:
>>> Do you think this is an issue with DecimalFormat.parse() and CNF
>>> should avoid parsing exponential numbers? Or, should CNF.parse() be
>>> modified to be consistent with DecimalFormat.parse() in this aspect?
I think DecimalFormat and CNF should behave the same, ie. 'E' should
be treated as the exponent without a quote.
Some more comments (all in CompactNumberFormat.java)
line 807: expandAffix() seems to treat localizable special pattern
characters, but currently the implementation only cares for the minus
sign. Should other localizable pattern chars be taken care of, such
as percent sign?
line 869, 888: Define what -1 means as a ret value.
line 897: iterMultiplier be better all capitalized as it is a
constant. And it could be statically defined in the class to be
shared with other locations that use "10" for arithmetic operation.
line 1531: Any possibility this could lead to divide-by-zero?
Naoto
Regards,
Nishit Jain
On 20-11-2018 00:33, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/18/18 10:29 PM, Nishit Jain wrote:
Hi Naoto,
Please check my comments inline.
On 17-11-2018 04:52, naoto.s...@oracle.com wrote:
Hi Nishit,
Here are my comments:
- CLDRConverter: As the compact pattern no more employs
List, can we eliminate stringListEntry/Element, and use
Array equivalent instead?
Since the CNF design does not put any limit on the size of compact
pattern, so at the time of parsing the CLDR xmls using SAX parser,
it becomes difficult to identify the size of array when the parent
element of compact pattern is encountered, so I think it is better
to keep the List while extracting the resources.
OK. However I'd not keep the List format on generating the
resource bundle, as there is no reason to introduce yet another
bundle format other than the existing array of String.
- CompactNumberFormat.java
Multiple locations: Use StringBuilder instead of StringBuffer.
RFR: 8212676 AIX's CDE/MWM support
Hello. Could you review the fix ? Bug:https://bugs.openjdk.java.net/browse/JDK-8212676 Change: https://cr.openjdk.java.net/~itakiguchi/8212676/webrev.00/ Test instructions and screen shots are in JDK-8212676. GUI environment for AIX platform still needs CDE support. This fix is required to avoid unexpected working behavior on AIX platform. I'd like to obtain a sponsor for this issue. Thanks, Ichiroh Takiguchi IBM Japan, Ltd. On 2018-06-18 18:57, Ichiroh Takiguchi wrote: Hello. This fix is really required for AIX's GUI. System color setting and window manager's working behavior are very important. I appreciate any feedback, and how I would go about obtaining a sponsor and contributor ? On 2018-05-25 11:02, Ichiroh Takiguchi wrote: Hello Phil. webrev file was extracted. Please see http://cr.openjdk.java.net/~aleonard/AIX_GUI/webrev.00/ On 2018-05-19 02:48, Ichiroh Takiguchi wrote: Hello Phil. Webrev.zip file is stored into http://cr.openjdk.java.net/~aleonard/AIX_GUI/webrev-aixgui.zip Test programs are also stored: No testcase is available for FontUtilities.java and XDecoratedPeer.java. MotifColorUtilities.java http://cr.openjdk.java.net/~aleonard/AIX_GUI/SystemColorTest2.java Run SystemColorTest2, system colors should be displayed AIX sample is http://cr.openjdk.java.net/~aleonard/AIX_GUI/aix_systemcolor.txt XWM.java http://cr.openjdk.java.net/~aleonard/AIX_GUI/XWMTest1.java On AIX CDE, isMotif and isCDE were true. On AIX MWM, every entry is false. XWindowPeer.java http://cr.openjdk.java.net/~aleonard/AIX_GUI/JFrameTest.java On AIX CDE, click inside of "Non-Focusable" window (not window frame). Window focus should not be changed because of "click on focus" feature. But input focus is moved to "Non-Focusable" window. On 2018-05-18 01:00, Phil Race wrote: I think we'd need to see the actual proposed changes and understand the implications for ongoing support as we no longer support any platform which has a CDE desktop. Solaris 11.3 uses Gnome, so we'd be more inclined to be ripping out such support rather than adding to it. -phil. On 05/17/2018 04:18 AM, Ichiroh Takiguchi wrote: Hello, IBM would like to contribute AIX's CDE (Common Desktop Environment) DTWM (Desktop Window Manager) /MWM (Motif Window Manager) support to OpenJDK project. I'd like contribute following 5 files: M src/java.desktop/share/classes/sun/font/FontUtilities.java (Add isAIX flag to determine AIX platform for GUI environment) M src/java.desktop/unix/classes/sun/awt/X11/MotifColorUtilities.java (Add High Color support on CDE, OpenJDK just supports Medium Color) [1] M src/java.desktop/unix/classes/sun/awt/X11/XDecoratedPeer.java (Avoid miss calculation for window position under DTWM/MWM by XMapRaised/XMapWindow) M src/java.desktop/unix/classes/sun/awt/X11/XWM.java (Detect MWM on AIX platform) M src/java.desktop/unix/classes/sun/awt/X11/XWindowPeer.java (Add non-focusable window support on DTWM/MWM for AIX, because DTWM/MWM does not have enough features for ICCCM) I appreciate any feedback please, and how I would go about obtaining a sponsor and contributor ? Thanks, Ichiroh Takiguchi IBM Japan, Ltd. [1] https://docs.oracle.com/cd/E19253-01/806-7492/fontsandcolors-15233/index.html
RFR: X11 default visual support for IM status window on VNC
Hello.
Could you review the fix ?
Bug:https://bugs.openjdk.java.net/browse/JDK-8212677
Change: https://cr.openjdk.java.net/~itakiguchi/8212677/webrev.00/
Screen shots are in JDK-8212677.
I'd like to obtain a sponsor for this issue.
Thanks,
Ichiroh Takiguchi
IBM Japan, Ltd.
On 2018-06-21 21:58, Ichiroh Takiguchi wrote:
Hello Phil.
I'm sorry, I forgot to put my comment against your question.
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.
I put following debug code:
===
diff -r e1b3def12624
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 13 06:35:04 2018 +0200
+++ b/src/java.desktop/unix/native/libawt_xawt/awt/awt_InputMethod.c
Thu Jun 21 21:23:11 2018 +0900
@@ -667,6 +667,7 @@
InputOutput,
adata->awt_visInfo.visual,
attribmask, &attrib);
+fprintf(stderr, "status window id = 0x%X\n", status);
XSelectInput(dpy, status,
ExposureMask | StructureNotifyMask | EnterWindowMask
|
LeaveWindowMask | VisibilityChangeMask);
@@ -680,6 +681,21 @@
statusWindow->fontset = XCreateFontSet(dpy,
"-*-*-medium-r-normal-*-*-120-*-*-*-*",
&mclr, &mccr, &dsr);
+{
+int cntFonts;
+for(cntFonts = 0; cntFonts < mccr; cntFonts++) {
+fprintf(stderr, "[M][%d] %s\n", cntFonts, mclr[cntFonts]);
+}
+}
+{
+XFontStruct **font_struct_list;
+char **font_name_list;
+int cntFonts;
+int numFonts = XFontsOfFontSet(statusWindow->fontset,
&font_struct_list, &font_name_list);
+for(cntFonts = 0; cntFonts < numFonts; cntFonts++) {
+fprintf(stderr, "[L][%d] %s\n", cntFonts,
font_name_list[cntFonts]);
+}
+}
/* In case we didn't find the font set, release the list of
missing characters */
if (mccr > 0) {
XFreeStringList(mclr);
===
I tested it on RHEL7.
I thought since window id was assigned, but it was gone on current
code.
===
$ java -jar Notepad.jar
status window id = 0x455
...
$ xwininfo -id 0x455
X Error: 9: Bad Drawable: 0x455
Request Major code: 14
Request serial number: 3
xwininfo: error: No such window with id 0x455.
===
===
$ java -jar Notepad.jar
status window id = 0x4CA
...
$ xwininfo -id 0x4CA
xwininfo: Window id: 0x4ca (has no name)
Absolute upper-left X: 0
Absolute upper-left Y: 600
Relative upper-left X: 0
Relative upper-left Y: 600
Width: 80
Height: 22
Depth: 24
Visual: 0x21
Visual Class: TrueColor
Border width: 0
Class: InputOutput
Colormap: 0x20 (installed)
Bit Gravity State: ForgetGravity
Window Gravity State: NorthWestGravity
Backing Store State: NotUseful
Save Under State: no
Map State: IsUnMapped
Override Redirect State: yes
Corners: +0+600 -944+600 -944-146 +0-146
-geometry 80x22+0+600
===
According to main window: (Left side: without fix, right side: with
fix)
===
xwininfo: Window id: 0x409 " (failure | xwininfo: Window id:
0x460007e " (failure
Absolute upper-left X: 4 Absolute upper-left
X: 4
Absolute upper-left Y: 25 Absolute upper-left
Y: 25
Relative upper-left X: 0 Relative upper-left
X: 0
Relative upper-left Y: 0 Relative upper-left
Y: 0
Width: 492 Width: 492
Height: 571 Height: 571
Depth: 24 Depth: 24
Visual: 0x169 | Visual: 0x21
Visual Class: TrueColor Visual Class:
TrueColor
Border width: 0 Border width: 0
Class: InputOutput Class: InputOutput
Colormap: 0x408 (not installed) | Colormap: 0x20
(installed)
Bit Gravity State: NorthWestGravity Bit Gravity State:
NorthWestGravity
Window Gravity State: NorthWestGravity Window Gravity
State: NorthWestGravity
Backing Store State: NotUseful Backing Store State:
NotUseful
Save Under State: noSave Under State: no
Map State: IsViewable Map State: IsViewable
Override Redirect State: no Override Redirect
State: no
Corners: +4+25 -528+25 -528-172 +4-1Corners: +4+25
-528+25 -528-172 +4-1
-geometry 492x571+0+0 -geometry 492x571+0+0
===
So main window's visual also changed by this fix.
Similarly for the fontset change .. this might change what others get.
The fontset spec. there seems very loose to me ..
Re: RFR 8177552: Compact Number Formatting support
Hi Nishit,
On 11/26/18 12:41 AM, Nishit Jain wrote:
Hi Naoto,
To add to my previous mail comment, the DecimalFormat spec also says that
/*"DecimalFormat can be instructed to format and parse scientific
notation only via a pattern; there is currently no factory method that
creates a scientific notation format. In a pattern, the exponent
character immediately followed by one or more digit characters indicates
scientific notation. "
*/That is, exponent formatting and parsing is instructed only via a
scientific notation pattern and I think should not be there with
*general number* formatting.
I am not sure the quoted sentence should be interpreted that way. My
understanding is that the section means there is no public
NumberFormat.getScientificInstance() method (cf. line 601 at
NumberFormat.java), so that users will have to use 'E' in their pattern
string.
Anyway, my point is that if you prefer to treat the scientific notation
differently between DecimalFormat and CompactDecimalFormat, then it will
need to be clarified in the spec. Personally I agree that it is not
practical to interpret E in the CNF.
Naoto
Updated webrev based on the other comments
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.02/
> Some more comments (all in CompactNumberFormat.java)
> line 807: expandAffix() seems to treat localizable special pattern
characters, but currently the implementation only cares for the minus
sign. Should other localizable pattern chars be taken care of, such as
percent sign?
- Other special characters like '%' percent sign are not allowed as per
CNF compact pattern spec
> line 869, 888: Define what -1 means as a ret value.
- OK.
> line 897: iterMultiplier be better all capitalized as it is a
constant. And it could be statically defined in the class to be shared
with other locations that use "10" for arithmetic operation.
- OK, made it static final and renamed it as RANGE_MULTIPLIER
> line 1531: Any possibility this could lead to divide-by-zero?
- None which I am aware of, unless you are pointing to the issue like
JDK-8211161, which we know is not an issue.
Regards,
Nishit Jain
On 23-11-2018 15:55, Nishit Jain wrote:
Hi Naoto,
> I think DecimalFormat and CNF should behave the same, ie. 'E' should
be treated as the exponent without a quote.
Personally I don't think that the exponential parsing should be
supported by CompactNumberFormat, because the objective of compact
numbers is to represent numbers in short form. So, parsing of number
format like "1.05E4K" should not be expected from CompactNumberFormat,
I am even doubtful that such forms ("1.05E4K") are used anywhere where
exponential and compact form are together used. If formatting and
parsing of exponential numbers are needed it should be done by
DecimalFormat scientific instance *not *with the general number
instance.So, I don't think that we should allow parsing of exponential
numbers.Comments welcome.
Regards,
Nishit Jain
On 22-11-2018 02:02, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/21/18 12:53 AM, Nishit Jain wrote:
Hi Naoto,
Updated the webrev based on suggestions
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/
Changes made:
- Replaced List with String[] to be added to the the
resource bundles
Good.
- refactored DecimalFormat.subparse() to be used by the CNF.parse(),
to reduce code duplication.
I presume CNF is calling package-private methods in DF to share the
same code. Some comments noting the sharing would be helpful.
- Also updated it with other changes as suggested in the comments
Sorry I missed your question the last time:
>>> Do you think this is an issue with DecimalFormat.parse() and CNF
>>> should avoid parsing exponential numbers? Or, should CNF.parse() be
>>> modified to be consistent with DecimalFormat.parse() in this aspect?
I think DecimalFormat and CNF should behave the same, ie. 'E' should
be treated as the exponent without a quote.
Some more comments (all in CompactNumberFormat.java)
line 807: expandAffix() seems to treat localizable special pattern
characters, but currently the implementation only cares for the minus
sign. Should other localizable pattern chars be taken care of, such
as percent sign?
line 869, 888: Define what -1 means as a ret value.
line 897: iterMultiplier be better all capitalized as it is a
constant. And it could be statically defined in the class to be
shared with other locations that use "10" for arithmetic operation.
line 1531: Any possibility this could lead to divide-by-zero?
Naoto
Regards,
Nishit Jain
On 20-11-2018 00:33, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/18/18 10:29 PM, Nishit Jain wrote:
Hi Naoto,
Please check my comments inline.
On 17-11-2018 04:52, naoto.s...@oracle.com wrote:
Hi Nishit,
Here are my comments:
- CLDRConverter: As the compact pattern no more employs
List, can we eliminate stringListEntry/Element, and use
Array equivalent instead?
[12] RFR: 8214170: ResourceBundle.Control.newBundle should throw IllegalAccessException when constructor of the resource bundle is not public.
Hi, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8214170 The proposed changeset is located at: http://cr.openjdk.java.net/~naoto/8214170/webrev.00/ The existing logic to determine if there is a pubic constructor for the ResourceBundle class is incorrect. Moved the catch clause for NoSuchMethodException to handle it correctly. A shell based test was modified according to this (intentionally omitted the case for windows as chmod does not work - it will be addressed with the test case clean up (8213127). Naoto
Re: RFR 8177552: Compact Number Formatting support
Hi Naoto,
On 26-11-2018 21:01, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/26/18 12:41 AM, Nishit Jain wrote:
Hi Naoto,
To add to my previous mail comment, the DecimalFormat spec also says
that
/*"DecimalFormat can be instructed to format and parse scientific
notation only via a pattern; there is currently no factory method
that creates a scientific notation format. In a pattern, the exponent
character immediately followed by one or more digit characters
indicates scientific notation. "
*/That is, exponent formatting and parsing is instructed only via a
scientific notation pattern and I think should not be there with
*general number* formatting.
I am not sure the quoted sentence should be interpreted that way. My
understanding is that the section means there is no public
NumberFormat.getScientificInstance() method (cf. line 601 at
NumberFormat.java), so that users will have to use 'E' in their
pattern string.
Anyway, my point is that if you prefer to treat the scientific
notation differently between DecimalFormat and CompactDecimalFormat,
then it will need to be clarified in the spec. Personally I agree that
it is not practical to interpret E in the CNF.
OK. If it is better to specify the parsing behavior w.r.t. the
exponential numbers, I have added a statement in the parse() API.
*/"CompactNumberFormat parse does not allow parsing exponential number
strings. For example, parsing a string "1.05E4K" in US locale breaks at
character 'E' and returns 1.05."/*
Updated the webrev
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.03/
Will also update the CSR and refinalize it.
Regards,
Nishit Jain
Naoto
Updated webrev based on the other comments
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.02/
> Some more comments (all in CompactNumberFormat.java)
> line 807: expandAffix() seems to treat localizable special pattern
characters, but currently the implementation only cares for the minus
sign. Should other localizable pattern chars be taken care of, such
as percent sign?
- Other special characters like '%' percent sign are not allowed as
per CNF compact pattern spec
> line 869, 888: Define what -1 means as a ret value.
- OK.
> line 897: iterMultiplier be better all capitalized as it is a
constant. And it could be statically defined in the class to be
shared with other locations that use "10" for arithmetic operation.
- OK, made it static final and renamed it as RANGE_MULTIPLIER
> line 1531: Any possibility this could lead to divide-by-zero?
- None which I am aware of, unless you are pointing to the issue like
JDK-8211161, which we know is not an issue.
Regards,
Nishit Jain
On 23-11-2018 15:55, Nishit Jain wrote:
Hi Naoto,
> I think DecimalFormat and CNF should behave the same, ie. 'E'
should be treated as the exponent without a quote.
Personally I don't think that the exponential parsing should be
supported by CompactNumberFormat, because the objective of compact
numbers is to represent numbers in short form. So, parsing of number
format like "1.05E4K" should not be expected from
CompactNumberFormat, I am even doubtful that such forms ("1.05E4K")
are used anywhere where exponential and compact form are together
used. If formatting and parsing of exponential numbers are needed it
should be done by DecimalFormat scientific instance *not *with the
general number instance.So, I don't think that we should allow
parsing of exponential numbers.Comments welcome.
Regards,
Nishit Jain
On 22-11-2018 02:02, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/21/18 12:53 AM, Nishit Jain wrote:
Hi Naoto,
Updated the webrev based on suggestions
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/
Changes made:
- Replaced List with String[] to be added to the the
resource bundles
Good.
- refactored DecimalFormat.subparse() to be used by the
CNF.parse(), to reduce code duplication.
I presume CNF is calling package-private methods in DF to share the
same code. Some comments noting the sharing would be helpful.
- Also updated it with other changes as suggested in the comments
Sorry I missed your question the last time:
>>> Do you think this is an issue with DecimalFormat.parse() and CNF
>>> should avoid parsing exponential numbers? Or, should
CNF.parse() be
>>> modified to be consistent with DecimalFormat.parse() in this
aspect?
I think DecimalFormat and CNF should behave the same, ie. 'E'
should be treated as the exponent without a quote.
Some more comments (all in CompactNumberFormat.java)
line 807: expandAffix() seems to treat localizable special pattern
characters, but currently the implementation only cares for the
minus sign. Should other localizable pattern chars be taken care
of, such as percent sign?
line 869, 888: Define what -1 means as a ret value.
line 897: iterMultiplier be better all capitalized as it is a
constant. And it could be statically defined in the class to be
shared
