Read appearance rules written on properties, not only on classes - #21
Conversation
`AppearanceAttribute` is usable on a class, a property, a method or an interface, and "Declare Conditional Appearance Rules in Code" teaches the property form first: a rule on `UnitPrice` and a rule on the class naming `TargetItems = "UnitPrice"` are two spellings of one rule. `ExtractAppearanceRules` read only `classDecl.AttributeLists`, so the property spelling produced nothing. `ExtractValidationRules`, immediately above it, had walked the properties as well as the class from the beginning. Every `[Appearance]` in every fixture happened to be class-level, so the suite agreed the class-only walk was enough -- the same shape as the `CustomMessageTemplate` blind spot, where a form no fixture used could not be seen to be missing. A property rule that does not name its own `TargetItems` now records the property it was written on, which is what the equivalent class-level spelling states outright; the two forms extract alike. An explicit `TargetItems` is left untouched, since overwriting it would silently narrow a rule naming several targets. Measured: WLNCentral 25 -> 33 rules, and a generated XAF app whose only appearance rules were property-level went 0 -> 2. 323 tests.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Reading rules off properties made an existing gap reachable. The fold that
carries a base class's rules down to its descendants keys appearance rules on
`rule.Id` alone, and `FoldInto` skips a key it has already seen -- so two rules
with no id looked like one rule and the second was dropped in silence.
An empty id is ordinary rather than an omission: a rule written on a property
already says what it governs, and the DevExpress non-persistent-objects demo
writes `[Appearance("", Enabled = false, TargetItems = "*")]`. Before rules were
read off properties this was hard to reach, since one unnamed rule per class is
the most anyone writes.
`ValidationRuleKey` already guards the same way -- it falls back to the attribute
and the property when a rule has no id. `AppearanceRuleKey` now mirrors it,
falling back to the targets, which for a property rule is the property.
It inherits that fallback's documented limitation unchanged: two unnamed rules
over the same targets still cannot be told apart.
`AuditedObject` gains the two unnamed rules that reproduce it, so the fixtures
now cover the form as well as the class-level one. Its folded totals are pinned
alongside the declared ones they exist to contrast with.
326 tests. WLNCentral stays at 33 rules and the generated app at 2 -- both name
their rules, so this changes nothing for them.
|
Reviewing this against itself before you spend time on it, since the last few rounds each turned up something the tests agreed with. It had a bug, now fixed in c06f27f. Reading rules off properties made an existing gap reachable rather than introducing one. An empty id is ordinary rather than an omission. A rule on a property already says what it governs, and your own reference points at the non-persistent-objects demo writing
Three things I checked and left alone, in case you disagree with any:
326 tests, no warnings. WLNCentral stays at 33 rules and the generated app at 2 — both name their rules, so the fold fix changes nothing for either; it is purely defensive. |
Both new loops existed only to skip what `ReadAppearanceRule` returned nothing for, which CodeQL reads as a filter written in the wrong place. Saying it with `Select` and `OfType` states the same thing directly, and matches how `AppearanceAttributesOf` above already selects and filters. No behaviour change; 326 tests.
|
Merged. Thank you — this is the second time you have found a form the whole suite agreed did not exist, and the reasoning in the description is why I could review it quickly rather than in spite of it.
|
| Element to be affected | Attribute Target | AppearanceItemType | TargetItems |
|---|---|---|---|
| Business class property — Approach 1 | Property | ViewItem | (blank) |
| Business class property — Approach 2 | Class | ViewItem | The required property's name |
The blank in Approach 1 is not missing information: it means "the property this is written on". Filling it in states what XAF itself resolves at run time. A separate TargetProperty would make the two approaches extract differently again — just in a new field — which is precisely what this PR exists to remove. No change wanted.
What your change made reachable
I probed the generated documents rather than the extraction result, and found three things. Before naming them: none is a regression from your PR. I checked, because it would have changed whether I merged. All three are on main today.
1. An unnamed rule renders with no name and no condition
Your two new fixture rules produce this in the Markdown, verbatim:
- **** — when ``: enabled=false (fields: ChangedBy) — inherited from `AuditedObject`
- **** — when ``: visibility=Hide (fields: AuditNotes) — inherited from `AuditedObject`
**** from the empty id, and when from a rule that has no criteria. The second half matters more than it looks: in XAF a rule with no criteria is always active, which is a stronger claim than a conditional one, and the page renders it as though the condition failed to load.
To confirm it predates you, I ran main — without your branch — against the class-level spelling the DevExpress non-persistent-objects demo uses:
[Appearance("", Enabled = false, TargetItems = "*")]
public class Article : BaseObjectmain, no PR: - **** — when ``: enabled=false (fields: *)
So this is ours and it is old. What your change does is make it common: rules on properties are the first form the documentation teaches, and a rule on a property is exactly the one an author leaves unnamed.
The fix is already half-written in this repo — HtmlExplainerGenerator.cs:654 says E(rule.Criteria ?? "always"). The HTML author had already decided the right word. MarkdownDocumentationGenerator.cs:544 and :696, and XafDetailTools.cs:802, never got it.
2. The diff cannot see an unnamed appearance rule appear or disappear
ProjectDiffEngine.cs:132-133 keys appearance rules on r.Id alone and collects them into a HashSet<string> — the same collapse you just fixed in FoldInto, one file over, and reachable now for the same reason.
Probed by removing one of the two unnamed rules and comparing the snapshots:
declared appearance rules: before=2 after=3
appearance-rule changes reported by the diff: 0
A rule was added to the application and the diff said nothing. AppearanceRuleKey is the fix; it just needs to be used in both places.
3. Criteria passed by position is dropped
This one is a genuinely separate defect, so it is #22 rather than a paragraph in a merged thread.
AppearanceAttribute has three constructors, and two of them take the criteria positionally:
AppearanceAttribute(string id)
AppearanceAttribute(string id, string criteria)
AppearanceAttribute(string id, AppearanceItemType appearanceItemType, string criteria)
ReadAppearanceRule reads Criteria only when it is named. Probed with a throwaway fixture:
[Appearance("Ticket_ClosedIsGrey", "Status = 'Closed'", FontColor = "Gray")]extracted: id=[Ticket_ClosedIsGrey] targets=[] criteria=[]
And the criteria index — the page that introduces itself as "every distinct expression in this application" — does not list Status = 'Closed'.
It is the same shape as the CustomMessageTemplate blind spot you cite in your description, one argument over: every fixture writes Criteria = named, so the suite agrees. ApplyPositionalArguments in the same file is the model for the fix.
Who takes what
1 and 2 are mine. They are our renderers and our diff, they predate your first contribution, and I do not want **** sitting in released output while it waits for someone else's time.
3 is yours if you want it — #22. It is the substantial one, it has a working model in the same file, and nothing blocks on it. If you would rather not, say so and I will pick it up; you have found two of these now and being handed the cleanup for it is the wrong reward.
4 participants
AppearanceAttributeis declaredand Declare Conditional Appearance Rules in Code lists applying it to a property as Approach 1, and applying it to the class with the property named in
TargetItemsas Approach 2 — two spellings of one rule.ExtractAppearanceRulesread onlyclassDecl.AttributeLists, so the property spelling produced nothing at all.ExtractValidationRules, immediately above it, has walked the properties as well as the class from the beginning — this brings the two into line.Why the suite agreed with it
Every
[Appearance]in every fixture happened to be class-level. So the tests confirmed the class-only walk was sufficient, and a form that no fixture used could not be seen to be missing. That is the same shape as theCustomMessageTemplateblind spot you found while closing #14, where all 299 tests agreed the last positional literal was the criteria because no fixture passed a message by position.It matters more here than a miscount would: an entity's section is presented as its complete inventory, so a rule that governs a property and is reported nowhere leads the reader to conclude the property is unconditionally editable.
The
TargetItemsdecisionA property rule that does not name its own
TargetItemsnow records the property it was written on — which is exactly what the equivalent class-level spelling states outright, so the two approaches extract alike rather than documenting differently depending on which the author happened to choose.An explicit
TargetItemsis left untouched. Overwriting it would silently narrow a rule that names several targets, soStockBatch'sTargetItems = "*"is pinned by a test.If you would rather the property name landed somewhere other than
TargetItems— a distinctTargetPropertyonExtractedAppearanceRule, mirroringExtractedValidationRule— say so and I will move it. I pickedTargetItemsbecause it needs no model or renderer change and reads identically to Approach 2, but the model is your call.Measured
Tests
PropertyLevelAppearanceRuleTests— the rule is found, keeps its criteria, targets the property it was written on, does not displace the class-level rule on the same entity, and does not overwrite an explicitTargetItems.Productin the demo fixture gains one property-level rule, soTheDemoApplicationKeepsItsShapemoves 9 → 10 deliberately, and the README test count moves 318 → 323.323 tests, no warnings.