ICU-23177 Add width-aware day period support and fix data loading #3611

sffc · 2025-08-27T03:38:16Z

Checklist

Required: Issue filed: ICU-23177
Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
Required: Each commit message must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
Issue accepted (done by Technical Committee after discussion)
Tests included, if applicable
API docs and/or User Guide docs changed or added, if applicable

sffc · 2025-08-27T03:43:53Z

API proposal has been sent. This is the C++ version. I will try to add the Java version tomorrow.

sffc · 2025-08-27T03:46:19Z

icu4c/source/i18n/unicode/dtfmtsym.h

+    UnicodeString*  fWideAmPms;
+    int32_t         fWideAmPmsCount;


This does increase the stack size of DateFormatSymbols, but it is already so big that the delta is only about 2-3%.

pedberg-icu · 2025-08-28T05:17:53Z

API proposal has been sent. This is the C++ version. I will try to add the Java version tomorrow.

@sffc But this is not yet in the ICU 78 API Proposals document

pedberg-icu · 2025-08-28T05:49:34Z

icu4c/source/i18n/dtfmtsym.cpp

@@ -2396,23 +2450,23 @@ DateFormatSymbols::initializeData(const Locale& locale, const char *type, UError
        assignArray(fStandaloneNarrowMonths, fStandaloneNarrowMonthsCount, fShortMonths, fShortMonthsCount);
    }

-    // Load AM/PM markers; if wide or narrow not available, use short
+    // Load AM/PM markers.
+    // Narrow falls back to Abbr which falls back to Wide.


That is not the way the dayPeriod fallback works in CLDR. There, within a given style (format or stand-alone), wide and narrow both fall back to abbreviated; and stand-alone abbreviated falls back to format abbreviated. The root locale only has the format abbreviated forms.

Copilot

Pull Request Overview

This PR adds width-aware day period support for AM/PM strings in SimpleDateFormat and DateFormatSymbols, allowing for abbreviated (default), wide, and narrow formatting styles. It also fixes data loading by properly handling separate AmPmMarkersAbbr and AmPmMarkers resources.

Implements width-aware AM/PM string formatting in SimpleDateFormat using pattern length (a, aaaa, aaaaa)
Adds new APIs to DateFormatSymbols for getting/setting AM/PM strings with width specification
Refactors resource loading to distinguish between abbreviated and wide AM/PM markers

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
SimpleDateFormat.java	Updates AM/PM formatting logic to use width-specific strings based on pattern length
DateFormatSymbols.java	Adds ampmsWide field and width-aware getter/setter methods for AM/PM strings
dtfmtsym.h	Adds C++ API declarations for width-aware AM/PM string methods
dtfmtsym.cpp	Implements C++ width-aware AM/PM string handling and resource loading fixes
smpdtfmt.cpp	Updates C++ SimpleDateFormat to use width-specific AM/PM formatting and parsing
Test files	Adds comprehensive tests for new AM/PM width functionality and updates existing test expectations

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

pedberg-icu · 2025-09-01T20:49:57Z

I am traveling but should have a chance to take a closer look at this sometime in the next 3 days or so.

pedberg-icu · 2025-09-06T18:58:17Z

@sffc This all looks good to me, with my one comment being that we should change the loading order & fallback for both C and J so that we load abbreviated first, and then have both wide and narrow fall back to abbreviated if they are missing. That way C and J would be consistent with each other and with the CLDR fallback order (in which wide/narrow fallback to abbreviated, and root only provides abbreviated). I will go ahead and approve in case you need to get this in and make those changes later, but I would like those changes at some point.

pedberg-icu

This all looks good to me, with my one comment being that we should change the loading order & fallback for both C and J so that we load abbreviated first, and then have both wide and narrow fall back to abbreviated if they are missing. That way C and J would be consistent with each other and with the CLDR fallback order (in which wide/narrow fallback to abbreviated, and root only provides abbreviated). I will go ahead and approve in case you need to get this in and make those changes later, but I would like those changes at some point.

macchiati · 2025-09-06T21:42:28Z

Hmmm. I wonder if there is a way with conformance tests to verify the order and targets of aliases.
Just a thought: Perhaps we can have one or more artificial locales in CLDR with constant data having "holes" in it to force fallback. It wouldn't be accessible in production releases, but could be used in conformance tests to verify that the order and targets of inheritance matches order and targets of CLDR aliases. @sffc @echeran

sffc · 2025-09-09T01:19:50Z

I don't believe that the manual fallback code as written in C++ actually does anything. The data loading sink should see the alias in root and do the fallback itself. I consider the C++ code to be merely a robust coding style, consistent with the rest of the symbols loading. In Java, most of the symbols also do not have explicit fallbacks written in code.

sffc · 2025-09-09T01:20:44Z

Just a thought: Perhaps we can have one or more artificial locales in CLDR with constant data having "holes" in it to force fallback. It wouldn't be accessible in production releases, but could be used in conformance tests to verify that the order and targets of inheritance matches order and targets of CLDR aliases. @sffc @echeran

Having a test locale in CLDR to exercise these sorts of things would be valuable. We could leverage en-XA perhaps.

pedberg-icu · 2025-09-09T03:45:17Z

icu4c/source/i18n/unicode/dtfmtsym.h

@@ -436,20 +436,27 @@ class U_I18N_API_CLASS DateFormatSymbols final : public UObject  {
    /**
     * Gets AM/PM strings with the specified width. For example: "A" and "P".
     * @param count    Filled in with length of the array.
+     * @param context   The usage context: FORMAT, STANDALONE. Currently ignored.


Should note that the values actually returned are the format styles.

pedberg-icu · 2025-09-09T03:45:44Z

icu4c/source/i18n/unicode/dtfmtsym.h


    /**
     * Sets AM/PM strings with the specified width. For example: "A" and "P".
     * @param ampms   The new AM/PM strings. (not adopted; caller retains ownership)
     * @param count   The number of strings in the array.
+     * @param context The usage context: FORMAT, STANDALONE. Currently ignored.


Note that the values returned are the format styles.

pedberg-icu

Thanks for the update!

See unicode-org#3611

jira-pull-request-webhook · 2025-09-10T06:07:03Z

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

sffc requested a review from Copilot August 27, 2025 03:43

sffc assigned pedberg-icu Aug 27, 2025

sffc marked this pull request as ready for review August 27, 2025 03:43

This comment was marked as outdated.

Sign in to view

sffc commented Aug 27, 2025

View reviewed changes

sffc requested a review from Copilot August 27, 2025 23:20

This comment was marked as outdated.

Sign in to view

pedberg-icu reviewed Aug 28, 2025

View reviewed changes

This comment was marked as resolved.

Sign in to view

sffc requested review from pedberg-icu and Copilot August 29, 2025 01:01

Copilot AI reviewed Aug 29, 2025

View reviewed changes

pedberg-icu previously approved these changes Sep 6, 2025

View reviewed changes

sffc dismissed pedberg-icu’s stale review via 1ac150e September 9, 2025 00:41

pedberg-icu reviewed Sep 9, 2025

View reviewed changes

sffc requested a review from pedberg-icu September 9, 2025 23:09

pedberg-icu approved these changes Sep 10, 2025

View reviewed changes

ICU-23177 Add width-aware day period support and fix data loading

509effe

See unicode-org#3611

sffc force-pushed the ICU-23177-ampm branch from c86d020 to 509effe Compare September 10, 2025 06:07

sffc merged commit 71b9755 into unicode-org:main Sep 10, 2025
103 checks passed

sffc deleted the ICU-23177-ampm branch September 10, 2025 21:24

Uh oh!

ICU-23177 Add width-aware day period support and fix data loading #3611

ICU-23177 Add width-aware day period support and fix data loading #3611

Uh oh!

Conversation

sffc commented Aug 27, 2025

Checklist

Uh oh!

sffc commented Aug 27, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

sffc Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

pedberg-icu commented Aug 28, 2025

Uh oh!

pedberg-icu Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

pedberg-icu commented Sep 1, 2025

Uh oh!

pedberg-icu commented Sep 6, 2025

Uh oh!

pedberg-icu left a comment

Choose a reason for hiding this comment

Uh oh!

macchiati commented Sep 6, 2025

Uh oh!

sffc commented Sep 9, 2025

Uh oh!

sffc commented Sep 9, 2025

Uh oh!

pedberg-icu Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

pedberg-icu Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

pedberg-icu left a comment

Choose a reason for hiding this comment

Uh oh!

jira-pull-request-webhook bot commented Sep 10, 2025

Uh oh!

Uh oh!

Uh oh!