Fix inline predictive text and keyboard suggestion toolbar use cases.

[langleyd]

This PR aims to fix a number of issues with predictive suggestions(both inline and in the toolbar) causing in consistent state between RTE model and text view content. Uses cases tested include:

• tapping on the suggestion to accept it
• using spacebar to accept it
• using backspace when on the boundary of a suggestion
• Replacing a selected word with one from the keyboard suggestion toolbar

It should help us address the problems in:
element-hq/element-x-ios#2345

This issues arise due to the old reconciliate solution.

The Problem:

In the simpler editor use cases one can assume keystokes and selection events are reflected in both the text view an the internal RTE representation and match.
This is not always the case though, fancy features like inline predictive suggestions, text dictation and keyboards for non-latin based languages often dump "pending" text updates into the text views text storage(an attributed string). This mismatch in content makes reconciling the rte and text view content and the selection indices difficult.

The old solution (reconciliate with string differ):

• The text view is the source of truth
• When it is different from our RTE content, we calculate the difference between them and try and insert it.
• This only works if there was a simple difference located in one place(which is generally the case for some of the use cases mentioned above). Any more complex differences meant it it errors out and tries to restore to a previous state.
• This solution works for some of he main autocomplete use cases but was a bit prone to bugs and didn't cover all cases.

The new solution:

• One fortunate behaviour of the iOS text view apis is that the shouldChangeTextIn delegate function is called when text is committed to the text view. So for example when the predictive text is still grey, it will be reflected in textVIew.attributedText, but crucially shouldChangeTextIn will not be called until the suggestion is actually accepted(say by tapping on it).
• The new solution creates a new text storage in the form of committedAttributedText which represents the text that has been committed. So in addition to applying mutations to text view content to be reflected in the UI, we also apply them to committedAttributedText.
• In committedAttributedText we have a way to determine when there is text in the editor that has not yet been committed by the user(e.g in line predictive text).
• This combination of data allows us to better address some of the edge cases on the uses cases mentioned at the top.

Edit: We determined using this committedAttributedText works well for latin character languages which make sensible use of the shouldChangeTextIn api for committed text. Some other languages like Japanese Kana however do not, and will substitute characters without sensibly notifying via the apis. We therefore we have kept the reconciliate function and fall back to it if the content of the editor changes beyond simple latin character mutations.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 62.50000% with 57 lines in your changes missing coverage. Please review.

Project coverage is 89.24%. Comparing base (7488319) to head (3fc7e64).
Report is 3 commits behind head on main.

Files	Patch %	Lines
...WysiwygComposerView/WysiwygComposerViewModel.swift	57.62%	50 Missing ⚠️
...ents/WysiwygComposerView/WysiwygComposerView.swift	0.00%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #890      +/-   ##
==========================================
- Coverage   90.32%   89.24%   -1.08%     
==========================================
  Files         177      160      -17     
  Lines       21173    20725     -448     
  Branches      280      280              
==========================================
- Hits        19124    18496     -628     
- Misses       2046     2226     +180     
  Partials        3        3              

Flag	Coverage Δ
uitests	?
uitests-ios	?
unittests	89.24% <62.50%> (-0.19%)	⬇️
unittests-ios	87.32% <62.50%> (-1.37%)	⬇️
unittests-react	88.66% <ø> (ø)
unittests-rust	89.67% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[nimau]

@langleyd, the "." shortcut seems problematic as it doesn't call func textView(_ textView: UITextView, shouldChangeTextIn range: NSRange, replacementText text: String) -> Bool. So the '.' is present in the textview but disappears once you delete something.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[aringenbach]

@langleyd, the "." shortcut seems problematic as it doesn't call func textView(_ textView: UITextView, shouldChangeTextIn range: NSRange, replacementText text: String) -> Bool. So the '.' is present in the textview but disappears once you delete something.

AFAIK any language that magically replace latin characters with symbols, have the exact same behaviour as the "." shortcut.

[langleyd]

I did test a number of CJK keyboards and from my initial tests I couldn't break it.

On testing again though, I can see that although shouldChangeTextIn does seem to be called (unlike the "." shortcut), the index in some cases is not correct. For example of you use the Koran 10 key keyboard and cycle through the characters by pressing 1 key multiple times, it replaces the character in the editor but adds each one in the RTE state as the index returned does not really seem correct. :/

[aringenbach]

@langleyd I'm not sure anymore about Korean keyboards behaviour, they might have less issues than some of the others.
I think you can try with Japanese Romaji Qwerty for example, this one had lots of issues, and most system events will not get triggered, just magically replaces the text.

[Velin92]

do we still need this?

[langleyd]

I think it is unless we have a sensible way to reduce the complexity of the function, the warning still exists when I remove this.

[Velin92]

okay I see then I would suggest replacing it with // swiftlint:disable:next cyclomatic_complexity which will only suppress the next warning, this line may disable the warning everywhere in the codebase

[langleyd]

The moving back of the dot after accepting inline text prediction was an optimisation brought in in 17.5. I was able to verify outside of out app(e.g. in the reminders app). This is why it was failing in the CI(17.2) but not for me locally(17.5).

I've added the version check, so should be all good now.

[Velin92]

the only thing left is to change the cyclomatic complexity part @langleyd

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud