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

Fix reconciliate mechanism after multiple new lines #705

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion platforms/ios/example/Wysiwyg/Views/AlertHelper.swift
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ struct AlertHelper<Content: View>: UIViewControllerRepresentable {
uiViewController.rootView = content
var alert = alert
alert.dismissAction = {
self.isPresented = false
isPresented = false
}
if isPresented, uiViewController.presentedViewController == nil {
context.coordinator.alertController = UIAlertController(alert: alert)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,15 @@ extension NSAttributedString {
return NSRange(location: start, length: end - start)
}

/// Computes a string with all characters of the `NSAttributedString` that are actually part of the HTML.
/// Positions in this string will return a range that conforms to the range returned by the Rust model.
public var htmlChars: String {
NSMutableAttributedString(attributedString: self)
.removeDiscardableContent()
.restoreReplacements()
.string
}

// MARK: - Internal

/// Compute an array of all detected occurences of discardable
Expand All @@ -82,30 +91,26 @@ extension NSAttributedString {
}

/// Compute an array of all parts of the attributed string that have been replaced
/// with `PermalinkReplacer` usage within the given range. Also computes
/// the offset between the replacement and the original part (i.e. if the original length
/// is greater than the replacement range, this offset will be negative).
/// with `PermalinkReplacer` usage within the given range.
///
/// - Parameter range: the range on which the elements should be detected. Entire range if omitted
/// - Returns: an array of range and offsets.
func replacementTextRanges(in range: NSRange? = nil) -> [(range: NSRange, offset: Int)] {
var ranges = [(NSRange, Int)]()
/// - Returns: an array of `Replacement`.
func replacementTextRanges(in range: NSRange? = nil) -> [Replacement] {
var replacements = [Replacement]()

enumerateTypedAttribute(.replacementContent) { (replacementContent: ReplacementContent, range: NSRange, _) in
ranges.append((range, range.length - replacementContent.originalLength))
enumerateTypedAttribute(.originalContent) { (originalContent: OriginalContent, range: NSRange, _) in
replacements.append(Replacement(range: range, originalContent: originalContent))
}

return ranges
return replacements
}

/// Compute an array of all parts of the attributed string that have been replaced
/// with `PermalinkReplacer` usage up to the provided index. Also computes
/// the offset between the replacement and the original part (i.e. if the original length
/// is greater than the replacement range, this offset will be negative).
/// with `PermalinkReplacer` usage up to the provided index.
///
/// - Parameter attributedIndex: the position until which the ranges should be computed.
/// - Returns: an array of range and offsets.
func replacementTextRanges(to attributedIndex: Int) -> [(range: NSRange, offset: Int)] {
func replacementTextRanges(to attributedIndex: Int) -> [Replacement] {
replacementTextRanges(in: .init(location: 0, length: attributedIndex))
}

Expand Down Expand Up @@ -167,3 +172,30 @@ extension NSAttributedString {
return attributedIndex
}
}

extension NSMutableAttributedString {
/// Remove all discardable elements from the attributed
/// string (i.e. list prefixes, zwsp placeholders, etc)
///
/// - Returns: self (discardable)
@discardableResult
func removeDiscardableContent() -> Self {
discardableTextRanges().reversed().forEach {
replaceCharacters(in: $0, with: "")
}

return self
}

/// Restore original content from `Replacement` within the attributed string.
///
/// - Returns: self (discardable)
@discardableResult
func restoreReplacements() -> Self {
replacementTextRanges().reversed().forEach {
replaceCharacters(in: $0.range, with: $0.originalContent.text)
}

return self
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ extension NSAttributedString.Key {
/// Attribute for parts of the string that should be removed for HTML selection computation.
/// Should include both placeholder characters such as NBSP and ZWSP, as well as list prefixes.
static let discardableText: NSAttributedString.Key = .init(rawValue: "DiscardableAttributeKey")
/// Attribute for a replacement element. This should be added anytime a part of the attributed string
/// Attribute for the original content of a replacement. This should be added anytime a part of the attributed string
/// is replaced, in order for the composer to compute the expected HTML/attributed range properly.
static let replacementContent: NSAttributedString.Key = .init(rawValue: "ReplacementContentAttributeKey")
static let originalContent: NSAttributedString.Key = .init(rawValue: "OriginalContentAttributeKey")
}
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,10 @@ extension NSMutableAttributedString {
url.absoluteString,
text: self.mutableString.substring(with: range)
) {
let originalText = self.attributedSubstring(from: range).string
self.replaceCharacters(in: range, with: replacement)
self.addAttribute(.replacementContent,
value: ReplacementContent(originalLength: range.length),
self.addAttribute(.originalContent,
value: OriginalContent(text: originalText),
range: .init(location: range.location, length: replacement.length))
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@
// limitations under the License.
//

/// A struct that can be used as an attribute to reflect replaced content in the `NSAttributedString`.
struct ReplacementContent {
/// The original length of the content that has been replaced.
let originalLength: Int
/// A struct that can be used as an attribute to persist the original content of a replaced part of an `NSAttributedString`.
struct OriginalContent {
/// The original text of the content that has been replaced.
let text: String
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
//
// Copyright 2023 The Matrix.org Foundation C.I.C
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

import Foundation

/// Represents a replacement in an instance of `NSAttributedString`
struct Replacement {
/// Range of the `NSAttributedString` where the replacement is located.
let range: NSRange
/// Data of the original content of the `NSAttributedString`.
let originalContent: OriginalContent
}

// MARK: - Helpers

extension Replacement {
/// Computes the offset between the replacement and the original part (i.e. if the original length
/// is greater than the replacement range, this offset will be negative).
var offset: Int {
range.length - originalContent.text.count
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -138,14 +138,14 @@ public class WysiwygComposerViewModel: WysiwygComposerViewModelProtocol, Observa
model.delegate = self
// Publish composer empty state.
$attributedContent.sink { [unowned self] content in
self.isContentEmpty = content.text.length == 0
isContentEmpty = content.text.length == 0
}
.store(in: &cancellables)

$isContentEmpty
.removeDuplicates()
.sink { [unowned self] isContentEmpty in
self.textView.shouldShowPlaceholder = isContentEmpty
textView.shouldShowPlaceholder = isContentEmpty
}
.store(in: &cancellables)

Expand Down Expand Up @@ -522,26 +522,24 @@ private extension WysiwygComposerViewModel {
/// Reconciliate the content of the model with the content of the text view.
func reconciliateIfNeeded() {
do {
guard let replacement = try StringDiffer.replacement(from: attributedContent.text.string,
to: textView.text ?? "") else {
guard let replacement = try StringDiffer.replacement(from: attributedContent.text.htmlChars,
to: textView.attributedText.htmlChars) else {
return
}
// Reconciliate
let rustRange = try attributedContent.text.htmlRange(from: replacement.range)
Logger.viewModel.logDebug(["Reconciliate from \"\(attributedContent.text.string)\" to \"\(textView.text ?? "")\""],
functionName: #function)

let replaceUpdate = model.replaceTextIn(newText: replacement.text,
start: UInt32(rustRange.location),
end: UInt32(rustRange.upperBound))
start: UInt32(replacement.range.location),
end: UInt32(replacement.range.upperBound))
applyUpdate(replaceUpdate, skipTextViewUpdate: true)

// Resync selectedRange
let rustSelection = try textView.attributedText.htmlRange(from: textView.selectedRange)
let selectUpdate = model.select(startUtf16Codeunit: UInt32(rustSelection.location),
endUtf16Codeunit: UInt32(rustSelection.upperBound))
applyUpdate(selectUpdate)

Logger.viewModel.logDebug(["Reconciliate from \"\(attributedContent.text.string)\" to \"\(textView.text ?? "")\""],
functionName: #function)
} catch {
switch error {
case StringDifferError.tooComplicated,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ extension ComposerModel {
return update
}

// swiftlint:enable cyclomatic_complexity

/// Returns currently reversed (active) actions on the composer model.
var reversedActions: Set<ComposerAction> {
Set(actionStates().compactMap { (key: ComposerAction, value: ActionState) in
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,10 +117,7 @@ private struct StringDiff {
private extension String {
/// Converts all whitespaces to NBSP to avoid diffs caused by HTML translations.
var withNBSP: String {
String(self
// FIXME: Not ideal, but avoids triggering reconciliate because of placeholder whitespaces
.filter { $0 != .zwsp }
.map { $0.isWhitespace ? Character.nbsp : $0 })
String(map { $0.isWhitespace ? Character.nbsp : $0 })
.trimmingCharacters(in: .whitespacesAndNewlines)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ final class NSAttributedStringRangeTests: XCTestCase {
crossHtmlRange)
XCTAssertEqual(attributed.attributedSubstring(from: crossAttributedRange).string,
"Item 1\n\t2.\tI")
assertHtmlCharsLengthMatchLastPosition(in: attributed)
}

func testAttributedBulletedLists() throws {
Expand All @@ -79,6 +80,7 @@ final class NSAttributedStringRangeTests: XCTestCase {
XCTAssertEqual(try attributed.attributedPosition(at: 8), 14)
XCTAssertEqual(try attributed.htmlPosition(at: 13), 7)
XCTAssertEqual(try attributed.htmlPosition(at: 3), 0)
assertHtmlCharsLengthMatchLastPosition(in: attributed)
}

func testMultipleAttributedLists() throws {
Expand All @@ -97,6 +99,7 @@ final class NSAttributedStringRangeTests: XCTestCase {
NSRange(location: 4, length: 16))
XCTAssertEqual(try attributed.htmlRange(from: .init(location: 4, length: 17)),
NSRange(location: 0, length: 13))
assertHtmlCharsLengthMatchLastPosition(in: attributed)
}

func testMultipleDigitsNumberedLists() throws {
Expand All @@ -115,6 +118,7 @@ final class NSAttributedStringRangeTests: XCTestCase {
let attributed = try HTMLParser.parse(html: html)
XCTAssertEqual(attributed.discardableTextRanges().count,
19)
assertHtmlCharsLengthMatchLastPosition(in: attributed)
}

func testPositionAfterList() throws {
Expand All @@ -128,6 +132,7 @@ final class NSAttributedStringRangeTests: XCTestCase {
try attributed.attributedRange(from: .init(location: 6, length: 0)),
NSRange(location: 12, length: 0)
)
assertHtmlCharsLengthMatchLastPosition(in: attributed)
}

func testPositionAfterListWithInput() throws {
Expand All @@ -141,6 +146,21 @@ final class NSAttributedStringRangeTests: XCTestCase {
try attributed.attributedRange(from: .init(location: 7, length: 0)),
NSRange(location: 12, length: 0)
)
assertHtmlCharsLengthMatchLastPosition(in: attributed)
}

func testPositionAfterDoubleLineBreak() throws {
let html = "<p>Test</p><p> </p><p>T</p>"
let attributed = try HTMLParser.parse(html: html)
XCTAssertEqual(
try attributed.htmlRange(from: .init(location: 7, length: 0)),
NSRange(location: 6, length: 0)
)
XCTAssertEqual(
try attributed.attributedRange(from: .init(location: 6, length: 0)),
NSRange(location: 7, length: 0)
)
assertHtmlCharsLengthMatchLastPosition(in: attributed)
}

func testOutOfBoundsIndexes() throws {
Expand All @@ -164,4 +184,15 @@ final class NSAttributedStringRangeTests: XCTestCase {
"Provided attributed index is out of bounds (50)")
}
}

/// Assert that the last computed HTML index inside given `NSAttributedString` matches the length of `htmlChars`.
///
/// - Parameter attributedString: the attributed string to test.
private func assertHtmlCharsLengthMatchLastPosition(in attributedString: NSAttributedString) {
let lastHtmlIndex = try? attributedString.htmlPosition(at: attributedString.length)
XCTAssertEqual(
attributedString.htmlChars.count,
lastHtmlIndex
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,11 @@ extension HTMLParserTests {
let attributed = try HTMLParser.parse(html: html, permalinkReplacer: CustomHTMLPermalinkReplacer())
// A text attachment is added.
XCTAssertTrue(attributed.attribute(.attachment, at: 0, effectiveRange: nil) is NSTextAttachment)
// The original length is added to the new part of the attributed string.
let replacementContent = attributed.attribute(.replacementContent, at: 0, effectiveRange: nil) as? ReplacementContent
// The original content is added to the new part of the attributed string.
let originalContent = attributed.attribute(.originalContent, at: 0, effectiveRange: nil) as? OriginalContent
XCTAssertEqual(
replacementContent?.originalLength,
5
originalContent?.text,
"Alice"
)
// HTML and attributed range matches
let htmlRange = NSRange(location: 0, length: 5)
Expand All @@ -40,6 +40,11 @@ extension HTMLParserTests {
try attributed.htmlRange(from: attributedRange),
htmlRange
)
// HTML chars match content.
XCTAssertEqual(
attributed.htmlChars,
"Alice:\(String.nbsp)"
)
}

func testReplaceMultipleLinks() throws {
Expand Down Expand Up @@ -89,6 +94,11 @@ extension HTMLParserTests {
try attributed.htmlRange(from: secondLinkAttributedRange),
secondLinkHtmlRange
)
// HTML chars match content.
XCTAssertEqual(
attributed.htmlChars,
"Alice Alice\(String.nbsp)"
)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,17 @@ final class StringDifferTests: XCTestCase {
XCTAssertThrowsError(try StringDiffer.replacement(from: "text", to: "fexf"), "doubleReplacementIsNotHandled") { error in
XCTAssertEqual(error as? StringDifferError,
StringDifferError.tooComplicated)
XCTAssertEqual(error.localizedDescription,
StringDifferError.tooComplicated.localizedDescription)
}
}

func testInsertionsDontMatchRemovalsLocation() throws {
XCTAssertThrowsError(try StringDiffer.replacement(from: "text", to: "extab"), "insertionsDontMatchRemovalsLocation") { error in
XCTAssertEqual(error as? StringDifferError,
StringDifferError.insertionsDontMatchRemovals)
XCTAssertEqual(error.localizedDescription,
StringDifferError.insertionsDontMatchRemovals.localizedDescription)
}
}

Expand Down