Skip to content

chore: separate non-render cleanup#865

Open
johnml1135 wants to merge 1 commit intomainfrom
cleanup/non-render-pr-split
Open

chore: separate non-render cleanup#865
johnml1135 wants to merge 1 commit intomainfrom
cleanup/non-render-pr-split

Conversation

@johnml1135
Copy link
Copy Markdown
Contributor

@johnml1135 johnml1135 commented May 5, 2026

Summary

This pulls the non-render cleanup out of #724 so the render-speedup PR can stay focused on Views rendering and render verification.

Changes include:

  • Build/test helper cleanup, including native artifact freshness helpers and clearer tracing/build switches.
  • Agent and repository instruction updates, including C# review guidance and OpenSpec workflow prompt/skill cleanup.
  • Installer and SDK/build documentation adjustments.
  • A deterministic TonePars generated-file comparison that ignores absolute Windows path prefixes in generated command content.

The xWorks dictionary/DeleteRecord routing changes that were previously part of this branch have been removed from PR #865. The underlying DeleteRecord command-routing issue on main is now tracked separately in LT-22514.

Validation

  • origin/main..cleanup/non-render-pr-split now has no Src/xWorks diff.
  • Before removing the xWorks scope, shell: Build passed on this branch.
  • Before removing the xWorks scope, shell: Test passed on this branch: 4150 total, 4090 passed, 60 skipped.
  • While switching between this branch and the render branch in the same worktree, stale generated native Views/Common artifacts had to be cleared once because the generated IDL outputs reflected the previously checked-out branch.

This change is Reviewable

Copilot AI review requested due to automatic review settings May 5, 2026 20:13
@johnml1135 johnml1135 force-pushed the cleanup/non-render-pr-split branch from f956492 to b9e003b Compare May 5, 2026 20:21
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR extracts non-render cleanup work into a dedicated change, including build/test workflow tightening, documentation/instruction updates, and xWorks headword-number configuration refactors (plus tests), along with a more deterministic TonePars generated-content comparison.

Changes:

  • Tighten build/test entrypoints and agent helpers (Windows-only enforcement, native artifact freshness checks, dependency verification tweaks).
  • Refactor xWorks headword-number custom digit handling (UI/controller/model serialization) with corresponding test updates.
  • Update docs/instructions and installer packaging configuration (CPM scoping for WiX packages, OpenSpec prompt/skill cleanup, misc whitespace/config normalization).

Reviewed changes

Copilot reviewed 81 out of 84 changed files in this pull request and generated no comments.

Show a summary per file
File Description
scripts/toolshims/environ.cmd Updates deprecated shim messaging to clarify non-Windows limitations.
scripts/Agent/Git-Search.ps1 Improves repo-root inference and errors when not in a git repo.
build.ps1 Renames tracing switch, blocks non-Windows builds, and refreshes stale Views native artifacts.
Test.runsettings Normalizes indentation/formatting.
Src/xWorks/xWorksTests/HeadwordNumbersControllerTests.cs Updates/extends tests for new custom digits + OK enablement behavior.
Src/xWorks/xWorksTests/DictionaryPublicationDecoratorTests.cs Adds regression test for refresh notifications when filtering changes.
Src/xWorks/xWorksTests/DictionaryConfigurationModelTests.cs Extends schema/clone tests to include custom homograph numbers.
Src/xWorks/XmlDocView.cs Adds delete handler behavior for reversal entries routing.
Src/xWorks/RecordClerk.cs Adjusts delete handler routing/return semantics and unsubscribes from DeleteRecord message.
Src/xWorks/IHeadwordNumbersView.cs Extends view interface with CustomDigitsChanged + WS factory hook.
Src/xWorks/HeadWordNumbersHelper.cs Removes helper used for splitting Unicode digits (now using config-driven digits).
Src/xWorks/HeadWordNumbersDlg.cs Updates dialog to use FwTextBox digits, apply stylesheet/WS, and emit CustomDigitsChanged.
Src/xWorks/HeadWordNumbersController.cs Drives view from model’s custom number list and validates OK enablement via event.
Src/xWorks/FwXWindow.cs Removes popup window icon assignment.
Src/xWorks/DictionaryPublicationDecorator.cs Adds change detection in Refresh and notifies roots when visible filtering changes.
Src/xWorks/DictionaryDetailsController.cs Ensures Headword numbers dialog receives stylesheet.
Src/xWorks/DictionaryConfigurationModel.cs Adds XML serialization for custom homograph numbers and cloning/export support.
Src/xWorks/ConfiguredLcmGenerator.cs Uses custom homograph digits for sense numbering when defined.
Src/Utilities/pcpatrflex/DisambiguateInFLExDB/DisambiguateInFLExDBTests/ToneParsInvokerTests.cs Makes TonePars output comparison deterministic by stripping absolute Windows path prefixes via regex.
Src/AppForTests.config Reformats diagnostics config and trims assembly-qualified type string.
SDK_MIGRATION.md Updates docs to reference -EnableTracing switch.
ReadMe.md Adds explicit Linux/macOS guidance (editing/review only; no builds/tests).
FLExInstaller/wix6/Shared/CustomActions/CustomActions/CustomActions.csproj Removes explicit WixToolset package versions (moves to local CPM).
FLExInstaller/Directory.Packages.props Enables CPM for installer subtree and centralizes WixToolset package versions.
Docs/CONTRIBUTING.md Clarifies Windows-only build/test/installer workflows; warns non-Windows users.
Directory.Build.targets Adds target to delete stale _wpftmp files before WPF markup pass 2.
Directory.Build.props Sets SolutionDir/SolutionName explicitly and globally excludes _wpftmp items.
Build/scripts/Invoke-CppTest.ps1 Uses new Views-native artifact freshness helper for TestViews prerequisites.
Build/Src/NativeBuild/NativeBuild.csproj Formatting-only change for PackageReference.
Build/Src/FwBuildTasks/CollectTargets.cs Collects ScrChecks info and wires ScrChecks dependency into certain test targets.
Build/Installer.legacy.targets Adds ScrChecks.dll/.pdb to installer merge modules list.
Build/Agent/fix-whitespace.ps1 Simplifies whitespace fixer and changes how files are written.
Build/Agent/Verify-FwDependencies.ps1 Minor refactor for required/optional dependency handling and array materialization.
Build/Agent/README.md Documents Run-AllRenders helper script.
Build/Agent/FwBuildHelpers.psm1 Adds artifact freshness helpers and exports new functions.
AGENTS.md Updates agent guidance and adds liblcm dependency note.
.vscode/settings.json Removes auto-approval for Windows-only build tasks and clarifies manual-approval list.
.github/workflows/patch-installer-cd.yml Whitespace-only YAML cleanup.
.github/skills/openspec-verify-change/SKILL.md Updates openspec-generated metadata version.
.github/skills/openspec-sync-specs/SKILL.md Updates openspec-generated metadata version.
.github/skills/openspec-onboard/SKILL.md Updates onboarding instructions and command reference around /opsx:propose and CLI install check.
.github/skills/openspec-new-change/SKILL.md Updates openspec-generated metadata version.
.github/skills/openspec-ff-change/SKILL.md Updates openspec-generated metadata version.
.github/skills/openspec-explore/SKILL.md Adjusts explore guidance to steer toward proposals rather than implementation.
.github/skills/openspec-continue-change/SKILL.md Updates openspec-generated metadata version.
.github/skills/openspec-bulk-archive-change/SKILL.md Updates messaging when no changes exist to archive.
.github/skills/openspec-archive-change/SKILL.md Updates sync instructions to invoke skill/tool-based sync prompt flow.
.github/skills/openspec-apply-change/SKILL.md Updates openspec-generated metadata version.
.github/skills/code-review-skill-main/scripts/pr-analyzer.py Adds new PR complexity analyzer script (new file).
.github/skills/code-review-skill-main/reference/typescript.md Adds TypeScript review guide reference (new file).
.github/skills/code-review-skill-main/reference/security-review-guide.md Adds security review guide (new file).
.github/skills/code-review-skill-main/reference/performance-review-guide.md Adds performance review guide (new file).
.github/skills/code-review-skill-main/reference/java.md Adds Java review guide (new file).
.github/skills/code-review-skill-main/reference/css-less-sass.md Adds CSS/Less/Sass review guide (new file).
.github/skills/code-review-skill-main/reference/cpp.md Adds C++ review guide (new file).
.github/skills/code-review-skill-main/reference/code-review-best-practices.md Adds review best-practices reference (new file).
.github/skills/code-review-skill-main/reference/c.md Adds C review guide (new file).
.github/skills/code-review-skill-main/reference/architecture-review-guide.md Adds architecture review guide (new file).
.github/skills/code-review-skill-main/assets/review-checklist.md Adds quick checklist asset (new file).
.github/skills/code-review-skill-main/assets/pr-review-template.md Adds PR review template asset (new file).
.github/skills/code-review-skill-main/SKILL.md Adds “code-review-excellence” skill definition (new file).
.github/skills/code-review-skill-main/README.md Adds skill documentation/readme (new file).
.github/skills/code-review-skill-main/LICENSE Adds MIT license for imported skill content (new file).
.github/skills/code-review-skill-main/CONTRIBUTING.md Adds contributing guide for skill content (new file).
.github/skills/code-review-skill-main/.gitignore Adds gitignore for skill subtree (new file).
.github/prompts/opsx-onboard.prompt.md Mirrors onboard skill updates in prompt form.
.github/prompts/opsx-ff.prompt.md Clarifies template vs context/rules handling in ff prompt.
.github/prompts/opsx-explore.prompt.md Updates explore prompt wording to steer toward proposals.
.github/prompts/opsx-bulk-archive.prompt.md Updates “no changes” message.
.github/prompts/opsx-archive.prompt.md Updates sync invocation instructions to tool/skill-based flow.
.github/instructions/terminal.instructions.md Removes “task-first for CI parity” rule text.
.github/instructions/repo.instructions.md Removes explicit VS Code CI task requirements.
.github/instructions/dotnet-framework.instructions.md Updates guidance for SDK-style net48 projects and Windows-only build policy.
.github/instructions/debugging.instructions.md Updates tracing docs to match -EnableTracing switch.
.github/instructions/csharp.instructions.md Adds new C# coding guidance specific to FieldWorks (new file).
.github/instructions/build.instructions.md Documents non-Windows host limitations and fail-fast behavior.
Files not reviewed (1)
  • Src/xWorks/HeadWordNumbersDlg.designer.cs: Language not supported
