LT-22802: Apply a tool's slice filter list in the Avalonia detail view - #1146
Conversation
Raised by Jason on #1135, after the merge. MoStemMsa.IsFieldRelevant withholds FromPartsOfSpeech ("Attaches to Categories") unless the owning entry has a proclitic or enclitic, and Morphology.fwlayout:39 declares that part visibility="always" -- so before the relevance gate the row composed on every stem MSA, and now it disappears for every entry without a clitic. That is the gate's most visible consequence and nothing pinned it. The test is Jason's, run and confirmed to bite: suppressing the gate fails its negative half with "Expected: False, But was: True", while the positive control -- add a proclitic allomorph, change nothing else -- keeps passing. Also corrects two comments that claimed more than the code does. The composer implements the SECOND of SliceFilter.IncludeSlice's two gates; the first looks the slice id up in the tool's filter list, and the id never reaches the composer, so that half is LT-22802. The fixture summary said it covered the remainder of the overrides, which was untrue while this test was missing, and still excludes the InflectionClass limb that withholds the row from a compound rule's left/right MSA. xWorksTests filter Avalonia 1642 passed, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Legacy SliceFilter.IncludeSlice has two gates: look the slice's authored id up in the filter list the tool's filterPath names and withhold the row when it is listed, then ask IsFieldRelevant. LT-22672 added the second. This adds the first. The id was discarded at import, so three pieces: - XmlLayoutImporter carries the slice's id onto ViewNode.SliceId, and id leaves the unhandled-attribute report. - DetailComposer.Walk withholds a node whose SliceId the tool lists, checked before the node kind is dispatched so a withheld node takes its subtree with it -- where legacy checks it, at the top of ProcessSubpartNode. - RecordEditView reads filterPath from its own configuration and parses the same file into the id set, memoized per view. A tool with no filterPath, or a file that cannot be read, filters nothing: an extra row beats a view that will not open. NOTHING VISIBLE CHANGES TODAY, and the ticket's symptom section overstates it. The only shipped filter entries that still resolve to a real slice are the five CmPossibility ids in basicPlusFilter.xml, and the only tool they reach is Exception "Features" (ProdRestrictEdit) -- which is not in the Avalonia tool catalog, so it renders legacy even under UIMode=New. The other 27 ids across the six filter files name nodes that no longer exist. Category Edit is in the catalog and carries the same filterPath, but its layout reaches none of those ids. So this is parity landed before it is needed: the day ProdRestrictEdit joins LexiconFeatureCatalog, the view already withholds what legacy withholds instead of growing five rows nobody looks for. Tested at the mechanism, not the wiring. Suppressing the composer gate fails the filter tests with the full row list -- Name, Abbreviation, Description, Status, Discussion, Confidence, Researchers, Restrictions -- and the importer tests pin the id. Reading filterPath in RecordEditView has NO test and cannot be checked in the app while the tool renders legacy; it becomes verifiable, by Jason's acceptance steps, when that tool is activated. FwAvaloniaTests 751 passed, xWorksTests filter Avalonia 1645 passed, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The filter-list read was the one part with no test, and the part that could quietly make the whole feature a no-op: every other test in this area supplies the id set by hand, so a broken read leaves them all green. Demonstrated rather than assumed. Pointing the XPath at the wrong element name fails only the new test; the three composer tests still pass. The read moves to an internal static taking the tool's configuration node, so it is reachable without a mediator or property table -- the same way the reference-vector menu entry points are tested. The property keeps only its memoization. One test drives the whole chain against a shipped filter file: the attribute name, the path resolution, the XPath and the id attribute. It asserts the ids are PRESENT rather than exhaustive, so editing that file leaves it alone while breaking any link in the chain does not. xWorksTests filter Avalonia 1650 passed, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1146 +/- ##
==========================================
+ Coverage 39.02% 39.03% +0.01%
==========================================
Files 1522 1522
Lines 352989 353035 +46
Branches 40738 40744 +6
==========================================
+ Hits 137737 137801 +64
+ Misses 185936 185917 -19
- Partials 29316 29317 +1
🚀 New features to boost your workflow:
|
…-lists ViewNode gained an optional parameter on each side -- sliceId here, and helpTopicId on main -- with the same conflict at its one call site in the importer. Both are kept; they are independent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…-lists Main now has #1145, which this branch also carried as its first commit. Squash-merging gave it a different identity, so both files it touched conflicted with that commit's own later edits there. Resolved toward main, whose comments had been corrected, with one change: the relevance doc named LT-22802 as covering the other gate. This branch is that gate, so the doc now points at IsFilteredOutByTool, in the same class. The fixture summary keeps its pointer to DetailSliceFilterTests, where that gate is tested. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The comment-hygiene wrapper broke the new pointer to IsFilteredOutByTool so that one word sat alone on the last line. Reflowed. Comments only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Two things from a review pass. The first is blocking; the second is a gap I think is worth closing before this merges, because the feature is inert today and this test is the only thing that will catch its removal later. 1.
|
| arg | value passed | parameter it now binds to |
|---|---|---|
| 36 | n.HelpTopicId |
sliceId |
| 37 | (omitted) | helpTopicId = null |
Both parameters are string, so this compiles without a warning. Before this PR the same 36 arguments were correct, because helpTopicId was the last parameter.
The two call sites are ViewDefinitionOverrideApplier.cs:306 and ViewDefinitionOverrideApplier.cs:318. The comment directly above the first one is the warning this tripped:
Every trailing optional constructor argument must be passed, or that field is stripped.
Effect. RebuildNode calls CloneWith for every node it visits, and Apply runs whenever the composer finds a non-empty override patch for that class and layout. So in any project where someone has used Field Visibility or Move Field, every row in that layout loses its authored help topic and ResolveHelpTopic falls back to a generated candidate or khtpNoHelpTopic. Fifty-five parts author an explicit helpTopicID, eleven of them in the entry and sense layouts the lexicon tool composes. The bogus SliceId the rows pick up is harmless today, since no shipped filter entry looks like a khtpField- id.
Suggested fix. Move sliceId to the end of the parameter list, or switch both clone helpers to named arguments for the tail. Named arguments are the more durable of the two, since the next parameter added will land in the same trap.
I checked every other new ViewNode( call site the same way. The rest either stop well before the tail or name their trailing arguments, including ViewDefinitionJsonSerializer.ReadNode, so these two are the only affected calls.
A test asserting that a node's HelpTopicId survives Apply would have caught this, and there is currently no such test in ViewDefinitionOverrideApplierTests.
2. No test covers the step that joins the read to the composer
The PR makes the case that the reading test exists so the feature cannot be a silent no-op. That argument holds for the read and for the gate, but not for the wiring between them.
Delete hiddenSliceIds: HiddenSliceIds from both DetailComposer.Compose calls in RecordEditView.Avalonia.cs and the whole suite stays green. SliceFilterListReadingTests proves the file parses; DetailSliceFilterTests supplies the id set by hand. Nothing asserts that what the first produces is what the second receives.
That matters more here than it usually would, because no shipped tool reaches a live filter id today. There is no failing acceptance test waiting to catch it, so the day ProdRestrictEdit joins the catalog is the day anyone would notice.
Something narrow would do it, for example asserting that a RecordEditView configured with the shipped basicPlusFilter.xml composes a CmPossibility without the Status row, going through the property rather than a hand-built set.
sliceId was inserted ahead of helpTopicId in ViewNode's constructor, when resolving a merge conflict between the two. Both override clone helpers pass that tail positionally, and both parameters are strings, so n.HelpTopicId bound to sliceId and helpTopicId fell back to null -- with no warning. Any layout with a Field Visibility or Move Field override lost every authored help topic, and each of its rows took the help id as its slice id. The helpers also never carried SliceId at all. Moving the parameter back alone would have restored help topics and left SliceId null after every override, so a tool's filter list would stop withholding rows in exactly the layouts users have customised. sliceId goes last again, and both helpers name the tail and pass both fields, so a parameter added ahead of them cannot shift either one. The existing test for fields an override must preserve now covers both, and a duplicated node is checked for them too. Rebuilding the merged code fails both tests exactly as reported: help topic null, slice id the help id. Dropping only the SliceId carry fails them on SliceId. Reported in review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The serializer wrote and read helpTopicID but had no slice id, so a layout loaded from canonical JSON would lose the id a tool's filter list names. No shipped path loads JSON today; this keeps that path from springing the same trap if it is enabled. Written as "sliceId", not "id": "id" is already the stable id's key, and sharing it would overwrite every node's stable id on the way out. The round-trip test that sets every property now sets helpTopicId and sliceId, which it had not been covering, and checks the two ids stay apart. Dropping the write fails it on SliceId. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The file reader and the composer's gate were each tested, but not the join between them: deleting the view's hiddenSliceIds argument left the whole suite green. No shipped tool reaches a live filter id today, so nothing else would have noticed. The view's composition moves out of ShowAvaloniaEntry into ComposeDetail, unchanged, so a test can reach it without the settle, adapter and message work around it. The test installs the shipped ProdRestrictEdit configuration, whose filter list is basicPlusFilter.xml, and composes a production restriction through the view. The same view with the filterPath removed is the control, and shows the Status row. Removing the argument fails the test, with Status among the composed rows. Reported in review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mark-sil
left a comment
mark-sil
left a comment
There was a problem hiding this comment.
@mark-sil reviewed 13 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on thejambi).
Extracting ComposeDetail left it under ShowAvaloniaEntry's summary as well as its own, and ShowAvaloniaEntry with none. That summary is back on its method. The memoized filter list caches a failed read too, which reads like an oversight; the comment now says it is deliberate, since a missing filter file is an install fault and re-reading it would log the same failure for every record shown. The reading tests' summary argued for their own existence, and its claim that every other test supplies the id set by hand stopped being true with the wiring test. It now says what the fixture reads. Reported in review. Comments only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3 participants
Teaches the Avalonia detail view to read a tool's slice filter list. Some tools attach a
small file naming fields to leave out. The WinForms view reads it; the Avalonia view was
never taught to.
Nothing visible changes today
The ticket's symptom section overstates this, so it is worth being plain: no user can
see a difference from this PR.
CmPossibilityids inbasicPlusFilter.xml.ProdRestrictEdit), which is notin
LexiconFeatureCatalog-- so it renders WinForms even under New UI.filterPath, but itsPartOfSpeechlayout reaches none of those ids.define.
So this is parity landed before it is needed: the day
ProdRestrictEditjoins thecatalog -- a one-line change -- the view already withholds what the WinForms view
withholds, instead of growing five rows nobody goes looking for.
What changed
SliceFilter.IncludeSliceapplies two gates: look the slice's authoredidup in thetool's filter list and withhold the row when listed, then ask
IsFieldRelevant. #1135added the second. This adds the first, which needed the
idto survive import.XmlLayoutImporteridontoViewNode.SliceId, andidleaves the unhandled-attribute reportDetailComposer.WalkSliceIdthe tool lists, checked before the node kind is dispatched so a withheld node takes its subtree with itRecordEditViewfilterPathfrom its own configuration and parses the same file, memoized per viewA tool with no
filterPath, or a file that cannot be read, filters nothing: a detail viewshowing an extra row beats one that will not open.
Tested at both ends
Both halves are falsified, not merely green:
Abbreviation, Description, Status, Discussion, Confidence, Researchers, Restrictions.
tests stay green, because they supply the id set by hand. That is exactly why the
reading test exists: without it the whole feature could be a silent no-op and still look
green.
The read is an
internal statictaking the tool's configuration node, so it is reachablewithout a mediator or property table -- the same approach #1133 used for the
reference-vector menu entry points. Its main test drives the whole chain against a shipped
filter file (attribute name, path resolution, XPath,
idattribute) and asserts the idsare present rather than exhaustive, so editing that file leaves the test alone while
breaking the chain does not. The property's memoization is the only untested line.
Verification
-TestFilter Avalonia: 1650 passed, 2 skipped, 0 failedbuild.ps1 -CommentHygiene -TokenHygienecleanNot manually verified, and cannot be: the ticket's acceptance steps need a tool that
renders in Avalonia. They become runnable when
ProdRestrictEditis activated.The
LexiconFirstSliceEditContextEdgeCaseTeststeardown error in the xWorks run ispre-existing and unrelated --
git blameputs it in LT-22625 (#964), and nothing heretouches that file.
Ticket corrections
Three things on LT-22802 need fixing, all found while building this:
ProdRestrictis the internal clerk name.changes.
🤖 Generated with Claude Code
This change is