Skip to content

Commit 5114e0d

Browse files
Move Layout Updates to NSTextStorage Delegate (#82)
### Description **Detailed Changes** - Makes `TextLayoutManager` a text storage delegate. - Updates `TextLayoutManager` to use the text storage methods to update internal state. - Remove `TextLayoutManager`'s layout transaction capabilities. - Updates `replaceCharacters` to reflect the new layout manager requirements. - Moves layout back into a `CATransaction`, now that method ordering is slightly different, macOS might call `layout` on the text view while we're laying out text. Doing that in a transaction ensures that doesn't happen and layout is atomic. **Tests** - Adds tests to the layout manager to ensure modifications are kept valid. - Adds tests for `rects(in:)` to ensure it doesn't invalidate layout information. - Adds a test ensuring editing text contents in a text view delegate callback doesn't break layout. ### Discussion This change untangles the text layout updating from text view code. Right now the hierarchy looks like this: ``` (Update occurs) TextView -> Text Layout Update -> Text Storage Update ``` This isn't great, because it means anything that modifies the text outside of the text view *must* remember to update layout, selection, send notifications, and do it all in the right order. I'd like to move it to this hierarchy: ``` (Update occurs) TextView -> Text Storage Update -> Text Layout Update ``` This way any component can modify the text storage, and that will be reflected in the layout manager. There's a myriad of small bugs in our editor right now that are effected by this. One key result of this change is the ability to correctly modify text in textView delegate callbacks. Right now, if the text is updated in a callback, it can lead to a recursive layout call to the text layout manager. This leads to bad internal state, and bugs where entire sections of text disappear entirely. This also comes with a performance boost. Since NSTextStorage automatically coagulates text changes, we'll perform less layout passes for each edit, and components can make use of the text storage's transaction capabilities to only cause layout after all edits have been applied in a transaction. > [!NOTE] > This change cannot be applied to the selection manager, the selection manager needs much more fine-grained information about edits than the layout manager to correctly update itself. > @hi2gage and I have also discussed some modifications to the selection API, since keeping it in sync with the text storage makes less sense than the layout manager. ### Related Issues * Could be related to CodeEditApp/CodeEditSourceEditor#196 ### Checklist - [x] Add Tests - [x] I read and understood the [contributing guide](https://github.com/CodeEditApp/CodeEdit/blob/main/CONTRIBUTING.md) as well as the [code of conduct](https://github.com/CodeEditApp/CodeEdit/blob/main/CODE_OF_CONDUCT.md) - [x] The issues this PR addresses are related to each other - [x] My changes generate no new warnings - [x] My code builds and runs on my machine - [x] My changes are all related to the related issue above - [x] I documented my code ### Screenshots
1 parent d5ef35f commit 5114e0d

12 files changed

+267
-102
lines changed

Sources/CodeEditTextView/TextLayoutManager/TextLayoutManager+Edits.swift

Lines changed: 56 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -10,17 +10,50 @@ import AppKit
1010
// MARK: - Edits
1111

1212
extension TextLayoutManager: NSTextStorageDelegate {
13-
/// Notifies the layout manager of an edit.
13+
/// Receives edit notifications from the text storage and updates internal data structures to stay in sync with
14+
/// text content.
1415
///
15-
/// Used by the `TextView` to tell the layout manager about any edits that will happen.
16-
/// Use this to keep the layout manager's line storage in sync with the text storage.
16+
/// If the changes are only attribute changes, this method invalidates layout for the edited range and returns.
1717
///
18-
/// - Parameters:
19-
/// - range: The range of the edit.
20-
/// - string: The string to replace in the given range.
21-
public func willReplaceCharactersInRange(range: NSRange, with string: String) {
18+
/// Otherwise, any lines that were removed or replaced by the edit are first removed from the text line layout
19+
/// storage. Then, any new lines are inserted into the same storage.
20+
///
21+
/// For instance, if inserting a newline this method will:
22+
/// - Remove no lines (none were replaced)
23+
/// - Update the current line's range to contain the newline character.
24+
/// - Insert a new line after the current line.
25+
///
26+
/// If a selection containing a newline is deleted and replaced with two more newlines this method will:
27+
/// - Delete the original line.
28+
/// - Insert two lines.
29+
///
30+
/// - Note: This method *does not* cause a layout calculation. If a method is finding `NaN` values for line
31+
/// fragments, ensure `layout` or `ensureLayoutUntil` are called on the subject ranges.
32+
public func textStorage(
33+
_ textStorage: NSTextStorage,
34+
didProcessEditing editedMask: NSTextStorageEditActions,
35+
range editedRange: NSRange,
36+
changeInLength delta: Int
37+
) {
38+
guard editedMask.contains(.editedCharacters) else {
39+
if editedMask.contains(.editedAttributes) && delta == 0 {
40+
invalidateLayoutForRange(editedRange)
41+
}
42+
return
43+
}
44+
45+
let insertedStringRange = NSRange(location: editedRange.location, length: editedRange.length - delta)
46+
removeLayoutLinesIn(range: insertedStringRange)
47+
insertNewLines(for: editedRange)
48+
invalidateLayoutForRange(editedRange)
49+
}
50+
51+
/// Removes all lines in the range, as if they were deleted. This is a setup for inserting the lines back in on an
52+
/// edit.
53+
/// - Parameter range: The range that was deleted.
54+
private func removeLayoutLinesIn(range: NSRange) {
2255
// Loop through each line being replaced in reverse, updating and removing where necessary.
23-
for linePosition in lineStorage.linesInRange(range).reversed() {
56+
for linePosition in lineStorage.linesInRange(range).reversed() {
2457
// Two cases: Updated line, deleted line entirely
2558
guard let intersection = linePosition.range.intersection(range), !intersection.isEmpty else { continue }
2659
if intersection == linePosition.range && linePosition.range.max != lineStorage.length {
@@ -38,25 +71,24 @@ extension TextLayoutManager: NSTextStorageDelegate {
3871
lineStorage.update(atIndex: linePosition.range.location, delta: -intersection.length, deltaHeight: 0)
3972
}
4073
}
74+
}
4175

76+
/// Inserts any newly inserted lines into the line layout storage. Exits early if the range is empty.
77+
/// - Parameter range: The range of the string that was inserted into the text storage.
78+
private func insertNewLines(for range: NSRange) {
79+
guard !range.isEmpty, let string = textStorage?.substring(from: range) as? NSString else { return }
4280
// Loop through each line being inserted, inserting & splitting where necessary
43-
if !string.isEmpty {
44-
var index = 0
45-
while let nextLine = (string as NSString).getNextLine(startingAt: index) {
46-
let lineRange = NSRange(start: index, end: nextLine.max)
47-
applyLineInsert((string as NSString).substring(with: lineRange) as NSString, at: range.location + index)
48-
index = nextLine.max
49-
}
81+
var index = 0
82+
while let nextLine = string.getNextLine(startingAt: index) {
83+
let lineRange = NSRange(start: index, end: nextLine.max)
84+
applyLineInsert(string.substring(with: lineRange) as NSString, at: range.location + index)
85+
index = nextLine.max
86+
}
5087

51-
if index < (string as NSString).length {
52-
// Get the last line.
53-
applyLineInsert(
54-
(string as NSString).substring(from: index) as NSString,
55-
at: range.location + index
56-
)
57-
}
88+
if index < string.length {
89+
// Get the last line.
90+
applyLineInsert(string.substring(from: index) as NSString, at: range.location + index)
5891
}
59-
setNeedsLayout()
6092
}
6193

6294
/// Applies a line insert to the internal line storage tree.
@@ -65,7 +97,7 @@ extension TextLayoutManager: NSTextStorageDelegate {
6597
/// - location: The location the string is being inserted into.
6698
private func applyLineInsert(_ insertedString: NSString, at location: Int) {
6799
if LineEnding(line: insertedString as String) != nil {
68-
if location == textStorage?.length ?? 0 {
100+
if location == lineStorage.length {
69101
// Insert a new line at the end of the document, need to insert a new line 'cause there's nothing to
70102
// split. Also, append the new text to the last line.
71103
lineStorage.update(atIndex: location, delta: insertedString.length, deltaHeight: 0.0)
@@ -96,18 +128,4 @@ extension TextLayoutManager: NSTextStorageDelegate {
96128
lineStorage.update(atIndex: location, delta: insertedString.length, deltaHeight: 0.0)
97129
}
98130
}
99-
100-
/// This method is to simplify keeping the layout manager in sync with attribute changes in the storage object.
101-
/// This does not handle cases where characters have been inserted or removed from the storage.
102-
/// For that, see the `willPerformEdit` method.
103-
public func textStorage(
104-
_ textStorage: NSTextStorage,
105-
didProcessEditing editedMask: NSTextStorageEditActions,
106-
range editedRange: NSRange,
107-
changeInLength delta: Int
108-
) {
109-
if editedMask.contains(.editedAttributes) && delta == 0 {
110-
invalidateLayoutForRange(editedRange)
111-
}
112-
}
113131
}

Sources/CodeEditTextView/TextLayoutManager/TextLayoutManager+Public.swift

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ extension TextLayoutManager {
121121
return nil
122122
}
123123
if linePosition.data.lineFragments.isEmpty {
124-
let newHeight = ensureLayoutFor(position: linePosition)
124+
let newHeight = preparePositionForDisplay(linePosition)
125125
if linePosition.height != newHeight {
126126
delegate?.layoutManagerHeightDidUpdate(newHeight: lineStorage.height)
127127
}
@@ -165,7 +165,8 @@ extension TextLayoutManager {
165165
/// - line: The line to calculate rects for.
166166
/// - Returns: Multiple bounding rects. Will return one rect for each line fragment that overlaps the given range.
167167
public func rectsFor(range: NSRange) -> [CGRect] {
168-
lineStorage.linesInRange(range).flatMap { self.rectsFor(range: range, in: $0) }
168+
ensureLayoutUntil(range.max)
169+
return lineStorage.linesInRange(range).flatMap { self.rectsFor(range: range, in: $0) }
169170
}
170171

171172
/// Calculates all text bounding rects that intersect with a given range, with a given line position.
@@ -190,6 +191,7 @@ extension TextLayoutManager {
190191
for fragmentPosition in line.data.lineFragments.linesInRange(relativeRange) {
191192
guard let intersectingRange = fragmentPosition.range.intersection(relativeRange) else { continue }
192193
let fragmentRect = fragmentPosition.data.rectFor(range: intersectingRange)
194+
guard fragmentRect.width > 0 else { continue }
193195
rects.append(
194196
CGRect(
195197
x: fragmentRect.minX + edgeInsets.left,
@@ -239,6 +241,8 @@ extension TextLayoutManager {
239241
// Combine the points in clockwise order
240242
let points = leftSidePoints + rightSidePoints
241243

244+
guard points.allSatisfy({ $0.x.isFinite && $0.y.isFinite }) else { return nil }
245+
242246
// Close the path
243247
if let firstPoint = points.first {
244248
return NSBezierPath.smoothPath(points + [firstPoint], radius: cornerRadius)
@@ -286,7 +290,7 @@ extension TextLayoutManager {
286290
for linePosition in lineStorage.linesInRange(
287291
NSRange(start: startingLinePosition.range.location, end: linePosition.range.max)
288292
) {
289-
let height = ensureLayoutFor(position: linePosition)
293+
let height = preparePositionForDisplay(linePosition)
290294
if height != linePosition.height {
291295
lineStorage.update(
292296
atIndex: linePosition.range.location,

Sources/CodeEditTextView/TextLayoutManager/TextLayoutManager+Transaction.swift

Lines changed: 0 additions & 38 deletions
This file was deleted.

Sources/CodeEditTextView/TextLayoutManager/TextLayoutManager+ensureLayout.swift

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,10 @@
88
import Foundation
99

1010
extension TextLayoutManager {
11-
/// Forces layout calculation for all lines up to and including the given offset.
12-
/// - Parameter offset: The offset to ensure layout until.
13-
package func ensureLayoutFor(position: TextLineStorage<TextLine>.TextLinePosition) -> CGFloat {
11+
/// Invalidates and prepares a line position for display.
12+
/// - Parameter position: The line position to prepare.
13+
/// - Returns: The height of the newly laid out line and all it's fragments.
14+
package func preparePositionForDisplay(_ position: TextLineStorage<TextLine>.TextLinePosition) -> CGFloat {
1415
guard let textStorage else { return 0 }
1516
let displayData = TextLine.DisplayData(
1617
maxWidth: maxLineLayoutWidth,

Sources/CodeEditTextView/TextLayoutManager/TextLayoutManager.swift

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -199,8 +199,6 @@ public class TextLayoutManager: NSObject {
199199
/// ``TextLayoutManager/estimateLineHeight()`` is called.
200200
private var _estimateLineHeight: CGFloat?
201201

202-
// MARK: - Layout
203-
204202
/// Asserts that the caller is not in an active layout pass.
205203
/// See docs on ``isInLayout`` for more details.
206204
private func assertNotInLayout() {
@@ -209,6 +207,8 @@ public class TextLayoutManager: NSObject {
209207
#endif
210208
}
211209

210+
// MARK: - Layout
211+
212212
/// Lays out all visible lines
213213
func layoutLines(in rect: NSRect? = nil) { // swiftlint:disable:this function_body_length
214214
assertNotInLayout()
@@ -217,9 +217,15 @@ public class TextLayoutManager: NSObject {
217217
let textStorage else {
218218
return
219219
}
220+
221+
// The macOS may call `layout` on the textView while we're laying out fragment views. This ensures the view
222+
// tree modifications caused by this method are atomic, so macOS won't call `layout` while we're already doing
223+
// that
224+
CATransaction.begin()
220225
#if DEBUG
221226
isInLayout = true
222227
#endif
228+
223229
let minY = max(visibleRect.minY - verticalLayoutPadding, 0)
224230
let maxY = max(visibleRect.maxY + verticalLayoutPadding, 0)
225231
let originalHeight = lineStorage.height
@@ -265,16 +271,17 @@ public class TextLayoutManager: NSObject {
265271
newVisibleLines.insert(linePosition.data.id)
266272
}
267273

274+
#if DEBUG
275+
isInLayout = false
276+
#endif
277+
CATransaction.commit()
278+
268279
// Enqueue any lines not used in this layout pass.
269280
viewReuseQueue.enqueueViews(notInSet: usedFragmentIDs)
270281

271282
// Update the visible lines with the new set.
272283
visibleLineIds = newVisibleLines
273284

274-
#if DEBUG
275-
isInLayout = false
276-
#endif
277-
278285
// These are fine to update outside of `isInLayout` as our internal data structures are finalized at this point
279286
// so laying out again won't break our line storage or visible line.
280287

Sources/CodeEditTextView/TextSelectionManager/TextSelectionManager.swift

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,8 @@ public class TextSelectionManager: NSObject {
8686
/// Set the selected ranges to new ranges. Overrides any existing selections.
8787
/// - Parameter range: The selected ranges to set.
8888
public func setSelectedRanges(_ ranges: [NSRange]) {
89+
let oldRanges = textSelections.map(\.range)
90+
8991
textSelections.forEach { $0.view?.removeFromSuperview() }
9092
// Remove duplicates, invalid ranges, update suggested X position.
9193
textSelections = Set(ranges)
@@ -99,8 +101,11 @@ public class TextSelectionManager: NSObject {
99101
return selection
100102
}
101103
updateSelectionViews()
102-
NotificationCenter.default.post(Notification(name: Self.selectionChangedNotification, object: self))
103104
delegate?.setNeedsDisplay()
105+
106+
if oldRanges != textSelections.map(\.range) {
107+
NotificationCenter.default.post(Notification(name: Self.selectionChangedNotification, object: self))
108+
}
104109
}
105110

106111
/// Append a new selected range to the existing ones.

Sources/CodeEditTextView/TextView/TextView+ReplaceCharacters.swift

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ extension TextView {
1616
public func replaceCharacters(in ranges: [NSRange], with string: String) {
1717
guard isEditable else { return }
1818
NotificationCenter.default.post(name: Self.textWillChangeNotification, object: self)
19-
layoutManager.beginTransaction()
2019
textStorage.beginEditing()
2120

2221
// Can't insert an empty string into an empty range. One must be not empty
@@ -25,7 +24,6 @@ extension TextView {
2524
(delegate?.textView(self, shouldReplaceContentsIn: range, with: string) ?? true) {
2625
delegate?.textView(self, willReplaceContentsIn: range, with: string)
2726

28-
layoutManager.willReplaceCharactersInRange(range: range, with: string)
2927
_undoManager?.registerMutation(
3028
TextMutation(string: string as String, range: range, limit: textStorage.length)
3129
)
@@ -39,7 +37,6 @@ extension TextView {
3937
}
4038

4139
textStorage.endEditing()
42-
layoutManager.endTransaction()
4340
selectionManager.notifyAfterEdit()
4441
NotificationCenter.default.post(name: Self.textDidChangeNotification, object: self)
4542

Sources/CodeEditTextView/TextView/TextView.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,6 @@ public class TextView: NSView, NSTextContent {
5858
textStorage.string
5959
}
6060
set {
61-
layoutManager.willReplaceCharactersInRange(range: documentRange, with: newValue)
6261
textStorage.setAttributedString(NSAttributedString(string: newValue, attributes: typingAttributes))
6362
}
6463
}
@@ -339,6 +338,7 @@ public class TextView: NSView, NSTextContent {
339338

340339
layoutManager = setUpLayoutManager(lineHeightMultiplier: lineHeightMultiplier, wrapLines: wrapLines)
341340
storageDelegate.addDelegate(layoutManager)
341+
342342
selectionManager = setUpSelectionManager()
343343
selectionManager.useSystemCursor = useSystemCursor
344344

Sources/CodeEditTextView/Utils/CEUndoManager.swift

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -179,21 +179,15 @@ public class CEUndoManager {
179179

180180
/// Groups all incoming mutations.
181181
public func beginUndoGrouping() {
182-
guard !isGrouping else {
183-
assertionFailure("UndoManager already in a group. Call `beginUndoGrouping` before this can be called.")
184-
return
185-
}
182+
guard !isGrouping else { return }
186183
isGrouping = true
187184
// This is a new undo group, break for it.
188185
shouldBreakNextGroup = true
189186
}
190187

191188
/// Stops grouping all incoming mutations.
192189
public func endUndoGrouping() {
193-
guard isGrouping else {
194-
assertionFailure("UndoManager not in a group. Call `endUndoGrouping` before this can be called.")
195-
return
196-
}
190+
guard isGrouping else { return }
197191
isGrouping = false
198192
// We just ended a group, do not allow the next mutation to be added to the group we just made.
199193
shouldBreakNextGroup = true

Tests/CodeEditTextViewTests/EmphasisManagerTests.swift

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,8 @@ import Foundation
44

55
@Suite()
66
struct EmphasisManagerTests {
7-
@Test() @MainActor
7+
@Test()
8+
@MainActor
89
func testFlashEmphasisLayersNotLeaked() {
910
// Ensure layers are not leaked when switching from flash emphasis to any other emphasis type.
1011
let textView = TextView(string: "Lorem Ipsum")

0 commit comments

Comments
 (0)