Skip to content

Fix new problem with LT-11746: Make Preview aware of publication#858

Merged
jtmaxwell3 merged 3 commits intomainfrom
LT-11746b
May 8, 2026
Merged

Fix new problem with LT-11746: Make Preview aware of publication#858
jtmaxwell3 merged 3 commits intomainfrom
LT-11746b

Conversation

@jtmaxwell3
Copy link
Copy Markdown
Collaborator

@jtmaxwell3 jtmaxwell3 commented Apr 30, 2026

This fixes a new problem with https://jira.sil.org/browse/LT-11746: Lexical relations shouldn't be shown if they are not in the current publication. The solution was to pass in a DictionaryPublicationDecorator like the one in XhtmlDocView.cs.


This change is Reviewable

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 30, 2026

NUnit Tests

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

Results for commit fcdad9a. ± Comparison against base commit 5334190.

This pull request removes 3 and adds 2 tests. Note that renamed tests count towards both.
SIL.FieldWorks.XWorks.HeadwordNumbersControllerTests ‑ ConstructorSetsCustomHeadwordNumbersInView
SIL.FieldWorks.XWorks.HeadwordNumbersControllerTests ‑ Ok_Disabled_WhenNotAllTenNumbersSet
SIL.FieldWorks.XWorks.HeadwordNumbersControllerTests ‑ Ok_Enabled_WithAllTenNumbers
SIL.FieldWorks.XWorks.HeadwordNumbersControllerTests ‑ ConstructorSetsHeadwordWritingSystemNumbersInView
SIL.FieldWorks.XWorks.HeadwordNumbersControllerTests ‑ CustomDigits_Default_Ok_Enabled_WhenNotAllTenNumbersSet

♻️ This comment has been updated with latest results.

@jasonleenaylor
Copy link
Copy Markdown
Contributor

Src/xWorks/XhtmlRecordDocView.cs line 170 at r1 (raw file):

				var configuration = new DictionaryConfigurationModel(configurationFile, Cache);
				PublicationDecorator.Refresh();
				var xhtmlPath = LcmXhtmlGenerator.SavePreviewHtmlWithStyles(new [] { cmo.Hvo }, Clerk, PublicationDecorator, configuration, m_propertyTable);

Did you check and see if there are any visual changes after this beyond the current bug fix?
We explicitly passed null here before and I think we have some code that expected that for the single entry preview styling. That was probably overloading the parameter, so the fix isn't to undo this but to make that single entry view explicit.

@jtmaxwell3
Copy link
Copy Markdown
Collaborator Author

I didn't see any other changes, but if there were other changes then I expect that we would want them. Adding the publication parameter makes the function call used by the preview to be identical to the function call used by the dictionary display. So the preview display should look like the dictionary display, which is what we want.

@jasonleenaylor
Copy link
Copy Markdown
Contributor

Src/xWorks/XhtmlRecordDocView.cs line 170 at r1 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

Did you check and see if there are any visual changes after this beyond the current bug fix?
We explicitly passed null here before and I think we have some code that expected that for the single entry preview styling. That was probably overloading the parameter, so the fix isn't to undo this but to make that single entry view explicit.

image.png

After getting past my dependency trouble I was able to build this branch locally and confirm that there are things we don't want in the single preview that were controlled by the lack of the publication decorator. Letter headers at least. I feared highlighting but I don't see that happening.

Copy link
Copy Markdown
Contributor

@jasonleenaylor jasonleenaylor left a comment

Choose a reason for hiding this comment

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

@jasonleenaylor reviewed 2 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on jtmaxwell3).

@jtmaxwell3 jtmaxwell3 merged commit 04f5d1c into main May 8, 2026
6 of 7 checks passed
@jtmaxwell3 jtmaxwell3 deleted the LT-11746b branch May 8, 2026 16:26
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