-
-
Notifications
You must be signed in to change notification settings - Fork 286
fix(editor): stop large SQL paste from freezing the editor #1654
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
Changes from all commits
f6a67d5
b634164
03a7f5b
347cfa2
0559fc4
82ae246
d46f267
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -277,4 +277,51 @@ final class HighlighterTests: XCTestCase { | |
| textView.insertText("func helloWorld() {\n\tprint(\"Hello World!\")\n}") | ||
| XCTAssertEqual(textView.string, "func helloWorld() {\n\tprint(\"Hello World!\")\n}") | ||
| } | ||
|
|
||
| @MainActor | ||
| func test_editDoesNotHighlightDocumentOverMaxLength() { | ||
| let highlightProvider = MockHighlightProvider(queryResponse: { .success([]) }) | ||
| let textView = Mock.textView() | ||
| textView.frame = NSRect(x: 0, y: 0, width: 1000, height: 1000) | ||
| textView.setText(String(repeating: "a", count: 64)) | ||
|
|
||
| let highlighter = Mock.highlighter( | ||
| textView: textView, | ||
| highlightProviders: [highlightProvider], | ||
| attributeProvider: attributeProvider | ||
| ) | ||
| highlighter.maxHighlightableLength = 32 | ||
|
|
||
| let baseline = highlightProvider.queryCount | ||
| textView.replaceCharacters(in: NSRange(location: 0, length: 0), with: "b") | ||
|
|
||
| XCTAssertEqual( | ||
| highlightProvider.queryCount, | ||
| baseline, | ||
| "Editing a document over maxHighlightableLength must not query highlights" | ||
| ) | ||
| } | ||
|
|
||
| @MainActor | ||
| func test_editHighlightsDocumentUnderMaxLength() { | ||
| let highlightProvider = MockHighlightProvider(queryResponse: { .success([]) }) | ||
| let textView = Mock.textView() | ||
| textView.frame = NSRect(x: 0, y: 0, width: 1000, height: 1000) | ||
| textView.setText("SELECT 1;") | ||
|
|
||
| let highlighter = Mock.highlighter( | ||
| textView: textView, | ||
| highlightProviders: [highlightProvider], | ||
| attributeProvider: attributeProvider | ||
| ) | ||
| highlighter.maxHighlightableLength = 1_000 | ||
|
|
||
| textView.replaceCharacters(in: NSRange(location: 0, length: 0), with: "x") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In this new test the Useful? React with 👍 / 👎. |
||
|
|
||
| XCTAssertGreaterThan( | ||
| highlightProvider.queryCount, | ||
| 0, | ||
| "A document under maxHighlightableLength should still be highlighted" | ||
| ) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,7 @@ extension TextLayoutManager: NSTextStorageDelegate { | |
| range editedRange: NSRange, | ||
| changeInLength delta: Int | ||
| ) { | ||
| guard processesEdits else { return } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a hidden minimap sets Useful? React with 👍 / 👎. |
||
| guard editedMask.contains(.editedCharacters) else { | ||
| if editedMask.contains(.editedAttributes) && delta == 0 { | ||
| invalidateLayoutForRange(editedRange) | ||
|
|
@@ -80,42 +81,44 @@ extension TextLayoutManager: NSTextStorageDelegate { | |
| /// - Parameter range: The range of the string that was inserted into the text storage. | ||
| private func insertNewLines(for range: NSRange) { | ||
| guard !range.isEmpty, let string = textStorage?.substring(from: range) as? NSString else { return } | ||
| // Loop through each line being inserted, inserting & splitting where necessary | ||
| // Loop through each line being inserted, inserting & splitting where necessary. Each line is described by its | ||
| // length and whether it ends in a line break rather than a materialized substring, so a large paste does not | ||
| // allocate one bridged `String` per line just to test its terminator. | ||
| var index = 0 | ||
| while let nextLine = string.getNextLine(startingAt: index) { | ||
| let lineRange = NSRange(start: index, end: nextLine.max) | ||
| applyLineInsert(string.substring(with: lineRange) as NSString, at: range.location + index) | ||
| applyLineInsert(length: nextLine.max - index, endsInLineBreak: true, at: range.location + index) | ||
| index = nextLine.max | ||
| } | ||
|
|
||
| if index < string.length { | ||
| // Get the last line. | ||
| applyLineInsert(string.substring(from: index) as NSString, at: range.location + index) | ||
| applyLineInsert(length: string.length - index, endsInLineBreak: false, at: range.location + index) | ||
| } | ||
| } | ||
|
|
||
| /// Applies a line insert to the internal line storage tree. | ||
| /// - Parameters: | ||
| /// - insertedString: The string being inserted. | ||
| /// - length: The length of the line being inserted. | ||
| /// - endsInLineBreak: Whether the inserted line is terminated by a line break. | ||
| /// - location: The location the string is being inserted into. | ||
| private func applyLineInsert(_ insertedString: NSString, at location: Int) { | ||
| if LineEnding(line: insertedString as String) != nil { | ||
| private func applyLineInsert(length: Int, endsInLineBreak: Bool, at location: Int) { | ||
| if endsInLineBreak { | ||
| if location == lineStorage.length { | ||
| // Insert a new line at the end of the document, need to insert a new line 'cause there's nothing to | ||
| // split. Also, append the new text to the last line. | ||
| lineStorage.update(atOffset: location, delta: insertedString.length, deltaHeight: 0.0) | ||
| lineStorage.update(atOffset: location, delta: length, deltaHeight: 0.0) | ||
| lineStorage.insert( | ||
| line: TextLine(), | ||
| atOffset: location + insertedString.length, | ||
| atOffset: location + length, | ||
| length: 0, | ||
| height: estimateLineHeight() | ||
| ) | ||
| } else { | ||
| // Need to split the line inserting into and create a new line with the split section of the line | ||
| guard let linePosition = lineStorage.getLine(atOffset: location) else { return } | ||
| let splitLocation = location + insertedString.length | ||
| let splitLocation = location + length | ||
| let splitLength = linePosition.range.max - location | ||
| let lineDelta = insertedString.length - splitLength // The difference in the line being edited | ||
| let lineDelta = length - splitLength // The difference in the line being edited | ||
| if lineDelta != 0 { | ||
| lineStorage.update(atOffset: location, delta: lineDelta, deltaHeight: 0.0) | ||
| } | ||
|
|
@@ -128,7 +131,7 @@ extension TextLayoutManager: NSTextStorageDelegate { | |
| ) | ||
| } | ||
| } else { | ||
| lineStorage.update(atOffset: location, delta: insertedString.length, deltaHeight: 0.0) | ||
| lineStorage.update(atOffset: location, delta: length, deltaHeight: 0.0) | ||
| } | ||
| } | ||
| } | ||
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.
When this new gate returns
truefor a large caret paste,applyEditnow takes the async executor path; that path still invokes the@MainActorcompletion directly fromTreeSitterExecutor's backgroundTask(operation: { completion(.success(operation())) }). In the inspected highlighter flow (Highlighter.textStorage(...didProcessEditing...)→HighlightProviderState.storageDidUpdate), that completion mutates highlight invalidation state and can touch UI-facing state off the main actor for exactly the large paste case this commit adds to the async path. The async branch should hop back to the main actor/queue before calling the completion, as the query and cancel paths already do.Useful? React with 👍 / 👎.