Skip to content

Character-only mutations should not overwrite paragraph-bound attributes in paragraph following mutation #1302

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
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
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ extension AttributedString.CharacterView: RangeReplaceableCollection {
_guts.runs.replaceUTF8Subrange(utf8Range, with: CollectionOfOne(run))

// Invalidate attributes surrounding the affected range. (Phase 2)
_guts._finalizeStringMutation(state)
_guts._finalizeStringMutation(state, type: .characters)
}

public mutating func replaceSubrange(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -457,15 +457,16 @@ extension AttributedString.Guts {
}

func _finalizeStringMutation(
_ state: (mutationStartUTF8Offset: Int, isInsertion: Bool, oldUTF8Count: Int, invalidationRange: Range<Int>)
_ state: (mutationStartUTF8Offset: Int, isInsertion: Bool, oldUTF8Count: Int, invalidationRange: Range<Int>),
type: _MutationType
) {
let utf8Delta = self.string.utf8.count - state.oldUTF8Count
self._finalizeTrackedIndicesUpdate(mutationStartOffset: state.mutationStartUTF8Offset, isInsertion: state.isInsertion, utf8LengthDelta: utf8Delta)
let lower = state.invalidationRange.lowerBound
let upper = state.invalidationRange.upperBound + utf8Delta
self.enforceAttributeConstraintsAfterMutation(
in: lower ..< upper,
type: .attributesAndCharacters)
type: type)
}

func replaceSubrange(
Expand Down Expand Up @@ -493,7 +494,7 @@ extension AttributedString.Guts {
let state = _prepareStringMutation(in: range)
self.string.unicodeScalars.replaceSubrange(range, with: replacementScalars)
self.runs.replaceUTF8Subrange(utf8TargetRange, with: replacementRuns)
_finalizeStringMutation(state)
_finalizeStringMutation(state, type: .attributesAndCharacters)
} else {
self.runs.replaceUTF8Subrange(utf8TargetRange, with: replacementRuns)
self.enforceAttributeConstraintsAfterMutation(in: range._utf8OffsetRange, type: .attributes)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ extension AttributedString.UnicodeScalarView: RangeReplaceableCollection {
_guts.runs.replaceUTF8Subrange(utf8Range, with: CollectionOfOne(run))

// Invalidate attributes surrounding the affected range. (Phase 2)
_guts._finalizeStringMutation(state)
_guts._finalizeStringMutation(state, type: .characters)
}

public mutating func replaceSubrange(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ extension AttributedString.Guts {
enum _MutationType {
case attributes
case attributesAndCharacters
case characters
}

/// Removes full runs of any attributes that have declared an
Expand Down Expand Up @@ -291,7 +292,7 @@ extension AttributedString.Guts {
).updateEach { attributes, _, modified in
modified = attributes.matchStyle(of: paragraphStyle, for: .paragraph)
}
} else if type == .attributesAndCharacters {
} else {
// If any character mutations took place, we apply the constrained styles from the start of each paragraph to the remainder of the paragraph
// The mutation range itself is already fixed-up, so we just need to correct the starting and ending paragraphs

Expand Down Expand Up @@ -324,8 +325,18 @@ extension AttributedString.Guts {
(startParagraph?.upperBound ?? 0) < utf8Range.upperBound,
_needsParagraphFixing(from: utf8Range.upperBound - 1, to: utf8Range.upperBound)
{
let r = _paragraphExtending(from: string.index(before: strRange.upperBound))
endParagraph = r._utf8OffsetRange
let justInsideIndex = string.index(before: strRange.upperBound)
let r = if type == .characters {
// If a character-only mutation, since we apply to the start of the paragraph we need the full real paragraph range
_paragraph(in: Range(uncheckedBounds: (justInsideIndex, justInsideIndex)))
} else {
// If an attribute + character mutation, we don't need the real start of the paragraph
_paragraphExtending(from: justInsideIndex)
}
// If the ending paragraph starts before the mutation we shouldn't store it here as it will incorrectly overwrite the expansion from startParagraph
if r.lowerBound >= strRange.lowerBound {
endParagraph = r._utf8OffsetRange
}
}
}

Expand All @@ -336,12 +347,21 @@ extension AttributedString.Guts {
from: startParagraph.lowerBound,
to: utf8Range.lowerBound ..< startParagraph.upperBound)
}
// If the end paragraph extends beyond the mutation, fixup the range outside the mutation
// If the end paragraph extends beyond the mutation, fixup the paragraph
if let endParagraph, endParagraph.upperBound > utf8Range.upperBound {
_applyStyle(
type: .paragraph,
from: endParagraph.lowerBound,
to: utf8Range.upperBound ..< endParagraph.upperBound)
if type == .attributesAndCharacters {
// If an attribute + characters mutation, the newly applied attributes expand to cover the remainder of the paragraph
_applyStyle(
type: .paragraph,
from: endParagraph.lowerBound,
to: utf8Range.upperBound ..< endParagraph.upperBound)
} else {
// If a character-only mutation, the attributes from the remainder of the paragraph expand to cover the newly inserted text
_applyStyle(
type: .paragraph,
from: utf8Range.upperBound,
to: endParagraph.lowerBound ..< utf8Range.upperBound)
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,10 @@ class TestAttributedStringConstrainingBehavior: XCTestCase {
result.characters.insert(contentsOf: "Test", at: result.index(result.startIndex, offsetByCharacters: 2))
verify(string: result, matches: [("HeTestllo, world\n", 1), ("Next Paragraph", 2)], for: \.testParagraphConstrained)

result = str
result.characters.insert(contentsOf: "Test", at: result.index(result.startIndex, offsetByCharacters: 13))
verify(string: result, matches: [("Hello, world\n", 1), ("TestNext Paragraph", 2)], for: \.testParagraphConstrained)

result = str
result.characters.append(contentsOf: "Test")
verify(string: result, matches: [("Hello, world\n", 1), ("Next ParagraphTest", 2)], for: \.testParagraphConstrained)
Expand Down Expand Up @@ -257,7 +261,7 @@ class TestAttributedStringConstrainingBehavior: XCTestCase {

result = str
result.characters.replaceSubrange(result.index(result.startIndex, offsetByCharacters: 8) ..< result.index(result.startIndex, offsetByCharacters: 15), with: "Test\nReplacement")
verify(string: result, matches: [("Hello, wTest\n", 1), ("Replacementxt Paragraph", 1)], for: \.testParagraphConstrained)
verify(string: result, matches: [("Hello, wTest\n", 1), ("Replacementxt Paragraph", 2)], for: \.testParagraphConstrained)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an intentional change in behavior that I feel was previously incorrect. When replacing text across a paragraph boundary, we shouldn't overwrite the second paragraph with the first paragraph's attribute value, instead the two paragraphs should maintain their individual attribute values. This is unlike replacing the subrange with an AttributedString where the client supplies attributes that should extend from the replacement into the rest of the ending paragraph

}

func testParagraphAttributedTextMutation() {
Expand Down