Skip to content

[net11.0] Controls: Annotate remaining converters and harden conversion behavior - #30875

Merged
kubaflo merged 18 commits into
dotnet:net11.0from
NirmalKumarYuvaraj:nullable-3
Jul 30, 2026
Merged

[net11.0] Controls: Annotate remaining converters and harden conversion behavior#30875
kubaflo merged 18 commits into
dotnet:net11.0from
NirmalKumarYuvaraj:nullable-3

Conversation

@NirmalKumarYuvaraj

@NirmalKumarYuvaraj NirmalKumarYuvaraj commented Jul 28, 2025

Copy link
Copy Markdown
Contributor

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 of Change

Annotates the remaining .NET MAUI Controls type converters for nullable reference types and updates the seven platform PublicAPI ledgers to match their actual override contracts.

The review also uncovered and fixes several converter failure paths:

  • StrokeShapeTypeConverter
    • Honors the culture supplied to direct TypeConverter calls.
    • Keeps XAML and XamlC numeric parsing invariant and exponent-aware.
    • Makes XamlC emit a RoundRectangle for a numeric radius instead of constructing a Rectangle and invoking a RoundRectangle setter on it.
  • FontAttributesConverter
    • Produces comma-separated output that round-trips.
    • Continues rejecting malformed comma tokens while treating whitespace-only input as FontAttributes.None.
  • BindablePropertyConverter
    • Returns null rather than throwing a NullReferenceException for unresolved direct-converter type names.
    • Validates truncated VisualState parent chains and reports XamlParseException with the most specific available diagnostic.
  • Simplifies converter implementations without changing valid-input mappings, including TextAlignmentConverter.

Root Cause

Several converters still used nullable-oblivious override signatures. While annotating them, tests exposed assumptions that were hidden by the old implementation: build-time parsing depended on the build host's culture, the compiled numeric StrokeShape fallback emitted the wrong shape type, FontAttributes used a non-round-trippable separator, and BindableProperty resolution indexed parent chains without checking their length.

Compatibility Notes

  • Runtime XAML, CSS, and the current SourceGen fallback use invariant conversion.
  • Direct TypeConverter.ConvertFrom(context, culture, value) calls honor the supplied culture.
  • No public members are added or removed; the PublicAPI updates record nullability annotations for existing overrides.

What NOT to Do

  • Do not use the build host's current culture when compiling numeric XAML literals.
  • Do not skip empty FontAttributes tokens; malformed comma-delimited values must remain errors.
  • Do not enable the optimized SourceGen StrokeShapeConverter until its numeric RoundRectangle fallback is implemented.

Validation

  • dotnet cake --target=dotnet-build --configuration=Release --verbosity=normal
  • 60 focused Controls Core converter tests
  • 24 focused XAML tests covering Runtime, XamlC, SourceGen fallback, culture isolation, and VisualState parent-chain diagnostics

Issues Fixed

No linked issue.

@dotnet-policy-service dotnet-policy-service Bot added the community ✨ Community Contribution label Jul 28, 2025
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Hey there @@NirmalKumarYuvaraj! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@dotnet-policy-service dotnet-policy-service Bot added the partner/syncfusion Issues / PR's with Syncfusion collaboration label Jul 28, 2025
@NirmalKumarYuvaraj
NirmalKumarYuvaraj marked this pull request as ready for review July 28, 2025 12:57
Copilot AI review requested due to automatic review settings July 28, 2025 12:57
@NirmalKumarYuvaraj
NirmalKumarYuvaraj requested a review from a team as a code owner July 28, 2025 12:57

Copilot AI left a comment

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.

Pull Request Overview

This pull request continues the series of updates to annotate remaining converter classes for nullability in the .NET MAUI Controls library. The changes focus on enabling nullable reference types and modernizing code syntax to improve type safety and code readability.

