Add Binding converter culture support#35821
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35821Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35821" |
AI code review refresh for net11.0 targetHead reviewed: SummaryIndependent read of the diff before the narrative.
The default getter What's good
Findings (non-blocking)
CI note
No PR-caused CI failure was identified. I did not exhaustively open every job, so attribution is high-confidence but not absolute. ConfidenceCode correctness: medium‑high. CI attribution: medium‑high.
|
|
@StephaneDelcroix would you mind taking a look at this one when you have a chance? This targets |
AI code review refresh for net11.0 targetHead reviewed: What changed since the last refresh (
|
kubaflo
left a comment
There was a problem hiding this comment.
PR #35821 — Add Binding ConverterCulture support (issue #5696)
Verdict: NEEDS_DISCUSSION (confidence: medium) — HEAD 0a97f91. The code is well-structured — 3 of 4 models (gpt-5.5, opus-4.6, gemini) return LGTM with only minor suggestions (opus-4.8 timed out without a verdict). The hold is a PR-specific CI failure, below.
Implementation
Adds a ConverterCulture to Binding/MultiBinding/TypedBinding, the XAML BindingExtension, and threads it through both the XamlC (SetPropertiesVisitor) and source-generated (CompiledBindingMarkup/KnownMarkups) binding paths, with PublicAPI.Unshipped.txt entries across all TFMs. Unit tests (BindingConverterCultureTests, MultiBindingTests) and a XAML test (Maui5696 using ConverterCulture='nl-NL') are included.
⚠️ Blocking question — Windows Helix unit-test (Debug) leg is red only on this PR
maui-pr → Run Helix Unit Tests Windows (Debug) is failing on this head (buildId 1461702), while the Release variant passes. I checked the same Debug leg on 4 other concurrently-open PRs (#35597, #35265, #34136, #35922) — it is green on all of them, so this is not the usual flake; it is specific to this PR, which adds the new binding-culture tests.
Likely root cause to check: whether the ConverterCulture string (e.g. 'nl-NL' in Maui5696.xaml) is converted to a CultureInfo (via CultureInfoConverter) consistently across all three binding paths — runtime reflection, XamlC (SetPropertiesVisitor), and SourceGen (KnownMarkups). If one path leaves it as a raw string or applies a different default (CurrentCulture vs InvariantCulture), a culture assertion would fail. Please confirm which test fails on the Windows Debug leg and fix, or confirm it's an unrelated flake via rerun.
Minor suggestions (inline, non-blocking)
- 💡
KnownMarkups.cs:455— generated code allocates a newCultureInfoConverterper binding; consider caching. - 💡
SetPropertiesVisitor.cs:387— considerConverterCultureparity forTemplateBindingExtension.
The other red legs (Build Analysis, AOT macOS, RunOniOS_MauiRelease/TrimFull/CoreCLR) are the known unrelated flakes.
| if (converterCultureNode is ValueNode { Value: string converterCulture }) | ||
| { | ||
| expression += $"ConverterCulture = (global::System.Globalization.CultureInfo)new global::System.ComponentModel.CultureInfoConverter().ConvertFromInvariantString({SymbolDisplay.FormatLiteral(converterCulture, true)})!, "; | ||
| } |
There was a problem hiding this comment.
The source-generated binding code instantiates a fresh CultureInfoConverter for each binding when a ConverterCulture string is present. Since CultureInfoConverter is stateless, consider emitting a single cached/static converter (or converting the culture string once at generation time into a CultureInfo.GetCultureInfo("...") call) to avoid a per-binding allocation on a hot path. Minor — not blocking.
There was a problem hiding this comment.
Good suggestion — addressed in e21a5d0. Literal converter culture values now emit CultureInfo.GetCultureInfo("..."), so generated code no longer allocates a CultureInfoConverter per binding.
|
|
||
| if (bindingExtensionType.Value.Item3 == "BindingExtension") | ||
| { | ||
| // extension.TypedBinding.ConverterCulture = extension.ConverterCulture; |
There was a problem hiding this comment.
For full parity, consider whether ConverterCulture should also be supported on TemplateBindingExtension, which shares much of this binding-construction path. Right now ConverterCulture is wired for Binding/MultiBinding but a TemplateBinding with a converter won't honor a culture. Non-blocking — flagging for completeness/consistency.
There was a problem hiding this comment.
Leaving this out intentionally for this PR. TemplateBindingExtension currently has no ConverterCulture API surface, so adding parity there would require a separate public API addition beyond the Binding/MultiBinding support in scope here.
|
Addressed the source-gen allocation review comment in e21a5d0 by emitting |
✅ LGTM — no blocking issues foundRe-review PR #35821 — Binding
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
e21a5d0 to
21eeedc
Compare
✅ LGTM — no blocking issues foundRe-review PR #35821 — Add Binding
|
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description
Adds
Binding.ConverterCulturesupport for #5696 so bindings can pass an explicitCultureInfotoIValueConverter.ConvertandConvertBackinstead of always usingCultureInfo.CurrentUICulture.The implementation preserves existing
Bindingconstructor signatures and wires the property through runtime bindings, typed bindings, runtime XAML, XamlC, and source-generated XAML.This intentionally does not add new
ConverterCultureAPIs toMultiBindingorTemplateBinding.MultiBindingparity can be considered separately, andTemplateBindinghas noConverterCultureproperty to propagate.Issues Fixed
Fixes #5696
Testing
git diff --checkdotnet build src/Controls/src/Build.Tasks/Controls.Build.Tasks.csproj --no-restoredotnet build src/Controls/src/SourceGen/Controls.SourceGen.csproj --no-restoredotnet test src/Controls/tests/BindingSourceGen.UnitTests/Controls.BindingSourceGen.UnitTests.csproj --no-restoredotnet test src/Controls/tests/SourceGen.UnitTests/SourceGen.UnitTests.csproj --no-restoreLANG=en_US.UTF-8 LC_ALL=en_US.UTF-8 dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj --no-restoreLANG=en_US.UTF-8 LC_ALL=en_US.UTF-8 dotnet test src/Controls/tests/Xaml.UnitTests/Controls.Xaml.UnitTests.csproj --no-restore -p:WarningsNotAsErrors=XC0618%3BXC0619%3BXC0022%3BXC0023%3BXC0025%3BXC0045%3BXC0067%3BMAUIG2045%3BMAUID1000%3BMAUIX2005%3BMAUIX2015%3BMAUIX2017