perf(reconciler): skip redundant automation-name write when value already matches (P3)#695
Draft
azchohfi wants to merge 5 commits into
Draft
perf(reconciler): skip redundant automation-name write when value already matches (P3)#695azchohfi wants to merge 5 commits into
azchohfi wants to merge 5 commits into
Conversation
…ed caption (P3) UpdateDefaultAutomationName ran a UIA GetName read + SetName write on every changed cell that goes through Update, even when the caption was unchanged - where the resulting Name write is a value no-op (or hits the author-override guard the GetName already protects). Add a caption-only fast-path that returns before touching the DP when the new caption is empty/whitespace or equals the old caption, and factor the remaining decision into a pure, DP-free ResolveDefaultAutomationNameUpdate helper so the caption/override policy is unit-testable headlessly. On a changed caption the original GetName + author-override + SetName path runs unchanged, so a genuine caption change still flows to UIA and author-set names are never clobbered. Scope is Reconciler.cs only (file-disjoint from the P1 PR #692). New headless tests pin the decision, including a teeth case: reverting the unchanged-caption skip makes the empty-live-Name assertion return the caption instead of null and fail. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce reconciler update overhead by avoiding unnecessary UIA AutomationProperties.Name read/write round-trips when a cell’s caption hasn’t changed, while keeping the author-override semantics intact. It also factors the decision logic into a pure helper so the policy can be verified with headless unit tests.
Changes:
- Added a caption-only fast-path to
Reconciler.UpdateDefaultAutomationNameto skip UIA DP interactions whennewCaptionis whitespace or unchanged. - Introduced
Reconciler.ResolveDefaultAutomationNameUpdate(current, oldCaption, newCaption)to make the update decision DP-free and unit-testable. - Added
ReconcilerAutomationNameTeststo pin the intended policy (including “teeth” cases).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Reactor/Core/Reconciler.cs | Adds the unchanged-caption fast-path and a pure helper to decide whether/how to update AutomationProperties.Name. |
| tests/Reactor.Tests/ReconcilerAutomationNameTests.cs | Adds headless tests covering the new helper’s decision policy and invariants around author overrides and trimming. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…guard Addresses Copilot review on #695. The first cut skipped UpdateDefaultAutomationName whenever oldCaption == newCaption. That is unsafe: ApplyModifiers runs BEFORE this (Update.cs:188-200) and can clear AutomationProperties.Name when an explicit .AutomationName() override is removed - even though the caption is unchanged. The blanket skip then left UIA Name empty instead of restoring the caption-derived default (which main does). Replace the unchanged-caption skip with an idempotent-write guard: keep main's GetName + author-override logic verbatim, compute the trimmed target, and skip only the SetName when the live Name already equals it (a value no-op). This still WRITES when the default must be (re)applied - including the cleared-Name-unchanged-caption restore case - so behavior is identical to main minus the redundant same-value write. Tests rewritten: the teeth now pin the idempotent guard (revert it -> the skip test sees a redundant 'X' write and fails) AND the restore-default case (a blanket unchanged-caption skip -> the restore test returns null and fails). Added an author-override-survives-unchanged-caption case. Full Reactor.Tests 9714 passed / 0 failed / 64 skipped; core-lib Release AOT 0W/0E. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GetName still runs for non-whitespace captions; the P3 saving is the skipped redundant SetName inside the helper. Reword the comment so it no longer implies the GetName read is removed for unchanged captions (Copilot review on #695). Comment-only; no behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
azchohfi
changed the title
pr-review (test-coverage L1) flagged that the P3 idempotent-write guard's live UIA seam was only pinned headlessly via the pure helper ResolveDefaultAutomationNameUpdate. The subtle restore-default branch — a removed .AutomationName() override clears the live Name (ApplyModifiers ClearValue) and the guard must re-apply the caption default even though the caption is unchanged — was not proven end-to-end through a real control. Extend the CoreCov_AccessibilityModifiers selftest with a third phase that drops the .AutomationName() override (caption unchanged) and asserts AutomationProperties.GetName restores the caption-derived default, plus assertions that the author override wins at mount and a changed override still flows through. Exercises UpdateDefaultAutomationName via a live WinUI control. Teeth: a blanket unchanged-caption skip (or dropping the live SetName) leaves the Name cleared at phase 2 -> A11y_Name_RestoredToCaptionDefault flips. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reword the UpdateDefaultAutomationName comment so it attributes the conditional DP access to the SetName *write*, not a vague "touch the DP" — the GetName read always runs for a non-whitespace caption; only the SetName write is skipped when the helper returns null. Comment-only; no behavior change. Addresses the Copilot review thread at Reconciler.cs:2693. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
P3 — skip the redundant per-cell automation-name write
Phase-1 finding.
Reconciler.UpdateDefaultAutomationNameruns on every changed cell that goes throughUpdate. In the steady state the caption-derived default it computes already equals the live UIA Name, so theSetNameit issues is a value no-op — pure per-cell overhead.Change
ResolveDefaultAutomationNameUpdate(current, oldCaption, newCaption)helper (headless-testable inReactor.Tests).SetNamewhen the live Name already equals it.GetNamestill runs for non-whitespace captions — only the redundant same-value write is skipped.GetNameread, exactly as main's first line does.)Correctness (MED-risk area — automation names)
The helper models main's
GetName+ author-override +SetNamelogic exactly, plus the idempotent guard:.AutomationName()override makesApplyModifiersclear the Name before this runs; with an unchanged caption the default is still re-applied (emptycurrent≠ trimmed → write). ✅ (This is the regression a naïve "skip when caption unchanged" cut would have introduced — caught in review, now pinned by a teeth test.)>100-char trim preserved. ✅Tests
ReconcilerAutomationNameTests(12 cases) pin the decision with teeth both ways:"X"write → fails.null→ fails.Scope / gate
src/Reactor/Core/Reconciler.csonly — file-disjoint from perf(reconciler): skip redundant element-modifier self-merge (P1) #692 (P1) and the rest of the perf fleet.Reactor.Tests9714 passed / 0 failed / 64 skipped./perf— that's fine data (the automation write isn't a StocksGrid bottleneck). The guard still removes provably-redundant writes for any control whose non-caption props change under an unchanged caption.🔒 DRAFT + merge-gated: does not merge until the improved
/perfcan resolve its diff and the user gives explicit GO (same rule as #692).