Skip to content
This repository has been archived by the owner on Sep 27, 2024. It is now read-only.

Fix inline predictive text and keyboard suggestion toolbar use cases. #890

Merged
merged 62 commits into from
Jul 15, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
62 commits
Select commit Hold shift + click to select a range
5239d95
Move from reconciliate to committed/uncommitted text.
langleyd Nov 24, 2023
817a9b1
Push uncommited changes.
langleyd Nov 24, 2023
0eb0577
Handle double space to dot conversion as the system does not do this …
langleyd Nov 24, 2023
3517e84
Merge branch 'main' of github.com:matrix-org/matrix-rich-text-editor …
langleyd Jun 12, 2024
b447ad1
Merge main and fix crash checkForDoubleSpaceToDotConversion
langleyd Jun 18, 2024
56c24e3
Put back reconciliate but don't use it if the diff is just latin char…
langleyd Jun 19, 2024
ba64d68
Remove comment
langleyd Jun 19, 2024
2a37312
Fix typos and move withNBSP
langleyd Jun 19, 2024
e1e3e94
Fix plain text mode.
langleyd Jun 19, 2024
a0e22cc
Fix selection + tests
langleyd Jun 19, 2024
fc41527
Merge branch 'main' of github.com:matrix-org/matrix-rich-text-editor …
langleyd Jul 5, 2024
3b49f18
Fix test that falls into the lastReplaceTextUpdate if statement in Wy…
langleyd Jul 7, 2024
c418d63
Fix clearing of plain text.
langleyd Jul 8, 2024
d0a6acc
Fix dictation, by correcting character check for reconciliate.
langleyd Jul 8, 2024
00e9667
added some tests
Velin92 Jul 10, 2024
9d538c6
added a script to force software keyboard
Velin92 Jul 10, 2024
2a15124
improve tests
Velin92 Jul 10, 2024
209c73a
fixed the tests
Velin92 Jul 10, 2024
c524832
test fix
Velin92 Jul 10, 2024
84bb1d0
added a test for deleting when text prediction is present
Velin92 Jul 10, 2024
865f967
improved the tests
Velin92 Jul 10, 2024
7314be8
better docs in tests
Velin92 Jul 10, 2024
fda08e0
improved the test
Velin92 Jul 10, 2024
e637ecb
Add comment to object replacement character test
langleyd Jul 11, 2024
eb70a6c
Add documentation for the replaceText return value
langleyd Jul 11, 2024
f9d1592
Fix naming of functions, clearer control flow and and re-enable cyclo…
langleyd Jul 11, 2024
df0275c
Add logging line and correct replaceText logic
langleyd Jul 12, 2024
c2a4783
Add another failing test for predictive text.
langleyd Jul 12, 2024
deb7c2c
keyboards based testing
Velin92 Jul 12, 2024
51b2090
moved the keyboard setup
Velin92 Jul 12, 2024
b143872
print the added keyboards
Velin92 Jul 12, 2024
0c93ce0
rework dot conversion and handle case of dot after an inline text pre…
langleyd Jul 12, 2024
49893ef
Merge branch 'langleyd/fix_predictive_text_and_suggestions' of github…
langleyd Jul 12, 2024
2f97a0e
moved the setup to the earliest possible
Velin92 Jul 12, 2024
f815b1c
Merge branch 'langleyd/fix_predictive_text_and_suggestions' of https:…
Velin92 Jul 12, 2024
9961ceb
fix
Velin92 Jul 12, 2024
c9c9e86
removed the use of the keyboard language change for now
Velin92 Jul 12, 2024
2f6e24b
restored keyboard based tests
Velin92 Jul 12, 2024
eb264df
adding language during the test itself!
Velin92 Jul 12, 2024
31d32b3
sped up the tests a bit
Velin92 Jul 12, 2024
9baac8e
better docs
Velin92 Jul 12, 2024
44edc08
Moving the dot back after predictive text was introduced in 17.5
langleyd Jul 12, 2024
ee0795e
Fix test.
langleyd Jul 12, 2024
d5bc8bd
Merge branch 'langleyd/fix_predictive_text_and_suggestions' into maur…
Velin92 Jul 12, 2024
13f6cda
better docs and improved the code
Velin92 Jul 15, 2024
bf53631
Merge branch 'mauroromito/cjk_testing' of https://github.com/matrix-o…
Velin92 Jul 15, 2024
81d7d8f
Merge pull request #1015 from matrix-org/mauroromito/cjk_testing
Velin92 Jul 15, 2024
ad8d163
fix
Velin92 Jul 15, 2024
1f06539
fix
Velin92 Jul 15, 2024
c7cd881
fix
Velin92 Jul 15, 2024
a961188
revert
Velin92 Jul 15, 2024
10942c6
added some delay for the CI
Velin92 Jul 15, 2024
282f746
fix cyclomatic_complexity check
langleyd Jul 15, 2024
dbafe83
Merge branch 'langleyd/fix_predictive_text_and_suggestions' of github…
langleyd Jul 15, 2024
9e810c8
possible fix by delay
Velin92 Jul 15, 2024
a499178
Merge branch 'langleyd/fix_predictive_text_and_suggestions' of https:…
Velin92 Jul 15, 2024
3fc7e64
more delay
Velin92 Jul 15, 2024
8f2a68b
possible fix
Velin92 Jul 15, 2024
0045c3b
disabled for now
Velin92 Jul 15, 2024
7868e2e
disabled for now
Velin92 Jul 15, 2024
f11702e
restored
Velin92 Jul 15, 2024
14f1889
improved the test code
Velin92 Jul 15, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,8 @@ extension NSMutableAttributedString {
/// - Returns: self (discardable)
@discardableResult
func removeDiscardableContent() -> Self {
discardableTextRanges().reversed().forEach {
replaceCharacters(in: $0, with: "")
for discardableTextRange in discardableTextRanges().reversed() {
replaceCharacters(in: discardableTextRange, with: "")
}

return self
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,9 @@ public struct WysiwygComposerView: View {

@ViewBuilder
private var placeholderView: some View {
if viewModel.isContentEmpty, !viewModel.textView.isDictationRunning {
// The content can be empty but the textview not, e.g. if you start dictation
// but have not committed the text yet.
if viewModel.textView.attributedText.length == 0 {
Text(placeholder)
.font(Font(UIFont.preferredFont(forTextStyle: .body)))
.foregroundColor(placeholderColor)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,13 @@ public class WysiwygComposerViewModel: WysiwygComposerViewModelProtocol, Observa
let textView = WysiwygTextView()
textView.linkTextAttributes[.foregroundColor] = parserStyle.linkColor
textView.mentionDisplayHelper = mentionDisplayHelper
textView.apply(attributedContent)
textView.apply(attributedContent, committed: &committedAttributedText)
return textView
}() {
didSet {
textView.linkTextAttributes[.foregroundColor] = parserStyle.linkColor
textView.mentionDisplayHelper = mentionDisplayHelper
textView.apply(attributedContent)
textView.apply(attributedContent, committed: &committedAttributedText)
}
}

Expand Down Expand Up @@ -135,7 +135,18 @@ public class WysiwygComposerViewModel: WysiwygComposerViewModelProtocol, Observa
}

private(set) var hasPendingFormats = false


/// This is used to track the text commited to the editor by the user, as opposed to text
/// that could be in the editor that is not yet committed (e.g. from inline predictive text or dictation ).
private lazy var committedAttributedText = NSAttributedString(string: "", attributes: defaultTextAttributes)

private var lastReplaceTextUpdate: ReplaceTextUpdate?

/// Wether the view contains uncommitted text(e.g. a predictive suggestion is shown in grey).
private var viewHasUncommitedText: Bool {
Velin92 marked this conversation as resolved.
Show resolved Hide resolved
textView.attributedText.htmlChars.withNBSP != committedAttributedText.htmlChars.withNBSP
}

// MARK: - Public

public init(minHeight: CGFloat = 22,
Expand Down Expand Up @@ -300,6 +311,7 @@ public extension WysiwygComposerViewModel {
compressedHeight = min(maxCompressedHeight, max(minHeight, idealTextHeight))
}

// swiftlint:disable cyclomatic_complexity
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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

func replaceText(range: NSRange, replacementText: String) -> Bool {
guard shouldReplaceText else {
return false
Expand All @@ -309,28 +321,57 @@ public extension WysiwygComposerViewModel {
return true
}

let nextTextUpdate = ReplaceTextUpdate(date: Date.now, range: range, text: replacementText)
// This is to specifically to work around an issue when tapping on an inline predictive text suggestion within the text view.
// Even though we have the delegate disabled during modifications to the textview we still get some duplicate
// calls to this method in this case specifically. It's very unlikely we would get a valid subsequent call
// with the same range and replacement text within such a short period of time, so should be safe.
if let lastReplaceTextUpdate, lastReplaceTextUpdate.isDuplicate(of: nextTextUpdate) {
return true
}

// This fixes a bug where the attributed string keeps link and inline code formatting
// when they are the last formatting to be deleted
if textView.attributedText.length == 0 {
textView.typingAttributes = defaultTextAttributes
}

let update: ComposerUpdate
let shouldAcceptChange: Bool
let skipTextViewUpdate: Bool

// The system handles certain auto-compelete use-cases with somewhat unusual replacementText/range
// combinations, some of those edge cases are handled below.

Velin92 marked this conversation as resolved.
Show resolved Hide resolved
// Are we backspacing from an inline predictive text suggestion.
// When this happens a range/replacementText of this combination is sent.
let isExitingPredictiveText = replacementText == ""
&& viewHasUncommitedText
&& range == attributedContent.selection && range.length == 0

// Are we replacing some selected text by tapping the suggestion toolbar
// When this happens a range/replacementText of this combination is sent.
let isReplacingWordWithSuggestion = replacementText == "" && !viewHasUncommitedText && range.length == 0

let isNormalBackspace = attributedContent.selection.length == 0 && replacementText == ""

// A no-op rte side is required here
if isReplacingWordWithSuggestion {
return true
}

if range != attributedContent.selection {
select(range: range)
}

if attributedContent.selection.length == 0, replacementText == "" {
if isNormalBackspace || isExitingPredictiveText {
update = model.backspace()
shouldAcceptChange = false
skipTextViewUpdate = false
} else if replacementText.count == 1, replacementText[String.Index(utf16Offset: 0, in: replacementText)].isNewline {
update = createEnterUpdate()
shouldAcceptChange = false
skipTextViewUpdate = false
} else {
update = model.replaceText(newText: replacementText)
shouldAcceptChange = true
skipTextViewUpdate = true
}

// Reconciliates the model with the text any time the link state changes
Expand All @@ -340,18 +381,18 @@ public extension WysiwygComposerViewModel {
case let .update(newState):
if newState[.link] != actionStates[.link] {
applyUpdate(update, skipTextViewUpdate: true)
textView.apply(attributedContent)
applyAtributedContent()
updateCompressedHeightIfNeeded()
return false
}
default: break
}

applyUpdate(update, skipTextViewUpdate: shouldAcceptChange)

return shouldAcceptChange
applyUpdate(update, skipTextViewUpdate: skipTextViewUpdate)
lastReplaceTextUpdate = nextTextUpdate
return skipTextViewUpdate
Velin92 marked this conversation as resolved.
Show resolved Hide resolved
}

func select(range: NSRange) {
do {
guard let text = textView.attributedText, !plainTextMode else { return }
Expand All @@ -378,13 +419,50 @@ public extension WysiwygComposerViewModel {
}
plainTextContent = textView.attributedText
} else {
reconciliateIfNeeded()
let dotInserted = checkForDoubleSpaceToDotConversion()
if !dotInserted {
reconciliateIfNeeded()
}
Velin92 marked this conversation as resolved.
Show resolved Hide resolved
applyPendingFormatsIfNeeded()
}

updateCompressedHeightIfNeeded()
}

func checkForDoubleSpaceToDotConversion() -> Bool {
let text = textView.attributedText.htmlChars.withNBSP
guard text.count > 0 else {
return false
}
let content = attributedContent.text.htmlChars.withNBSP
let dotStart = textView.selectedRange.location - 1
let dotEnd = textView.selectedRange.location
guard dotStart >= 0, dotEnd <= text.count else {
return false
}
let dotStartIndex = text.index(text.startIndex, offsetBy: dotStart)
let dotEndIndex = text.index(after: dotStartIndex)
guard
dotStartIndex < text.endIndex,
dotEndIndex <= text.endIndex,
dotStartIndex < content.endIndex,
dotEndIndex <= content.endIndex
else {
return false
}
let dotRange = dotStartIndex..<dotEndIndex
let textPotentialDot = String(text[dotRange])
let contentPotentialDot = String(content[dotRange])
if textPotentialDot == ".", contentPotentialDot != "." {
let replaceUpdate = model.replaceTextIn(newText: ".",
start: UInt32(dotStart),
end: UInt32(dotEnd))
applyUpdate(replaceUpdate, skipTextViewUpdate: true)
return true
Velin92 marked this conversation as resolved.
Show resolved Hide resolved
}
return false
}

func applyLinkOperation(_ linkOperation: WysiwygLinkOperation) {
let update: ComposerUpdate
switch linkOperation {
Expand Down Expand Up @@ -422,6 +500,10 @@ public extension WysiwygComposerViewModel {
return CGSize(width: width,
height: maximised ? maxExpandedHeight : min(maxCompressedHeight, max(minHeight, idealHeight)))
}

func applyAtributedContent() {
textView.apply(attributedContent, committed: &committedAttributedText)
}
}

// MARK: - Private
Expand All @@ -444,8 +526,12 @@ private extension WysiwygComposerViewModel {
applyReplaceAll(codeUnits: codeUnits, start: start, end: end)
// Note: this makes replaceAll act like .keep on cases where we expect the text
// view to be properly updated by the system.
if !skipTextViewUpdate {
textView.apply(attributedContent)
if skipTextViewUpdate {
// We skip updating the text view as the system did that for us but that
// is not reflected in committedAttributedText yet, so update it.
committedAttributedText = attributedContent.text
} else {
applyAtributedContent()
updateCompressedHeightIfNeeded()
}
case let .select(startUtf16Codeunit: start,
Expand Down Expand Up @@ -560,15 +646,23 @@ private extension WysiwygComposerViewModel {
plainTextContent = NSAttributedString()
}
}

/// Reconciliate the content of the model with the content of the text view.
func reconciliateIfNeeded() {
do {
guard !textView.isDictationRunning,
let replacement = try StringDiffer.replacement(from: attributedContent.text.htmlChars,
to: textView.attributedText.htmlChars) else {
to: textView.attributedText.htmlChars)
else {
Velin92 marked this conversation as resolved.
Show resolved Hide resolved
return
}

// Don't use reconciliate if the replacement is only latin character languages
// as it shouldn't be needed. It is needed for CJK lanuages like Japanese Kana.
if replacement.text.containsLatinAndCommonCharactersOnly {
return
}

// Reconciliate
Logger.viewModel.logDebug(["Reconciliate from \"\(attributedContent.text.string)\" to \"\(textView.text ?? "")\""],
functionName: #function)
Expand All @@ -588,7 +682,7 @@ private extension WysiwygComposerViewModel {
case StringDifferError.tooComplicated,
StringDifferError.insertionsDontMatchRemovals:
// Restore from the model, as otherwise the composer will enter a broken state
textView.apply(attributedContent)
applyAtributedContent()
updateCompressedHeightIfNeeded()
Logger.viewModel.logError(["Reconciliate failed, content has been restored from the model"],
functionName: #function)
Expand All @@ -607,8 +701,7 @@ private extension WysiwygComposerViewModel {
/// to apply (e.g. we hit the bold button with no selection).
func applyPendingFormatsIfNeeded() {
guard hasPendingFormats else { return }

textView.apply(attributedContent)
applyAtributedContent()
updateCompressedHeightIfNeeded()
hasPendingFormats = false
}
Expand All @@ -618,7 +711,8 @@ private extension WysiwygComposerViewModel {
/// - Returns: A markdown string.
func computeMarkdownContent() -> String {
let markdownContent: String
if let mentionReplacer, let attributedText = textView.attributedText {
if let mentionReplacer,
let attributedText = textView.attributedText {
// `MentionReplacer` should restore altered content to valid markdown.
markdownContent = mentionReplacer.restoreMarkdown(in: attributedText)
} else {
Expand Down Expand Up @@ -656,3 +750,18 @@ extension WysiwygComposerViewModel: ComposerModelWrapperDelegate {
private extension Logger {
static let viewModel = Logger(subsystem: subsystem, category: "ViewModel")
}

private struct ReplaceTextUpdate {
static let debounceThreshold = 0.1
var date: Date
var range: NSRange
var text: String
}

private extension ReplaceTextUpdate {
func isDuplicate(of other: ReplaceTextUpdate) -> Bool {
range == other.range
&& text == other.text
&& fabs(date.timeIntervalSince(other.date)) < Self.debounceThreshold
Velin92 marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ public protocol MentionDisplayHelper { }

public class WysiwygTextView: UITextView {
private(set) var isDictationRunning = false

/// Internal delegate for the text view.
weak var wysiwygDelegate: WysiwygTextViewDelegate?

Expand Down Expand Up @@ -72,9 +71,6 @@ public class WysiwygTextView: UITextView {
}

private func commonInit() {
if #available(iOS 17.0, *) {
inlinePredictionType = .no
}
contentMode = .redraw
NotificationCenter.default.addObserver(self,
selector: #selector(dictationDidStart),
Expand Down Expand Up @@ -115,13 +111,15 @@ public class WysiwygTextView: UITextView {
///
/// - Parameters:
/// - content: Content to apply.
func apply(_ content: WysiwygComposerAttributedContent) {
func apply(_ content: WysiwygComposerAttributedContent, committed: inout NSAttributedString) {
guard content.text.length == 0
|| content.text != attributedText
|| content.selection != selectedRange
else { return }

performWithoutDelegate {
// Update committed text at the same time we update the text view.
committed = content.text
self.attributedText = content.text
// Set selection to {0, 0} then to expected position
// avoids an issue with autocapitalization.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,13 @@ extension String {
var utf16Length: Int {
(self as NSString).length
}

/// Converts all whitespaces to NBSP to avoid diffs caused by HTML translations.
var withNBSP: String {
String(map { $0.isWhitespace ? Character.nbsp : $0 })
}

var containsLatinAndCommonCharactersOnly: Bool {
range(of: "[^\\p{Latin}\\p{Common}]", options: .regularExpression) == nil
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,6 @@ private struct StringDiff {
}

private extension String {
/// Converts all whitespaces to NBSP to avoid diffs caused by HTML translations.
var withNBSP: String {
String(map { $0.isWhitespace ? Character.nbsp : $0 })
}

/// Computes the diff from provided string to self. Outputs UTF16 locations and lengths.
///
/// - Parameter otherString: Other string to compare.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -206,11 +206,11 @@ final class WysiwygComposerViewModelTests: XCTestCase {
func testPendingFormatFlagAfterReselectingListItem() {
viewModel.apply(.bold)
viewModel.apply(.italic)
mockTrailingTyping("Text")
mockTrailingTyping("Text1")
viewModel.enter()
viewModel.apply(.orderedList)
let inListSelection = viewModel.attributedContent.selection
let insertedText = "Text"
let insertedText = "Text2"
mockTyping(insertedText, at: 0)
// After re-selecting the empty list item, pending format flag is still on
viewModel.select(range: NSRange(location: inListSelection.location + insertedText.utf16Length,
Expand Down Expand Up @@ -280,7 +280,7 @@ extension WysiwygComposerViewModelTests {
let shouldAcceptChange = viewModel.replaceText(range: range, replacementText: text)
if shouldAcceptChange {
// Force apply since the text view should've updated by itself
viewModel.textView.apply(viewModel.attributedContent)
viewModel.applyAtributedContent()
viewModel.didUpdateText()
}
}
Expand All @@ -304,7 +304,7 @@ extension WysiwygComposerViewModelTests {
let shouldAcceptChange = viewModel.replaceText(range: range, replacementText: "")
if shouldAcceptChange {
// Force apply since the text view should've updated by itself
viewModel.textView.apply(viewModel.attributedContent)
viewModel.applyAtributedContent()
viewModel.didUpdateText()
}
}
Expand Down
Loading
Loading