Refactor quill-scripture.ts to allow testing

[siltomato]

This PR refactors quill-scripture.ts into several files to improve testability. Unit tests were created. Claude 3.5 sonnet heavily used in developing unit tests.

I marked the PR as 'testing not required', as blots/attributors were not modified other than moving, but if someone thinks otherwise, feel free to change this.

---

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 93.69369% with 21 lines in your changes missing coverage. Please review.

Project coverage is 82.89%. Comparing base (3ebdc9a) to head (cf0d8be).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...l-editor-registration/quill-formats/quill-blots.ts	95.07%	8 Missing and 2 partials ⚠️
...ed/text/quill-editor-registration/quill-history.ts	86.66%	8 Missing ⚠️
.../text/quill-editor-registration/quill-clipboard.ts	84.21%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3032      +/-   ##
==========================================
+ Coverage   82.67%   82.89%   +0.21%     
==========================================
  Files         553      558       +5     
  Lines       32044    32010      -34     
  Branches     5189     5190       +1     
==========================================
+ Hits        26492    26534      +42     
+ Misses       4782     4708      -74     
+ Partials      770      768       -2     

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

[RaymondLuong3]

Thanks for the changes. Splitting this out will help us track down problems and improve testing. I did see one problem in my testing. When I pasted text with 2 \n characters I think only the first newLine symbol was stripped out. The resulting text doc ops liked like this.

Reviewed 19 of 19 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @siltomato)

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-editor-registration/quill-history.ts line 107 at r1 (raw file):

  // skip inserted embeds when determining last edit
  let changeIndex = 0;
  let curIndex = 0;

What is the reason for changeIndex? It seems that we can just track curIndex and return that.

Code quote:

  let curIndex = 0;

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-editor-registration/quill-clipboard.spec.ts line 25 at r1 (raw file):

    mockRange = { index: 5, length: 0 };
    mockFormat = { segment: 'verse-1', bold: true };

Let's make this a valid segment. It should look like verse_1_1

Code quote:

