-
Notifications
You must be signed in to change notification settings - Fork 23
Fix inline predictive text and keyboard suggestion toolbar use cases. #890
Conversation
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@langleyd, the "." shortcut seems problematic as it doesn't call |
…for us with shouldChangeTextIn.
Kudos, SonarCloud Quality Gate passed! |
AFAIK any language that magically replace latin characters with symbols, have the exact same behaviour as the "." shortcut. |
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 |
@langleyd I'm not sure anymore about Korean keyboards behaviour, they might have less issues than some of the others. |
…into langleyd/fix_predictive_text_and_suggestions
…acters/whitespace.
…into langleyd/fix_predictive_text_and_suggestions
…siwygComposerViewModel. This is very unlikely to happen in a real world example.
Dictation can add characters like numbers and emojis. We don't want to use reconciliate for this, only for non-latin writing systems. Use the "Common" set
@@ -300,6 +311,7 @@ public extension WysiwygComposerViewModel { | |||
compressedHeight = min(maxCompressedHeight, max(minHeight, idealTextHeight)) | |||
} | |||
|
|||
// swiftlint:disable cyclomatic_complexity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we still need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
...mposer/Sources/WysiwygComposer/Components/WysiwygComposerView/WysiwygComposerViewModel.swift
Outdated
Show resolved
Hide resolved
...mposer/Sources/WysiwygComposer/Components/WysiwygComposerView/WysiwygComposerViewModel.swift
Outdated
Show resolved
Hide resolved
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. |
…oromito/cjk_testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the only thing left is to change the cyclomatic complexity part @langleyd
…rg/matrix-wysiwyg into mauroromito/cjk_testing
….com:matrix-org/matrix-rich-text-editor into langleyd/fix_predictive_text_and_suggestions
…//github.com/matrix-org/matrix-wysiwyg into langleyd/fix_predictive_text_and_suggestions
Quality Gate passedIssues Measures |
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:
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 new solution:
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 intextVIew.attributedText
, but cruciallyshouldChangeTextIn
will not be called until the suggestion is actually accepted(say by tapping on it).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 tocommittedAttributedText
.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).Edit: We determined using this
committedAttributedText
works well for latin character languages which make sensible use of theshouldChangeTextIn
api for committed text. Some other languages likeJapanese 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.