[Net11]Improve label mapping performance and ensure complete coverage including ToPlatform and subsequent property changes#35892
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35892Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35892" |
dotnet-policy-service
Bot
added
the
partner/syncfusion
github-actions
Bot
added
area-controls-label
AI code review refresh for net11.0 targetHead reviewed: Verdict: Needs discussion (WIP)Solid, well-reasoned approach with a large global blast radius and some scope creep. No definite bugs found in the changed source, but several items below warrant confirmation before this leaves WIP. What the change does (independent reading of the diff)
Prior review reconciliationNo prior PR reviews, review comments, or earlier fleet markers exist (only the dogfood bot comment). Nothing to reconcile; no outstanding ❌ findings. CI statusBuild 1461094 still in progress. Most build/pack/Helix/integration jobs pass; 3 iOS jobs fail: Blast radius / failure‑mode probingThe Core handler files are cross‑platform, so the mapper reordering + connection guards affect every Label, Entry, and Button instance on all platforms, not just iOS.
Concise findings
ConfidenceMedium. Source‑level reasoning is high‑confidence (changes read cleanly, key invariants verified). Runtime/visual correctness across non‑iOS platforms and snapshot validity are unverified here and depend on the in‑progress CI + device/UI tests. Automated non‑binding review. This is not an approval and does not request changes; a human maintainer owns the merge decision. |
vishnumenon2684
changed the title
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
PR #35892 — [Net11] Improve Label mapping performance + complete coverage (ToPlatform & subsequent property changes)
Verdict: NEEDS_DISCUSSION (confidence: medium) — HEAD aedd137. The core change is correct — the deep reviews (opus-4.8, opus-4.6) confirmed no Label/Entry mappings were dropped and runtime (non-connect) behavior is preserved. Three precise, non-blocking items remain (2 inline + an API note); CI red is the known flake set.
What it does (sound)
Consolidates the Label/Entry/Button mappers so Text is mapped first (into a dedicated TextMapper) and IsConnectingHandler() guards skip redundant re-mapping during connect — avoiding repeated AttributedText rebuilds on iOS. The ToNSAttributedString LineBreakMode plumbing is a real fix. Mapper consolidation and ordering were independently vetted as coherent by 3 of 4 models.
Findings (inline)
⚠️ EntryHandler.cs:30 — the Text-ordering comment is inaccurate (HorizontalTextAlignment/VerticalTextAlignment are declared beforeText) and the resulting order diverges from Label/Button. Benign at runtime, but misleading in an ordering-focused PR. (opus-4.8 + opus-4.6 consensus.)- 💡 EntryHandler.cs:39 —
EntryPriorityMapperis now orphaned dead code (confirmed viagit grepat HEAD); remove it. - 💡 FormattedStringExtensions.cs:43 — adding the optional
defaultLineBreakModeis a binary-breaking change to a shipped publicPlatformextension; handled via*REMOVED*in PublicAPI (gate satisfied) — flagging to confirm the break is intentional vs. adding a preserving overload.
Dropped (false positive)
A model flagged Entry.Mapper.cs MapTextTransform → MapText as "bypassing UpdateValue/user customizations." Dropped: the ReplaceMapping(TextTransform, MapText) wiring is pre-existing — the PR only adds an IsConnectingHandler() guard around it (a perf optimization), and opus-4.8 confirmed runtime behavior is preserved. Not a new defect.
CI — known flakes
Red legs are the universal set seen across all PRs today: Build Analysis, AOT macOS, RunOniOS_MauiRelease/MauiReleaseTrimFull/TrimFull_CoreCLR. The Debug-passes / Release+Trim+AOT-fails pattern matches trimming-baseline noise; RunOniOS_MauiDebug, MauiRelease_CoreCLR, MauiNativeAOT pass. Not PR-caused.
| [nameof(IEntry.IsPassword)] = MapIsPassword, | ||
| [nameof(IEntry.HorizontalTextAlignment)] = MapHorizontalTextAlignment, | ||
| [nameof(IEntry.VerticalTextAlignment)] = MapVerticalTextAlignment, | ||
| // Ensure Text is mapped before LineHeight/Decorations/CharacterSpacing/HorizontalTextAlignment/TextColor/Font |
There was a problem hiding this comment.
This ordering comment doesn't match the actual order, and the order diverges from Label/Button. The comment says "Ensure Text is mapped before LineHeight/Decorations/CharacterSpacing/HorizontalTextAlignment/TextColor/Font", but in this same TextMapper HorizontalTextAlignment (line 28) and VerticalTextAlignment (line 29) are declared before Text (line 32). It's benign at runtime since text alignment on UITextField is independent of the AttributedText built when mapping Text — but in a PR whose whole point is mapper ordering, the misleading comment plus the inconsistency with the Label/Button mappers is a maintenance trap. Either move Text above the alignment entries, or scope the comment to the properties that truly must follow Text (Font, CharacterSpacing, TextColor).
| [nameof(IEntry.TextColor)] = MapTextColor | ||
| }; | ||
|
|
||
| public static IPropertyMapper<IEntry, IEntryHandler> Mapper = new PropertyMapper<IEntry, IEntryHandler>(TextMapper, ViewHandler.ViewMapper) |
There was a problem hiding this comment.
EntryPriorityMapper (line 20) is now dead code. Mapper here chains (TextMapper, ViewHandler.ViewMapper) and no longer references EntryPriorityMapper, and its only entry (MaxLength) was relocated into TextMapper (line 33). Confirmed via git grep at this commit that nothing else references it. Removing the orphaned field (lines 20-23) avoids confusion.
| TextTransform defaultTextTransform = TextTransform.Default) | ||
| => formattedString.ToNSAttributedString(fontManager, defaultLineHeight, defaultHorizontalAlignment, defaultFont, defaultColor, defaultTextTransform, LineBreakMode.WordWrap, defaultCharacterSpacing: 0d); | ||
| TextTransform defaultTextTransform = TextTransform.Default, | ||
| LineBreakMode defaultLineBreakMode = LineBreakMode.WordWrap) |
There was a problem hiding this comment.
Adding the optional defaultLineBreakMode parameter changes the signatures of the public ToNSAttributedString(this FormattedString, ...) and (this Span, ...) extension methods. You've correctly recorded this in PublicAPI.Unshipped.txt via *REMOVED* + the new signature, so the API gate is satisfied — but note that adding an optional parameter is a binary-breaking change: already-compiled external consumers bound to the old metadata signature will fail to resolve. If preserving binary compat for these public Platform extensions matters, consider keeping the existing overload and adding a new one that takes defaultLineBreakMode. If the break is intentional (internal-use platform helper), this is fine as-is — flagging for an explicit decision.
…ing ToPlatform and subsequent property changes Co-Authored-By: Tamilarasan-Paranthaman <93904422+Tamilarasan-Paranthaman@users.noreply.github.com> Co-Authored-By: albyrock87 <albyrock87@gmail.com> Co-Authored-By: Copilot <223556219+Copilot@users.noreply.github.com>
devanathan-vaithiyanathan
force-pushed
the
net11-label-perf-imp
branch
from
aedd137 to
97f0354
Compare
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
PR #35892 - [Net11] Improve Label mapping performance + complete coverage
Re-review at HEAD 0c2e170 (prior review: NEEDS_DISCUSSION at aedd137). Consensus verdict from all 4 models: NEEDS_CHANGES (high confidence).
What changed
This PR continues the iOS Label/Entry/Button mapping-performance work from #30864. It introduces a high-priority TextMapper so Text is mapped before dependent text-style properties, guards MapText/MapFormatting with IsConnectingHandler() to avoid rebuilding the iOS AttributedText repeatedly during handler connection, routes EntryHandler.iOS.MapFormatting through handler.UpdateValue(...), fixes span-property propagation in FormattedStringExtensions, and re-saves a large set of Android/iOS/Mac/Windows FeatureMatrix snapshots (587 additions across 104 files).
Consensus verdict: NEEDS_CHANGES
The latest push introduced a build-breaking regression. EntryHandler.iOS.cs now declares MapBackground(IEntryHandler, IEntry) twice with an identical signature and body (lines 56-78 and 80-102) - a CS0111 "member already defined" error. The file compiles for both iOS and MacCatalyst, which directly explains the red Build macOS (Debug/Release) and Pack macOS legs in CI; the duplicate did not exist at the prior head aedd137. All four models flagged this independently, and it is the single blocking issue: remove one of the two copies and the build should recover.
Prior concerns (still unaddressed, not re-filed inline)
Two non-blocking items from the prior NEEDS_DISCUSSION review remain, but are already documented in the PR discussion, so they are not re-filed as fresh inline comments: (1) EntryPriorityMapper (EntryHandler.cs:20-23) is orphaned dead code - it is no longer passed to the Mapper constructor (which chains TextMapper -> ViewMapper) and its only entry, MaxLength, is already present in TextMapper; and (2) the comment at EntryHandler.cs:30-31 states Text is mapped before HorizontalTextAlignment, yet HorizontalTextAlignment/VerticalTextAlignment (lines 28-29) are declared before Text (line 32) - harmless for iOS UITextField (alignment is view-level) but misleading in an ordering-focused PR. The core mapper consolidation itself was independently vetted as correct: no mappings were dropped and runtime (non-connect) behavior is preserved.
| } | ||
| } | ||
|
|
||
| public static void MapBackground(IEntryHandler handler, IEntry entry) |
There was a problem hiding this comment.
MapBackground(IEntryHandler, IEntry) is declared twice in this file with an identical signature and body (lines 56-78 and 80-102) - a CS0111 "member already defined" compile error that breaks every iOS/MacCatalyst build and matches the failing Build macOS (Debug/Release) and Pack macOS legs in CI. The base branch already defined this method; the latest push re-added it. Remove one of the two copies.
4 participants
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
The iOS Label performance was improved in PR Improve iOS Label performance #30864. In that PR, the Label and Entry Feature Matrix test sample and script were modified, which caused discrepancies in the expected images due to changes which is due to test sample's default property values. In this PR, I updated the test sample and re-saved the images accordingly.
Windows - The Entry is now unfocused, so I re-saved the latest image.
Android - I modified the test sample by altering the default values and re-saved two images.
Additionally, while working on the test sample changes, I identified and fixed issues in FormattedStringExtensions.
These updates improve formatted text rendering on iOS by correctly propagating span properties (font, character spacing, and line-break settings) from the label to each span. The layout logic is also more robust, falling back to MAUI’s calculated size when iOS has not yet provided a valid label size, preventing incorrect text positioning and rendering issues.
Issues Fixed
Contributing to #30864