Comments suppressed due to low confidence (5)

Src/xWorks/DictionaryConfigurationModel.cs:1

  • The serialization delimiter is inconsistent with how the new/updated tests populate CustomHomographNumbers (several tests use ; separators, but this setter only splits on ,). This will collapse \"0;1;...\" into a single list item and can break downstream behavior that expects 10 digits. Recommendation: standardize on one delimiter repo-wide and enforce it in both tests and serialization; or accept both delimiters in the setter (e.g., split on , and ;) while emitting a single canonical delimiter from the getter. Also, WebUtility.HtmlDecode(value) will throw if value is null; handle null as empty.
    Src/xWorks/HeadWordNumbersController.cs:1
  • _view.CustomDigits is treated as non-null, but the interface allows implementations to return null (and this controller itself sets _view.CustomDigits from _homographConfig.CustomHomographNumberList, which may be null). If _view.CustomDigits is null at runtime, this handler will throw. Suggest normalizing to an empty sequence inside the handler (e.g., var digits = _view.CustomDigits ?? Enumerable.Empty<string>();) and then computing OkButtonEnabled from digits.
    Src/xWorks/xWorksTests/HeadwordNumbersControllerTests.cs:1
  • TriggerCustomDigitsChanged() will throw NullReferenceException if CustomDigitsChanged has no subscribers (or if a test constructs the view without creating a controller). Use a null-safe invoke and pass EventArgs.Empty rather than null to better match typical .NET event patterns.
    Build/Agent/fix-whitespace.ps1:1
  • This changes file-write semantics in a way that can be non-deterministic across hosts and PowerShell versions: -Encoding utf8 means UTF-8 with BOM in Windows PowerShell 5.1 but typically UTF-8 without BOM in PowerShell 7+, and the script no longer preserves the original BOM state (the previous implementation explicitly did). Recommendation: restore explicit BOM preservation (or explicitly standardize on BOM/no-BOM) by writing with a known System.Text.UTF8Encoding choice, or by using the explicit utf8BOM/utf8NoBOM encodings where available, so whitespace-fix does not churn file encodings unexpectedly.
    Src/xWorks/DictionaryPublicationDecorator.cs:1
  • The dictionary comparison uses OrderBy(...).SequenceEqual(...), which allocates and adds O(n log n) cost on every Refresh() call. Since Refresh() can be frequent, prefer an O(n) comparison without sorting (e.g., compare Count, then iterate one dictionary and TryGetValue into the other for value equality). This keeps the change-detection logic while reducing per-refresh overhead.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 5, 2026

NUnit Tests

    1 files  ±0      1 suites  ±0   6m 44s ⏱️ +19s
4 102 tests ±0  4 031 ✅ ±0  71 💤 ±0  0 ❌ ±0 
4 111 runs  ±0  4 040 ✅ ±0  71 💤 ±0  0 ❌ ±0 

Results for commit fe6f187. ± Comparison against base commit db39df6.

♻️ This comment has been updated with latest results.

@johnml1135
Copy link
Copy Markdown
Contributor Author

johnml1135 commented May 6, 2026

Copilot rationale note 1/4: why this PR exists

This PR is intentionally the non-render half of the split from the render-speedup branch. I reviewed it as real code only and ignored the agent/skills/prompt updates.

The theme is not a single product feature. It is stabilization work around several recent FieldWorks changes that were previously riding along with the rendering branch:

  • native Views/TestViews build prerequisites can exist but be stale after rebases or branch switches;
  • SDK/WPF build behavior can be tripped up by stale _wpftmp files and missing solution metadata;
  • dependency and whitespace helper scripts needed small reliability cleanups;
  • ScrChecks is still a hidden runtime/test dependency even after recent project layout changes;
  • installer/WiX package management has different constraints from the main app projects;
  • TonePars tests need to validate generated command semantics rather than local absolute paths.

The xWorks dictionary/delete-routing changes that were previously discussed here have been removed from this PR. The underlying DeleteRecord architecture issue on main is now tracked separately as LT-22514.

So the main value of PR #865 is: keep the render PR focused, while preserving the non-render build/test/installer/TonePars fixes that make current mainline development and tests less brittle.

@johnml1135
Copy link
Copy Markdown
Contributor Author

Copilot rationale note 2/4: build/test reliability changes

The build/test pieces are here because FieldWorks is a mixed native/managed Windows repo, and simple existence checks are not enough for generated native artifacts.

Build/Agent/FwBuildHelpers.psm1, build.ps1, and Build/scripts/Invoke-CppTest.ps1 add and use timestamp-based freshness checks for Views/TestViews prerequisites. This matters because files like Views.dll, views.lib, generated FwKernel headers, and Views object files can exist but still be stale relative to native sources or IDL/interface changes. After a branch switch or rebase, stale native artifacts produce confusing C++/COM build errors. This PR makes the build refresh native prerequisites when they are out of date, instead of assuming existing artifacts are valid.

Directory.Build.props and Directory.Build.targets handle two repo-wide MSBuild irritants: setting default SolutionDir/SolutionName when projects are built outside the solution, and excluding/deleting stale WPF _wpftmp temp projects before markup compilation. That makes SDK-style project evaluation and WPF builds less sensitive to interrupted builds and ad-hoc project invocation.

Build/Agent/Verify-FwDependencies.ps1 wraps required/missing/optional query results as arrays so PowerShell scalar-vs-array behavior does not corrupt counts or classifications. That improves the dependency preflight script’s reliability.

Build/Src/FwBuildTasks/CollectTargets.cs restores ScrChecks as an explicit generated target dependency for affected tests. ScrChecks is not always discoverable as a normal project reference, but ScrChecks.dll still needs to land under DistFiles/Editorial Checks. This connects to the recent ScrChecks project-layout changes and prevents hidden runtime/test dependency loss.

@johnml1135
Copy link
Copy Markdown
Contributor Author

Copilot rationale note 3/4: installer and TonePars changes

The installer changes are here because FLExInstaller does not behave like an ordinary app project. WiX custom actions, DTF packages, and patch installer versioning have constraints that should not be accidentally governed by the same package-management assumptions as the main FieldWorks projects.

FLExInstaller/Directory.Packages.props and the WiX custom action project changes isolate installer package handling so the installer can keep the package/version behavior it needs while the rest of the repo continues moving toward shared package conventions. The patch installer workflow/targets adjustments are in the same family: make patch build/version semantics clearer and less fragile.

The TonePars change in ToneParsInvokerTests.cs is a deterministic-test fix. Recent work brought PCPATR/TonePars into this repo, and the tests compare generated command/log content. Those generated files can contain absolute Windows paths, which vary by developer machine and worktree. Normalizing absolute path prefixes lets the test validate the meaningful generated content rather than failing because one machine used a different checkout path.

Keep build/test helper cleanup out of the render commit.

Keep agent, installer, and xWorks cleanup separate.

Normalize TonePars generated-file comparisons for Windows paths.
@johnml1135 johnml1135 force-pushed the cleanup/non-render-pr-split branch from b9e003b to fe6f187 Compare May 6, 2026 18:16
@johnml1135
Copy link
Copy Markdown
Contributor Author

Copilot update: the xWorks dictionary/DeleteRecord routing changes have been removed from this PR. The DeleteRecord pub/sub architecture issue on main is now tracked separately in LT-22514, with the recommended scoped command-router path and test plan. PR #865 should now be reviewed as build/test/installer/TonePars cleanup only.

@sillsdev sillsdev deleted a comment from github-actions Bot May 6, 2026
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