Skip to content

InterlinMaster's own InterlinearTab sets no longer broadcast - #1216

Open
mark-sil wants to merge 1 commit into
mainfrom
claude/pubsub-interlinear-tab-1d1843
Open

mark-sil wants to merge 1 commit into
mainfrom
claude/pubsub-interlinear-tab-1d1843

Conversation

@mark-sil

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

Copy link
Copy Markdown
Contributor

Start here: Src/LexText/Interlinear/InterlinMaster.cs, ShowRecord — the fForcedRawText flag and the direct ShowTabView() in the same-text branch. The setter change near the end of the file is one flag.

InterlinMaster's own sets of the InterlinearTab property — the tab click, the getter's normalizing fallback, and the Baseline tab forced for an empty text — no longer broadcast OnPropertyChanged. The view moves its tab control itself. The handler stays for the one sender left: a followed link that carries the tab (Go Back / Go Forward, silfw: URLs).

Does anything still need the queued ShowTabView? Two of the three sets never did: after a click the tab control already shows the clicked tab, and the fallback fires from ShowTabView's own read or from readers that run after it, so the index already matches when the broadcast drains. The forced set did one piece of work: when an empty text is shown again on an analysis tab (F5), nothing else in ShowRecord moves the tab control, and the queued handler switched it to Baseline afterwards. That call is now made directly in that branch. Same end state; the switch happens inside ShowRecord instead of at the next Mediator drain, so the analysis tab is no longer selected and focused in between.

Where to look

  • The forced-tab branch in ShowRecord (same text): the direct call follows today's SelectAnnotation, so every call today's path makes still runs, in order. Pinned by ShowRecord_EmptyTextOnAnalysisTab_MovesToRawTextWithoutBroadcast.
  • The kept handler: still reached from LinkListener.FollowActiveLink; checked manually with Go Back.
  • ShowTabView inside ShowRecord is already the case through SwitchText, so no new re-entry context.

Deliberately not here: the link sender and its broadcast (the follow-link workflow). The fallback's "" value is no longer created by reads since #1213.

Verification: build.ps1 -CommentHygiene -TokenHygiene clean; ITextDllTests InterlinMasterTabTests, InterlinMasterTests, RawTextPaneTests 8/8 (4 new; the tab-click and ShowRecord tests fail on main). Manual on Sena 3: tab clicks with the pane bar's "Add Words to Lexicon" following the tab; Go Back restoring Gloss on the same text; restart restoring Analyze; Parser menu items following the tab; an empty text on Gloss moved to Baseline by F5. Not run: the full ITextDllTests suite.

Next: approve.


Reading this a year from now -- start here

One of the small PRs converting PropertyTable broadcasts (OnPropertyChanged through the xCore Mediator) to direct calls ahead of the Pub/Sub delivery design. The inventory that classifies every broadcasting site is a gitignored working document, so the reasoning for this site lives here.

Decisions, and why
  • Direct ShowTabView rather than keeping the broadcast flag on the forced set. The forced set's row cited only re-entry (the handler calls ShowTabView, which reads the property), which the conversion's step order resolves with a direct call. Keeping the flag would have left one broadcasting self-set and the row in the inventory.
  • A local flag, not a tab-index comparison, in the same-text branch. else if (m_tabCtrl.SelectedIndex != (int)InterlinearTab) would also catch a link-set tab on the text already shown; that path belongs to the follow-link workflow and keeps its queued handler.
  • SelectAnnotation kept before the direct call. It runs on the analysis pane (for an empty text its bookmark converts to no occurrence and SelectOccurrence(null) hides the focus box) and ShowTabView runs it again on the Baseline pane — the same two calls today's path makes, in the same order. Dropping the first would be the one real behaviour change.
  • Handler kept. Its one sender left is LinkListener.FollowActiveLink, which sets every property entry a link carries with a broadcast; the entries come from UpdateContextHistory (history links) and from "Copy Location as Hyperlink" URLs. It does work only for a link to the text already shown with a different tab; other links go through SwitchText or a new view's Init, which read the property before the drain.
Evidence
  • Sets of the name: the three in InterlinMaster.cs and LinkListener.cs:584; ParserListener.cs:1059 only reads it.
  • Listener: InterlinMaster.OnPropertyChanged is the only InterlinearTab handler; it calls ShowTabView only when m_tabCtrl.SelectedIndex differs from the property.
  • F5 path to the same-text branch: FwXWindow.OnMasterRefresh → FinishRefresh → RefreshDisplay → clerk.ReloadIfNeeded → ReloadList → OnListChanged(Normal) → BroadcastChange → RecordNavigation → RecordView.ShowRecord(rni) → InterlinMaster.ShowRecord, with RootStText unchanged.
  • Test fixture: a TabControl with a created handle raises Deselecting/Selected on a programmatic SelectedIndex change (set_SelectedIndex → WmSelChange → OnSelected), so ClickTab exercises the real click handler; before the handle exists the change is silent, which StartOnTab relies on.
  • Hosts: one InterlinMaster (or InterlinMasterNoTitleBar) per tool in the Texts and Concordance configurations, so one view is a target at a time.
Preflight review details

Code Review Summary

Branch: claude/pubsub-interlinear-tab-1d1843
Base: main (merge base 936ae91)
Date: 2026-10-29
Review model: Claude Fable 5.1 (Claude Code)
Files changed: 3

Overview

InterlinMaster's own sets of the InterlinearTab property (the tab click, the getter's normalizing fallback, the Baseline tab forced for an empty text) no longer broadcast OnPropertyChanged; the view moves its tab control itself. The forced set calls ShowTabView directly in the branch where the same text is shown again, which is the only path where the queued handler did work. The handler stays for the one sender left, a followed link that carries the tab (LinkListener.FollowActiveLink).

Analysis (all four passes run directly; small managed diff, no native, COM, localization, build or installer surface): no Critical or Important finding. The change is a reordering with the same end state: the tab switch happens inside ShowRecord instead of at the next Mediator drain.

Contract/API Changes

None. InterlinearTab is internal; its setter stops broadcasting. The RawTextPaneTests.ListeningColleague test helper goes from private to internal so the new fixture can reuse it.

Findings

Critical - Must address before merge

None.

Important - Should address before merge

None.

Minor - Consider

  • SelectAnnotation() runs on the analysis pane right before the forced ShowTabView(), which calls it again on the Baseline pane (deliberate: keeps today's call sequence exactly, moving only the ShowTabView; author chose this shape over dropping the call)
  • ListeningColleague duplicated from RawTextPaneTests (fixed during review: the new fixture reuses RawTextPaneTests.ListeningColleague, widened to internal)
  • Redundant m_tabCtrl.Handle access after CreateControl() in the test fixture (fixed during review: removed)

Required Validation / Evidence

  • .\build.ps1 -CommentHygiene -TokenHygiene: build succeeded, 0 warnings, both hygiene gates clean.
  • .\test.ps1 -CommentHygiene -TokenHygiene -TestProject ITextDllTests -TestFilter "FullyQualifiedName~InterlinMaster|FullyQualifiedName~RawTextPaneTests": 8/8 passed (4 new InterlinMasterTabTests, 2 InterlinMasterTests, 2 RawTextPaneTests). The two tab tests and the ShowRecord test fail on main (JobItems 1; the tab control left on Gloss).
  • Manual tests on Sena 3 (author, 2026-10-29): tab clicks with the pane bar's "Add Words to Lexicon" check box following the tab; Go Back restoring the Gloss tab on the same text (the kept handler); restart restoring the Analyze tab; Parser menu items following the tab; an empty text on Gloss moved to Baseline by F5. All passed.
  • Not run: the full ITextDllTests suite; the getter fallback is test-only.
  • Developer-only in effect (same end state); no Jira ticket by the project's standing decision for the Pub/Sub conversion PRs.

Positive Observations

  • Tests prove the absence of a broadcast with a bare Mediator and a listening colleague (JobItems == 0), and prove the tab control moved, so they fail on main for the right reason.
  • The forced-tab direct call is placed where ShowTabView already runs inside ShowRecord (the SwitchText branch), so no new re-entry context.

Interview Notes

  • Forced Baseline set: the author chose a direct ShowTabView (option B) over keeping the broadcast flag (option A) after reviewing the timing difference: the analysis tab is no longer selected and focused for the span between ShowRecord and the Mediator drain; end state identical.
  • The redundant SelectAnnotation before the direct call is kept on purpose so every call today's path makes still happens in today's order.
  • ListeningColleague stays in RawTextPaneTests (author declined a separate file).

In-Review Quality Check

Build and the three fixtures re-run after the two test-fixture cleanups: 8/8 passed, hygiene clean.

Suggested Review Focus

  • InterlinMaster.ShowRecord: the fForcedRawText flag and the direct ShowTabView() in the same-text branch.
  • InterlinMasterTabTests.ShowRecord_EmptyTextOnAnalysisTab_MovesToRawTextWithoutBroadcast as the pin for that branch.

🤖 Generated with Claude Code


This change is Reviewable

InterlinMaster now moves its tab control itself whenever it sets the
InterlinearTab property: the tab click, the getter's normalizing fallback
and the Baseline tab forced for an empty text all store the value without
a broadcast, and the forced set calls ShowTabView directly when the same
text is shown again. Until now each of those sets queued an
OnPropertyChanged broadcast whose only work was that one ShowTabView,
run after the tab control already matched the property or, for the
forced set, after the rest of ShowRecord had run with the analysis tab
still selected.

The OnPropertyChanged handler stays for the one sender left: a followed
link that carries the tab.

Property whose own sets stop broadcasting:

* InterlinearTab

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

Copy link
Copy Markdown

NUnit Tests

    1 files  ±0      1 suites  ±0   14m 57s ⏱️ -10s
6 952 tests +4  6 867 ✅ +4  85 💤 ±0  0 ❌ ±0 
6 961 runs  +4  6 876 ✅ +4  85 💤 ±0  0 ❌ ±0 

Results for commit 7bdef08. ± Comparison against base commit 936ae91.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 40.40%. Comparing base (936ae91) to head (7bdef08).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1216      +/-   ##
==========================================
+ Coverage   40.36%   40.40%   +0.04%     
==========================================
  Files        1550     1550              
  Lines      355615   355621       +6     
  Branches    41476    41477       +1     
==========================================
+ Hits       143536   143688     +152     
+ Misses     182770   182640     -130     
+ Partials    29309    29293      -16     
Files with missing lines Coverage Δ
Src/LexText/Interlinear/InterlinMaster.cs 23.56% <100.00%> (+12.88%) ⬆️

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

@thejambi thejambi 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:

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

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