Skip to content

LT-22691: Preserve trailing ViewNode fields when applying an override - #1096

Merged
mark-sil merged 2 commits into
mainfrom
LT-22691c
Aug 20, 2026
Merged

LT-22691: Preserve trailing ViewNode fields when applying an override#1096
mark-sil merged 2 commits into
mainfrom
LT-22691c

Conversation

@mark-sil

@mark-sil mark-sil commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

ViewDefinitionOverrideApplier rebuilds every node when it applies a patch, but its CloneWith and CloneWithId helpers stopped short of the ViewNode constructor's last three optional parameters. Omitting them let the defaults win, so EnumStringList, VisibleWritingSystems and ToggleValue were stripped from the whole tree as soon as a project carried any override -- a single SetVisibility operation was enough.


This change is Reviewable

ViewDefinitionOverrideApplier rebuilds every node when it applies a patch,
but its CloneWith and CloneWithId helpers stopped short of the ViewNode
constructor's last three optional parameters. Omitting them let the
defaults win, so EnumStringList, VisibleWritingSystems and ToggleValue
were stripped from the whole tree as soon as a project carried any
override -- a single SetVisibility operation was enough.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown

NUnit Tests

    1 files  ±0      1 suites  ±0   9m 21s ⏱️ -27s
5 835 tests +2  5 754 ✅ +2  81 💤 ±0  0 ❌ ±0 
5 844 runs  +2  5 763 ✅ +2  81 💤 ±0  0 ❌ ±0 

Results for commit 51da2bb. ± Comparison against base commit 70076f6.

♻️ This comment has been updated with latest results.

@codecov-commenter

codecov-commenter commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 38.33%. Comparing base (70076f6) to head (51da2bb).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1096      +/-   ##
==========================================
+ Coverage   38.31%   38.33%   +0.01%     
==========================================
  Files        1507     1507              
  Lines      350524   350554      +30     
  Branches    40288    40291       +3     
==========================================
+ Hits       134310   134385      +75     
+ Misses     186983   186939      -44     
+ Partials    29231    29230       -1     
Files with missing lines Coverage Δ
...ia/ViewDefinition/ViewDefinitionOverrideApplier.cs 93.30% <100.00%> (ø)

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jasonleenaylor

Copy link
Copy Markdown
Contributor

Src/Common/FwAvalonia/FwAvaloniaTests/ViewDefinitionOverrideApplierTests.cs line 46 at r1 (raw file):

		// Every node is rebuilt on apply, so a clone that omits a field strips it tree-wide once
		// any override exists. These three fields are outside ToSnapshot(), so the empty-patch
		// test misses it.

and aren't covered by the EmptyPatch test

Code quote:

		// any override exists. These three fields are outside ToSnapshot(), so the empty-patch
		// test misses it.

@mark-sil mark-sil left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mark-sil made 1 comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on jasonleenaylor).


Src/Common/FwAvalonia/FwAvaloniaTests/ViewDefinitionOverrideApplierTests.cs line 46 at r1 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

and aren't covered by the EmptyPatch test

I change the comment.

@jasonleenaylor jasonleenaylor 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.

:lgtm:

@jasonleenaylor reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on mark-sil).

@mark-sil
mark-sil merged commit 717d865 into main Aug 20, 2026
9 checks passed
@mark-sil
mark-sil deleted the LT-22691c branch August 20, 2026 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants