Skip to content

PropertyTable: a stored default no longer broadcasts - #1213

Open
mark-sil wants to merge 1 commit into
mainfrom
claude/pubsub-silent-defaults-aa1873
Open

mark-sil wants to merge 1 commit into
mainfrom
claude/pubsub-silent-defaults-aa1873

Conversation

@mark-sil

@mark-sil mark-sil commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Start here: Src/XCore/xCoreInterfaces/PropertyTable.cs, GetValueInternal and the two SetDefault overloads: the whole behaviour change is those three methods; every other file only drops an argument.

A read with a default (GetBoolProperty, GetStringProperty, GetIntProperty, GetValue<T>(name, default)) and SetDefault still create the key when it is absent, but no longer queue OnPropertyChanged for it. SetDefault loses its doBroadcastIfChanged parameter. Until now the first reader of an unset property was a hidden setter that broadcast its placeholder (null, "", "????") before the real value was set.

The unknown is "which handler stops being called?". None that could use the call. A handler receives only the name and reads the table when the queued item drains, after the code that read the default has set the real value, so the read's item either repeated the set's own broadcast or carried a placeholder. The review question is therefore the per-name list below, not the engine line.

Where to look:

  • Startup view build. The first view used to be built on the null currentContentControl item from the primed menus; it is now built on AreaListener's own set, queued right behind it with the same currentContentControlParameters node (xWindow.OnPropertyChanged). Pinned by the fresh-launch probes.
  • SetDefault flag removal. The flag never reached a colleague: the window-level config defaults load before any colleague registers, the tool-level ones find their keys already set, and the four code callers passed false.
  • Keys must still be created. DataTree.cs:1073 calls SetPropertyPersistence on a key its read just created; SetPropertyPersistence throws on a missing key. The store stays, only the broadcast goes.
  • Tests that count queue items after reads (SidebarAdapterSelectionTests, RawTextPaneTests) can only get easier; both pass.

Deliberately not here: the choice groups' "????" sentinel reads (ChoiceGroup.SinglePropertyValue) still store that placeholder; harmless, left alone. The read-then-set sites (EditingHelper, GetCurrentConfiguration) keep their real-value broadcast and convert as ordinary rows later.

Verification: build with both hygiene gates; xCoreInterfacesTests 20 passed including the new colleague-spy test GetWithDefaultAndSetDefault_StoreWithoutBroadcast; the two filtered runs above; two fresh-launch probes with persisted keys identical to the baseline build; six manual checks on Sena 3 (details below). Not run: the full suite, left to CI.

Next: approve, or name a handler you believe depended on the placeholder broadcast.


Reading this a year from now -- start here

This PR is one step of converting PropertyTable.BroadcastPropertyChange (Mediator OnPropertyChanged) to the FwUtils Pub/Sub system. The enumeration that justified it (303 default-carrying read sites, 111 names, which 12 reached a live handler and when) lives in the gitignored working inventory Docs/migration/working/propertytable-broadcast-buckets.md, section 0, and its per-site table beside it. Nothing was deleted from the tree for this PR.

Decisions, and why
  • Stop the broadcast, keep the store. The alternative, returning the default without creating the key, breaks callers that act on the key immediately (SetPropertyPersistence throws on a missing key) and changes BestSettings local-versus-global resolution and what gets persisted.
  • Remove the SetDefault flag instead of passing false. No caller benefited from true, and a parameter that promises a broadcast which cannot happen misleads the next reader. The method comments say what the method does and do not mention broadcasting, since the Mediator broadcast is on its way out.
  • Leave the "????" sentinel. It is a config default (ChoiceGroup.cs, defaultPropertyValue absent) that the first selection change replaces; after this PR it stores silently and nothing reads it for display.
Evidence

Names whose first read used to reach a live handler, with what the placeholder was and who heard it:

  • currentContentControl, areaChoice: null at window build, every launch (persistence off in AreaListener); xWindow/FwXWindow, AreaListener, NavBarAdapter.
  • BestStyleName: "????" from the Styles combo at the first active idle; xWindow, every SimpleRootSite.
  • InterlinearTab: "" from the Parser menu's InInterlinearText at window build; InterlinMaster.
  • ShowHiddenFields-<tool>, ShowFailingItems-lexiconClassifiedDictionary, ShowInvisibleSpaces, ClickInvisibleSpace: pane-bar and menu items at window build or tool entry; DataTree, RecordEditView, PaneBar, XhtmlDocView, RawTextPane.
  • DictionaryPublicationLayout, ReversalIndexPublicationLayout: "" in GetCurrentConfiguration before it sets the path; XhtmlDocView, ReversalClerk.
  • currentFilterForRecordClerk_<id>, ReversalIndexGuid: the clerk's own handler, ExportDialog's export-time read.

Fresh-launch probes (project test222222, its ConfigurationSettings set aside, window activated so idle work runs, CloseMainWindow): straight into lexiconEdit and straight into lexiconClassifiedDictionary, on the baseline build and on this branch. Persisted keys identical in names and values (48 and 40 keys). No error dialog.

Manual checks on Sena 3, all passed:

  1. Fresh settings, Lexicon Edit opens with the entry list and detail view populated.
  2. Styles combo enabled with the field's style after clicking into an entry field.
  3. View > Invisible Spaces and Insert > Click Inserts Invisible Space toggle in the text pane.
  4. View > Show Hidden Fields shows hidden fields; pane-bar checkbox follows.
  5. Fresh settings, Interlinear Texts opens on Baseline; Parser > Parse Words in Text enabled.
  6. Classified Dictionary renders; Show Unused Items re-renders with unused items.
Preflight review details

Code Review Summary

Branch: claude/pubsub-silent-defaults-aa1873
Base: main (merge base 5cbbfe8)
Date: 2026-10-28
Review model: Claude Fable 5.1
Files changed: 6

Contract/API Changes

  • PropertyTable.SetDefault(string, object, bool) -> SetDefault(string, object).
  • PropertyTable.SetDefault(string, object, SettingsGroup, bool) -> SetDefault(string, object, SettingsGroup).
  • A read with a default never queues OnPropertyChanged; it still creates the key.
  • All callers are in Src and updated: xWindow (4), DataTree, BrowseViewer (2), RecordClerk, PropertyTableTests (21).

Findings

Critical: none. Important: none.

Minor, all fixed during review: dead SetDefaultInternal call in GetValueInternal's null-value branch removed; the new test now covers the null-valued-key branch; bounded drain loop; ProcessMessages asserted before the negative assertion; using directives instead of fully qualified names; BestSettings overload summary reworded.

Required Validation / Evidence

  • .\build.ps1 -CommentHygiene -TokenHygiene: clean.
  • .\test.ps1 -TestProject Src/XCore/xCoreInterfaces/xCoreInterfacesTests: 20 passed.
  • .\test.ps1 -TestProject Src/XCore/xCoreTests -TestFilter SidebarAdapterSelectionTests: 4 passed; -TestProject Src/LexText/Interlinear/ITextDllTests -TestFilter RawTextPaneTests: 2 passed.
  • Fresh-launch probes and manual checks as in Evidence above.
  • Not run: the full test suite.

Interview Notes

Nothing further flagged by the author; PR title equals the commit title. Developer-only change, no Jira ticket.

Suggested Review Focus

  • PropertyTable.GetValueInternal: the null-value branch now stores directly with no further call.
  • Any out-of-tree caller of SetDefault with the old four-argument shape.

🤖 Generated with Claude Code


This change is Reviewable

A Get*Property or GetValue read with a default, and SetDefault, now
store the value for an absent key without queueing OnPropertyChanged,
and SetDefault has no broadcast flag. Until now the first reader of an
unset property was a hidden setter: it created the key and queued a
broadcast of the default, so a handler could run on a placeholder
before the real value was set.

Dropping that broadcast loses nothing a handler could use. A handler
receives only the name and reads the table when the queued item drains,
which is after the code that read the default has set the real value;
so the read's item either repeated the set's own broadcast with the
same value, or delivered a placeholder nobody wanted (null, "" or
"????") when no set followed. At startup the first view was built on
the null currentContentControl item from the primed menus; it is now
built on AreaListener's own set, which queues right behind it with the
same parameters node.

The names whose first read used to reach a live handler, now stored
silently:
* currentContentControl and areaChoice (null, at window build, every
  launch, because their persistence is switched off)
* BestStyleName ("????" from the Styles combo at the first idle)
* InterlinearTab ("" from the Parser menu at window build)
* ShowHiddenFields-<tool> (one per tool), ShowFailingItems-
  lexiconClassifiedDictionary, ShowInvisibleSpaces and
  ClickInvisibleSpace (pane-bar and menu items)
* DictionaryPublicationLayout and ReversalIndexPublicationLayout (""
  before GetCurrentConfiguration sets the path)
* currentFilterForRecordClerk_<id> and ReversalIndexGuid

The SetDefault flag never reached a colleague: the window-level config
defaults load before any colleague registers, the tool-level ones find
their keys already set, and every code caller passed false.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

NUnit Tests

    1 files  ±0      1 suites  ±0   11m 19s ⏱️ - 2m 12s
6 906 tests +1  6 821 ✅ +1  85 💤 ±0  0 ❌ ±0 
6 915 runs  +1  6 830 ✅ +1  85 💤 ±0  0 ❌ ±0 

Results for commit 28ac733. ± Comparison against base commit 10f08c9.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.29%. Comparing base (5cbbfe8) to head (28ac733).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
Src/XCore/xWindow.cs 75.00% 1 Missing ⚠️
Src/xWorks/RecordClerk.cs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1213      +/-   ##
==========================================
+ Coverage   40.09%   40.29%   +0.19%     
==========================================
  Files        1544     1547       +3     
  Lines      355309   355551     +242     
  Branches    41357    41446      +89     
==========================================
+ Hits       142475   143255     +780     
+ Misses     183533   182978     -555     
- Partials    29301    29318      +17     
Files with missing lines Coverage Δ
Src/Common/Controls/DetailControls/DataTree.cs 47.91% <100.00%> (+0.15%) ⬆️
Src/Common/Controls/XMLViews/BrowseViewer.cs 50.89% <100.00%> (ø)
Src/XCore/xCoreInterfaces/PropertyTable.cs 78.89% <100.00%> (-0.05%) ⬇️
Src/XCore/xWindow.cs 39.66% <75.00%> (-0.11%) ⬇️
Src/xWorks/RecordClerk.cs 44.79% <0.00%> (ø)

... and 33 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.

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.

2 participants