Skip to content

Conversation

siltomato
Copy link
Collaborator

@siltomato siltomato commented Sep 17, 2025

This PR fixes an issue where the offset for the insert that is passed to the lynx library for potential smart-quote swap detection was incorrect due to a blank embed being removed when starting a quote at the start of a verse.

The solution involves adding a call sequence count to detect possible changes that may have occurred during await. If the sequence count changed, the call is aborted, as the subsequent call will delta.compose() with the aborted call's delta, and the offset will be recalculated from the resulting delta.

This PR doesn't address the unmatched single-quote issue, as that behavior may be by design (or could be improved in lynx library).


This change is Reviewable

@siltomato siltomato added the will require testing PR should not be merged until testers confirm testing is complete label Sep 17, 2025
Copy link

codecov bot commented Sep 17, 2025

Codecov Report

❌ Patch coverage is 66.66667% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.19%. Comparing base (7b8371e) to head (6887af2).
⚠️ Report is 11 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ate/editor/lynx/insights/lynx-workspace.service.ts 66.66% 12 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3450      +/-   ##
==========================================
- Coverage   82.20%   82.19%   -0.01%     
==========================================
  Files         611      611              
  Lines       36433    36449      +16     
  Branches     5980     5984       +4     
==========================================
+ Hits        29951    29961      +10     
- Misses       5621     5627       +6     
  Partials      861      861              

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

@siltomato siltomato marked this pull request as ready for review September 17, 2025 22:02
@RaymondLuong3 RaymondLuong3 self-assigned this Sep 18, 2025
Copy link
Collaborator

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

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

The code and the reasoning look good, but I am not able to get the punctuation to automatically be corrected. I type the sentence

“It’s not that I"m so smart, it"s just that I stay with problems longer.'

But the auto-correct doesn't work. Am I doing something wrong? I have the lynx auto-correct enabled.

@RaymondLuong3 reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @siltomato)


src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-workspace.service.ts line 207 at r1 (raw file):

      // 'offset' may be stale here. For example, a blank embed may have been auto-deleted while awaiting the doc.
      // Check if a newer call has superseded this one while we were awaiting the document.

Thanks for the helpful comments!

Code quote:

      // 'offset' may be stale here. For example, a blank embed may have been auto-deleted while awaiting the doc.
      // Check if a newer call has superseded this one while we were awaiting the document.

@siltomato siltomato force-pushed the fix/sf-3553-lynx-autocorrect-issues branch from 7868c6a to 6887af2 Compare September 22, 2025 05:49
Copy link
Collaborator Author

@siltomato siltomato left a comment

Choose a reason for hiding this comment

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

Are you getting any auto conversion to smart quotes? Single quotes do not always convert due to the current rules for detecting apostrophes. There is is an open issue for improving single quotes in the lynx lib repo. The double quotes, however, should convert. Is there a book/chapter/verse on Stp22 project that you can reproduce this?

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @siltomato)

Copy link
Collaborator

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

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

I see the problem. It was because I had not embeds in my chapter. I can see the double quotes being converted now.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @siltomato)

Copy link
Collaborator

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

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

Oh, hold on, when I am typing single quotes, the first gets the opening quote, but the ending quote remains a straight quote.
image.png

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @siltomato)

Copy link
Collaborator Author

@siltomato siltomato left a comment

Choose a reason for hiding this comment

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

Yes, that is an issue. It is somewhat by design, but perhaps the rules could be improved. I have tracked it in the lynx lib repo: sillsdev/lynx#47

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @siltomato)

Copy link
Collaborator

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation. Since that is a known behaviour, I'll go ahead and move this to testing.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @siltomato)

@RaymondLuong3 RaymondLuong3 added ready to test and removed will require testing PR should not be merged until testers confirm testing is complete labels Sep 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants