Skip to content

DecimalFormatter: Add locale-aware formatting - #23888

Draft
morning4coffe-dev wants to merge 8 commits into
masterfrom
agents/decimalformatter-locale-api-implementation
Draft

DecimalFormatter: Add locale-aware formatting#23888
morning4coffe-dev wants to merge 8 commits into
masterfrom
agents/decimalformatter-locale-api-implementation

Conversation

@morning4coffe-dev

@morning4coffe-dev morning4coffe-dev commented Jul 26, 2026

Copy link
Copy Markdown
Member

GitHub Issue: related to #6908

Related to #6908

PR Type:

✨ Feature

What changed? 🚀

Ports the locale-aware DecimalFormatter(IEnumerable<string>, string) constructor and geographic-region properties, makes formatting/parsing culture-aware, and implements GlobalizationPreferences.Languages and HomeGeographicRegion across active Uno targets. This supplies the public globalization prerequisite used by NumberBox, with native WinRT contract probes and deterministic locale, grouping, numeral-system, and platform-preference coverage.

PR Checklist ✅

  • 🧪 Added Runtime tests, UI tests, or a manual test sample (112 focused unit tests pass)
  • 📚 Docs have been added/updated following the documentation template (not applicable)
  • 🖼️ Validated PR Screenshots Compare Test Run (not applicable)
  • ❗ Contains NO breaking changes
  • 👀 Reviewed 2 other open pull requests (optional)

Source fidelity status

DecimalFormatter and GlobalizationPreferences are Windows.Globalization/Windows.System.UserProfile WinRT APIs rather than microsoft-ui-xaml implementation files. Their public behavior was verified against native WinRT probes (including region validation, ZZ resolution, M49/pseudo regions, and signed grouping) and 112 focused tests. This prerequisite remains draft because it is not a literal microsoft-ui-xaml source port.

@github-actions github-actions Bot added the area/automation Categorizes an issue or PR as relevant to project automation label Jul 26, 2026
@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @morning4coffe-dev's task in 4m 3s —— View job


Code Review

  • Gather context and understand the PR
  • Read changed files
  • Review implementation correctness
  • Review test coverage
  • Post inline comments
  • Post summary

Overall this is a well-structured port. The GeographicRegionHelper abstraction correctly isolates the validation/resolution logic for future reuse by sibling formatters (CurrencyFormatter etc.), the Arabic numeral system double-localization guard is properly handled, and the round-trip tests across multiple locales are solid. 3 findings below.

Findings: 3 (1 style, 1 style/comment-length, 1 convention)

Inline:

Not inline (pre-existing code, couldn't be anchored to a diff line):

  • DecimalFormatter.FormatDouble (line 117–118) has a double blank line between the NumberRounder branch and var stringBuilder = ...; one blank line is the convention.

Cross-cutting notes

  • NumeralSystem setter on a default-constructed formatter: it now calls GeographicRegionHelper.ResolveNumberFormat("en-US", "ZZ", ...), which (for non-Arab systems) falls through "en-ZZ" → "en-US" → returns en-US's NumberFormatInfo. Since en-US and InvariantCulture have identical decimal/group punctuation this is functionally correct and the existing When_ParseDoubleUsingSpeceficNumeralSystem tests cover it. Worth a brief note in the setter or a test assertion to nail the contract explicitly.
  • The en-IN lakh/crore test (When_NonUniformGroupSizeLocale) hardcodes "12,34,567.00". ICU on Linux and NLS on Windows both report NumberGroupSizes = {3, 2} for en-IN, so this should be portable — fine as-is.
  • The Generated/ file change correctly removes [NotImplemented] guards for the newly implemented constructor and properties; no issues there.

Comment thread src/Uno.UWP/Globalization/NumberFormatting/GeographicRegionHelper.cs Outdated
Comment thread src/Uno.UWP/Globalization/NumberFormatting/FormatterHelper.cs
@unodevops

Copy link
Copy Markdown
Contributor

🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23888/wasm-skia-net9/index.html

@unodevops

Copy link
Copy Markdown
Contributor

⚠️⚠️ The build 224180 has failed on Uno.UI - CI.

@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @morning4coffe-dev's task in 5m 30s —— View job


Code Review

  • Gather context and understand the PR
  • Read changed files
  • Review implementation correctness
  • Review test coverage
  • Post inline comments
  • Post summary

Verdict: ✅ ship (1 style nit)

All three findings from the previous review pass are resolved:

  • FormatterHelper.NumberFormat XML doc trimmed to 2 lines ✓
  • TryResolveCulture allocation eliminated (if/else if chain, no new[]) ✓
  • [GitHubWorkItem] applied to all new locale-aware test methods ✓

Findings: 1 (style only)

Inline:

  • GeographicRegionHelper.cs:87ResolveNumberFormat XML doc is ~30 lines; the HasInvalidGroupSize known-limitation paragraph duplicates the note already on that method and can be dropped. The Arab-system invariant and InvariantGlobalization caveat are genuinely non-obvious and should be kept, but trimming to ≤8 lines would match the style correction applied to FormatterHelper.NumberFormat.

Cross-cutting notes

  • Double blank line in DecimalFormatter.FormatDouble between the NumberRounder block and var stringBuilder = ... (lines 119–120): flagged in the previous review and still present. This is pre-existing code not touched in this PR, so it can't be anchored to a diff line — but it's a one-line cleanup if the author is amending the file anyway.

  • ResolvedGeographicRegion is statically "ZZ": The property is set to UnresolvedGeographicRegion in both constructors and never updated. WinRT's contract says it reflects "the geographic region most recently used to format or parse" — so it would update per call. The current implementation diverges from that contract after the first format/parse call. The test only checks the post-construction state, which is correct for "ZZ" before any call. Worth a follow-up issue to wire in dynamic updating if WinRT parity is ever needed for this property, but it's out of scope for this PR.

  • Implementation quality overall: the TryResolveCulture cascade (primary-lang+region → full tag → bare language) is the right priority order, the Arab/ArabExt guard is correct and well-tested, and the NumeralSystem setter re-resolves NumberFormat to avoid double-localization when switching to/from Arabic. The round-trip test coverage across fr-FR, ar-SA, en-IN, and negative-zero is solid.

@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @morning4coffe-dev's task in 6m 26s —— View job


Code Review

  • Gather context and understand the PR
  • Read changed files
  • Review implementation correctness
  • Review test coverage
  • Post inline comments
  • Post summary

Verdict: ✅ ship (1 docs nit, 1 test-coverage suggestion)

All three findings from the previous round are confirmed resolved:

  • FormatterHelper.NumberFormat XML doc trimmed ✓
  • TryResolveCulture heap allocation eliminated ✓
  • [GitHubWorkItem] applied to all new locale-aware test methods ✓
  • ResolveNumberFormat XML doc trimmed to ≤8 lines ✓

The NumeralSystemTranslator.TranslateBackArab correctly restores both digit characters and the Arabic decimal/group separators (٫., ٬,), so the Arabic round-trip tests (When_ArabicLanguage_Then_GroupingAndDecimalPunctuationRoundTrip, When_NumeralSystemChangedAfterConstruction_Then_ArabicRoundTrips) are sound.

Findings: 2 (1 docs, 1 test-coverage)

Inline:

  • DecimalFormatter.cs:100ResolvedGeographicRegion XML doc says "most recently used to format or parse" but the property is hardcoded to "ZZ" post-construction and never updates. The doc accurately describes the WinRT contract but misleads readers about current behaviour. Flagged in the previous pass as out-of-scope; surfacing again so it's tracked before merge.

  • Given_DecimalFormatter.cs:421When_NonUniformGroupSizeLocale_Then_GroupsUsingLocaleGroupSizes verifies FormatDouble output but doesn't assert that ParseDouble("12,34,567.00") round-trips back to 1234567. Adding the assertion would guard HasInvalidGroupSize for non-uniform group sizes against future regressions.

Cross-cutting notes

  • Double blank line in FormatDouble (lines 119–120, between the NumberRounder block and var stringBuilder): flagged in both previous passes, still present, but is pre-existing code not touched in this diff so it cannot be anchored as an inline comment. One blank line is the convention.

  • GeographicRegion property is immutable ({ get; }): correct — WinRT's GeographicRegion reflects the constructor argument and is not mutable post-construction.

  • HasInvalidGroupSize multi-group parsing: there is a pre-existing logic gap (Index - preIndex != groupSize should account for the separator length), but the en-IN case happens to work correctly due to its 2-digit inner groups. This is not introduced by this PR.

Comment thread src/Uno.UWP/Globalization/NumberFormatting/DecimalFormatter.cs
@unodevops

Copy link
Copy Markdown
Contributor

🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23888/wasm-skia-net9/index.html

@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @morning4coffe-dev's task in 2m 26s —— View job


Code Review

  • Gather context and understand the PR
  • Read changed files
  • Check if previous round's findings are resolved
  • Review implementation correctness
  • Post inline comments
  • Post summary

Verdict: ✅ ship (0 new findings)

Latest commit da52d021 resolves the last open finding from the previous pass:

  • ParseDouble round-trip for When_NonUniformGroupSizeLocale (Given_DecimalFormatter.cs:422) ✓ — assertion added, en-IN parse round-trip now explicitly guarded.

All three previous blocking findings are confirmed resolved:

  • FormatterHelper.NumberFormat XML doc trimmed ✓
  • TryResolveCulture heap allocation eliminated ✓
  • [GitHubWorkItem] on all new locale-aware test methods ✓
  • ResolveNumberFormat XML doc trimmed ✓

Carried-forward (non-blocking, pre-existing)

  • ResolvedGeographicRegion XML doc (DecimalFormatter.cs:98): still says "most recently used to format or parse" but the property is static post-construction. An inline comment from the previous pass already tracks this. Confirmed out-of-scope for this PR.
  • Double blank line (DecimalFormatter.cs:119–120): pre-existing, between the NumberRounder block and var stringBuilder = .... Flagged in all three prior passes; one blank line is the convention, but this is not introduced by the PR.

@unodevops

Copy link
Copy Markdown
Contributor

🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23888/wasm-skia-net9/index.html

@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @morning4coffe-dev's task in 6m 26s —— View job


Code Review

  • Gather context and understand the PR
  • Read changed files
  • Review implementation correctness
  • Review test coverage
  • Post inline comments
  • Post summary

Verdict: ✅ ship (2 findings: 1 test-quality, 1 correctness-minor)

All findings from the four previous review passes are confirmed resolved in ab685a6f. This pass found two new items.

Findings: 2

Inline:

  • Given_GlobalizationPreferences.cs:28Assert.AreEqual(CultureInfo.InvariantCulture, CultureInfo.CurrentCulture) in When_HomeGeographicRegion_Then_ReturnsNonEmptyRegion asserts test-runner environment state, not the feature under test. If the environment has a non-invariant culture, the test fails with a misleading message rather than documenting its own assumption.

  • GlobalizationPreferences.cs:134MarshalString.DisposeAbi(handle) is not in a try/finally. If FromAbi or Split throws, the native handle leaks. Practically safe (OOM), but the robust pattern is always to dispose in finally.

Cross-cutting notes

  • Double blank line in DecimalFormatter.FormatDouble between the NumberRounder block and var stringBuilder = (lines 119–120): flagged in all four prior passes, still present, pre-existing code not in this diff — can't anchor as inline. One blank line is the convention.

  • ValidateGeographicRegion parameter type (GeographicRegionHelper.cs:22): the parameter is string (non-nullable) but the method defensively checks is null. The tests call it with null! to probe this path. Annotating it as string? would make the null branch visible to nullable analysis without requiring a ! at every legitimate call site. Very minor; consistent with existing Uno validation helpers.

  • GetCurrentCultureLanguages (Skia/WASM shared helper, GlobalizationPreferences.cs:58): the fallback to "en-US" when CurrentUICulture.Name is empty is a safe invariant-culture default; the equivalent NormalizeRegionOrFallback fallback to "US" for HomeGeographicRegion is consistent.

  • NativeMethods DllImports (winlangdb.dll, bcp47langs.dll): these are semi-private Windows APIs. They are guarded by OperatingSystem.IsWindows() at every call site and only compiled under #if __SKIA__, so the risk is contained. The existing kernel32.dll/bcp47langs.dll pattern appears elsewhere in the Skia runtime layer.

Comment thread src/Uno.UI.UnitTests/Windows_System/UserProfile/Given_GlobalizationPreferences.cs Outdated
Comment thread src/Uno.UWP/System/UserProfile/GlobalizationPreferences.cs Outdated
@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @morning4coffe-dev's task in 4m 45s —— View job


Code Review

  • Gather context and understand the PR
  • Read changed files
  • Review implementation correctness
  • Review test coverage
  • Post inline comments
  • Post summary

Verdict: ✅ ship (0 new findings)

Both findings from the previous review pass (commit 557a9fdb) are confirmed resolved:

  • Given_GlobalizationPreferences.csAssert.AreEqual(CultureInfo.InvariantCulture, CultureInfo.CurrentCulture) removed; tests now validate only the product contract (IsTrue/IsFalse on shape and non-emptiness) ✓
  • GlobalizationPreferences.csMarshalString.FromAbi/Split wrapped in try/finally; native ABI handle is always disposed even on OOM ✓

All findings from the four earlier review passes are also confirmed resolved in this branch:

  • FormatterHelper.NumberFormat XML doc trimmed ✓
  • TryResolveCulture heap allocation eliminated (fallback chain, no new[]) ✓
  • [GitHubWorkItem] on all new locale-aware test methods ✓
  • ResolveNumberFormat XML doc trimmed to ≤8 lines ✓
  • en-IN ParseDouble round-trip assertion added ✓

Cross-cutting notes

  • Double blank line in DecimalFormatter.FormatDouble (lines 119–120, between the NumberRounder block and var stringBuilder): flagged across all five prior passes, pre-existing, outside the diff — can't anchor as an inline comment. One blank line is the convention.
  • The unit test project references Uno.UWP/Uno.Skia.csproj, so __SKIA__ is active during the test build and the GlobalizationPreferences implementations are exercised directly; no null-member risk.
  • ValidateGeographicRegion(string geographicRegion): parameter is non-nullable but the method defensively checks is null (test probes it with null!). Annotating as string? would remove the ! at the test call site and make the null branch statically visible. Very minor; consistent with existing Uno validation helpers.

#endif
#if __ANDROID__ || __IOS__ || __TVOS__ || __WASM__ || __SKIA__ || __NETSTD_REFERENCE__
[global::Uno.NotImplemented("__ANDROID__", "__IOS__", "__TVOS__", "__WASM__", "__SKIA__", "__NETSTD_REFERENCE__")]
#if false || false || false || false || false || __NETSTD_REFERENCE__
#endif
#if false || false || __TVOS__ || __WASM__ || false || __NETSTD_REFERENCE__
[global::Uno.NotImplemented("__TVOS__", "__WASM__", "__NETSTD_REFERENCE__")]
#if false || false || false || false || false || __NETSTD_REFERENCE__
#endif
#if __ANDROID__ || __IOS__ || __TVOS__ || __WASM__ || __SKIA__ || __NETSTD_REFERENCE__
[global::Uno.NotImplemented("__ANDROID__", "__IOS__", "__TVOS__", "__WASM__", "__SKIA__", "__NETSTD_REFERENCE__")]
#if false || false || false || false || false || __NETSTD_REFERENCE__
#endif
#if false || false || __TVOS__ || __WASM__ || false || __NETSTD_REFERENCE__
[global::Uno.NotImplemented("__TVOS__", "__WASM__", "__NETSTD_REFERENCE__")]
#if false || false || false || false || false || __NETSTD_REFERENCE__
#endif
#if __ANDROID__ || __IOS__ || __TVOS__ || __WASM__ || __SKIA__ || __NETSTD_REFERENCE__
[global::Uno.NotImplemented("__ANDROID__", "__IOS__", "__TVOS__", "__WASM__", "__SKIA__", "__NETSTD_REFERENCE__")]
#if false || false || false || false || false || __NETSTD_REFERENCE__
#endif
#if false || false || __TVOS__ || __WASM__ || false || __NETSTD_REFERENCE__
[global::Uno.NotImplemented("__TVOS__", "__WASM__", "__NETSTD_REFERENCE__")]
#if false || false || false || false || false || __NETSTD_REFERENCE__
@unodevops

Copy link
Copy Markdown
Contributor

🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23888/wasm-skia-net9/index.html

@unodevops

Copy link
Copy Markdown
Contributor

⚠️⚠️ The build 224227 has failed on Uno.UI - CI.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 838452d3-d864-4283-8690-7260d18c0672
@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @morning4coffe-dev's task in 3m 41s —— View job


Code Review

  • Gather context and understand the PR
  • Read changed files
  • Review implementation correctness
  • Review test coverage
  • Post inline comments
  • Post summary

Verdict: ✅ ship (1 style nit carried forward)

Commit 14223239 correctly addresses three WinRT contract gaps:

TryResolveCulture priority inversion fixed — language now takes precedence over geographic region (resolvedLanguage → primaryLang-region → primaryLang). This is the correct WinRT semantic: DecimalFormatter(["en-IN"], "US") formats with en-IN grouping, not en-US. The new When_LanguageResolves_Then_RegionDoesNotOverrideNumberFormat test pins all three cases (language-wins, region-ignored, bare-language fallback) cleanly.

ParseDouble positive-sign fixNumberStyles.Float | AllowThousands replaced with explicit flags excluding AllowLeadingWhite/AllowTrailingWhite. Positive-sign prefixes (+1, +1.2) now correctly return null; whitespace-padded strings are rejected by TryParse rather than by a separate index scan. The When_UniformGroupingHasMultipleSeparators_Then_RoundTrips assertion was corrected from AreEqual(1234567d, ...) to IsNull for "+1,234,567.00".

M49 region whitelistIsNumericM49 now validates against the NLS-backed _supportedNumericM49Regions sorted array (binary search). Pseudo-region 900–999 is accepted unconditionally. New test rows for 003/123/899 (invalid) and 000/840/900 (valid) cover the boundary correctly.

Findings: 1 (style, carried forward)

Inline:

  • GeographicRegionHelper.cs:52ValidateGeographicRegion(string geographicRegion) should be string?; null is a valid input path probed by the test, so it should be statically visible to nullable analysis. Non-blocking; flagged in prior passes.

Cross-cutting notes

  • Double blank line in DecimalFormatter.FormatDouble (lines 119–120, between the NumberRounder block and var stringBuilder): flagged in all six prior passes, pre-existing, not in this diff — cannot be anchored as an inline comment. One blank line is the convention.

  • #if false || false || false || false || false || __NETSTD_REFERENCE__ in Generated/.../GlobalizationPreferences.cs (lines 41 and 51): CodeQL flags this as false || A simplifiable to A. The pattern is generated-code boilerplate left behind when platforms are removed from [NotImplemented]. #if __NETSTD_REFERENCE__ would be correct and cleaner, but touching the generated file is already a deliberate exception made by this PR, and the semantics are identical.

  • NumberStyles.AllowExponent in the new ParseDouble flags: this was already present implicitly via NumberStyles.Float in the old code, so it is not a new behaviour change — no regression introduced.

/// for a null/empty/unrecognized region (mirrors <see cref="NumeralSystemTranslator"/>'s validation
/// conventions for <c>languages</c>).
/// </summary>
public static void ValidateGeographicRegion(string geographicRegion)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor (nullable annotation): geographicRegion is declared string (non-nullable) but the method body defensively checks is null. This means callers must use null! to probe the null path — visible in the test: new DecimalFormatter(new[] { "en-US" }, geographicRegion: null!). Annotating the parameter as string? would make the null guard statically visible to nullable analysis and remove the forced cast at the test call site:

Suggested change
public static void ValidateGeographicRegion(string geographicRegion)
public static void ValidateGeographicRegion(string? geographicRegion)

(Non-blocking; this pattern was flagged in prior passes and is still present.)

@unodevops

Copy link
Copy Markdown
Contributor

🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23888/wasm-skia-net9/index.html

@unodevops

Copy link
Copy Markdown
Contributor

⚠️⚠️ The build 225073 has failed on Uno.UI - CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/automation Categorizes an issue or PR as relevant to project automation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants