Skip to content

LT-22691: Add the transient Show-all writing-system reveal - #1119

Merged
jasonleenaylor merged 1 commit into
mainfrom
LT-22691g
Sep 2, 2026
Merged

LT-22691: Add the transient Show-all writing-system reveal#1119
jasonleenaylor merged 1 commit into
mainfrom
LT-22691g

Conversation

@mark-sil

@mark-silmark-sil commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

"Show all right now" on a detail row's Writing Systems menu did nothing. This makes it work: the click reveals every writing system the field can show, for as long as you stay on the record.

The reveal is transient by design — it is view state, not configuration. It survives clicking between fields and editing within the record, and it ends when you navigate to another record or persist a new selection through the writing-system toggles or the Configure dialog. Nothing is written to .viewoverride.json, so a peek can never silently pin a field's writing systems.

One reveal covers the whole part, matching what the legacy view does: revealing Gloss on sense 1 reveals it on every sense.

Why record-scoped expiry

The legacy slice's comments say the reveal ends when the slice loses currency, and its SetCurrentState does call ReloadWssToDisplayForPart to that end — but that method only resets when WritingSystemsSelectedForDisplay is null, and its getter never returns null. The revert has been dead code for years. Testing the shipped behaviour confirmed it: a reveal survives moving between fields and ends only when the rows are rebuilt. This implementation matches the measured behaviour rather than the comment.

Refresh (F5) is a deliberate divergence: the reveal survives it. Legacy rebuilds its slices there, but its F5 handling is itself unreliable in this area (it can leave a block of blank space where a row was), so it is not a parity target. Clearing on every recompose would also kill the reveal whenever an in-record edit completes.

Also in this branch

RefreshAvaloniaDetail now resolves the record to show through the same root walk ShowRecord uses. Without it, a showDescendantInRoot tool (Notebook) recomposed the subrecord instead of the root on every refresh — a pre-existing defect that also made the reveal clear itself the instant it was set.

The override write path clears the reveal and recomposes only after the save succeeds. MutateOverrideAndRefresh is split into a TryMutateOverride that reports success without recomposing. Previously a failed save ended the reveal invisibly, so the row collapsed at the next unrelated refresh with nothing to explain it.

Validation

build.ps1 -CommentHygiene green; 23/23 targeted tests pass across the three affected fixtures with no skips (4 new tests). Manually tested in Sena 3: reveal, part scope, record-navigation expiry, replacement by a persisted selection, and confirmation that nothing reaches the override file.

Found while testing, not fixed here

LT-22777 — writing systems holding data are hidden. The legacy view never hides an alternative that contains data, regardless of the per-field selection or the project-level checkbox (SkipEmptyWritingSystem), and its option list includes active-but-unticked writing systems. The Avalonia composer has neither rule: it resolves only current writing systems and intersects them with the per-field restriction. Two consequences — restricting a field can hide data, and data in an unticked writing system is unreachable. This also bounds the feature in this PR: on a field with no restriction, "Show all right now" has nothing to add, because the composer never resolved the wider set. Filed separately because the fix is a composer-level change to the writing-system model, not to this command.

Move Field is flaky. Move Up/Down works sometimes and not others. A real cause was identified — a row at the top level of its own layout gets a null ParentStableId, which enablement ignores but ApplyMoveField bails on, so the item enables and silently does nothing, even though the applier already reorders roots under an empty parent key. A one-line fix and a test were written and then backed out of this branch, because intermittency means that cause is incomplete. To be investigated on its own.

Deferred with reasons. The reveal is left in place on the copy-bail paths (unreadable or stale adapter slice): nothing was persisted there, so keeping the view as the user left it is more honest than collapsing the row. A tidy-up of eight stale "gear-menu" comments is left for a separate commit to keep this diff on topic.

Preflight review details

Code Review Summary

Branch: LT-22691g

Base: origin/main (merge-base 8f46a80)

Date: 2026-09-02

Review model: Claude Opus 5

Files changed: 5

Overview

Makes the Avalonia detail view's "Show all right now" writing-system command work.
It was deliberately inert since the c4b menu work (#1108): the menu item existed but
nothing consulted it. The command now marks the clicked row's part in a host-held set
of template StableIds, and the composer skips that part's per-field
visibleWritingSystems restriction, so every row sharing the part shows its full
writing-system set until the shown record changes.

Expiry semantics were chosen against measured legacy behavior rather than the legacy
code comments: the legacy slice's documented "expires when the slice loses currency"
path is dead code (ReloadWssToDisplayForPart only resets when
WritingSystemsSelectedForDisplay is null, which its getter never returns), so legacy
reveals actually survive until the slices are rebuilt. The reveal is therefore
record-scoped, never persisted.

Contract/API Changes

DetailComposer.Compose (both overloads) gains an optional trailing
ISet<string> showAllWritingSystemsFields parameter; all existing callers are
source-compatible. MutateOverrideAndRefresh is split, adding a private
TryMutateOverride that saves without recomposing and reports success.

Findings

Critical - Must address before merge

None.

Important - Should address before merge

  • Reveal was row-scoped where the legacy reveal covers the whole part
    (fixed during review: keyed the set on the template StableId, which is the same
    equivalence class as the legacy part-ref; added a test proving one reveal covers
    every sense's Gloss row)
  • A persist cleared only the clicked row's reveal while the override write
    applies to every row sharing the template
    (fixed during review: same template-id
    rekey)
  • RefreshAvaloniaDetail recomposed Clerk.CurrentObject without the
    showDescendantInRoot root walk, so the reveal self-cleared on click in Notebook

    (fixed during review: added ResolveShownRecord, shared with ShowRecord; this
    also fixes a pre-existing subrecord-recompose defect)
  • A failed override save cleared the reveal without recomposing, so the row
    collapsed at the next unrelated refresh
    (fixed during review: TryMutateOverride
    split; the reveal is cleared and the view recomposed only after a successful save)

Minor - Consider

  • Reveal membership was computed at the call site and threaded as a positional
    bool
    (fixed during review: the check moved inside ResolveTextRowWritingSystems)
  • Repeated invoke-Show-all sequence and a duplicated row locator in the host
    fixture
    (fixed during review: InvokeShowAllRightNow and FindLexemeFormRow
    helpers)
  • Lone <param> tag on an eight-parameter method breaks the comment standard's
    all-or-nothing rule
    (author does not accept the all-or-nothing rule; only the new
    parameter is documented)
  • The reveal survives F5 / master refresh where legacy rebuilds slices
    (author decision: legacy's F5 handling is itself defective here, leaving blank space
    where a removed row was, so it is not a parity target; clearing on every recompose
    would also kill the reveal when an in-record edit completes)
  • Reveal state can drift on copy-bail paths (unreadable or stale adapter slice)
    (nothing was persisted on those paths, so keeping the reveal leaves the view as the
    user left it; clearing it would imply a configuration change that did not happen)

Required Validation / Evidence

  • ./build.ps1 -CommentHygiene - green (a -Clean run was needed once to regenerate
    MIDL headers after PR Speed up bulk Find & Replace with a native single-session string replace #1065 added IVwPattern2; unrelated to this branch).
  • ./test.ps1 -CommentHygiene -TestProject xWorksTests -TestFilter "FullyQualifiedName~FieldTypeComposerTests|FullyQualifiedName~DetailObjectCommandExecutionTests|FullyQualifiedName~DetailComposerOverrideTests"
    • 23/23 passed, 0 skipped.
  • Author manual testing in Sena 3 against the checklist below: reveal, part scope,
    record-navigation expiry, persistence replacement, never-persisted.
  • Not run: full suite; native tests (no native change); installer validation (none).

Positive Observations

  • The composer bypass is placed where restrictions are applied, so a restriction from
    the shipped layout is honored the same way as one from the project override.
  • The reveal is never written to .viewoverride.json, verified by a test that asserts
    the stored override still holds the restriction after a reveal.

Interview Notes

  • Expiry semantics: the author tested legacy directly and established that a reveal is
    NOT reverted by moving between fields, contradicting both the legacy doc comments and
    the initial analysis. Record-navigation expiry was chosen to match measured behavior.
  • The author established that legacy always displays a writing system that holds data
    regardless of the per-field selection or the project-level checkbox
    (SkipEmptyWritingSystem). The Avalonia composer has no such rule and resolves only
    current writing systems. Filed as LT-22777; out of scope here.
  • Move Field was found to be flaky during manual testing. A confirmed root-level cause
    was identified and a one-line fix plus test were written, then backed out at the
    author's request because the intermittency proves the cause is incomplete. To be
    investigated separately.
  • Author does not understand: nothing recorded. The author reviewed the diff directly
    and drove the design decisions on expiry and scope.

In-Review Quality Check

All fixes above were made before the commit; build and targeted tests were re-run green
after each. Comment hygiene is clean, and comments comparing behavior to legacy were
removed at the author's request so they do not rot when legacy is retired.

Suggested Review Focus

  • Template-StableId keying: is the template id the right equivalence class for
    "the same part" in every layout, not just the lexicon ones tested?
  • ResolveShownRecord now runs on every Avalonia refresh, changing behavior for
    showDescendantInRoot tools (Notebook) that are not yet Avalonia-enabled.
  • The reveal set lives on the host and is passed into every compose; confirm no
    other host composes these rows and would need the same wiring.

This change is Reviewable

The Avalonia detail view's "Show all right now" menu item was inert. It now
marks the clicked row's part in a host-held set of template StableIds, and
DetailComposer skips the per-field visibleWritingSystems restriction for those
parts, so every row sharing the part shows its full writing-system set.
The reveal survives focus changes and edits within the record and expires when
the shown record changes; a writing-system toggle or the Configure dialog
replaces it. It is never written to the view override.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

NUnit Tests

1 files ±0 1 suites ±0 12m 30s ⏱️ + 4m 26s
6 002 tests +6 5 921 ✅ +7 81 💤 ±0 0 ❌ - 1 
6 011 runs +6 5 930 ✅ +7 81 💤 ±0 0 ❌ - 1 

Results for commit ae6fa34. ± Comparison against base commit 8f46a80.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 72.54902% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.55%. Comparing base (8f46a80) to head (ae6fa34).

Files with missing linesPatch %Lines
...xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs70.58%6 Missing and 4 partials ⚠️
Src/xWorks/RecordEditView.cs50.00%2 Missing and 2 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #1119 +/- ##
=======================================
Coverage 38.55% 38.55% =======================================
Files 1513 1513 Lines 350968 350997 +29 Branches 40345 40350 +5 =======================================
+ Hits 135322 135341 +19 - Misses 186429 186438 +9 - Partials 29217 29218 +1 
Files with missing linesCoverage Δ
Src/xWorks/Avalonia/Composer/DetailComposer.cs67.49% <100.00%> (+0.08%)⬆️
Src/xWorks/RecordEditView.cs59.51% <50.00%> (+0.40%)⬆️
...xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs57.07% <70.58%> (+1.78%)⬆️

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

@mark-silmark-sil mentioned this pull request Sep 2, 2026
Comment threadSrc/xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs
@jasonleenaylor
jasonleenaylor merged commit f2829d6 into mainSep 2, 2026
8 of 9 checks passed
@jasonleenaylor
jasonleenaylor deleted the LT-22691g branch September 2, 2026 22:49
Sign up for freeto 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.

4 participants

@mark-sil@codecov-commenter@papeh@jasonleenaylor