SF-3437 Remove the blank: true properties from the texts data model #3339

pmachapman · 2025-07-30T00:12:20Z

This PR moves the blank: true ops from the texts data model to the view model.

It also disables editing on older version of the Scripture Forge client, requiring them to update to the latest version before editing resumes.

The frontend can update text docs that either do or do not contain blank: true ops. This means that there is no need to migrate all text documents, and that restoring older text documents will still work correctly.

This change is

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/text-view-model.ts

codecov · 2025-07-30T00:21:15Z

Codecov Report

❌ Patch coverage is 91.24424% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.15%. Comparing base (f4c61a0) to head (25c63ab).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...e/ClientApp/src/app/shared/text/text-view-model.ts	84.16%	15 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3339      +/-   ##
==========================================
- Coverage   82.21%   82.15%   -0.07%     
==========================================
  Files         608      608              
  Lines       36199    36299     +100     
  Branches     5935     5957      +22     
==========================================
+ Hits        29761    29821      +60     
- Misses       5563     5619      +56     
+ Partials      875      859      -16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/text-view-model.ts

RaymondLuong3

This is amazing. I see that when I synchronize my project, the data is updated to not have the blanks. I was able to add a draft to my project. I worked with multiple windows open. I tested offline and online. Using notes. Synchronizing. I am really impressed that it handled everything.
The only thing I found was that without the blank segments, SF cannot accurately calculate the segments that need to be translated. And example is on the translate overview page where all books are now at 100% complete. My Exodus is almost entirely empty, but this shows that it is completely translated.

@RaymondLuong3 reviewed 26 of 31 files at r1, 2 of 4 files at r2, 10 of 10 files at r3, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @pmachapman)

src/SIL.XForge.Scripture/ClientApp/src/app/core/text-doc.service.ts line 117 at r3 (raw file):

   */
  isUpdateRequired(project: SFProjectProfile | undefined): boolean {
    return (project?.editingRequires ?? 0) > MaxSupportedEditingRequiresValue;

Maybe I am not understanding this correctly, but I would have expected that isUpdateRequired() should return true if editingRequires < MaxSupportedEditingRequiresValue

Code quote:

    return (project?.editingRequires ?? 0) > MaxSupportedEditingRequiresValue;

src/SIL.XForge.Scripture/ClientApp/src/app/shared/test-utils.ts line 166 at r3 (raw file):

  delta.insert({ verse: { number: '3', style: 'v' } });
  if (modelHasBlanks) delta.insert({ blank: true }, { segment: 'verse_1_3' });
  delta.insert('\n\n', { para: { style: 'p' } });

What is the reason for the double newline?

Code quote:

  delta.insert('\n\n', { para: { style: 'p' } });

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.spec.ts line 159 at r3 (raw file):

describe('EditorComponent', () => {
  [true, false].forEach(modelHasBlanks => {

I wonder if we should just move to support only models without blank embeds. It would simplify testing. Oh hey, I see. We do need to support the case where blanks are present in the data model because they are only removed after the project is synchronized with Paratext.

Code quote:

  [true, false].forEach(modelHasBlanks => {

src/SIL.XForge.Scripture/ClientApp/src/app/core/text-doc.service.spec.ts line 83 at r3 (raw file):

      const env = new TestEnvironment();
      const project = createTestProjectProfile({
        editingRequires: EditingRequires.ParatextEditingEnabled | EditingRequires.ViewModelBlankSupport,

Nit: It looks like this is the default value for editingRequires so this is not necessary.

Code quote:

editingRequires: EditingRequires.ParatextEditingEnabled | EditingRequires.ViewModelBlankSupport,

src/SIL.XForge.Scripture/ClientApp/src/app/core/text-doc.service.spec.ts line 263 at r3 (raw file):

    });

    it('should return true if the project is has been upgraded to a version beyond the supported version', () => {

Typo

Code quote:

project is has been upgraded

src/SIL.XForge.Scripture/ClientApp/src/app/core/text-doc.service.spec.ts line 274 at r3 (raw file):

    });

    it('should return true if the project is has not been upgraded to view model support', () => {

Typo

Code quote:

project is has not been upgrade

pmachapman

The only thing I found was that without the blank segments, SF cannot accurately calculate the segments that need to be translated. And example is on the translate overview page where all books are now at 100% complete. My Exodus is almost entirely empty, but this shows that it is completely translated.

Yes, that was starting to dawn on me as I was writing another PR (SF-2518 / #3384). I haven't written a fix for this yet, as I need to have a think on it, and it possibly overlaps with a change request that emerged from the Paratext realtime editing work. I will re-request your review when I have a solution.

@pmachapman dismissed @github-advanced-security[bot] from 3 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @RaymondLuong3)

src/SIL.XForge.Scripture/ClientApp/src/app/core/text-doc.service.ts line 117 at r3 (raw file):