From cef7a3937a83e35151a9b5d298fef7005d4e7449 Mon Sep 17 00:00:00 2001 From: Brandon Bloom Date: Sat, 14 Mar 2026 13:33:56 -0700 Subject: [PATCH] Avoid scrolling whole selections to keep undo responsive Undoing a delete after selecting a very large range can become slow on AppKit. The expensive work happens in STTextView.layout() when the view tries to make the restored selection visible. The existing code always asks TextKit for the frame of the entire current selection, so undoing Cmd+A/Delete effectively requests geometry for the whole document. Make the scroll-to-selection path target only the geometry that is needed: - if the selection already intersects the current viewport, do not scroll - if the selection is outside the viewport, scroll only the nearest selection edge instead of the entire selection range - share the same gutter-aware rect adjustment logic for range-based and location-based scrolling Add focused AppKit tests for the selection-to-scroll-target logic in UndoTests so this behavior is covered on the branch without depending on any other test harness changes. This keeps undo responsive for large selection restores while preserving the existing selection visibility behavior. --- .../STTextView+Scrolling.swift | 35 +++-- Sources/STTextViewAppKit/STTextView.swift | 30 ++++- Tests/STTextViewAppKitTests/UndoTests.swift | 120 ++++++++++++++++++ 3 files changed, 173 insertions(+), 12 deletions(-) diff --git a/Sources/STTextViewAppKit/STTextView+Scrolling.swift b/Sources/STTextViewAppKit/STTextView+Scrolling.swift index 2ee560d4..6ad68989 100644 --- a/Sources/STTextViewAppKit/STTextView+Scrolling.swift +++ b/Sources/STTextViewAppKit/STTextView+Scrolling.swift @@ -6,27 +6,42 @@ import AppKit extension STTextView { + private func adjustedScrollRect(_ rect: CGRect) -> CGRect { + var adjustedRect = rect + + if adjustedRect.width.isZero { + // add padding around the point to ensure the visibility the segment + // since the width of the segment is 0 for a selection + adjustedRect = adjustedRect.inset(by: .init(top: 0, left: -textContainer.lineFragmentPadding, bottom: 0, right: -textContainer.lineFragmentPadding)) + } + + // scroll to visible IN clip view (ignoring gutter view overlay) + // adjust rect to mimick it's size to include gutter overlay + adjustedRect.origin.x -= gutterView?.frame.width ?? 0 + adjustedRect.size.width += gutterView?.frame.width ?? 0 + return adjustedRect + } + override open func scroll(_ point: NSPoint) { contentView.scroll(point.applying(.init(translationX: -(gutterView?.frame.width ?? 0), y: 0))) } @discardableResult func scrollToVisible(_ textRange: NSTextRange, type: NSTextLayoutManager.SegmentType) -> Bool { - guard var rect = textLayoutManager.textSegmentFrame(in: textRange, type: type) else { + guard let rect = textLayoutManager.textSegmentFrame(in: textRange, type: type) else { return false } - if rect.width.isZero { - // add padding around the point to ensure the visibility the segment - // since the width of the segment is 0 for a selection - rect = rect.inset(by: .init(top: 0, left: -textContainer.lineFragmentPadding, bottom: 0, right: -textContainer.lineFragmentPadding)) + return contentView.scrollToVisible(adjustedScrollRect(rect)) + } + + @discardableResult + func scrollToVisible(_ textLocation: NSTextLocation, type: NSTextLayoutManager.SegmentType) -> Bool { + guard let rect = textLayoutManager.textSegmentFrame(at: textLocation, type: type) else { + return false } - // scroll to visible IN clip view (ignoring gutter view overlay) - // adjust rect to mimick it's size to include gutter overlay - rect.origin.x -= gutterView?.frame.width ?? 0 - rect.size.width += gutterView?.frame.width ?? 0 - return contentView.scrollToVisible(rect) + return contentView.scrollToVisible(adjustedScrollRect(rect)) } override open func centerSelectionInVisibleArea(_ sender: Any?) { diff --git a/Sources/STTextViewAppKit/STTextView.swift b/Sources/STTextViewAppKit/STTextView.swift index 717bd850..a8ec6c8c 100644 --- a/Sources/STTextViewAppKit/STTextView.swift +++ b/Sources/STTextViewAppKit/STTextView.swift @@ -1339,8 +1339,10 @@ open class STTextView: NSView, NSTextInput, NSTextContent, STTextViewProtocol { super.layout() layoutText() - if needsScrollToSelection, let textRange = textLayoutManager.textSelections.last?.textRanges.last { - scrollToVisible(textRange, type: .standard) + if needsScrollToSelection, + let textRange = textLayoutManager.textSelections.last?.textRanges.last, + let textLocation = textLocationForScrollingSelection(toVisible: textRange) { + scrollToVisible(textLocation, type: .standard) } needsScrollToSelection = false @@ -1384,6 +1386,30 @@ open class STTextView: NSView, NSTextInput, NSTextContent, STTextViewProtocol { } } + func textLocationForScrollingSelection(toVisible textRange: NSTextRange) -> NSTextLocation? { + guard !textRange.isEmpty else { + return textRange.location + } + + guard let viewportRange = textLayoutManager.textViewportLayoutController.viewportRange else { + return textRange.location + } + + if textRange.intersects(viewportRange) { + return nil + } + + let selectionEndsBeforeViewport = textContentManager.offset( + from: textRange.endLocation, + to: viewportRange.location + ) > 0 + if selectionEndsBeforeViewport { + return textRange.endLocation + } + + return textRange.location + } + private var effectiveVisibleRect: CGRect { visibleRect.isInfinite ? bounds : visibleRect } diff --git a/Tests/STTextViewAppKitTests/UndoTests.swift b/Tests/STTextViewAppKitTests/UndoTests.swift index 992653dd..a0ae197c 100644 --- a/Tests/STTextViewAppKitTests/UndoTests.swift +++ b/Tests/STTextViewAppKitTests/UndoTests.swift @@ -1,7 +1,9 @@ #if os(macOS) + import AppKit import XCTest @testable import STTextViewAppKit + @MainActor final class UndoTests: XCTestCase { func testInsertingAtEndAndUndo() { let textView = STTextView() @@ -80,5 +82,123 @@ textView.redo(nil) XCTAssertEqual(textView.text!, "123a789") } + + func testSelectionScrollLocationSkipsSelectionsAlreadyInViewport() { + let harness = ScrollViewHarness() + let textView = harness.textView + + textView.isHorizontallyResizable = false + textView.isVerticallyResizable = true + textView.text = Array(repeating: "alpha beta gamma delta epsilon zeta eta theta iota kappa", count: 200).joined(separator: "\n") + + harness.flushLayout() + + XCTAssertNil(textView.textLocationForScrollingSelection(toVisible: textView.textLayoutManager.documentRange)) + } + + func testSelectionScrollLocationUsesNearestSelectionEdgeOutsideViewport() throws { + let harness = ScrollViewHarness() + let textView = harness.textView + + textView.isHorizontallyResizable = false + textView.isVerticallyResizable = true + textView.text = Array(repeating: "alpha beta gamma delta epsilon zeta eta theta iota kappa", count: 200).joined(separator: "\n") + + harness.flushLayout() + + guard let initialViewportRange = textView.textLayoutManager.textViewportLayoutController.viewportRange else { + return XCTFail("Missing initial viewport range") + } + + let documentStart = textView.textLayoutManager.documentRange.location + let initialViewportEndOffset = textView.textContentManager.offset(from: documentStart, to: initialViewportRange.endLocation) + let afterRange = try XCTUnwrap( + NSTextRange( + NSRange(location: min(initialViewportEndOffset + 1, textView.text!.utf16.count - 1), length: 1), + in: textView.textContentManager + ) + ) + let afterLocation = try XCTUnwrap(textView.textLocationForScrollingSelection(toVisible: afterRange)) + XCTAssertEqual( + textView.textContentManager.offset(from: documentStart, to: afterLocation), + NSRange(afterRange, in: textView.textContentManager).location + ) + + harness.scrollToBottom() + + guard let viewportRange = textView.textLayoutManager.textViewportLayoutController.viewportRange else { + return XCTFail("Missing viewport range") + } + + let viewportStartOffset = textView.textContentManager.offset(from: documentStart, to: viewportRange.location) + let beforeRange = try XCTUnwrap( + NSTextRange( + NSRange(location: 0, length: max(1, viewportStartOffset - 1)), + in: textView.textContentManager + ) + ) + let beforeLocation = try XCTUnwrap(textView.textLocationForScrollingSelection(toVisible: beforeRange)) + + XCTAssertEqual( + textView.textContentManager.offset(from: documentStart, to: beforeLocation), + NSMaxRange(NSRange(beforeRange, in: textView.textContentManager)) + ) + } + } + + @MainActor + private final class ScrollViewHarness { + let window: NSWindow + let scrollView: NSScrollView + let textView: STTextView + + init() { + let scrollView = STTextView.scrollableTextView() + self.window = NSWindow( + contentRect: NSRect(x: 0, y: 0, width: 480, height: 320), + styleMask: [.titled, .closable, .resizable], + backing: .buffered, + defer: false + ) + self.scrollView = scrollView + self.textView = scrollView.documentView as! STTextView + + guard let contentView = window.contentView else { + fatalError("Missing window content view") + } + + scrollView.translatesAutoresizingMaskIntoConstraints = false + contentView.addSubview(scrollView) + NSLayoutConstraint.activate([ + scrollView.leadingAnchor.constraint(equalTo: contentView.leadingAnchor), + scrollView.trailingAnchor.constraint(equalTo: contentView.trailingAnchor), + scrollView.topAnchor.constraint(equalTo: contentView.topAnchor), + scrollView.bottomAnchor.constraint(equalTo: contentView.bottomAnchor) + ]) + + window.makeKeyAndOrderFront(nil) + } + + func flushLayout() { + window.contentView?.layoutSubtreeIfNeeded() + textView.layoutSubtreeIfNeeded() + + RunLoop.current.run(until: Date().addingTimeInterval(0.01)) + + window.contentView?.layoutSubtreeIfNeeded() + textView.layoutSubtreeIfNeeded() + } + + func scrollToBottom() { + let documentHeight = textView.frame.height + let visibleHeight = scrollView.contentView.bounds.height + guard documentHeight > visibleHeight else { + return + } + + scrollView.contentView.scroll(to: CGPoint(x: 0, y: documentHeight - visibleHeight)) + scrollView.reflectScrolledClipView(scrollView.contentView) + flushLayout() + } } #endif