'verse-1',

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-editor-registration/quill-clipboard.spec.ts line 105 at r1 (raw file):

    ];

    invalidCases.forEach(({ name, setup }) => {

This is an interesting way of writing multiple tests. I think this should work. Was this AI generated?

Code quote:

 invalidCases.forEach(({ name, setup }) => {

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-editor-registration/quill-formats/quill-blots.ts line 326 at r1 (raw file):

}

export class UnmatchedEmbed extends QuillEmbedBlot {

Am I understanding correctly that this is the Embed that will match anything that we do not have a specific embed for (i.e. jump links)? I still see the problem with the unable to create link blot error on TNN.

Code quote:

export class UnmatchedEmbed extends QuillEmbedBlot {

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-editor-registration/quill-history.spec.ts line 64 at r1 (raw file):

        name: 'commenter selection removal',
        input: new Delta().insert('text', {
          segment: 'verse-1',

Let's update the segment refs here to be valid segment refs

Code quote:

     segment: 'verse-1'

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-editor-registration/quill-history.spec.ts line 128 at r1 (raw file):

    it('should clean up highlighting during undo', () => {
      const delta = new Delta().insert('text', { segment: 'verse-1' });

Let's fix this segment ref

Code quote:

      const delta = new Delta().insert('text', { segment: 'verse-1' });

[siltomato]

Good catch! This looks like the way that multiple newlines are currently handled. Should this fix be in a separate PR?

Reviewable status: 17 of 19 files reviewed, 5 unresolved discussions (waiting on @RaymondLuong3)

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-editor-registration/quill-clipboard.spec.ts line 25 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Let's make this a valid segment. It should look like verse_1_1

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-editor-registration/quill-clipboard.spec.ts line 105 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

This is an interesting way of writing multiple tests. I think this should work. Was this AI generated?

Yes! I used Claude 3.5 Sonnet heavily. It was interesting to see some new patterns that it generated.

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-editor-registration/quill-history.ts line 107 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

What is the reason for changeIndex? It seems that we can just track curIndex and return that.

You might be right. For this PR, I didn't want to change the utility functions--just move them. Would this change be better suited for another PR?

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-editor-registration/quill-history.spec.ts line 64 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Let's update the segment refs here to be valid segment refs

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-editor-registration/quill-history.spec.ts line 128 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Let's fix this segment ref

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-editor-registration/quill-formats/quill-blots.ts line 326 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Am I understanding correctly that this is the Embed that will match anything that we do not have a specific embed for (i.e. jump links)? I still see the problem with the unable to create link blot error on TNN.

UnmatchedEmbed is not a recent addition. From looking at DeltaUsxMapperTests.cs, it looks like it has something to do with an 'unmatched marker'. My understanding is lacking here.

As for a fallback blot, that is not in this PR, but I hope to get one working soon, and I'll be sure to check it on TNN. =)

[RaymondLuong3]

I think so, let's create a new issue for this.

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @siltomato)

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-editor-registration/quill-clipboard.spec.ts line 105 at r1 (raw file):

Previously, siltomato wrote…

Yes! I used Claude 3.5 Sonnet heavily. It was interesting to see some new patterns that it generated.

Nice, it does look like it does what I expected. I should explore AI generated test cases more

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-editor-registration/quill-history.ts line 107 at r1 (raw file):

Previously, siltomato wrote…

You might be right. For this PR, I didn't want to change the utility functions--just move them. Would this change be better suited for another PR?

Let's create a new PR for this to keep this one cleaner.

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-editor-registration/quill-formats/quill-blots.ts line 326 at r1 (raw file):

Previously, siltomato wrote…

UnmatchedEmbed is not a recent addition. From looking at DeltaUsxMapperTests.cs, it looks like it has something to do with an 'unmatched marker'. My understanding is lacking here.

As for a fallback blot, that is not in this PR, but I hope to get one working soon, and I'll be sure to check it on TNN. =)

Ah yes I see, it was already something we had.

[RaymondLuong3]

@Nateowami This is a fairly major refactor. Maybe you will want to skim over it too before merging this in.

[siltomato]

Created SF-3228 to track above issue.

Reviewable status: 17 of 19 files reviewed, all discussions resolved (waiting on @RaymondLuong3)

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-editor-registration/quill-history.ts line 107 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Let's create a new PR for this to keep this one cleaner.

On closer inspection, it looks like the current code is not counting trailing embeds ops in the change index. That is why changeIndex is not updated if op.insert is not a string. However, changeIndex will include embed ops in the count if there is a retain op or a string insert op after the embed op.

It looks like getLastChangeIndex was overridden from the quill source code to do this. Does this sound correct?

[siltomato]

Reviewable status: 17 of 19 files reviewed, all discussions resolved (waiting on @RaymondLuong3)

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-editor-registration/quill-history.ts line 107 at r1 (raw file):

Previously, siltomato wrote…

On closer inspection, it looks like the current code is not counting trailing embeds ops in the change index. That is why changeIndex is not updated if op.insert is not a string. However, changeIndex will include embed ops in the count if there is a retain op or a string insert op after the embed op.

It looks like getLastChangeIndex was overridden from the quill source code to do this. Does this sound correct?

@RaymondLuong3 Had some reviewable issues yesterday, so I wanted to make sure you saw my above comment.

[RaymondLuong3]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @siltomato)

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-editor-registration/quill-history.ts line 107 at r1 (raw file):

Previously, siltomato wrote…

@RaymondLuong3 Had some reviewable issues yesterday, so I wanted to make sure you saw my above comment.

Based on the description of the method, this was on purpose. So it looks like we want to keep this as is.

[RaymondLuong3]

Let's merge this, it looks ready. The regression tests on QA will determine whether there are any issues.