Skip to content

This fixes LT-22616: Word Cat contents change when bundle selected#1007

Open
jtmaxwell3 wants to merge 3 commits into
mainfrom
LT-22616
Open

This fixes LT-22616: Word Cat contents change when bundle selected#1007
jtmaxwell3 wants to merge 3 commits into
mainfrom
LT-22616

Conversation

@jtmaxwell3

@jtmaxwell3 jtmaxwell3 commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

This fixes https://jira.sil.org/browse/LT-22616. The root problem was that SandboxBase and InterlinVc were making different guesses about the word cat and gloss, which confused the user. The general principle is that when you first click on a word in the interlinear text, the sandbox should show whatever was showing in the interlinear text. The word cat and gloss should only change after the user makes a change to the sandbox.

I also added code to guess the word gloss and category whenever the morphemes change.


This change is Reviewable

Devin's analysis:

The interlinear display (InterlinVc) and the editing sandbox (SandboxBase) had independent implementations for guessing word glosses and parts of speech from morph bundles. This caused inconsistencies—the display could show one guess while the sandbox applied a different one.
This PR:
Extracts the private guess methods from an inner class of InterlinVc into internal static methods at the InterlinVc class level, returning richer types (ILexSense, IPartOfSpeech) instead of just display strings.
Adds SandboxBase.GuessWordCatAndGloss which delegates to those same InterlinVc methods, ensuring consistent guesses.
Renames CopyLexEntryInfoToMonomorphemicWordGlossAndPos → CopyLexEntryInfoToWordGlossAndPosIfEmpty and adjusts CopySenseToWordGloss to accept a real HVO directly.
After morpheme updates in HandleSelectMorphComboItem, calls CopyLexEntryInfoToWordGlossAndPosIfEmpty to refresh guesses.

@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown

NUnit Tests

    1 files  ± 0      1 suites  ±0   10m 8s ⏱️ + 1m 58s
4 313 tests +11  4 240 ✅ +49  73 💤 ±0  0 ❌  - 38 
4 322 runs  +11  4 249 ✅ +49  73 💤 ±0  0 ❌  - 38 

Results for commit 14fb9e9. ± Comparison against base commit dda17ab.

♻️ This comment has been updated with latest results.

@codecov-commenter

codecov-commenter commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 49.59350% with 62 lines in your changes missing coverage. Please review.
✅ Project coverage is 33.03%. Comparing base (d4e14ac) to head (14fb9e9).
⚠️ Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
Src/LexText/Interlinear/InterlinVc.cs 38.88% 40 Missing and 15 partials ⚠️
Src/LexText/Interlinear/SandboxBase.cs 81.25% 4 Missing and 2 partials ⚠️
...c/LexText/Interlinear/SandboxBase.ComboHandlers.cs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1007      +/-   ##
==========================================
+ Coverage   32.96%   33.03%   +0.07%     
==========================================
  Files        1202     1202              
  Lines      278291   278272      -19     
  Branches    37166    37177      +11     
==========================================
+ Hits        91733    91929     +196     
+ Misses     158688   158491     -197     
+ Partials    27870    27852      -18     
Files with missing lines Coverage Δ
...c/LexText/Interlinear/SandboxBase.ComboHandlers.cs 43.37% <0.00%> (ø)
Src/LexText/Interlinear/SandboxBase.cs 36.73% <81.25%> (+0.01%) ⬆️
Src/LexText/Interlinear/InterlinVc.cs 46.64% <38.88%> (+1.47%) ⬆️

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

@jasonleenaylor

Copy link
Copy Markdown
Contributor

Src/LexText/Interlinear/SandboxBase.cs line 4264 at r1 (raw file):

			{
				m_editMonitor.DoPendingMorphemeUpdates();
				CopyLexEntryInfoToMonomorphemicWordGlossAndPos(true);

If a user has manually chosen a word gloss and word category will this overwrite them when they choose a different msa? I'm not sure that is desired if that is the case.

@jtmaxwell3

Copy link
Copy Markdown
Collaborator Author

@jasonleenaylor CopyLexEntryInfoToMonomorphemicWordGlossAndPos is poorly named because it only does the copy if the word gloss and/or category don't already exist. It should be something like GuessWordGlossAndPos.

@jtmaxwell3
jtmaxwell3 marked this pull request as draft July 15, 2026 16:56
@jtmaxwell3
jtmaxwell3 marked this pull request as ready for review July 15, 2026 20:29
@jasonleenaylor

Copy link
Copy Markdown
Contributor

Src/LexText/Interlinear/InterlinVc.cs line 2402 at r2 (raw file):

				}
			}
			if (stemPOS == null)

For clear developer understanding of this statement a comment would be good. With domain knowledge in mind I think this is probably correct, but just reading the code I would wonder "What if we have a derivedPOS and no stemPOS, shouldn't we just return the derived?"

@jasonleenaylor jasonleenaylor 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.

I had one small comment request otherwise this change seems sound to me.
We want to make sure this is really hitting the felt user need of consistency between the focus box and the other sandbox views.
It seems like it does that, hopefully this will be an acceptable way.

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

@jtmaxwell3 jtmaxwell3 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@jtmaxwell3 made 2 comments.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on jtmaxwell3).


Src/LexText/Interlinear/InterlinVc.cs line 2402 at r2 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

For clear developer understanding of this statement a comment would be good. With domain knowledge in mind I think this is probably correct, but just reading the code I would wonder "What if we have a derivedPOS and no stemPOS, shouldn't we just return the derived?"

I didn't write this code, I just moved it. I don't feel comfortable adding a comment about what it is supposed to do.


Src/LexText/Interlinear/SandboxBase.cs line 4264 at r1 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

If a user has manually chosen a word gloss and word category will this overwrite them when they choose a different msa? I'm not sure that is desired if that is the case.

I renamed the function to make it clear that it doesn't overwrite the word gloss and word category.

@jasonleenaylor

Copy link
Copy Markdown
Contributor

Src/LexText/Interlinear/InterlinVc.cs line 2364 at r2 (raw file):

				return null;
			}
			// This is modeled after SandboxBase.SyncMonomorphemicGlossAndPos.
// Guess the word category from the morphemes' MSAs, matching
// SyncMonomorphemicGlossAndPos so the display and the sandbox agree:
//   - if there is more than one stem and they have different parts of
//     speech, give up (return null);
//   - if there is more than one derivational affix (DA), give up;
//   - if there is no stem, give up — a derived-to category is only
//     meaningful relative to the stem it attaches to, so a lone DA is
//     not enough to guess from;
//   - otherwise use the DA's 'to' POS if there is one, else the stem's POS.
//     (we don't insist that the DA's 'from' POS matches the stem)

Code quote:

// This is modeled after SandboxBase.SyncMonomorphemicGlossAndPos.

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