ticket:11018: Support CLDR time separator (ICU4J)

ticket:11018: Support CLDR time separator (ICU4J)

This change will treat the ':' character in patterns as a symbol
(instead of as a literal) and if the Locale defines timeSeparator then
use that character instead when formatting and accept both when parsing.

It will also define the '.' as an alternate time separator, that will be
accepted when parsing in lenient mode.

R=rocketman@google.com

Committed: http://bugs.icu-project.org/trac/changeset/36898

[tkeep, 2014-12-01 18:36:18]

Thanks for doing this work.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
File main/classes/core/src/com/ibm/icu/text/DateFormat.java (right):

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/DateFormat.java:442: */
New public API always requires an API proposal to be submitted to the PMC during
our Wednesday ICU meetings. Please make this field package or private. If it
must be public for everything to work then you can mark it internal like this:

@internal For ICU use only.

On the last line of the comment. However, it is always better to limit scope if
possible rather than declare something as internal in documentation.

If you do submit an API proposal to have this field added and it is approved,
then you can use

@draft ICU 55.

*** Throughout this changeset I found similar issues: new public API that has
not been through a proposal. Be sure to fix this everywhere as I only call it
out here to save you time.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/DateFormat.java:2353: */
Same. Limit scope or declare @internal.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
File main/classes/core/src/com/ibm/icu/text/DateFormatSymbols.java (right):

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/DateFormatSymbols.java:1439: String ns =
desiredLocale.getKeywordValue("numbers");
Looks like you fixed a caching problem. Was this a separate bug?

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/DateFormatSymbols.java:1441: key += "+" +
ns;
While this key construction may work for now, it will complicate things later if
we want to expand the key space. If later we want to add another "optional"
field to the key then it may be possible for two different locales to map to the
same key. If a key looks like "en+someType+foo" how do you know if "foo" goes
with the numbering system or a new optional field?

Either make each field required so that each key will always have the same
number of pluses or if you must have optional fields of your key, take the json
approach and embed the field name in the key with the field value.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
File main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java (right):

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java:760: //   :   ;   <
  =   >   ?
Optional: Add documentation that PATTERN_CHAR_BASE is the code point
corresponding to the first index in this array.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java:1394: if (':' <= ch
&& ch <= 'z') {
Maybe I am paranoid, but since historically patterns involved [A-Za-z] will you
rewrite this as:

if (ch == ':' || ('A' <= ch && ch <= 'z'))

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java:1470: if (':' <= ch
&& ch <= 'z') {
Extract a method here since this is duplicate code.

Maybe call the method something like boolean isValidPatternCharacter(char ch)?

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java:1922: if ((ch >=
'a' && ch <= 'z') || (ch >= 'A' && ch <= 'Z') || (ch == ':')) {
This could also be replaced with isValidPatternCharacter()

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java:2769: if (':' <= ch
&& ch <= 'z') {
Same. replace with isValidPatternCharacter()

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java:3248: String[] data
= DateFormatSymbols.DEFAULT_TIME_SEPARATOR.equals(formatData.timeSeparator)
Please add a comment here. I do not understand what this block of code does.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java:3683: || (ch ==
0x003A /*':'*/))) {
Yet another rendition of isValidPatternCharacter(). I know these aren't your
fault, but we should try to clean up the code whenever we can.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java:3777: if (':' <= ch
&& ch <= 'z') {
isValidPatternCharacter()

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java:3892: if (':' <= ch
&& ch <= 'z') {
isValidPatternCharacter()

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java:3934: if (':' <= ch
&& ch <= 'z') {
ditto.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
File main/classes/core/src/com/ibm/icu/util/Calendar.java (right):

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/util/Calendar.java:4221: case TIME_SEPARATOR:
Why do time separators have limits? Please document why min = 0 and max = 1.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
File main/classes/core/src/com/ibm/icu/util/GregorianCalendar.java (right):

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/util/GregorianCalendar.java:281: {/*          
                       */}, // TIME_SEPARATOR
In Calendar TIME_SEPARATOR had 0, 0, 1, 1. In this subclass it doesn't have
limits. Why?

https://codereview.appspot.com/180370043/diff/1/main/tests/core/src/com/ibm/i...
File main/tests/core/src/com/ibm/icu/dev/test/format/DateFormatTest.java
(right):

https://codereview.appspot.com/180370043/diff/1/main/tests/core/src/com/ibm/i...
main/tests/core/src/com/ibm/icu/dev/test/format/DateFormatTest.java:2007: public
void TestFormattingLocaleTimeSeparator() {
How does this test the functionality that you added? Even if 'ar' currently uses
':' in its pattern, CLDR data may change so that later 'ar' no longer explicitly
uses ':'. Then your changes won't be tested anymore. While its fine to have this
test, I'd like to see a test that explicitly sets a pattern that uses ':'

https://codereview.appspot.com/180370043/diff/1/main/tests/core/src/com/ibm/i...
main/tests/core/src/com/ibm/icu/dev/test/format/DateFormatTest.java:2009:
TimeZone tz = TimeZone.getTimeZone("CET");
The other fun part about dates is that they use skeletons as well as patterns.
patterns are agnostic to locale in that a date pattern is specific enough that
you can correctly format a date without knowing the locale. Skeletons are
strings like patterns except that the order of the fields doesn't matter, and
skeletons lack explicit delimiters. A skeleton needs a locale before it can be
reconciled to a pattern string. With the locale, our system can resolve the
order of the fields in the skeleton and add the right delimiters to make a fully
qualified pattern string.

For example:

The skeleton "yMd" may resolve to "dd/MM/yyyy" in "en-UK"
But "yMd" may resolve to "MM/dd/yyyy" in "en-US"

You have to decide what role, if any, the new ':' pattern char will play in
skeletons. There isn't a right or wrong answer here, but a decision has to be
made. If your work does involve skeletons, then there should be a test that
creates a DateFormat object from a skeleton with a ':' in it and a locale and
formats a date. Hint: DateFormat.getDateFromSkeleton() can be used to do this.

[roubert (google), 2014-12-02 20:55:21]

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
File main/classes/core/src/com/ibm/icu/text/DateFormat.java (right):

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/DateFormat.java:442: */
On 2014/12/01 18:36:17, tkeep wrote:
> Please make this field package or private. If it
> must be public for everything to work then you can mark it internal like this:

Done.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/DateFormat.java:2353: */
On 2014/12/01 18:36:17, tkeep wrote:
> Same. Limit scope or declare @internal.

Done.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
File main/classes/core/src/com/ibm/icu/text/DateFormatSymbols.java (right):

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/DateFormatSymbols.java:1439: String ns =
desiredLocale.getKeywordValue("numbers");
On 2014/12/01 18:36:18, tkeep wrote:
> Looks like you fixed a caching problem. Was this a separate bug?

No, the caching issue is something that appears because of the other changes:
Because the numbers keyword now can change what time separator gets used, it
needs to become part of the cache key.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/DateFormatSymbols.java:1441: key += "+" +
ns;
On 2014/12/01 18:36:17, tkeep wrote:
> While this key construction may work for now, it will complicate things later
if
> we want to expand the key space.

No, it will not. This cache key is only used within this initializeData()
method, so it's trivial to change it to whatever it needs to become in the
future.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
File main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java (right):

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java:760: //   :   ;   <
  =   >   ?
On 2014/12/01 18:36:18, tkeep wrote:
> Optional: Add documentation that PATTERN_CHAR_BASE is the code point
> corresponding to the first index in this array.

Done.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java:1394: if (':' <= ch
&& ch <= 'z') {
On 2014/12/01 18:36:18, tkeep wrote:
> Maybe I am paranoid, but since historically patterns involved [A-Za-z] will
you
> rewrite this as:
> 
> if (ch == ':' || ('A' <= ch && ch <= 'z'))

No, I think that the point of this if-statement is to check whether there is an
entry for 'ch' in PATTERN_CHAR_TO_INDEX, so just checking for a lower and an
upper bound is the right thing to do. The following if-statement then checks
whether that entry was -1.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java:1470: if (':' <= ch
&& ch <= 'z') {
On 2014/12/01 18:36:18, tkeep wrote:
> Extract a method here since this is duplicate code.
> 
> Maybe call the method something like boolean isValidPatternCharacter(char ch)?

There is a ton of such refactoring that could be done here. I think it would be
clearer and easier to do all such refactoring separate from implementing time
separator support.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java:1922: if ((ch >=
'a' && ch <= 'z') || (ch >= 'A' && ch <= 'Z') || (ch == ':')) {
On 2014/12/01 18:36:18, tkeep wrote:
> This could also be replaced with isValidPatternCharacter()

Same reason for leaving it alone here; better do refactoring separately.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java:2769: if (':' <= ch
&& ch <= 'z') {
On 2014/12/01 18:36:18, tkeep wrote:
> Same. replace with isValidPatternCharacter()

Same.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java:3248: String[] data
= DateFormatSymbols.DEFAULT_TIME_SEPARATOR.equals(formatData.timeSeparator)
On 2014/12/01 18:36:18, tkeep wrote:
> Please add a comment here. I do not understand what this block of code does.

Done.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java:3683: || (ch ==
0x003A /*':'*/))) {
On 2014/12/01 18:36:18, tkeep wrote:
> I know these aren't your
> fault, but we should try to clean up the code whenever we can.

I find it much clearer and easier to implement new features and clean up old
cruft separate from each other.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java:3777: if (':' <= ch
&& ch <= 'z') {
On 2014/12/01 18:36:18, tkeep wrote:
> isValidPatternCharacter()

Acknowledged.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java:3892: if (':' <= ch
&& ch <= 'z') {
On 2014/12/01 18:36:18, tkeep wrote:
> isValidPatternCharacter()

Acknowledged.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java:3934: if (':' <= ch
&& ch <= 'z') {
On 2014/12/01 18:36:18, tkeep wrote:
> ditto.

Acknowledged.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
File main/classes/core/src/com/ibm/icu/util/Calendar.java (right):

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/util/Calendar.java:4221: case TIME_SEPARATOR:
On 2014/12/01 18:36:18, tkeep wrote:
> Why do time separators have limits? Please document why min = 0 and max = 1.

Hm, this is just here to satisfy the API, I can't see it used for anything
meaningful, but I've changed the limits to Character.MIN_VALUE and
Character.MAX_VALUE now in an attempt to make them less arbitrary.

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
File main/classes/core/src/com/ibm/icu/util/GregorianCalendar.java (right):

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/util/GregorianCalendar.java:281: {/*          
                       */}, // TIME_SEPARATOR
On 2014/12/01 18:36:18, tkeep wrote:
> In Calendar TIME_SEPARATOR had 0, 0, 1, 1. In this subclass it doesn't have
> limits. Why?

See comment at Calendar.handleGetLimit(), subclasses need only specify those
fields that the base class doesn't. That's why there is no data for most of the
fields here.

https://codereview.appspot.com/180370043/diff/1/main/tests/core/src/com/ibm/i...
File main/tests/core/src/com/ibm/icu/dev/test/format/DateFormatTest.java
(right):

https://codereview.appspot.com/180370043/diff/1/main/tests/core/src/com/ibm/i...
main/tests/core/src/com/ibm/icu/dev/test/format/DateFormatTest.java:2007: public
void TestFormattingLocaleTimeSeparator() {
On 2014/12/01 18:36:18, tkeep wrote:

> How does this test the functionality that you added?

By creating two locales that only differ in the value of the numbers keyword,
where one is set to a numbering system other than the default, it then can
verify that date formatters using these two different locales use different time
separators.

> Even if 'ar' currently uses
> ':' in its pattern, CLDR data may change so that later 'ar' no longer
explicitly
> uses ':'. Then your changes won't be tested anymore.

It is true that any change to the time separator in CLDR for 'ar' would break
this test, but such a change would cause a visible test failure requiring the
strings expectedArab and expectedLatn to be updated, so at least there is no
risk of the test silently becoming a no-op.

Do you know of any simple way to supply a test like this with custom CLDR data
for testing?

https://codereview.appspot.com/180370043/diff/1/main/tests/core/src/com/ibm/i...
main/tests/core/src/com/ibm/icu/dev/test/format/DateFormatTest.java:2009:
TimeZone tz = TimeZone.getTimeZone("CET");
On 2014/12/01 18:36:18, tkeep wrote:
> You have to decide what role, if any, the new ':' pattern char will play in
> skeletons.

I have not thought about this at all.

Mark: Have you thought about skeletons? Do you have any opinion?

[roubert (google), 2014-12-03 15:16:02]

https://codereview.appspot.com/180370043/diff/1/main/tests/core/src/com/ibm/i...
File main/tests/core/src/com/ibm/icu/dev/test/format/DateFormatTest.java
(right):

https://codereview.appspot.com/180370043/diff/1/main/tests/core/src/com/ibm/i...
main/tests/core/src/com/ibm/icu/dev/test/format/DateFormatTest.java:2009:
TimeZone tz = TimeZone.getTimeZone("CET");
On 2014/12/01 18:36:18, tkeep wrote:
> Skeletons are
> strings like patterns except that the order of the fields doesn't matter, and
> skeletons lack explicit delimiters.

Mark says that the : character never is used in skeletons, so that this change
won't interfere with skeletons in any way.

[tkeep, 2014-12-04 00:42:36]

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
File main/classes/core/src/com/ibm/icu/util/Calendar.java (right):

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/util/Calendar.java:4221: case TIME_SEPARATOR:
On 2014/12/02 20:55:20, roubert (google) wrote:
> On 2014/12/01 18:36:18, tkeep wrote:
> > Why do time separators have limits? Please document why min = 0 and max = 1.
> 
> Hm, this is just here to satisfy the API, I can't see it used for anything
> meaningful, but I've changed the limits to Character.MIN_VALUE and
> Character.MAX_VALUE now in an attempt to make them less arbitrary.

Some of the fields such as EXTENDED_YEAR don't have any values in them. I am
wondering if you can make TIME_SEPARATOR have no min/max values at all since
they don't really make sense.

[roubert (google), 2014-12-04 18:07:44]

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
File main/classes/core/src/com/ibm/icu/util/Calendar.java (right):

https://codereview.appspot.com/180370043/diff/1/main/classes/core/src/com/ibm...
main/classes/core/src/com/ibm/icu/util/Calendar.java:4221: case TIME_SEPARATOR:
On 2014/12/04 00:42:36, tkeep wrote:
> Some of the fields such as EXTENDED_YEAR don't have any values in them.

Oh, they do, it's just that some of the values are specified by the subclass
instead. For EXTENDED_YEAR, it's defined in GregorianCalendar like this:

        { -5838269, -5838269, 5828963, 5838270 }, // EXTENDED_YEAR

[tkeep, 2014-12-04 20:55:17]

I marked this ticket as fixed. Thanks for the hard work.

On 4 December 2014 at 10:07, <roubert@google.com> wrote:

>
> https://codereview.appspot.com/180370043/diff/1/main/
> classes/core/src/com/ibm/icu/util/Calendar.java
> File main/classes/core/src/com/ibm/icu/util/Calendar.java (right):
>
> https://codereview.appspot.com/180370043/diff/1/main/
> classes/core/src/com/ibm/icu/util/Calendar.java#newcode4221
> main/classes/core/src/com/ibm/icu/util/Calendar.java:4221: case
> TIME_SEPARATOR:
> On 2014/12/04 00:42:36, tkeep wrote:
>
>> Some of the fields such as EXTENDED_YEAR don't have any values in
>>
> them.
>
> Oh, they do, it's just that some of the values are specified by the
> subclass instead. For EXTENDED_YEAR, it's defined in GregorianCalendar
> like this:
>
>         { -5838269, -5838269, 5828963, 5838270 }, // EXTENDED_YEAR
>
> https://codereview.appspot.com/180370043/
>



-- 
100 / 9900 = .01010101010101010101...

100 / 9801 = .01020304050607080910...

100 / 9899 = .01010203050813213455... (Fibonacci)

[roubert (google), 2014-12-05 01:36:04]

On Thu, Dec 4, 2014 at 9:55 PM, Travis Keep <rocketman@google.com> wrote:

> I marked this ticket as fixed.

Please don't, it still needs C++ changes.

-- 
Fredrik Roubert
roubert@google.com

[tkeep, 2014-12-08 20:00:33]

On 2014/12/05 01:36:04, roubert (google) wrote:
> On Thu, Dec 4, 2014 at 9:55 PM, Travis Keep <mailto:rocketman@google.com>
wrote:
> 
> > I marked this ticket as fixed.
> 
> Please don't, it still needs C++ changes.
> 
> -- 
> Fredrik Roubert
> mailto:roubert@google.com

acknowledge

[roubert (google), 2014-12-12 17:03:00]

I've now updated this change with new handling of the time separator in lenient
parsing, accepting also an alternate separator. Please take another look.

[tkeep, 2014-12-12 20:54:58]

https://codereview.appspot.com/180370043/diff/80001/main/classes/core/src/com...
File main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java (right):

https://codereview.appspot.com/180370043/diff/80001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java:3251:
Vector<String> data = new Vector<String>(3);
ArrayList<String> data?

ArrayList is preferred to Vector. Even if you need thread safety, there is no
reason to use Vector. JAVA collections API offers methods that turn thread
unsafe collections into a thread safe collections.

[roubert (google), 2014-12-15 12:49:28]

https://codereview.appspot.com/180370043/diff/80001/main/classes/core/src/com...
File main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java (right):

https://codereview.appspot.com/180370043/diff/80001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java:3251:
Vector<String> data = new Vector<String>(3);
On 2014/12/12 20:54:57, tkeep wrote:
> ArrayList<String> data?
> 
> ArrayList is preferred to Vector. Even if you need thread safety, there is no
> reason to use Vector. JAVA collections API offers methods that turn thread
> unsafe collections into a thread safe collections.

Done.

[roubert (google), 2014-12-15 22:51:01]

Do you have any further comments about this change, or is it good to submit now?

[roubert (google), 2014-12-19 07:20:23]

Committed patchset #7 (id:120001) manually as 36898 (presubmit successful).