Key Changes:

  • Enables nullable reference types (#nullable enable) across multiple type converter classes
  • Updates method signatures in type converters to support nullable parameters
  • Modernizes code syntax with file-scoped namespaces and improved conditional statements

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
TextAlignment.cs Enabled nullability and modernized namespace declaration with improved conditional formatting
ShellSection.cs Updated type converter method signatures for nullable parameters
ShellNavigationState.cs Enabled nullability for ShellNavigationStateTypeConverter
ShellItem.cs Updated ShellItemConverter with nullable parameter support
ShellContent.cs Added nullability annotations to ShellContentConverter
StrokeShapeTypeConverter.cs Comprehensive refactoring with nullability, modernized syntax, and improved error handling
ResourceDictionary.cs Restructured RDSourceTypeConverter with nullable support and method reordering
FormattedString.cs Updated FormattedStringConverter with nullable annotations
FontAttributes.cs Enhanced FontAttributesConverter with nullability and improved conditional formatting
Button.cs Updated ButtonContentTypeConverter with nullable support
BindablePropertyConverter.cs Major refactoring with nullability enabled and method reorganization
PublicAPI files Added new public API entries for all updated converter method overrides

Comment thread src/Controls/src/Core/TextAlignment.cs Outdated
Comment thread src/Controls/src/Core/TextAlignment.cs Outdated
Comment thread src/Controls/src/Core/ResourceDictionary.cs Outdated
@jsuarezruiz

Copy link
Copy Markdown
Contributor

@NirmalKumarYuvaraj Could you rebase and fix the conflicts?

@NirmalKumarYuvaraj

Copy link
Copy Markdown
Contributor Author

@NirmalKumarYuvaraj Could you rebase and fix the conflicts?

@jsuarezruiz , I have rebased and resolved the conflicts. please let me know if you have any concerns.

@jsuarezruiz

Copy link
Copy Markdown
Contributor

@NirmalKumarYuvaraj Could you rebase and fix the conflicts? (Sorry, again!)

@NirmalKumarYuvaraj

Copy link
Copy Markdown
Contributor Author

@NirmalKumarYuvaraj Could you rebase and fix the conflicts? (Sorry, again!)

@jsuarezruiz , I have resolved the conflicts.

@rmarinho

Copy link
Copy Markdown
Member

needs rebase again sorry

@NirmalKumarYuvaraj

Copy link
Copy Markdown
Contributor Author

needs rebase again sorry

@rmarinho , rebased

@PureWeen

Copy link
Copy Markdown
Member

/azp run

@PureWeen PureWeen added this to the .NET 10.0-rc2 milestone Sep 18, 2025
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen moved this from Todo to Ready To Review in MAUI SDK Ongoing Sep 18, 2025
Comment thread src/Controls/src/Core/TextAlignment.cs Outdated
@PureWeen

PureWeen commented Oct 6, 2025

Copy link
Copy Markdown
Member

/azp run

@PureWeen PureWeen modified the milestones: .NET 10.0-rc2, .NET 10.0 GA Oct 6, 2025
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 3 pipeline(s).

MauiBot

This comment was marked as outdated.

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

This comment has been minimized.

Copilot AI left a comment

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.

Pull request overview

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

MauiBot

This comment was marked as outdated.

@kubaflo

This comment has been minimized.

1 similar comment
@kubaflo

This comment has been minimized.

@MauiBot MauiBot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

AI Review Summary

@NirmalKumarYuvaraj — new AI review results are available based on this last commit: bac9168. To request a fresh review after new comments or commits, comment /review rerun.

Gate Passed Confidence Low Platform Android


🗂️ Review Sessions — click to expand
🚦 Gate — Test Before & After Fix

Gate Result: ✅ PASSED

Platform: ANDROID · Base: net11.0 · Merge base: 7f139ed4

Test Without Fix (expect FAIL) With Fix (expect PASS)
🧪 FontAttributeConverterUnitTests FontAttributeConverterUnitTests ✅ FAIL — 113s ✅ PASS — 82s
🔴 Without fix — 🧪 FontAttributeConverterUnitTests: FAIL ✅ · 113s

Error-relevant lines (filtered from the build log):

     at Microsoft.Maui.Controls.FontAttributesConverter.ParseSingleAttribute(String part, String originalvalue) in /_/src/Controls/src/Core/FontAttributes.cs:line 63
   at Microsoft.Maui.Controls.FontAttributesConverter.ConvertFrom(ITypeDescriptorContext context, CultureInfo culture, Object value) in /_/src/Controls/src/Core/FontAttributes.cs:line 48
   at Microsoft.Maui.Controls.Core.UnitTests.FontAttributeConverterUnitTests.FontAttributesRoundTripThroughString(FontAttributes attributes) in /_/src/Controls/tests/Core.UnitTests/FontAttributeConverterUnitTests.cs:line 31
   at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
   at System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(Object obj, Span`1 copyOfArgs, BindingFlags invokeAttr)
🟢 With fix — 🧪 FontAttributeConverterUnitTests: PASS ✅ · 82s

(no coded error found; showing last 1200 chars)

125)
[xUnit.net 00:00:00.30]   Discovering: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:02.61]   Discovered:  Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:02.64]   Starting:    Microsoft.Maui.Controls.Core.UnitTests
  Passed FontAttributesRoundTripThroughString(attributes: Bold | Italic) [27 ms]
  Passed FontAttributesRoundTripThroughString(attributes: Bold) [< 1 ms]
  Passed FontAttributesRoundTripThroughString(attributes: Italic) [< 1 ms]
  Passed FontAttributesRoundTripThroughString(attributes: None) [< 1 ms]
  Passed TestFontAttributeConverter(input: "Bold", expected: Bold) [< 1 ms]
  Passed TestFontAttributeConverter(input: "oblique", expected: Italic) [< 1 ms]
  Passed TestFontAttributeConverter(input: "italic", expected: Italic) [< 1 ms]
  Passed TestFontAttributeConverter(input: "None", expected: None) [< 1 ms]
  Passed TestFontAttributeConverter(input: "oblique, bold", expected: Bold | Italic) [< 1 ms]
[xUnit.net 00:00:02.80]   Finished:    Microsoft.Maui.Controls.Core.UnitTests
  Passed TestFontAttributeConverter(input: "bold italic", expected: Bold | Italic) [< 1 ms]

Test Run Successful.
Total tests: 10
     Passed: 10
 Total time: 3.5527 Seconds

📁 Fix files reverted (18 files)
  • src/Controls/src/Core/BindablePropertyConverter.cs
  • src/Controls/src/Core/Button/Button.cs
  • src/Controls/src/Core/FontAttributes.cs
  • src/Controls/src/Core/FormattedString.cs
  • src/Controls/src/Core/PublicAPI/net-android/PublicAPI.Unshipped.txt
  • src/Controls/src/Core/PublicAPI/net-ios/PublicAPI.Unshipped.txt
  • src/Controls/src/Core/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt
  • src/Controls/src/Core/PublicAPI/net-tizen/PublicAPI.Unshipped.txt
  • src/Controls/src/Core/PublicAPI/net-windows/PublicAPI.Unshipped.txt
  • src/Controls/src/Core/PublicAPI/net/PublicAPI.Unshipped.txt
  • src/Controls/src/Core/PublicAPI/netstandard/PublicAPI.Unshipped.txt
  • src/Controls/src/Core/ResourceDictionary.cs
  • src/Controls/src/Core/Shapes/StrokeShapeTypeConverter.cs
  • src/Controls/src/Core/Shell/ShellContent.cs
  • src/Controls/src/Core/Shell/ShellItem.cs
  • src/Controls/src/Core/Shell/ShellNavigationState.cs
  • src/Controls/src/Core/Shell/ShellSection.cs
  • src/Controls/src/Core/TextAlignment.cs

📱 UI Tests — Button,Shape,Shell

Detected UI test categories: Button,Shape,Shell

Deep UI tests — 414 passed, 0 failed across 3 categories on platform-pool agent (replaces in-process counts above).

🧪 UI Test Execution Results (deep, platform pool)

Category Tests Snapshot diffs
Button 67/69 ✓
Shape 35/35 ✓
Shell 312/312 ✓
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs)

📋 Pre-Flight — Context & Validation

Issue: #N/A - No linked issue found in PR body
PR: #30875 - [Net 11] - Annotated remaining Converter classes for nullability - 3
Platforms Affected: all Controls platforms; testing platform requested: android
Files Changed: 18 implementation/API, 1 test

Key Findings

  • PR is a nullability/API annotation update for Controls converters with behavior fixes around converter round-tripping and culture/null handling.
  • No linked issue was present (Fixes # is empty), so context came from PR description, changed files, and review comments.
  • Prior inline reviews flagged FontAttributesConverter.ConvertTo separator round-trip behavior, StrokeShapeTypeConverter null culture handling, ShellNavigationState.ConvertTo nullable mismatch, and PublicAPI nullability-entry formatting.
  • Gate result provided by caller: ✅ PASSED — tests fail without fix and pass with fix. Gate was not re-run and gate/content.md was not modified.
  • GitHub CLI is unauthenticated in this environment, so PR/check context was gathered from public GitHub API and local branch state where possible.

Code Review Summary

Verdict: NEEDS_DISCUSSION
Confidence: low
Errors: 0 | Warnings: 0 | Suggestions: 0

Key code review findings:

  • ℹ No high-confidence code findings remained after prior review items were reconciled.
  • ℹ CI status remained undetermined/red from available public status context, so the code-review verdict could not be LGTM.
  • ℹ Blast radius is localized to converter paths and PublicAPI metadata; no startup/global-state risk identified.

Fix Candidates

# Source Approach Test Result Files Changed Notes
PR PR #30875 Enable nullable annotations across remaining converter classes; preserve converter behavior while fixing FontAttributesConverter.ConvertTo to emit comma-separated values that round-trip; add PublicAPI nullability entries and a FontAttributes round-trip unit test. ✅ PASSED (Gate) FontAttributes.cs, converter files, PublicAPI files, FontAttributeConverterUnitTests.cs Original PR

🔬 Code Review — Deep Analysis

Code Review — PR #30875

Independent Assessment

What this changes: Annotates remaining Controls converter classes for nullable reference types, adds/updates TypeConverter overrides and PublicAPI entries, and adds FontAttributes converter round-trip coverage.
Inferred motivation: Reduce nullable warnings/API drift while preserving converter behavior.

Reconciliation with PR Narrative

Author claims: Nullability improvements, converter refactoring, syntax modernization, PublicAPI updates.
Agreement/disagreement: Matches the code. Prior converter issues appear addressed.

Prior Review Reconciliation

Prior Finding Source Status Evidence
PublicAPI entries incorrectly ~-prefixed MauiBot [major] ✅ Fixed Current entries are annotated without ~: PublicAPI/net/PublicAPI.Unshipped.txt:115-143
StrokeShapeTypeConverter ignored culture for double parsing Copilot ✅ Fixed Uses double.TryParse(strValue, NumberStyles.Number, culture, ...): StrokeShapeTypeConverter.cs:146
FontAttributesConverter.ConvertTo bad separator MauiBot summary ✅ Fixed Uses string.Join(", ", parts) and has round-trip test: FontAttributes.cs:102, test lines 26-31
TargetType typo MauiBot summary ✅ Fixed Current messages spell TargetType

Blast Radius Assessment

  • Runs for all instances: No; only converter paths / XAML/CSS conversions.
  • Startup impact: Low; no new startup path.
  • Static/shared state: No new mutable static state in reviewed changes.

CI Status

  • Required-check result: fail. gh pr checks --required unavailable due missing auth; GitHub check-runs fallback shows maui-pr failed.
  • Classification: undetermined. AzDO build 1509587 failed in Windows Helix Unit Tests (Release) due CreateTimerNonRepeatingDoesNotRepeat NRE in Core dispatcher tests; base net11.0 build 1501450 had that leg green. Also one iOS integration job was canceled during artifact download.
  • Action taken: Loaded azdo-build-investigator; confidence capped low.

Findings

No high-confidence code findings. Step 2 expert reviewer produced inline-findings.json with [].

Failure-Mode Probing

  • Null converter inputs: Existing/updated converters either return default/null or throw expected NotSupportedException/InvalidOperationException; no new NRE found.
  • Culture-sensitive shape parsing: culture ??= CurrentCulture and NumberStyles.Number are now used consistently.
  • PublicAPI nullability: New converter entries match nullable annotations and are not incorrectly oblivious-prefixed.

Verdict: NEEDS_DISCUSSION

Confidence: low
Summary: Code review found no remaining blocking code issues, and prior review findings appear fixed. I cannot LGTM while required CI is red and not confidently unrelated; the PR needs human/CI follow-up on the Helix failure/canceled leg before merge.


🛠️ Fix — Analysis & Comparison

Fix Candidates

# Source Approach Test Result Files Changed Notes
1 try-fix Accept the legacy apostrophe separator by trimming trailing ' before enum parsing. ✅ PASS 1 file Works, but preserves malformed legacy output and broadens accepted syntax.
2 try-fix Emit CSS-style space-separated tokens (Bold Italic) from ConvertTo. ✅ PASS 1 file Works via existing parser branch, but changes serialized output away from enum/XAML comma style.
3 try-fix Replace manual flag list with attr.ToString() for non-None values. ✅ PASS 1 file Best alternative: smaller than PR fix, preserves comma-style output, automatically handles future flags.
PR PR #30875 Manually build known flag names and join with ", "; includes broader nullable converter/PublicAPI updates. ✅ PASSED (Gate) 18 implementation/API + 1 test Original PR.

Expert Review / Learn Loop

Candidate Expert-review input Learned outcome
1 Code-review skill + MAUI expert rules found no remaining blocking converter issue; candidate probed parser-side tolerance. Passing but not preferred because the bad legacy output remains observable.
2 Candidate incorporated candidate 1 tradeoff by fixing output instead of parser tolerance. Passing but not preferred because output switches to CSS-style space tokens.
3 Candidate incorporated candidate 2 tradeoff by keeping comma-style output while removing manual known-flag enumeration. Passing and selected as best alternative.

Test command used for candidates: dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj --filter FontAttributeConverterUnitTests --logger "console;verbosity=minimal"

Environment note: The main worktree had unrelated dirty changes that blocked EstablishBrokenBaseline.ps1. Candidate implementation/testing used an isolated temporary worktree created from PR HEAD, then established the broken baseline there with -BaseBranch net11.0. Gate verification was not re-run and gate/content.md was not modified.

Exhausted: Yes — three meaningfully different root-cause approaches were tested: parser tolerance, alternate output contract, and runtime enum flag formatting. Candidate 3 passes all targeted regression tests and is demonstrably better than the PR's current manual List<string> fix.

Selected Fix: Candidate #3Enum.ToString() formatting. It is the smallest passing change, maintains the intended comma-separated flag output, and avoids future maintenance bugs if FontAttributes gains additional named flags.


📝 Recommended PR Title & Description

Assessment: ✏️ Recommend updating — the current title is vague/noisy and the description overstates broad refactoring while missing the actual winning fix and the key FontAttributesConverter round-trip behavior.

Recommended title

[Controls] Converters: Annotate remaining types for nullability and fix FontAttributes round-trip

Recommended description

### Description of Change

Enables nullable reference type annotations across the remaining `Microsoft.Maui.Controls` converter-related classes and updates their `TypeConverter` override signatures to match nullable `System.ComponentModel.TypeConverter` contracts.

The converter updates include:
- `BindablePropertyConverter`
- `ButtonContentTypeConverter`
- `FontAttributesConverter`
- `ListStringTypeConverter`
- `RowDefinitionCollectionTypeConverter`
- `ColumnDefinitionCollectionTypeConverter`
- `GridLengthTypeConverter`
- `ResourceDictionary.RDSourceTypeConverter`
- `StrokeShapeTypeConverter`
- `ShellContentConverter`
- `ShellItemConverter`
- `ShellSectionConverter`
- `ShellNavigationStateTypeConverter`
- `TextAlignmentConverter`

The winning fix for the `FontAttributesConverter` round-trip regression is to return `FontAttributes.ToString()` for non-`None` values. This emits the standard comma-separated flags format (for example, `Bold, Italic`) that the existing comma-separated parser already accepts, instead of the legacy apostrophe-separated output that did not round-trip.

Public API annotations were updated in each `PublicAPI.Unshipped.txt` file for the nullable converter override signatures.

### Key Technical Details

- `FontAttributes.None` continues to convert to an empty string.
- Non-`None` `FontAttributes` values use enum flag formatting rather than manually listing known flags, so future named flags will format consistently without additional converter changes.
- Existing malformed comma-separated inputs should continue to fail fast rather than being silently accepted.

### Issues Fixed

Fixes #

🏁 Report — Final Recommendation

Comparative Candidate Report — PR #30875

Candidates

Candidate Result Assessment
pr ✅ Gate passed Fixes the FontAttributesConverter.ConvertTo round-trip bug, but expert review found a blocking parser regression: malformed comma-separated values such as ",", ",,", and "Bold,,Italic" are now silently accepted because of StringSplitOptions.RemoveEmptyEntries.
pr-plus-reviewer ✅ Targeted sandbox tests passed Preserves the PR's broader nullability/PublicAPI changes and fixes the expert-review finding by restoring fail-fast comma parsing. This is the best PR-shaped candidate if the broader converter nullability annotation work is required.
try-fix-1 ✅ Passed Parser-side tolerance for the legacy apostrophe output works, but it preserves malformed serialized output and broadens accepted syntax. It is ranked below output-side fixes.
try-fix-2 ✅ Passed Emits CSS-style space-separated tokens, which round-trip through the existing parser, but changes serialized output away from enum/XAML comma style.
try-fix-3 ✅ Passed Uses FontAttributes.ToString() for non-None values. This produces the same comma-separated style needed for round-trip compatibility, preserves the existing comma parser behavior, and avoids hard-coding every known flag.

Ranking

  1. try-fix-3 — best isolated fix for the proven regression. It is the smallest passing change, preserves the expected comma-separated enum/XAML output, avoids the raw PR's parser-leniency regression, and is more future-proof than manually listing Bold and Italic.
  2. pr-plus-reviewer — best PR-shaped option. It keeps the PR's broader nullability annotation and PublicAPI changes and addresses the expert reviewer's blocking feedback, but its ConvertTo implementation remains more manual than necessary.
  3. try-fix-2 — passes, but changes the output contract to CSS-style space-separated tokens.
  4. try-fix-1 — passes, but fixes the symptom by tolerating legacy malformed output rather than correcting output generation.
  5. pr — passes the supplied gate, but ranks below all passing alternatives because it introduces a newly confirmed malformed-input parsing regression.

Winner

Winner: try-fix-3

try-fix-3 is the strongest candidate for the specific regression covered by the gate: it fixes round-tripping by delegating flag formatting to the runtime enum formatter, preserves comma-separated output, avoids accepting malformed comma input, and naturally handles future FontAttributes flags without additional code changes. No candidate failed regression tests, so the ranking is based on correctness, behavioral compatibility, and maintenance risk.


🧭 Next Steps — alternative fix proposed (try-fix-3)

Automated review — alternative fix proposed

The expert-reviewer evaluation compared the PR fix against automatically generated candidates and selected try-fix-3 as the strongest fix.

Why: try-fix-3 wins because it is the smallest passing fix for the FontAttributes round-trip regression, emits the expected comma-separated enum format, preserves existing malformed comma-input failures, and avoids manual flag enumeration.

Please consider applying the candidate diff below (or use it as guidance). Once you push an update, this workflow will re-trigger and re-evaluate.

Candidate diff (try-fix-3)
diff --git a/src/Controls/src/Core/FontAttributes.cs b/src/Controls/src/Core/FontAttributes.cs
--- a/src/Controls/src/Core/FontAttributes.cs
+++ b/src/Controls/src/Core/FontAttributes.cs
@@ -69,12 +69,7 @@ namespace Microsoft.Maui.Controls
 				throw new NotSupportedException();
 			if (attr == FontAttributes.None)
 				return "";
-			var parts = new List<string>();
-			if ((attr & FontAttributes.Bold) == FontAttributes.Bold)
-				parts.Add(nameof(FontAttributes.Bold));
-			if ((attr & FontAttributes.Italic) == FontAttributes.Italic)
-				parts.Add(nameof(FontAttributes.Italic));
-			return string.Join("' ", parts);
+			return attr.ToString();
 		}
 	}
 }

@kubaflo

kubaflo commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

🔍 This is an AI-generated comment posted on @kubaflo's behalf.

Fresh AI review completed on bac9168 (improved-reviewer, Android)

Gate: ✓ PASSEDFontAttributeConverterUnitTests fails without the fix and passes with it (Android · net11.0 · merge-base 7f139ed4).
Deep UI tests: ✓ 414 passed / 0 failed across Button, Shape, Shell.

Final Recommendation: ✗ not yet APPROVE — Confidence Low. The expert-reviewer pass found one blocking parser regression that keeps this from a clean LGTM:

  • ConvertTo currently emits a malformed separator, and the ConvertFrom split now silently accepts malformed comma input such as ",", ",,", and "Bold,,Italic" (empty entries no longer fail fast).

ℹ Path to APPROVE — the reviewer ranked candidate try-fix-3 as the winner. Delegate flag formatting to the runtime enum formatter so output round-trips as comma-separated enum/XAML style, and keep malformed comma input failing fast:

-			var parts = new List<string>();
-			if ((attr & FontAttributes.Bold) == FontAttributes.Bold)
-				parts.Add(nameof(FontAttributes.Bold));
-			if ((attr & FontAttributes.Italic) == FontAttributes.Italic)
-				parts.Add(nameof(FontAttributes.Italic));
-			return string.Join("' ", parts);
+			return attr.ToString();

This is the smallest passing change, preserves the expected comma-separated output, avoids hard-coding each flag, and — paired with rejecting empty split entries in ConvertFrom — clears the regression.

@NirmalKumarYuvaraj — pushing that update will auto re-trigger the review workflow and re-evaluate. Once the parser-strictness finding is resolved, the next round should reach APPROVE.

@kubaflo

kubaflo commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).

@kubaflo

This comment has been minimized.

@github-actions

Copy link
Copy Markdown
Contributor

Tests Failure Analysis

@NirmalKumarYuvaraj — test-failure review results are available based on commit bac9168.
To request a fresh review after new comments, commits, or CI runs, comment /review tests.

Overall Not ready Failures 192 Regressed vs base 154 Baseline 11 on base

Test Failure Review: Not ready - click to expand

Overall verdict: Not ready. The deterministic gate flags 154 UITest legs as red on this PR but green across the 5 recent net11.0 base builds sampled per definition, so they are computed regressions vs base; another 38 failures (device-test crashes, build-analysis breaks, provisioning) could not be attributed either way, and 11 image visual-diff failures are flaky on base. No failure qualifies as cleanly pre-existing or a known issue, so a green verdict is impossible.

  • ✗ PR-related — UITest OneTimeSetUp harness failures marked regressed vs base (~154 legs): every leg shares the same Appium symptom — OpenQA.Selenium.UnknownErrorException: Could not proxy command to the remote server (proxy timeout / ECONNREFUSED 127.0.0.1:10100) — which reads like an Appium harness connection loss rather than the PR's nullability annotations, but it is deterministically green on base and red here, so a human must confirm; representative ScrollViewShouldScroll.
  • i Uncertain — Essentials device-test crashes / unverified device tests (~20 tests + 6 unverified legs): repeated System.NullReferenceException and crashed/incomplete Helix work items on iOS/MacCatalyst, plus 6 green device-test checks whose Failed==0 could not be confirmed; representative OpenAppPackageFileAsync_Can_Load_File.
  • i Uncertain — build-analysis breaks, Android SDK provisioning, and unexplained build legs (~24 legs): CA1416/CS0012 Essentials build errors, platform-tools;35.0.2 provisioning warning, and DeviceTests build-error legs with no extractable test name.
  • i Uncertain — Image visual-diff flakiness also present on base (~11 tests): VisualTestUtils.VisualTestFailedException flaky on base (indeterminate), not dismissible; representative VerifyImageButtonAspect_AspectFillWithImageSourceFromUri.

Coverage: 141 checks · 124 passing · 16 failing · 1 pending · 0 inaccessible · 1 unmapped · 24 unexplained build legs · 0 unaccounted failing checks · 3 aborted failing checks · 0 canceled-build checks · 6 device-test unverified · 38 unattributed · 154 regressed-vs-base. Deterministic ceiling: Not ready — 154 legs regressed vs base plus a pending check and 24 unexplained build legs.

Builds (this PR): maui-pr 1514757, maui-pr-devicetests 1514760, maui-pr-uitests 1514758. Base sampling (net11.0, 5 recent builds per definition): maui-pr 1501450, maui-pr-devicetests 1501451, maui-pr-uitests 1501538.

Recommended action

Have a human confirm whether the 154 regressed-vs-base UITest legs are a genuine regression or an Appium harness/infrastructure flake (rerun the maui-pr-uitests legs), and let the pending MacCatalyst CarouselView check finish before merging.

@kubaflo kubaflo left a comment

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.

Can you please resolve conflicts?

Copilot AI added 5 commits July 29, 2026 17:32
Resolve PublicAPI.Unshipped.txt conflicts while retaining both converter annotations and current net11 API additions.

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

Copilot-Session: a6db2315-d16d-4f19-94ec-2e5a7cbccda6
Reject malformed FontAttributes tokens, retain exponent-aware StrokeShape parsing, and align the compiled converter with runtime behavior. Add runtime, XamlC, and SourceGen regression coverage and restore final newlines in touched files.

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

Copilot-Session: a6db2315-d16d-4f19-94ec-2e5a7cbccda6
Cover invalid BindableProperty values, whitespace-only font attributes, and invariant compiled StrokeShape parsing under a non-invariant build culture.

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

Copilot-Session: a6db2315-d16d-4f19-94ec-2e5a7cbccda6
Preserve specific VisualState parent errors while guarding every truncated chain, annotate the nullable type lookup, document StrokeShape culture semantics, and record the SourceGen fallback prerequisite.

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

Copilot-Session: a6db2315-d16d-4f19-94ec-2e5a7cbccda6
Restore the test culture explicitly and cover every specific VisualState parent-chain diagnostic.

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

Copilot-Session: a6db2315-d16d-4f19-94ec-2e5a7cbccda6

Copilot AI left a comment

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.

Pull request overview

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

Comments suppressed due to low confidence (2)

src/Controls/src/Core/FontAttributes.cs:53

  • When parsing space-delimited (CSS/single-value) inputs, strValue.Split(' ') will produce empty tokens for repeated spaces (e.g., "bold italic"), which then throws in ParseSingleAttribute. Since this converter already trims and treats whitespace-only input as None, it should also ignore empty tokens caused by repeated spaces by using StringSplitOptions.RemoveEmptyEntries.
			// CSS or single value
			foreach (var part in strValue.Split(' '))
			{
				attributes |= ParseSingleAttribute(part, strValue);
			}

src/Controls/tests/Core.UnitTests/FontAttributeConverterUnitTests.cs:20

  • Assert.Equal expects (expected, actual); the current argument order is reversed, which makes failures harder to interpret.
			Assert.Equal(conv.ConvertFromInvariantString(input), expected);

@kubaflo

kubaflo commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

/azp run maui-pr

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@kubaflo

kubaflo commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

/azp run maui-pr

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

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

Labels

community ✨ Community Contribution partner/syncfusion Issues / PR's with Syncfusion collaboration s/agent-fix-win AI found a better alternative fix than the PR s/agent-gate-passed AI verified tests catch the bug (fail without fix, pass with fix) s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.