Uh oh!
There was an error while loading. Please reload this page.
Reduce optional COM usage - #904
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR reduces optional COM exposure and centralizes SIL Encoding Converters usage behind a FieldWorks-owned provider seam, while updating OpenSpec documentation and tests to enforce the new boundaries.
Changes:
- Remove COM visibility/build-manifest inputs for
ManagedLgIcuCollator, and remove dormant native OLE clipboard shutdown cleanup. - Introduce
IEncodingConvertersProvider/EncodingConvertersProviderand migrate call sites away from directnew EncConverters()construction. - Isolate
DebugProcsCOM activation behind a debug-only transport seam and add characterization tests.
Reviewed changes
Copilot reviewed 43 out of 47 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| openspec/changes/reducing-com-usage/tasks.md | Adds an implementation/validation task checklist for COM reduction slices. |
| openspec/changes/reducing-com-usage/specs/integration/external/encoding/spec.md | Specifies the new provider seam requirement for Encoding Converters access. |
| openspec/changes/reducing-com-usage/specs/architecture/testing/com-reduction/spec.md | Adds testing/validation requirements for COM reduction work. |
| openspec/changes/reducing-com-usage/specs/architecture/interop/com-contracts/spec.md | Updates COM contract requirements + manifest cleanup expectations. |
| openspec/changes/reducing-com-usage/proposal.md | Documents rationale/scope/non-goals for the COM reduction initiative. |
| openspec/changes/reducing-com-usage/design.md | Details architectural decisions (provider seam, manifest hygiene, test gates). |
| openspec/changes/reducing-com-usage/COM_USAGE.md | Adds a repo-specific COM usage audit within the OpenSpec change folder. |
| openspec/changes/reducing-com-usage/COM_FIXES_NOW.md | Adds a prioritized “COM fixes now” plan within the OpenSpec change folder. |
| openspec/changes/reducing-com-usage/.openspec.yaml | Declares OpenSpec metadata for the change directory. |
| Src/xWorks/xWorksStrings.resx | Whitespace/formatting normalization in .resx content. |
| Src/Utilities/SfmToXml/Sfm2Xml.csproj | Adds reference to FwUtils to consume the shared provider seam. |
| Src/Utilities/SfmToXml/Converter.cs | Switches converter acquisition to EncodingConvertersProvider. |
| Src/Utilities/SfmToXml/ClsLanguage.cs | Updates API to accept IEncConverters instead of concrete EncConverters. |
| Src/ParatextImport/SCTextEnum.cs | Replaces direct EncConverters usage with provider seam (and error handling). |
| Src/ParatextImport/SCScriptureText.cs | Wires provider seam into Paratext import enumerator creation. |
| Src/ParatextImport/ParatextImportTests/SCTextEnumTests.cs | Updates tests to inject provider seam and adds a seam-focused test. |
| Src/ManagedLgIcuCollator/LgIcuCollator.cs | Removes COM exposure from ManagedLgIcuCollator. |
| Src/LexText/Lexicon/LexEdDll.csproj | Whitespace/formatting normalization. |
| Src/LexText/LexTextControls/SfmToTextsAndWordsMappingBaseDlg.cs | Populates converter UI via provider (no direct EncConverters construction). |
| Src/LexText/LexTextControls/Sfm2FlexTextWords.cs | Uses provider to lazily obtain converters. |
| Src/LexText/LexTextControls/LexImportWizardLanguage.cs | Populates converter UI via provider. |
| Src/LexText/LexTextControls/LexImportWizard.cs | Uses provider for existence check and controlled access to concrete repository. |
| Src/LexText/LexTextControls/DataNotebook/NotebookImportWiz.cs | Uses provider and typed enumeration helper for converter names. |
| Src/LexText/LexTextControls/DataNotebook/ImportEncCvtrDlg.cs | Populates converter UI via provider. |
| Src/LexText/Interlinear/LinguaLinksImportDlg.cs | Uses provider for converter existence + controlled concrete access. |
| Src/LexText/Interlinear/LinguaLinksImport.cs | Uses provider for converter repository access. |
| Src/Generic/ModuleEntry.h | Removes dormant clipboard ownership tracking API/state. |
| Src/Generic/ModuleEntry.cpp | Removes dormant clipboard shutdown flush logic/state. |
| Src/FwCoreDlgs/FwWritingSystemSetupModel.cs | Replaces direct converter creation with provider seam for UI model. |
| Src/FwCoreDlgs/FwCoreDlgsTests/FwWritingSystemSetupModelTests.cs | Updates tests to mock provider seam. |
| Src/FwCoreDlgs/ConverterTester.cs | Uses provider for concrete converter repository access. |
| Src/FwCoreDlgs/CnvtrPropertiesCtrl.cs | Uses provider for concrete converter repository access. |
| Src/FwCoreDlgs/AddCnvtrDlg.cs | Uses provider (including reset) for concrete converter repository access. |
| Src/Common/FwUtils/Win32Wrappers.cs | Removes unused OLE clipboard P/Invokes tied to dormant native logic. |
| Src/Common/FwUtils/FwUtilsTests/DebugProcsTests.cs | Adds tests for debug transport seam and failure/disposal tolerance. |
| Src/Common/FwUtils/FwUtils.csproj | Adds encoding-converters-core dependency for provider implementation. |
| Src/Common/FwUtils/EncodingConvertersProvider.cs | Introduces provider seam + helpers for typed enumeration/contains. |
| Src/Common/FwUtils/DebugProcs.cs | Adds debug transport seam and isolates COM activation behind it. |
| Src/Common/FieldWorks/BuildInclude.targets | Removes ManagedLgIcuCollator from managed COM manifest inputs. |
| Src/Common/Controls/XMLViews/BulkEditBar.cs | Uses provider + typed enumeration helper instead of direct EncConverters. |
| DistFiles/Language Explorer/Configuration/Parts/LexSenseParts.xml | Whitespace normalization. |
| COM_USAGE.md | Adds top-level repo COM audit documentation. |
| COM_FIXES_NOW.md | Adds top-level prioritized COM fixes documentation. |
| Build/mkall.targets | Removes excluded CLSID entry for ManagedLgIcuCollator. |
| Build/Src/FwBuildTasks/FwBuildTasksTests/RegFreeCreatorTests.cs | Adds regression tests ensuring COM-visible=false doesn’t emit clrClass. |
| Build/RegFree.targets | Removes ManagedLgIcuCollator from managed COM manifest inputs. |
Comments suppressed due to low confidence (1)
Src/ParatextImport/SCTextEnum.cs:1
SCTextEnumis a public class and previously exposed aprotected IEncConverters m_encConvertersfield; changing it to aprivateprovider removes subclass access and is a breaking change for any derived types outside this PR. If inheritance is intended/supported, consider restoring a protected accessor (e.g., a protected property exposingIEncodingConvertersProviderorIEncConverters) to preserve extensibility while still preventing directnew EncConverters()usage at call sites.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
johnml1135
commented
May 20, 2026
ContributorAuthor
Also addressed Copilot's low-confidence SCTextEnum extensibility note in 7f7aa13 by adding protected accessors for the provider and repository without reintroducing direct EncConverters construction at call sites. |
jasonleenaylor
commented
May 22, 2026
Contributor
Clipbloard cleanup and managedlgicucollator are good, the rest are not adding immediate value and make some code more complicated for no benefit. |
johnml1135
commented
May 22, 2026
ContributorAuthor
Jason, I reduced this PR to the requested scope: the clipboard cleanup plus the ManagedLgIcuCollator COM visibility/manifest cleanup. The broader COM-reduction/OpenSpec work is out of the branch now. I created LT-22528 for the optional future work and copied the COM-reduction markdown/OpenSpec notes into that Jira issue so it is easy to pick up later if it becomes worth doing. |
johnml1135
commented
Jun 4, 2026
ContributorAuthor
@jasonleenaylor - what is still needed here? |
jasonleenaylor
commented
Jun 5, 2026
Contributor
region can go too. |
jasonleenaylor
approved these changes
Jun 5, 2026
jasonleenaylor
left a comment
Contributor
There was a problem hiding this comment.
@jasonleenaylor reviewed 47 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on johnml1135).
Uh oh!
There was an error while loading. Please reload this page.
jasonleenaylor added a commit
that referenced
this pull request
Jun 10, 2026
Commit 846c123 ("Reduce optional COM usage", #904) stopped emitting the reg-free COM manifest ManagedLgIcuCollator.manifest, but the base release still ships it. WiX 3 pyro then fails the patch build with PYRO0305 because files cannot be removed in a patch. Add the manifest to the RescuePatching target's RemovedSinceLastBase list so a zero-byte placeholder is staged into the build output. The file is then present in both the Master and Update harvests, so pyro treats it as changed rather than removed. This mirrors the existing ManagedVwWindow.manifest and SimpleRootSite.manifest entries. Also document the error and this fix in FLExInstaller/AGENTS.md for future occurrences. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
johnml1135 added a commit
that referenced
this pull request
Jul 6, 2026
- VersionInfoProvider: copyright year no longer freezes at whatever year the constant was last edited, ApplicationVersion resolves from the correct assembly instead of always falling back to the entry assembly, and MajorVersion/ParseInformationalVersion index defensively instead of assuming a fixed part count. Covered by new VersionInfoProviderTests.cs. - RegFree.targets: removes a dangling ManagedVwWindow.dll entry; the project was already retired in #904/#906, so the entry pointed at nothing. - opsx-*.prompt.md: replace inlined instructions with delegation to the existing .claude/skills/openspec-*/SKILL.md files, per this repo's skills-over-inline-prompts convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
johnml1135 added a commit
that referenced
this pull request
Aug 12, 2026
- VersionInfoProvider: copyright year no longer freezes at whatever year the constant was last edited, ApplicationVersion resolves from the correct assembly instead of always falling back to the entry assembly, and MajorVersion/ParseInformationalVersion index defensively instead of assuming a fixed part count. Covered by new VersionInfoProviderTests.cs. - RegFree.targets: removes a dangling ManagedVwWindow.dll entry; the project was already retired in #904/#906, so the entry pointed at nothing. - opsx-*.prompt.md: replace inlined instructions with delegation to the existing .claude/skills/openspec-*/SKILL.md files, per this repo's skills-over-inline-prompts convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
johnml1135 added a commit
that referenced
this pull request
Aug 14, 2026
- VersionInfoProvider: copyright year no longer freezes at whatever year the constant was last edited, ApplicationVersion resolves from the correct assembly instead of always falling back to the entry assembly, and MajorVersion/ParseInformationalVersion index defensively instead of assuming a fixed part count. Covered by new VersionInfoProviderTests.cs. - RegFree.targets: removes a dangling ManagedVwWindow.dll entry; the project was already retired in #904/#906, so the entry pointed at nothing. - opsx-*.prompt.md: replace inlined instructions with delegation to the existing .claude/skills/openspec-*/SKILL.md files, per this repo's skills-over-inline-prompts convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
johnml1135 added a commit
that referenced
this pull request
Aug 26, 2026
- VersionInfoProvider: copyright year no longer freezes at whatever year the constant was last edited, ApplicationVersion resolves from the correct assembly instead of always falling back to the entry assembly, and MajorVersion/ParseInformationalVersion index defensively instead of assuming a fixed part count. Covered by new VersionInfoProviderTests.cs. - RegFree.targets: removes a dangling ManagedVwWindow.dll entry; the project was already retired in #904/#906, so the entry pointed at nothing. - opsx-*.prompt.md: replace inlined instructions with delegation to the existing .claude/skills/openspec-*/SKILL.md files, per this repo's skills-over-inline-prompts convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
johnml1135 added a commit
that referenced
this pull request
Sep 1, 2026
…, thin opsx prompts (#978) * chore: small fixes pulled off from PR #964 - VersionInfoProvider: copyright year no longer freezes at whatever year the constant was last edited, ApplicationVersion resolves from the correct assembly instead of always falling back to the entry assembly, and MajorVersion/ParseInformationalVersion index defensively instead of assuming a fixed part count. Covered by new VersionInfoProviderTests.cs. - RegFree.targets: removes a dangling ManagedVwWindow.dll entry; the project was already retired in #904/#906, so the entry pointed at nothing. - opsx-*.prompt.md: replace inlined instructions with delegation to the existing .claude/skills/openspec-*/SKILL.md files, per this repo's skills-over-inline-prompts convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Drop stale ManagedVwWindow.dll references from RegFree build files ManagedVwWindow.dll was retired with the Linux-era view shims (#906). The RegFree.targets fallback entry was removed earlier in this PR, but BuildInclude.targets pre-populates ManagedComAssemblies explicitly for FieldWorks.exe (bypassing that fallback), so the "Could not find file" warning kept firing for the main product. Also drops the matching stale CLSID exclusion in mkall.targets. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * style: use ASCII punctuation in new prompt/doc/test prose This branch's new content used em-dashes and a Unicode "<=" sign in a few places (opsx-explore.prompt.md, opsx-onboard.prompt.md, AGENTS.md, VersionInfoProviderTests.cs), which render poorly in some git tooling. Switch them to the ASCII "--" and "<=" forms the rest of the repo's prose and comments already use. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Address review comments on VersionInfoProvider Shorten the comment blocks flagged over the 200-character cap (MajorVersion 241, CopyrightString 214, ApplicationVersion 204) and rewrite the four that narrated repo history, named callers, or restated the member name. Guard the one line in this file that could actually throw: Convert.ToInt32(versionParts[1]) raised FormatException on a non-numeric token, out of a property getter the splash screen and About box read. int.TryParse leaves date at 0, which the existing date > 0 check already treats as no date, so the fallback path is unchanged. This matches BaseBuildNumber, which already uses int.TryParse in the same file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011arsbDno5H5y2pLGqGgb9L --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scope
This PR reduces optional COM usage and centralizes Encoding Converters access without changing the required FieldWorks native COM boundaries.
Included in this PR:
ManagedLgIcuCollatorCOM visibility and its class-specific reg-free manifest/build plumbing.IDataObjectbehavior unchanged.DebugProcsCOM activation behind an injectable transport seam; COM remains the debug fallback rather than leaking through general managed code.IEncodingConvertersProvider/EncodingConvertersProviderand migrate product-level directnew EncConverters()construction behind that FieldWorks-owned provider.Explicitly out of scope:
IPicture,IStream, orUnknownPropABI rewrite.encoding-converters-coreruntime; this PR creates the provider boundary only.ViewInputManager/ManagedVwWindowshims; that work was split to branchretire-linux-era-view-shims.Validation
./build.ps1./build.ps1 -BuildTests/ broader managed validation during implementation./test.ps1 -TestProject ManagedLgIcuCollatorTests./test.ps1 -TestProject FiltersTests./test.ps1 -TestProject LexEdDllTests./test.ps1 -TestProject FwBuildTasksTests -TestFilter "FullyQualifiedName~RegFreeCreator"./test.ps1 -TestProject SimpleRootSiteTests -TestFilter "FullyQualifiedName~EditingHelperTests"./test.ps1 -SkipManaged -TestProject TestGeneric./test.ps1 -TestProject FwUtilsTests -TestFilter "FullyQualifiedName~DebugProcs"./test.ps1 -TestProject ParatextImportTests./test.ps1 -TestProject FwCoreDlgsTests./test.ps1 -TestProject Sfm2XmlTests./test.ps1 -TestProject LexTextControlsTests./test.ps1 -TestProject XMLViewsTests./test.ps1 -TestProject ITextDllTestsCI: Whitespace checkCI: Commit messagesFollow-up / Merge Notes
origin/main:61579e344 Reduce optional COM usage.retire-linux-era-view-shims.This change is